fix(tonic): Preserve HTTP method in interceptor #912
Conversation
| let mut request = request.into_http( | ||
| uri, | ||
| http::Method::POST, | ||
| http::Version::HTTP_2, | ||
| SanitizeHeaders::Yes, | ||
| ); |
There was a problem hiding this comment.
Im not following why these values are not just hard coded intointo_http since they should always be POST and HTTP2 and can be overridden outside (aka for grpc-web).
There was a problem hiding this comment.
Thanks for taking the time to review! Firstly, see my response on issue #911 to make sure we are on the same page.
If Rust supported default parameters, then I would propose making POST and HTTP2 defaults for Request::into_http.
But one issue here is that Request::from_http will happily accept HTTP requests of any version and method, silently dropping that information and creating subtle bugs like this one. By adding method and version to Request::into_http, we are making it more explicit that method and version are not preserved.
In any case, Request::into_http only has two callers currently, and it is restricted to the crate, so the stakes aren't very high here. I could leave grpc.rs alone and instead write a new method Request::from_http_with_custom_method if you think that would be cleaner.
There was a problem hiding this comment.
Yup, thanks for writing that up it all makes sense!
|
looks like a fmt failure is causing ci to not pass here |
|
I fixed the format issue, and I rebased to fix the minor conflict with #842 . |
This reverts commit b971c54.
|
Whoops, not sure what happened there; during rebasing, the fmt check was fixed in the initial commit, the next commit added a fat-fingered "g" breaking the compile, and I omitted a |
Testing out hyperium/tonic#912
Motivation
See Issue #911
Solution
The function for converting a Tonic request into an HTTP request,
Request::into_http, is public only to the crate and only used in a few places. Currently, HTTP2 and POST are hardcoded for HTTP method and version. Thus, we can simply addmethodandversionparams and update the callers, and then fix the interceptor to preserve them.