-
Notifications
You must be signed in to change notification settings - Fork 51
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
base: master
Are you sure you want to change the base?
Support nested fields, path segments #31
Conversation
a9d09c1
to
3b14d8a
Compare
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)
3b14d8a
to
07fb69d
Compare
There was a problem hiding this 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.
There was a problem hiding this 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, "?.")) |
There was a problem hiding this comment.
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!
This fix works perfect for our use case, please merge it. @mbarrien |
There was a problem hiding this 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.
we really need this patch, can someone do some magic and make ci happy ? 😭 |
Closes #28
Adds support for 2 related cases in URLs:
URL contains a pattern like
post: "/v1/{parent=publishers/*}/books"
like:
/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)