Grpc-gateway: URLs using verb no longer work after upgrading to v1.5.0

Created on 1 Oct 2018  路  3Comments  路  Source: grpc-ecosystem/grpc-gateway

We tested out the latest 1.5.0 release and noticed it broke our URLs that use colons with a custom verb, like described here: https://cloud.google.com/apis/design/custom_methods

In our case we have a both a "Get" method and a custom method sharing parts of the URL, like this:

  rpc GetDevice(GetDeviceRequest) returns (Device) {
    option (google.api.http) = { get: "/v2/{name=projects/*/devices/*}" };
  }
  rpc StreamDevices(StreamDevicesRequest) returns (stream StreamDevicesResponse) {
      option (google.api.http) = { get: "/v2/{name=projects/*/devices/*}:stream" };
   }

When upgrading from v1.4.1 to v1.5.0 requests no longer seem to reach the StreamDevices method.

Steps you follow to reproduce the error:

By extending the TestMuxServeHTTP test with the following test-case, I'm able to trigger the same issue:

{
            patterns: []stubPattern{
                {
                    method: "GET",
                    ops:    []int{int(utilities.OpLitPush), 0, int(utilities.OpPush), 0, int(utilities.OpConcatN), 1, int(utilities.OpCapture), 1},
                    pool:   []string{"foo", "id"},
                },
                {
                    method: "GET",
                    ops:    []int{int(utilities.OpLitPush), 0, int(utilities.OpPush), 0, int(utilities.OpConcatN), 1, int(utilities.OpCapture), 1},
                    pool:   []string{"foo", "id"},
                    verb:   "verb",
                },
            },
            reqMethod: "GET",
            reqPath:   "/foo/bar:verb",
            headers: map[string]string{
                "Content-Type": "application/json",
            },
            respStatus:  http.StatusOK,
            respContent: "GET /foo/{id=*}:verb",
        },

What did you expect to happen instead:

I would expect the test to reach the /foo/{id=*}:verb pattern, or in our case reach the StreamDevices method as in the previous releases.

What's your theory on why it isn't working:

It seems to be a side-effect of fixing #224.

bug

Most helpful comment

Yeah, backwards compatibility is paramount here, of course. It also seems like the _wrong thing_ to support this tbh, I'm just disappointed we didn't discover it earlier and didn't break users. I'll see what some of the others think but we'll likely just revert #708 and continue the discussion in #224.

All 3 comments

Argh, thanks for raising this issue! Terribly sorry to have broken your workflow. It looks like we have to revert https://github.com/grpc-ecosystem/grpc-gateway/pull/708, is that correct?

One solution could be to introduce a ServeMuxOption to allow Patterns created without a verb to match requests using a verb. This way, using colons in IDs is an opt-in feature instead of breaking backward compatibility.

Yeah, backwards compatibility is paramount here, of course. It also seems like the _wrong thing_ to support this tbh, I'm just disappointed we didn't discover it earlier and didn't break users. I'll see what some of the others think but we'll likely just revert #708 and continue the discussion in #224.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

pzsfeng picture pzsfeng  路  5Comments

rexlv picture rexlv  路  3Comments

tamalsaha picture tamalsaha  路  3Comments

Aleksion picture Aleksion  路  3Comments

yearm picture yearm  路  3Comments