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 routing errors handler #1517

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

octo47
Copy link
Contributor

@octo47 octo47 commented Jul 13, 2020

This pull request adds routing error handler to address issue #1513

gRPC Gateway v2 had new error handling infrastructure added.
As a side effect of unified error handling all errors now handled as gRPC errors by standardised handler ErrorHandlerFunc.
As it works very well for gRPC errors, for cases when routing errors happen generalised error handler making some assumptions and can change expected http behaviour:

  1. If no routing found gRPC error NotFound will be returned, so no way to distinguish real no handler situation from grpc method returning NotFound
  2. If unexpected method requested for known handler Unimplemented grpc status returned, after conversions response will have http status code 501 (StatusNotImplemented) instead of expected 405 (StatusMethodNotAllowed) due of http -> grpc -> http status codes conversion.

To fix this PR adds additional error handler: routing error handler. Routing error handler is only called for errors related to routing problems:

  • No handler -> http status NotFound
  • Handler found but incorrect method -> MethodNotAllowed
  • URI has incorrect format -> BadRequst

By default with no routing handler installed current behaviour will be preserved and errors will be converted to gRPC errors with following default error handler call.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@octo47
Copy link
Contributor Author

octo47 commented Jul 13, 2020

@googlebot I fixed it.

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.

This looks great! Could I just ask that you add a little blurb to the migration guide (https://github.com/grpc-ecosystem/grpc-gateway/blob/v2/docs/_docs/v2-migration.md) and the error handling documentation (https://github.com/grpc-ecosystem/grpc-gateway/blob/v2/docs/_docs/customizingyourgateway.md#error-handler) to tell users about this new handler? Thanks!

@octo47
Copy link
Contributor Author

octo47 commented Jul 13, 2020

@johanbrandhorst
Updated docs!

For some reason google bot doesn't believe me I have a CLA, is it requires some time to get synced?

@octo47
Copy link
Contributor Author

octo47 commented Jul 13, 2020

@googlebot I fixed it.

@johanbrandhorst
Copy link
Collaborator

The issue with the CLA bot is that your commits are made from a different email (or account) than the account you've used here. You can see it in the commit log. Could you see if you can push some new commits with the correct email?

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.

Some small wording corrections, thanks for the docs additions!

docs/_docs/customizingyourgateway.md Outdated Show resolved Hide resolved
docs/_docs/customizingyourgateway.md Outdated Show resolved Hide resolved
docs/_docs/customizingyourgateway.md Outdated Show resolved Hide resolved
docs/_docs/customizingyourgateway.md Outdated Show resolved Hide resolved
docs/_docs/v2-migration.md Outdated Show resolved Hide resolved
@octo47
Copy link
Contributor Author

octo47 commented Jul 13, 2020

@johanbrandhorst thanks for suggestions! applied

Co-authored-by: Johan Brandhorst <johan.brandhorst@gmail.com>
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@johanbrandhorst
Copy link
Collaborator

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@johanbrandhorst johanbrandhorst merged commit 050e889 into grpc-ecosystem:v2 Jul 13, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

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.

3 participants