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

Respect "grpc-status-details-bin" trailer on error response #51394

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Respect "grpc-status-details-bin" trailer on error response #51394

merged 5 commits into from
Nov 29, 2023

Conversation

alrz
Copy link
Contributor

@alrz alrz commented Oct 15, 2023

Fixes #49196

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-grpc Includes: GRPC wire-up, templates label Oct 15, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 15, 2023
@ghost
Copy link

ghost commented Oct 15, 2023

Thanks for your PR, @alrz. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@alrz alrz changed the title Respect "grpc-status-details-bin" trailer on error response (json trascoding) Respect "grpc-status-details-bin" trailer on error response Oct 15, 2023
@alrz alrz marked this pull request as ready for review October 15, 2023 23:29
@ghost
Copy link

ghost commented Oct 26, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 26, 2023
@adityamandaleeka
Copy link
Member

@JamesNK PTAL

@alrz
Copy link
Contributor Author

alrz commented Nov 29, 2023

@JamesNK for review. Thanks.

JamesNK
JamesNK reviewed Nov 29, 2023
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Looks good. There are some changes required before merging. The main one is support for ServerCallContext.ResponseTrailers.

...g/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonTranscodingServerCallContext.cs Outdated Show resolved Hide resolved
...g/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonTranscodingServerCallContext.cs Outdated Show resolved Hide resolved
...nscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/UnaryServerCallHandlerTests.cs Show resolved Hide resolved
...JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonRequestHelpers.cs Outdated Show resolved Hide resolved
alrz
alrz commented Nov 29, 2023
View reviewed changes
...g/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonTranscodingServerCallContext.cs Outdated
@@ -164,7 +168,7 @@ protected override Metadata RequestHeadersCore

protected override CancellationToken CancellationTokenCore => HttpContext.RequestAborted;

protected override Metadata ResponseTrailersCore => throw new NotImplementedException();
protected override Metadata ResponseTrailersCore { get; } = new();
Copy link
Contributor Author

@alrz alrz Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this is supposed to end up in response headers for non-binary entries.

@alrz alrz requested a review from JamesNK November 29, 2023 14:49
@JamesNK JamesNK removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 29, 2023
@JamesNK JamesNK merged commit 61c5858 into dotnet:main Nov 29, 2023
26 checks passed
@ghost ghost added this to the 9.0-preview1 milestone Nov 29, 2023
@JamesNK
Copy link
Member

JamesNK commented Nov 29, 2023

Thanks!

@alrz
Copy link
Contributor Author

alrz commented Dec 1, 2023

@JamesNK I'm not familar with how PRs are triaged here, could this kind of fix possibly make it to a point release?

@ghost
Copy link

ghost commented Dec 1, 2023

Hi @alrz. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@JamesNK
Copy link
Member

JamesNK commented Dec 2, 2023

No, this is a new feature. It will come in .NET 9

@alrz alrz mentioned this pull request Apr 27, 2024
Merged
@JamesNK JamesNK mentioned this pull request Apr 28, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesNK JamesNK JamesNK approved these changes

@captainsafia captainsafia Awaiting requested review from captainsafia captainsafia is a code owner

@mgravell mgravell Awaiting requested review from mgravell mgravell is a code owner

Assignees

@JamesNK JamesNK

Labels
area-grpc Includes: GRPC wire-up, templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
9.0-preview1
Development

Successfully merging this pull request may close these issues.

[gRPC] How to correctly transmit error details to client with gRPC-JSON transcoding?
3 participants
@alrz @adityamandaleeka @JamesNK