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

Support nested fields, path segments #31

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mbarrien
Copy link

Closes #28

Adds support for 2 related cases in URLs:

  • Support path segments like in https://google.aip.dev/127, where the
    URL contains a pattern like post: "/v1/{parent=publishers/*}/books"
  • Support nested field names in the URL, where the URL is structured
    like:
      option (google.api.http) = {
        patch: "/v3/{intent.name=projects/*/locations/*/agents/*/intents/*}"
        body: "intent"
      };
    
    This gets translated to /v3/${req["intent"]["name"]}

While here, use the newer protoc-gen-go-grpc plugin for generating
server test code, due to deprecated usage of plugin=grpc (see
https://github.com/protocolbuffers/protobuf-go/releases/tag/v1.20.0#v1.20-grpc-support)

@mbarrien mbarrien force-pushed the nested-fields-path-segments branch 8 times, most recently from a9d09c1 to 3b14d8a Compare March 30, 2022 04:16
Closes grpc-ecosystem#28

Adds support for 2 related cases in URLs:
* Support path segments like in https://google.aip.dev/127, where the
  URL contains a pattern like `post: "/v1/{parent=publishers/*}/books"`
* Support nested field names in the URL, where the URL is structured
  like:
  ```
    option (google.api.http) = {
      patch: "/v3/{intent.name=projects/*/locations/*/agents/*/intents/*}"
      body: "intent"
    };
  ```
  This gets translated to `/v3/${req["intent"]["name"]}`

While here, use the newer protoc-gen-go-grpc plugin for generating
server test code, due to deprecated usage of plugin=grpc (see
https://github.com/protocolbuffers/protobuf-go/releases/tag/v1.20.0#v1.20-grpc-support)
@mbarrien mbarrien force-pushed the nested-fields-path-segments branch from 3b14d8a to 07fb69d Compare March 30, 2022 04:20
@mbarrien mbarrien marked this pull request as ready for review March 30, 2022 04:23
Copy link

@mckelveygreg mckelveygreg left a comment

Choose a reason for hiding this comment

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

I got this to generate a lovely client with your changes!
One suggestion is in order to make strict typescript happy, you'll need to do a null check on the nested fields because they are optional.

generator/template.go Show resolved Hide resolved
Copy link

@mckelveygreg mckelveygreg left a comment

Choose a reason for hiding this comment

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

Now things compile! 🚀

@@ -485,7 +488,7 @@ func renderURL(r *registry.Registry) func(method data.Method) string {
partNames = append(partNames, fmt.Sprintf(`["%s"]`, subFieldName))
}
fieldName := strings.Join(subFieldNames, ".")
part := fmt.Sprintf(`${req%s}`, strings.Join(partNames, ""))
part := fmt.Sprintf(`${req%s}`, strings.Join(partNames, "?."))

Choose a reason for hiding this comment

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

Thanks! This worked great for me!

@hcwang512
Copy link

This fix works perfect for our use case, please merge it. @mbarrien

Copy link
Collaborator

@lyonlai lyonlai left a comment

Choose a reason for hiding this comment

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

@mbarrien Thanks for the contribution. Can you take a loot at the failed CI pipeline and fix it first? also I don't see the client proto generated I suspect that's one of the reason that failed the CI pipeline.

@yeluyang
Copy link

yeluyang commented Oct 8, 2022

we really need this patch, can someone do some magic and make ci happy ? 😭

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.

Google-recommended way of structuring resource path is not supported (https://google.aip.dev/127)
5 participants