Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the indication of destination message type into Codec #599

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

jhump
Copy link
Member

@jhump jhump commented Oct 3, 2023

This is so that a custom Codec can choose what details to reveal to clients, in cases where emitting the destination message type is considered to leak too much information.

See #584 for more context.

@jhump jhump changed the title move the indication of destination message type into Codec Move the indication of destination message type into Codec Oct 3, 2023
@jhump jhump requested a review from akshayjshah October 3, 2023 14:40
Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix, Josh.

@akshayjshah
Copy link
Member

Actually...should we do the same on the marshaling side?

@jhump
Copy link
Member Author

jhump commented Oct 3, 2023

Actually...should we do the same on the marshaling side?

We don't actually include the type anywhere in marshal errors today. Instead we just return an internal error with "marshal message" as the error context.

But while searching, I did totally miss a spot that needed to be updated 🤦. I had updated only one spot where we unmarshal messages -- in the GET part of the Connect protocol. The other part (that handles all other protocols, unmarshalling from the body) is now also fixed.

For consistency, the code now just adds "unmarshal message" as error context, to decorate whatever the codec returns. It is up to the codec to include any other details (such as the target or source type). Do you think I should add the type to the default codecs' marshal methods? (I assumed not and that your suggestion was just to be sure we were consistent for both halves of the codec.)

@akshayjshah
Copy link
Member

Do you think I should add the type to the default codecs' marshal methods?

Nope! This LGTM, let's merge it.

akshayjshah added a commit to akshayjshah/connectproto that referenced this pull request Oct 3, 2023
Following connectrpc/connect-go#599, make the
errors returned by this package's codecs include the protobuf type name.
akshayjshah added a commit to akshayjshah/connectproto that referenced this pull request Oct 3, 2023
Following connectrpc/connect-go#599, make the
errors returned by this package's codecs include the protobuf type name.
@akshayjshah akshayjshah merged commit 969bcab into main Oct 4, 2023
7 checks passed
@akshayjshah akshayjshah deleted the jh/codec-errors branch October 4, 2023 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants