Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly expand path variables for Update Methods. #5041

Conversation

nullaus
Copy link
Contributor

@nullaus nullaus commented Dec 9, 2024

Refer to AIP-134 for more information.

Update Methods require the resource type name as the path parameter instead of a simple name (e.g. {vendor_contact.name=vendor_contacts/*} instead of {name=vendor_contacts/*})

This would case the following error to occur when generating an OpenAPIv2 specification:

panic: Path parameter "vendorContact.name" not found in path parameters

goroutine 1 [running]:
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2/internal/genopenapi.expandPathPatterns({0x14000798600?, 0x33?, 0x14000001b00?}, {0x14000b6a660?, 0x4?, 0x4?}, 0x14000001b00)

This was due to templateToParts applying the GetUseJSONNamesForFields flag and expandPathPatterns not factoring in JSON names when finding the path parameter by name in the slice of path parameters. It must look for the path parameter by JSON name.

Brief description of what is fixed or changed

I modified the find function to compare using JSON names when appropriate by looking at the UseJSONNamesForFields flag.

Also fixed are minor linter complaints when using go 1.23. I can omit these if preferred.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nullaus
Copy link
Contributor Author

nullaus commented Dec 9, 2024

Thank you for the ⚡ fast review @johanbrandhorst!

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually surprised this passed without any generation changes. Do you think you could add a test to template_test that exercises this? Alternatively, add an annotation somewhere in one of our protobuf files that would exercise this. I think we should have one with user_json_names_for_fields set as an option. Thanks!

Comment on lines +4163 to +4164
{"/test/{test_type.name=test_cases/*}/", "/test/test_cases/{testCase}/", getParameters([]string{"test_type.name"}), []string{"testCase"}, true},
{"/test/{test_type.name=test_cases/*}/", "/test/test_cases/{test_case}/", getParameters([]string{"test_type.name"}), []string{"test_case"}, false},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johanbrandhorst Here are the test cases that exercise this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp I missed it in the cosmetic changes, thanks for pointing it out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! Thanks again for your responsiveness. :)

@johanbrandhorst johanbrandhorst merged commit 84a432e into grpc-ecosystem:main Dec 9, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants