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

feat: add expand_slashed_path_patterns flag #4813

Merged

Conversation

czabaj
Copy link
Contributor

@czabaj czabaj commented Oct 9, 2024

References to other Issues or PRs

Closes #4784 - a feature request

Have you read the Contributing Guidelines?

Yes 👍

Brief description of what is fixed or changed

This adds an experimental flag into the OpenAPI generator, that expands path patterns containing URI sub-paths into the URI with new path parameters. This improves the readability and OpenAPI compatibility of protobuf APIs based on Google AIP. For more context read the linked issue.

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.

This is a great start. Could you please make sure to also:

  1. Add this new flag to the bazel rules for the openapiv2 generator. Just follow the pattern for the other flags.
  2. Add a small section to our docs about this new flag with some example use cases? The file you want is https://github.com/grpc-ecosystem/grpc-gateway/blob/main/docs/docs/mapping/customizing_openapi_output.md.

Thanks!

protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
@czabaj
Copy link
Contributor Author

czabaj commented Oct 14, 2024

I created a single clean function that patches the path parts & params only when the flag is turned on. It consumes the parts from the templateToParts function which I reverted to its original shape. While running the test, I encountered a bug in templateToParts, or at least, I think it is a bug.

If you pass into the templateToParts URI template with a path param pattern, that uses a snake-case URI, like

/test/{name=test_cases/*}/

where the path pattern contains snake-kase test_cases_ and the useJSONNamesForFields` is turned on, then this is split into parts

test
name=testCases/*

so the path pattern is camelcased. I believe this is wrong, since only the path parameter names should be camelcased, not other parts of the URI - am I right?

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 believe this is wrong, since only the path parameter names should be camelcased, not other parts of the URI - am I right?

Yeah this looks wrong, thanks for finding it. Can you fix it?

internal/descriptor/registry.go Outdated Show resolved Hide resolved
protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
protoc-gen-openapiv2/internal/genopenapi/template.go Outdated Show resolved Hide resolved
@czabaj czabaj force-pushed the vg/4784-expand-slashed-path-patterns branch from 348d4ea to 9a7d834 Compare October 18, 2024 13:59
@@ -988,30 +998,24 @@ func templateToParts(path string, reg *descriptor.Registry, fields []*descriptor
var parts []string
depth := 0
buffer := ""
jsonBuffer := ""
Copy link
Contributor Author

@czabaj czabaj Oct 18, 2024

Choose a reason for hiding this comment

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

This second jsonBuffer was just a complexity gainer, I added a single branch to the } case, where it extracts the parameter name and camel-case it, when the camel-casing is enabled.

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.

This is really cool, as a final request, do you think you could add a little blurb about this setting to our docs? This page: https://github.com/grpc-ecosystem/grpc-gateway/blob/main/docs/docs/mapping/customizing_openapi_output.md

@johanbrandhorst
Copy link
Collaborator

Looks like there was a test failure, I think it's from the camelcase fix you made.

@johanbrandhorst johanbrandhorst merged commit 03453e3 into grpc-ecosystem:main Oct 23, 2024
17 checks passed
@johanbrandhorst
Copy link
Collaborator

Thank you for your contribution!

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.

Openapi generator - explode the AIP style path parameters into URL with multiple path parameters
2 participants