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.
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",
},
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.
It seems to be a side-effect of fixing #224.
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.
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.