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.

Sign up for GitHub

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

Jump to bottom

feat: Add metadata to error responses #348

Merged
merged 4 commits into from
May 10, 2020
Merged

feat: Add metadata to error responses #348

merged 4 commits into from
May 10, 2020

Conversation

dvshur
Copy link
Contributor

@dvshur dvshur commented May 10, 2020

Motivation

Issue #343 as well as some Discord discussions around May 5th—7th 2020 contain motivation for the PR.

Solution

  • Added metadata field to the Status struct, like in the Response struct
    • If the provided metadata contains any headers with names reserved either by the gRPC spec or by Status fields (e.g. grpc-status-details-bin), they will be ignored.
  • Updated existing Status constructors and conversions to support metadata
  • Added Status constructors with metadata only or also with details
  • Updated Debug and Display implementations for Status to support the new field
  • Added an integration test for the functionality
  • This change should be backward compatible

@dvshur dvshur changed the title Metadata error response Add metadata to error responses May 10, 2020
LucioFranco
LucioFranco reviewed May 10, 2020
View reviewed changes
tonic/src/status.rs Outdated Show resolved Hide resolved
LucioFranco
LucioFranco reviewed May 10, 2020
View reviewed changes
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Looks great, one small nit and we can merge. I can try to get a release out today.

@dvshur
Copy link
Contributor Author

dvshur commented May 10, 2020 •
edited
Loading

@LucioFranco Are you good with the constrictor naming? with_details_and_metadata? It's a little bit long, but explicit :)

@LucioFranco
Copy link
Member

@dvshur yeah, I think for now it is fine, I suspect in the future we might refactor but its a code path that is not hit often enough to really require good ergonomics.

LucioFranco
LucioFranco approved these changes May 10, 2020
View reviewed changes
Copy link
Member

@LucioFranco LucioFranco 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!

@LucioFranco LucioFranco changed the title Add metadata to error responses feat: Add metadata to error responses May 10, 2020
@LucioFranco LucioFranco merged commit 372da52 into hyperium:master May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucioFranco LucioFranco LucioFranco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dvshur @LucioFranco