Fix 2494 #3646

amanraja · 2023-10-06T08:11:49Z

coderabbitai · 2023-10-06T08:11:53Z

Walkthrough

The changes introduced add a new check in the template to ensure compatibility between path and query parameters. Additionally, updates to the proto file include a oneof_decl named "primary_key" and two new fields, enhancing the service options for HTTP GET requests.

Changes

File	Summary
.../genopenapi/template.go	Introduced a check to prevent conflicting field types from being used together.
.../path_item_object.prototext	Added a `oneof_decl` named "primary_key", two new fields "uuid" and "id", and additional bindings for HTTP GET requests.

🐇💻

In the land of code, where logic is king,

A rabbit hops forth, changes to bring.

With each line written, with each test run,

The software evolves, the work is done.

So let's celebrate this commit, so neat,

For every bug squashed, is a feat! 🎉🥕

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

Mention @coderabbitai in any review comment for bot assistance.
Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
resume reviews, enter @coderabbitai resume as any issue or review comment.

coderabbitai

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between d271d84 and b1ee40f.

Files ignored due to filter (1)

protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.swagger.yaml

Files selected for processing (2)

protoc-gen-openapiv2/internal/genopenapi/template.go (1 hunks)
protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.prototext (2 hunks)

Files skipped from review due to trivial changes (1)

protoc-gen-openapiv2/internal/genopenapi/template.go

Additional comments (Suppressed): 2

protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.prototext (2)

14-32: The oneof_decl named "primary_key" and the fields "uuid" and "id" have been added to the message type. Ensure that these changes are reflected in the corresponding service methods and their usage throughout the codebase.

40-50: Additional HTTP GET bindings have been added for the fields "value", "uuid", and "id". Make sure that the server-side implementation can handle these new routes correctly, and that they do not conflict with existing routes.

amanraja · 2023-10-08T13:18:36Z

Hey @johanbrandhorst , Would you please look into this PR ?

johanbrandhorst

Hi! Thanks for this PR. The change looks reasonable, but I'm not sure I understand what the testdata changes do. Could you add a test to template_test.go? Or perhaps add an example of this circumstance to one of the test protobuf files and regenerate the files? Thanks!

amanraja · 2023-10-09T08:59:51Z

Hi! Thanks for this PR. The change looks reasonable, but I'm not sure I understand what the testdata changes do. Could you add a test to template_test.go? Or perhaps add an example of this circumstance to one of the test protobuf files and regenerate the files? Thanks!

Hey @johanbrandhorst , sure i will add tests for template_test.go, i made changes to testdata file to show the issue that we are addressing, hope that would suffice for us ?

amanraja · 2023-10-09T09:01:55Z

protoc-gen-openapiv2/internal/genopenapi/testdata/generator/path_item_object.swagger.yaml

+          schema:
+            $ref: '#/definitions/StringMessage'
+      parameters:
+        - name: uuid


without this fix Id would also come up here, it's not now because uuid is of type one_of

zfy0701 · 2024-11-08T17:20:10Z

any change this get merge? @johanbrandhorst it's super useful

johanbrandhorst

Don't know how I forgot about this PR, but it looks reasonable to me.

johanbrandhorst · 2024-11-09T05:22:56Z

I'm going to close and reopen this to re-trigger the CI

johanbrandhorst · 2024-11-09T05:29:16Z

Looks like we still need to rerun the generation script (see CONTRIBUTING.md). If the original author @amanraja cannot pick this back up, feel free to copy the changes to a new PR.

amanraja · 2024-11-09T18:37:10Z

Hey @johanbrandhorst , have ran the generation script now, Please check the PR again , there are few changes changes have come up because of it.

johanbrandhorst

I think there's still a bug here

johanbrandhorst · 2024-11-10T17:51:42Z

examples/internal/proto/examplepb/echo_service.swagger.json

-          {
-            "name": "status.progress",
-            "in": "query",
-            "required": false,
-            "type": "string",
-            "format": "int64"
-          },
-          {
-            "name": "status.note",
-            "in": "query",
-            "required": false,
-            "type": "string"
-          },


I think these were removed incorrectly, no? There's nothing in this RPC definition that means the status query parameters shouldn't show up, I think? Here's the definition:

grpc-gateway/examples/internal/proto/examplepb/echo_service.proto

Line 68 in 442bd3a

additional_bindings: {get: "/v1/example/echo/{id}/{num}/{lang}"}

. I think it's removing all oneofs instead of just the lineNum oneof.

fix: added fix for grpc-ecosystem#2494

b1ee40f

coderabbitai bot reviewed Oct 6, 2023

View reviewed changes

johanbrandhorst changed the title ~~fix: added fix for https://github.com/grpc-ecosystem/grpc-gateway/iss…~~ Fix 2494 Oct 9, 2023

johanbrandhorst requested changes Oct 9, 2023

View reviewed changes

amanraja commented Oct 9, 2023

View reviewed changes

johanbrandhorst approved these changes Nov 9, 2024

View reviewed changes

johanbrandhorst closed this Nov 9, 2024

johanbrandhorst reopened this Nov 9, 2024

amanraja added 3 commits November 9, 2024 23:20

fix: regenerate the files

fdd5488

Merge branch 'main' into main

f3d3ce3

fix: conflict

f0b5ac7

johanbrandhorst requested changes Nov 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 2494 #3646

Fix 2494 #3646

amanraja commented Oct 6, 2023 •

edited by johanbrandhorst

Loading

coderabbitai bot commented Oct 6, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

Pause Incremental Reviews

coderabbitai bot left a comment

amanraja commented Oct 8, 2023

johanbrandhorst left a comment

amanraja commented Oct 9, 2023

amanraja Oct 9, 2023

zfy0701 commented Nov 8, 2024

johanbrandhorst left a comment

johanbrandhorst commented Nov 9, 2024

johanbrandhorst commented Nov 9, 2024

amanraja commented Nov 9, 2024

johanbrandhorst left a comment

johanbrandhorst Nov 10, 2024

Fix 2494 #3646

Are you sure you want to change the base?

Fix 2494 #3646

Conversation

amanraja commented Oct 6, 2023 • edited by johanbrandhorst Loading

coderabbitai bot commented Oct 6, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

Pause Incremental Reviews

coderabbitai bot left a comment

Choose a reason for hiding this comment

amanraja commented Oct 8, 2023

johanbrandhorst left a comment

Choose a reason for hiding this comment

amanraja commented Oct 9, 2023

amanraja Oct 9, 2023

Choose a reason for hiding this comment

zfy0701 commented Nov 8, 2024

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst commented Nov 9, 2024

johanbrandhorst commented Nov 9, 2024

amanraja commented Nov 9, 2024

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst Nov 10, 2024

Choose a reason for hiding this comment

amanraja commented Oct 6, 2023 •

edited by johanbrandhorst

Loading

coderabbitai bot commented Oct 6, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)