Fix 2494#3646
Conversation
WalkthroughThe 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 Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
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_declnamed "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.
|
Hey @johanbrandhorst , Would you please look into this PR ? |
johanbrandhorst
changed the title
johanbrandhorst
left a comment
johanbrandhorst
left a comment
There was a problem hiding this comment.
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 |
| schema: | ||
| $ref: '#/definitions/StringMessage' | ||
| parameters: | ||
| - name: uuid |
There was a problem hiding this comment.
without this fix Id would also come up here, it's not now because uuid is of type one_of
|
any change this get merge? @johanbrandhorst it's super useful |
johanbrandhorst
left a comment
johanbrandhorst
left a comment
There was a problem hiding this comment.
Don't know how I forgot about this PR, but it looks reasonable to me.
|
I'm going to close and reopen this to re-trigger the CI |
|
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. |
|
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
left a comment
johanbrandhorst
left a comment
There was a problem hiding this comment.
I think there's still a bug here
| { | ||
| "name": "status.progress", | ||
| "in": "query", | ||
| "required": false, | ||
| "type": "string", | ||
| "format": "int64" | ||
| }, | ||
| { | ||
| "name": "status.note", | ||
| "in": "query", | ||
| "required": false, | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
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:
. I think it's removing all oneofs instead of just thelineNum oneof.
4 participants
Fixes #2494