feat(gapic-generator-java): Extract resource name heuristicly#12207
feat(gapic-generator-java): Extract resource name heuristicly#12207
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new heuristic for extracting resource names during GAPIC client library generation, allowing for more intelligent resource path construction. This heuristic is selectively enabled for specific proto packages to ensure controlled application. Additionally, the changes improve the robustness of endpoint context parsing, preventing failures when encountering malformed server address or port information. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
blakeli0
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new functionality to the PathTemplate class, adding getResourceLiterals and getCanonicalResourceName methods to extract resource literals and canonical resource names from path templates, including support for complex patterns and named bindings. The GAPIC generator has been updated to leverage these new PathTemplate capabilities, enabling automatic generation of resource name extractors for client stubs, especially for methods in heuristic-enabled packages that lack explicit resource references. This involved defining AIP standard methods and heuristic-enabled packages, and updating associated golden files and proto definitions. Additionally, the EndpointContext in GAX was improved to handle invalid endpoint parsing for server address and port more gracefully, treating these as non-critical for client requests. Feedback includes refactoring the complex getCanonicalResourceName method, removing a dead keepBinding variable, correcting typos in test method names, addressing a debugging test method, fixing a typo in service.protoPakkage(), and improving the robustness of binding variable checks in createResourceNameExtractorClassInstance from String.contains() to a more precise regex.
sdk-platform-java/api-common-java/src/main/java/com/google/api/pathtemplate/PathTemplate.java
Show resolved
Hide resolved
sdk-platform-java/api-common-java/src/main/java/com/google/api/pathtemplate/PathTemplate.java
Outdated
Show resolved
Hide resolved
...latform-java/api-common-java/src/test/java/com/google/api/pathtemplate/PathTemplateTest.java
Show resolved
Hide resolved
...latform-java/api-common-java/src/test/java/com/google/api/pathtemplate/PathTemplateTest.java
Outdated
Show resolved
Hide resolved
...latform-java/api-common-java/src/test/java/com/google/api/pathtemplate/PathTemplateTest.java
Outdated
Show resolved
Hide resolved
...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
Show resolved
Hide resolved
...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
Outdated
Show resolved
Hide resolved
blakeli0
changed the title
diegomarquezp
left a comment
diegomarquezp
left a comment
There was a problem hiding this comment.
Thanks for the PR. Added a few comments.
| if (inBinding) { | ||
| canonicalSegments.add(value); | ||
| } else if (i + 1 < segments.size() && segments.get(i + 1).kind() == SegmentKind.BINDING) { | ||
| canonicalSegments.add(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm probably wrong, but what I read here is two ways to add a literal to the canonical segments:
- previous node is a binding (
inBinding) and we are now in a literal - next node is a binding and we are now in a literal
From the javadoc, A resource literal is a literal followed by a binding. Why are we also considering binding first then literal as well?
There was a problem hiding this comment.
Added more comments. It is for a special case named binding, which looks like {name=projects/*/locations/*}
| * literal/binding pair or named binding, and then extracting the segments between the version | ||
| * literal and the last binding. | ||
| */ | ||
| // For example, projects/{project} is a literal/binding pair. {bar=projects/*/locations/*/bars/*} |
There was a problem hiding this comment.
Can we also add a comment line saying what would be the output given an exemplary template?
| if (seg.kind() == SegmentKind.LITERAL) { | ||
| String value = seg.value(); | ||
| if (value.matches("^v\\d+[a-zA-Z0-9]*$")) { | ||
| startIndex = i + 1; |
There was a problem hiding this comment.
Is this if statement guaranteed to end up setting the start index?
There was a problem hiding this comment.
Yes, if there are not versions found, the start index would be 0.
| int literalCountInBinding = 0; | ||
| int currentBindingStartIndex = -1; | ||
|
|
||
| for (int i = 0; i < segments.size(); i++) { |
There was a problem hiding this comment.
+1 to gemini's comment on complexity of this method and splitting into private helpers. We could also add comments explaining the purpose of each if or nested for loop.
There was a problem hiding this comment.
I significantly simplified the logic and added more comments now. PTAL
2 participants
Extract resource name heuristically if there are no resource_reference configured for a request.
This PR adds logic to apply the heuristic resource name extraction if the proto package matches certain patterns:
google.cloud.compute.**google.cloud.sql.**google.cloud.bigquery.**The exact logic is described in go/client-libraries:destination-resource-name. In a nutshell, a canonical resource name is extracted from the template by finding the version literal, then finding the last binding that is a literal/binding pair or named binding, and then extracting the segments between the version literal and the last binding.
The logic is mostly implemented in PathTemplate, and used by AbstractTransportServiceStubClassComposer#createResourceNameExtractorClassInstance.
The generated code would be called in the same way as a regular resourceNameExtractor by TracedUnaryCallable.