"GET" route matches both GET & POST requests?

[uynap]

I found a "get" route will match both HTTP GET and HTTP POST requests. For example:

rpc aaa(User) returns (User) {
  option (google.api.http) = {
  get: "/api/v1/aaa"
};

It matches both curl -v -X GET -k https://127.0.0.1/api/v1/aaa and curl -v -X POST -k https://127.0.0.1/api/v1/aaa.

I was wondering if it's possible to strictly match all routes including methods?

[achew22]

Hrm, that's really unfortunate. Do you think you could put together a PR with a test case in our integration tests to demonstrate this error?

[UladzimirTrehubenka]

From https://stackoverflow.com/questions/45901389/why-golang-grpc-gateways-get-route-matches-post-request

By spec, it is allowed to encode GET request into a POST method with Content-Type: application/x-www-url-encoded.

See https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/mux.go#L254

So the request routing table in grpc-gateway tries to fallback from POST method to GET when the Content-Type of your request is application/x-www-url-encoded.

From https://groups.google.com/d/msg/grpc-io/Xqx80hG0D44/1gwmwBcnNScJ

(Note) URL has a length limitation (go/longer-urls). It's enfoced by some browsers and
proxies. If your GET request exceeds the limitation, browser may reject to send them.
You may change to use a POST request with content type x-www-form-urlencoded instead.

Another one example:

rpc Read(ReadReq) returns (ReadResp) {
  option (google.api.http) = {
  get: "/api/v1/user/{id}"
};

rpc Create(CreateReq) returns (CreateResp) {
  option (google.api.http) = {
  post: "/api/v1/user"
};

curl -X POST http://127.0.0.1:8040/api/v1/user/127 -d '{}'
200 OK ("Content-Type: application/x-www-url-encoded" is in header and POST becomes GET, actually we have no route POST "/api/v1/user/{id}" at all)

curl -X POST http://127.0.0.1:8040/api/v1/user/127
501 NOT_IMPLEMENTED (expected behaviour)

The question is - how to disable this behaviour? This is weird that any "Content-Type" causes expected error 501 NOT_IMPLEMENTED except "application/x-www-url-encoded".

[johanbrandhorst]

@UladzimirTrehubenka thanks for that incredibly interesting summary of the issue, but I'm not sure I understand what needs to be done. Should we implement an option in the generator/mix to disable this behaviour?

[UladzimirTrehubenka]

Separate option would be good enough. I just wondering do we have some workaround right now?

[johanbrandhorst]

Not as far as I know. So would this be a runtime option or a generator option?

[UladzimirTrehubenka]

I guess generator option, BTW could we have some global option in proto to disable "application/x-www-url-encoded" for all POST calls? Probably if we will have a lot of POST calls it will be not so useful to disable "application/x-www-url-encoded" for each call explicitly?

[johanbrandhorst]

Hm, I made a simple string search for x-www-url-encoded and couldn't find it used anywhere, are you sure the grpc-gateway implements this handling?

[UladzimirTrehubenka]

See https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/mux.go#L254

[johanbrandhorst]

Thanks for pointing that out, seeing as it is tied to the mux I think we can make this a runtime option.

[johanbrandhorst]

Would you be interested in submitting a patch for this @UladzimirTrehubenka?

[UladzimirTrehubenka]

@yugui could you please provide some comments?

[johanbrandhorst]

I think it should be fairly straightforward, add an option that sets a variable, then check the value of the variable before handling the fallback.

[UladzimirTrehubenka]

Any progress here?

[johanbrandhorst]

Unfortunately none of the maintainers are able to dedicate any work to this right now, what can I do to help you get a pull request fix for this?

[UladzimirTrehubenka]

add an option that sets a variable

Don't follow - where we need to set the option?

[johanbrandhorst]

Something like this: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/mux.go#L99. Except WithDisablePathLengthFallback. It could set a boolean on the Mux type, which we can check in the function call you found.

[UladzimirTrehubenka]

As I understand you propose to add another one ServeMuxOption and set the override behaviour at runtime?

[johanbrandhorst]

Yes, it seems suitable as a runtime option, not something that's specific to your API. Do you disagree?

[UladzimirTrehubenka]

I like this solution, so I try to create PR for fix this.

[UladzimirTrehubenka]

The PR has been added.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[UladzimirTrehubenka]

The issue was fixed.