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

Combine headers + trailers into ConnectError.metadata #239

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

rebello95
Copy link
Collaborator

As described by @jhump:

Currently, users may need to check both headers and trailers when looking for metadata in an error response for an operation with a unary reply (unary and client-stream RPCs). The reason they may have to look in two places is because where the metadata shows up isn't straight-forward - it depends on if the protocol being used and, if gRPC or gRPC-Web, whether the server used a trailers-only response or not. In a trailers-only response, all metadata would show up in the same bag of metadata (which are technically HTTP response headers, but may be classified as "trailers" since they include the status and there will be no subsequent HTTP response trailers in the reply).

This PR updates ConnectError's initialization to combine both headers + trailers into its metadata so that it's easier for consumers to query for the keys they need. This also matches connect-go and connect-es.

@rebello95 rebello95 requested review from jhump and eseay January 20, 2024 19:55
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Mostly LG. One question and one suggestion.

Libraries/Connect/PackageInternal/ConnectError+GRPC.swift Outdated Show resolved Hide resolved
Libraries/Connect/Public/Interfaces/ConnectError.swift Outdated Show resolved Hide resolved
@rebello95
Copy link
Collaborator Author

@eseay would you mind taking a look at this as well? 🙏🏽

Copy link
Contributor

@eseay eseay left a comment

Choose a reason for hiding this comment

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

All makes sense!

@rebello95 rebello95 merged commit 1701d3d into main Feb 1, 2024
13 checks passed
@rebello95 rebello95 deleted the grpc-combine-headers-trailers branch February 1, 2024 06:00
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.

3 participants