With v2 we should be able to detect real NotFound responses from gRPC service NotFound responses

[octo47]

🚀 Feature

With new unified error handler in v2 it is now not possible to detect if grpc-gateway has no uri registered or gRPC method returned real NotFound.

Proposal:
Add option grpcgwruntime.WithNotFoundHandler(errorHandler) so user can handle code correctly.

Current Behaviour is misleading and can cause issues:

• Code returns NotFound which may clash with gRPC method returning NotFound
• In replacement of MethodNotAllowed gRPC gateway returns codes.Unimplemented which in turn converted to a different http status code: 501 instead of 405

[johanbrandhorst]

This sounds like a good idea, I did wrestle with the issue of how to represent nonexistent URIs in the new error handling, and I think it makes sense to split it out of the existing handling altogether. Another thing that v1 did is differentiate between ErrUnknownURI and ErrMethodNotAllowed (e.g. sending GET to a POST endpoint). We might want to weave that into this new handler too, maybe we should make it more generic, how about WithHTTPErrorHandler or something that makes it obvious that it is pre any gRPC concerns?

CC @jhump who I know had some comments on the new error handling in v2 as well.

[octo47]

@johanbrandhorst
Thanks for commenting!

Right, WithHTTPErrorHandler should be event better and cleaner way to handle pre-gRPC errors!

[johanbrandhorst]

Sounds good, what can I do to help you contribute this change? I would start looking at how the existing error handler is defined and used which should be easy enough to adapt to this new handler. We'll also need to update the v2 migration guide, which already has a section on error handling.

[octo47]

I believe I can come up with PR in a few days and evaluate it on our codebase to see how it may fit.
Thanks for quick response!

[johanbrandhorst]

Fixed with #1517