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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

czabaj
Copy link

@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:/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
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
Comment on lines 1169 to 1170
// The modified pathParts. Also mutates the pathParams slice.
func expandPathPatterns(pathParts []string, pathParams *[]descriptor.Parameter) []string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realized that we have a package for parsing these paths already: https:/grpc-ecosystem/grpc-gateway/blob/main/internal/httprule. What do you think about reusing this?

Copy link
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.

I tried that package but unfortunately, it doesn't fit as a replacement for the templateToParts function, I encountered these problems

  • the templateToParts split the template by slashes, so a template with a leading slash creates an empty string at the start of the output slice, trailing slash in the template creates an empty string at the end of the slice - I can go around this, but httprule.Parse do not even accept templates without trailing slash, which broke some tests - I guess that all templates should generally be with trailing slash, but I'm not certain.
  • the httprule.Parse ignores and omits the path patterns, these are completely lost in the Compile output object, this was a blocker,
  • the Compile.Pool contains template segments, but the order is not guaranteed, I encountered a test with a path param where the order of the segments was different than in the original template, this is hard to resolve,

Overall, httprule.Parse does a lot of stuff, but most of it is not useful for the templateToParts, and the important stuff is missing and hard to add. At least, I managed to simplify the templateToParts and fix the camel-casing, which wrongly affected the parameter pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation, that makes sense!

@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
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:/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.

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