Populating query param semantics changed since v2.15.0

[roehrijn]

🐛 Bug Report

When dealing with nested messages having same field name like the field name of nested message itself in its parent, the population of HTTP Query parameters seems to be broken since v2.15.0.

Example:

    message Identity {
      string id = 1;
      string foo = 2;
    }
    
    message Entity {
      Identity id = 1;
    }
    
    rpc Echo(Entity) returns (Entity) {
      option (google.api.http) = {
        get: "/{id.id}"
      };
    }

For the sake of simplicity the implementation of this endpoint just returns the message it received.

Expected behavior

When I query the endpoint using a HTTP GET like /someid?id.foo=myqueryvalue I would expect id.foo in the resulting response message to be set to myqueryvalue, like it was the case prior to v2.15.0

Actual Behavior

The field id.foo isn't populated and remains empty.

To Reproduce

I forked the repo and created an integration test showing exactly this behavior: roehrijn@012a9d9

Your Environment

Discovered this issue with v2.15.2 but main is affected since v2.15.0.

Additional comments

Most properly the behavior has been introduced with #3072. I tracked it further down the following lines:
https://github.com/grpc-ecosystem/grpc-gateway/pull/3072/files#diff-9648d8792ce733c8bbce0d613a620fffdb73cff98a91763507a1984eb520d5feR80-R82

Looks like the added awareness of JSON names breaks the query param filtering behavior when using nested fields with colliding names. IMHO in the example case only id.id should be excluded from being a possible query param field name but actually id.* is too because the JSON name of the field in the nested message is not set and thus, just the field name is used.

When changing definition of the Identity message to:

    message Identity {
      string id = 1 [json_name = "somethingElseThanId"];
      string foo = 2;
    }

the particular issue is resolved and the created test turns green. However, this obviously breaks the JSON representation of the messages when using protojson or jsonpb.

_{Jan Roehrich [email protected], Mercedes-Benz Tech Innovation GmbH, legal info/Impressum}

[johanbrandhorst]

Hi Jan, thank you so much for this detailed issue. First of all let me express my apologies for this. Secondly, I'm extremely impressed with the lengths you've gone to, both to create a test to reproduce the issue and to drill down and discover the cause. Bravo! Would you be interested in submitting a fix for this? I'm ready to revert the PR that introduced the issue of course but if you intend to submit a fix we can avoid it. Thanks!

[roehrijn]

Hi Johan. You're welcome. I want this to be fixed and thus, I'm doing my best to support wherever I can. However there are two aspects which hinder me in providing a fix:

1. I have no glue about the very complex internal data model so far. I couldn't figure out how to create unit test in protoc-gen-grpc-gateway/internal/gengateway/template_test.go for this issue because there are some examples with nested items in repo, but they all work with HTTP POSTs and I wasn't able to turn them into working examples for GET.
2. I have not yet a real idea how to fix the issue. Somehow all parents of nested targets and all their JSON names need to be taken into consideration I guess. But again, currently I have no overview of the data model and what would be possible with the amount of metadata available at this point.

However, I'm eager to help and if somebody could assist by providing a test in the context of 1), it could give me a big knowledge-boost in that area. Maybe enough to fix that issue.

[johanbrandhorst]

Thanks for getting back to me. I think the safest thing for us to do is simply revert the PR that introduced the regression for now, then consider a new implementation that isn't subject to this problem. I'll take care of that.