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

use error value for bad URI so custom error handler could treat it special #932

Merged

Conversation

jhump
Copy link
Contributor

@jhump jhump commented May 10, 2019

When a custom ProtoErrorHandlerFunc is used, it is given an error with Unimplemented gRPC code for cases where the server gets a request for an unrecognized URI path.

When no such handler is in place, it defaults to a "404 Not Found" error, which is what basically any REST client actually expect for such an error.

Unfortunately, there is not currently any way to distinguish between "the server received an unrecognized URI path" and an actual "Unimplemented" error returned by a gRPC server handler. So it is not possible for a custom handler to turn those into "404 NotFound" errors, as a REST client would actually expect.

This change hoists the error out into an exported error value: ErrUnknownURI. That way, a custom error handler can check if err == runtime.ErrUnknownURI and choose to return a 404 for those errors (instead of "501 Not Implemented", which can be used for other errors with an "Unimplemented" gRPC code).

@codecov-io
Copy link

codecov-io commented May 10, 2019

Codecov Report

Merging #932 into master will increase coverage by 0.28%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #932      +/-   ##
==========================================
+ Coverage   53.53%   53.81%   +0.28%     
==========================================
  Files          40       40              
  Lines        3964     3954      -10     
==========================================
+ Hits         2122     2128       +6     
+ Misses       1645     1630      -15     
+ Partials      197      196       -1
Impacted Files Coverage Δ
runtime/mux.go 54.16% <33.33%> (+9.28%) ⬆️
runtime/proto_errors.go 0% <0%> (-34.89%) ⬇️
runtime/errors.go 47.36% <0%> (+2.63%) ⬆️
runtime/handler.go 40.62% <0%> (+5.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f005bec...fcb8708. Read the comment docs.

@achew22
Copy link
Collaborator

achew22 commented May 10, 2019

This seems eminently reasonable. Would you be willing to add a test or two to confirm the differentiation between a 404 not found and a 404 unimplemented?

@jhump
Copy link
Contributor Author

jhump commented May 10, 2019

@achew22, I added tests. PTAL.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Thanks!

@johanbrandhorst johanbrandhorst merged commit 4ff41ee into grpc-ecosystem:master May 13, 2019
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
…ecial (grpc-ecosystem#932)

* use error value for bad URI; so custom error handler could treat it special

* add test for ErrUnknownURI being distinct from other 'unimplemented' errors

* use status.Convert instead of status.FromError
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants