fix(gengateway): use opaque chain for setting path params#6215
Conversation
kellen-miller
changed the title
kellen-miller
changed the title
kellen-miller
changed the title
johanbrandhorst
left a comment
johanbrandhorst
left a comment
There was a problem hiding this comment.
Thanks for this, could you please add an example that uses this logic to some of our test files, it if doesn't already have it? See internal/examples/proto.
kellen-miller
force-pushed
the
fix-opaqueapi-patch-enum
branch
3 times, most recently
from
b0b8e0d to
50a2e39
Compare
|
@johanbrandhorst PR should be good to review now. Added an example rpc that the PR is fixing (somewhat contrived). The getter chains in the gateway file are what is changing: |
johanbrandhorst
left a comment
johanbrandhorst
left a comment
There was a problem hiding this comment.
Great, thanks for the fix!
|
Looks like you'll need to regenerate the protobuf files with that change again, if you're unsure, check CONTRIBUTING.md for the steps. Thanks! |
|
@johanbrandhorst addressed the CI issues, do we want to commit the updated |
kellen-miller
changed the title
johanbrandhorst
left a comment
johanbrandhorst
left a comment
There was a problem hiding this comment.
Unfortuantely another PR snuck in before yours so you'll need to regenerate the files again after rebasing on main 😅. Sorry for the inconvenience!
Lets see what the |
|
Looks like |
[WHY] Opaque-enabled protos expose nested fields only through getter/setter chains, so generated Setkey.* calls failed to compile yet path params remained unset. [WHAT] Added FieldPath helpers to build setter chains, swapped template setter sites to the helper, and codified the behavior with unit tests. [HOW] Build the accessor expression once from the descriptor so every opaque path-param assignment reuses it. [TEST] go test ./internal/descriptor; go test ./protoc-gen-grpc-gateway/internal/gengateway [RISK] low; changes affect only opaque path-param generation paths. [RAG] finxact_engineering_domain_customerv1poc_proto::module::buf|1.30,finxact_engineering_domain_customerv1poc_proto::module::finxact_type|1.59,finxact_engineering_domain_customerv1poc_gen_go::repo_overview|1.70
[WHY] Prevent regressions for the nested enum path-param case that triggered the bug. [WHAT] Add a generator test that synthesizes a proto3 file with an opaque nested enum binding and asserts the emitted setter chain. [HOW] Load the fake file into the descriptor registry so LookupEnum succeeds and search for the expected SetKind expression. [TEST] go test ./protoc-gen-grpc-gateway/internal/gengateway [RISK] none; test-only change [RAG] finxact_engineering_domain_customerv1poc_proto::module::buf|1.30,finxact_engineering_domain_customerv1poc_proto::module::finxact_type|1.59,finxact_engineering_domain_customerv1poc_gen_go::repo_overview|1.70
[WHY] The opaque setter bug isn’t limited to enums or PATCH bindings, so tests should cover both scalar and enum parameters across HTTP methods. [WHAT] Simplified the generator test to synthesize a proto with top-level path params and assert the opaque setter is emitted for GET (string) and PATCH (enum). [HOW] Build descriptors directly, cross-link them once, and verify the generated code contains the expected setter expression for each case. [TEST] go test ./protoc-gen-grpc-gateway/internal/gengateway [RISK] none; test-only change [RAG] finxact_engineering_domain_customerv1poc_proto::module::buf|1.30,finxact_engineering_domain_customerv1poc_proto::module::finxact_type|1.59,finxact_engineering_domain_customerv1poc_gen_go::repo_overview|1.70
kellen-miller
force-pushed
the
fix-opaqueapi-patch-enum
branch
from
4f84f87 to
39715cb
Compare
|
@johanbrandhorst rebased and regenerated, hopefully good to go now! |
|
Thanks for your contribution! |
2 participants
Summary
Testing