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

add HTTP error helper #337

Merged
merged 3 commits into from
Aug 29, 2022
Merged

add HTTP error helper #337

merged 3 commits into from
Aug 29, 2022

Conversation

rhbuf
Copy link
Contributor

@rhbuf rhbuf commented Aug 2, 2022

The mapping from connect error codes to HTTP status codes is currently private and not accessible to users who are implementing connect in their existing HTTP servers. This helper has been added to allow those users to easily update existing HTTP middlewares to handle connect errors.

fixes #331

The mapping from connect error codes to HTTP status codes is currently private and not accessible to users who are implementing connect in their existing HTTP servers. This helper has been added to allow those users to easily update existing HTTP middlewares to handle connect errors.
@rhbuf rhbuf requested review from pkwarren and akshayjshah August 2, 2022 11:30
http_util.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pkwarren pkwarren 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 - just a couple minor things.

http_util_test.go Outdated Show resolved Hide resolved
http_util.go Outdated Show resolved Hide resolved
http_util.go Outdated Show resolved Hide resolved
@akshayjshah
Copy link
Member

akshayjshah commented Aug 2, 2022

Awesome, I'm glad you've got some bandwidth to take this on @rhbuf! This is a great start.

Unfortunately, this function only returns valid responses for unary requests using the Connect protocol. If we're going to ship this, we shouldn't make users figure out the protocol in use, the compression scheme, etc - I think we should respond appropriately to unary Connect, streaming Connect, gRPC, and gRPC-Web requests, so that RPC clients get full-fidelity error codes, messages, and details. Doing that requires access to the request headers.

@rhbuf
Copy link
Contributor Author

rhbuf commented Aug 5, 2022

Awesome, I'm glad you've got some bandwidth to take this on @rhbuf! This is a great start.

Unfortunately, this function only returns valid responses for unary requests using the Connect protocol. If we're going to ship this, we shouldn't make users figure out the protocol in use, the compression scheme, etc - I think we should respond appropriately to unary Connect, streaming Connect, gRPC, and gRPC-Web requests, so that RPC clients get full-fidelity error codes, messages, and details. Doing that requires access to the request headers.

Roger that, I will come back in a week or so and flesh this out. I've switched directions in my current work and no longer needs this HTTP helper function, but I want to finish what I've started when I have some time.

@akshayjshah
Copy link
Member

I've switched directions in my current work and no longer needs this HTTP helper function, but I want to finish what I've started when I have some time.

Don't sweat it! This isn't a "you touch it, you buy it" system :) I'll try to take over and finish up; if I don't get to it and you're free, feel free to pick it up again.

Amend the error writing helper to make it protocol-aware. The new
ErrorWriter struct accepts HandlerOptions, so that it's fully aware of
the supported Codecs and RPC protocols, and it supports gRPC, gRPC-Web,
and Connect clients.
@akshayjshah akshayjshah marked this pull request as ready for review August 29, 2022 05:44
@akshayjshah akshayjshah requested a review from pkwarren August 29, 2022 05:45
@akshayjshah
Copy link
Member

@rhbuf and @pkwarren, I took a shot at finishing this up - let me know what you think!

@akshayjshah akshayjshah merged commit edbc6ba into main Aug 29, 2022
@akshayjshah akshayjshah deleted the rhenderson/error-helper branch August 29, 2022 16:33
@mattrobenolt
Copy link
Contributor

🙏 thanks all, this is what I've been waiting for as well.

akshayjshah added a commit that referenced this pull request Jul 26, 2023
Add a protocol-aware helper type to write RPC errors from net/http middleware. 
The new ErrorWriter struct accepts HandlerOptions, so that it's fully aware of
the supported Codecs and RPC protocols, and it supports gRPC, gRPC-Web,
and Connect clients.

Co-authored-by: Akshay Shah <[email protected]>
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.

Provide function to write RPC errors from HTTP middleware
4 participants