fix: Generator callable generation is based on method type #3075

lqiu96 · 2024-07-30T15:14:18Z

lqiu96 · 2024-07-30T19:31:59Z

...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java

+  private VariableExpr getCallableExpr(Method protoMethod, String callableName) {
+    return VariableExpr.withVariable(
+        Variable.builder().setName(callableName).setType(getCallableType(protoMethod)).build());
+  }
+
+  private VariableExpr getPagedCallableExpr(
+      TypeStore typeStore, Method protoMethod, String callableName) {
+    return VariableExpr.withVariable(
+        Variable.builder()
+            .setName(callableName)
+            .setType(
+                TypeNode.withReference(
+                    getCallableType(protoMethod)
+                        .reference()
+                        .copyAndSetGenerics(
+                            Arrays.asList(
+                                protoMethod.inputType().reference(),
+                                typeStore
+                                    .get(
+                                        String.format(
+                                            PAGED_RESPONSE_TYPE_NAME_PATTERN, protoMethod.name()))
+                                    .reference()))))
+            .build());
+  }
+
+  private VariableExpr getOperationCallableExpr(Method protoMethod, String callableName) {
+    return VariableExpr.withVariable(
+        Variable.builder()
+            .setName(callableName)
+            .setType(
+                TypeNode.withReference(
+                    ConcreteReference.builder()
+                        .setClazz(OperationCallable.class)
+                        .setGenerics(
+                            Arrays.asList(
+                                protoMethod.inputType().reference(),
+                                protoMethod.lro().responseType().reference(),
+                                protoMethod.lro().metadataType().reference()))
+                        .build()))
+            .build());
+  }


Extract these out to helper methods to be used later in createCallableInitExprs. These were originally used in createCallableClassMembers to create a mapping to be used, but createCallableInitExprs not longer uses that output.

I can't remove createCallableClassMembers method yet as that is still being used elsewhere and replacing it would be a much larger refactor/ effort

I am a bit concerned that remaining createCallableClassMembers usages will cause us weird bugs in the future. Perhaps file a separate issue to backlog for refactor and move away from it?

I think callableClassMemberVarExprs itself is fine, since it already uses the Method to determine if the RPC is an LRO or not. The Map<String, VariableExpr> though, it is slightly concerning because if someone uses the key to determine the type of a callable again, then it would be a problem. Ideally we can have a Callable object to wrap both name and type, but I think that is OOS for now.

Perhaps file a separate issue to backlog for refactor and move away from it?

I'll create an issue in the backlog for this. Personally, I think should we move away from any usage of Map<String, VariableExpr> and just generate the VariableExpr whenever needed. From reading the logic, I think the reason the Map was added was to cache the VariableExpr as it's being used a few places.

someone uses the key to determine the type of a callable again, then it would be a problem

Yep, the callable name shouldn't be the indicator of the RPC type. We don't know if there are any downstream usages of it, but I removed the constants and see no other usages. I think this was the only case where this type heuristic exists.

Ideally we can have a Callable object to wrap both name and type, but I think that is OOS for now.

If we want to keep this mapping structure to cache the VariableExpr, I think Map<Method, VariableExpr> would work. Method already contains the name and type of the RPC.

If possible, I'd rather not have multiple functions take multiple Map<String, VariableExpr> parameters or the equivalent Map<Method, VariableExpr>:

sdk-platform-java/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java

Lines 538 to 540 in 6913db5

Map<String, VariableExpr> classMemberVarExprs,

Map<String, VariableExpr> callableClassMemberVarExprs,

Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,

sdk-platform-java/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java

Lines 648 to 650 in 6913db5

Map<String, VariableExpr> classMemberVarExprs,

Map<String, VariableExpr> callableClassMemberVarExprs,

Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,

Refactoring something like this would be a much larger effort and much harder to get completely right. Perhaps multiple milestones/ steps could be made, but I don't know how feasible or time consuming it would be without a much thorough investigation.

If we want to keep this mapping structure to cache the VariableExpr, I think Map<Method, VariableExpr> would work. Method already contains the name and type of the RPC.

Correction on this, it wouldn't work for callableClassMemberVarExprs as a Method could have multiple VariableExprs.

Looks like it would work for protoMethodNameToDescriptorVarExprs as it's a mapping of one Method -> one VariableExpr. And wouldn't matter for classMemberVarExprs as it turns out it's not a mapping of method -> variablexpr

I think should we move away from any usage of Map<String, VariableExpr> and just generate the VariableExpr whenever needed

I agree, that's a good long term solution.

lqiu96 · 2024-07-30T19:33:06Z

...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java

-      String callableVarName,
-      VariableExpr callableVarExpr,


callableVarExpr was the VariableExpr from createCallableClassMembers above. We no longer use the map from createCallableClassMembers and now generate the VariableExpr inside this method directly.

snippet-bot · 2024-07-30T19:43:21Z

Here is the summary of possible violations 😱

There is a possible violation for not having product prefix.

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/servicesettings/stub/SyncGetApiOperation.golden:19, tag goldensample_generated_NamingServiceStubSettings_GetApiOperation_sync

The end of the violation section. All the stuff below is FYI purposes only.

Here is the summary of changes.

You are about to add 1 region tag.

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/servicesettings/stub/SyncGetApiOperation.golden:19, tag goldensample_generated_NamingServiceStubSettings_GetApiOperation_sync

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

...-generator-java/src/test/java/com/google/api/generator/test/protoloader/TestProtoLoader.java

gapic-generator-java/src/test/resources/naming_grpc_service_config.json

blakeli0 · 2024-07-31T00:00:28Z

...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java

+  private VariableExpr getCallableExpr(Method protoMethod, String callableName) {
+    return VariableExpr.withVariable(
+        Variable.builder().setName(callableName).setType(getCallableType(protoMethod)).build());
+  }
+
+  private VariableExpr getPagedCallableExpr(
+      TypeStore typeStore, Method protoMethod, String callableName) {
+    return VariableExpr.withVariable(
+        Variable.builder()
+            .setName(callableName)
+            .setType(
+                TypeNode.withReference(
+                    getCallableType(protoMethod)
+                        .reference()
+                        .copyAndSetGenerics(
+                            Arrays.asList(
+                                protoMethod.inputType().reference(),
+                                typeStore
+                                    .get(
+                                        String.format(
+                                            PAGED_RESPONSE_TYPE_NAME_PATTERN, protoMethod.name()))
+                                    .reference()))))
+            .build());
+  }
+
+  private VariableExpr getOperationCallableExpr(Method protoMethod, String callableName) {
+    return VariableExpr.withVariable(
+        Variable.builder()
+            .setName(callableName)
+            .setType(
+                TypeNode.withReference(
+                    ConcreteReference.builder()
+                        .setClazz(OperationCallable.class)
+                        .setGenerics(
+                            Arrays.asList(
+                                protoMethod.inputType().reference(),
+                                protoMethod.lro().responseType().reference(),
+                                protoMethod.lro().metadataType().reference()))
+                        .build()))
+            .build());
+  }


I think callableClassMemberVarExprs itself is fine, since it already uses the Method to determine if the RPC is an LRO or not. The Map<String, VariableExpr> though, it is slightly concerning because if someone uses the key to determine the type of a callable again, then it would be a problem. Ideally we can have a Callable object to wrap both name and type, but I think that is OOS for now.

blakeli0 · 2024-07-31T00:02:03Z

gapic-generator-java/src/test/proto/naming.proto

Can we rename this file to something more specific to the test case? Also can we have an RPC that is actually a LRO in the testing proto?

Let me know your thoughts on this, but I called this file naming.proto and not something more specific for this one case because I was looking to create a generic proto where I could throw in this and future tests cases for weird naming conventions. Ideally, it would be a running list where we add tests cases as we get more name related bugs.

The idea for naming.proto is to have RPCs/ messages/ field names which previously caused some weird issues, but are now resolved. When new generator changes are made, ideally the generated output for these old, weird edge cases should not trigger any change.

Also can we have an RPC that is actually a LRO in the testing proto?

The reason I didn't add an LRO RPC is because and LRO would always be generated as an LRO (i.e. RPC name rpc OperationCallable(...) would be generated correctly) . The issue was the when normal RPCs (unary, streaming, batching) would be mistaken as either an LRO or Paged. The two cases in the testing proto are for unary callables that previously would have been identified as an LRO or Paged just by the RPC name.

i.e.

GetApiOperation would have name GetApiOperationCallable with the OperationCallable suffix

ApiPaged would have name ApiPagedCallable with the PagedCallable suffix

I was looking to create a generic proto where I could throw in this and future tests cases for weird naming conventions

I think that's a good intention but that intention could easily get lost if the name is not clear. naming.proto is too generic just by looking at the name, I think we should at least name it something like naming_conflict.proto.

In addition, unit tests should be quick and precise. Keep a running list is like adding test cases to a single test file, it works but not ideal. We may have different types of naming conflict in the future, but we don't know if it makes sense to put them in the same testing file. I would prefer to have a more specific proto in most cases.

The reason I didn't add an LRO RPC is because and LRO would always be generated as an LRO

Usually we want to test anything we touched. In this case, we should make sure the behavior is still the same as before for valid LROs. However, giving it more thought, that case is probably already tested somewhere else, probably in echo.proto. It is not ideal, as we should have a feature specific LRO_operation_testing.proto that is dedicated to LRO generation. But I think it's OK to not adding additional LRO testing for now.

Makes sense. I'll rename it to something like naming_conflict.proto since I can see how this can easily be overlooked/ get confusing in the future.

Agreed that unit tests should be simple and precise. I was looking for a way to only test createCallableInitExprs, but ending up having to recreate a service proto with new request messages and response messages + all the other configurations required to generate the java class and then link it to it set up to generate diffs, which is why I was looking to just have a single proto that could be re-used in every composer in the future.

LROs are indirectly tested with echo.proto and probably should have a more feature specific LRO.proto. I think this is the same case with Unary, Streaming, Paging, and Batching.

...enerator/gapic/composer/grpc/goldens/samples/servicesettings/stub/SyncGetApiOperation.golden

sonarcloud · 2024-08-01T20:36:58Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarcloud · 2024-08-01T20:40:32Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

zhumin8

LGTM.

zhumin8 · 2024-08-02T20:29:39Z

...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java

+  // Can return multiple Exprs for a single RPC. Each of the Exprs will initialize a callable
+  // in the constructor. The possible combinations are Normal (Unary, Streaming, Batching) and
+  // either Operation or Paged (if needed). It is not possible to have three callable Exprs
+  // returned because LROs are not paged, so it will either be an additional LRO or paged callable.
+  private List<Expr> createCallableInitExprs(


This comment provided helpful context for understanding. Thank you!

🤖 I have created a release *beep* *boop* --- <details><summary>2.44.0</summary> ## [2.44.0](v2.43.0...v2.44.0) (2024-08-16) ### Features * update ErrorDetails to allow unpacking arbitrary messages ([#3073](#3073)) ([6913db5](6913db5)) ### Bug Fixes * Generator callable generation is based on method type ([#3075](#3075)) ([c21a013](c21a013)) * improve warnings for Direct Path xDS set via env ([#3019](#3019)) ([7a26115](7a26115)) ### Dependencies * update dependency argcomplete to v3.5.0 ([#3099](#3099)) ([0654a28](0654a28)) * update dependency black to v24.8.0 ([#3082](#3082)) ([a864f62](a864f62)) * update dependency com.google.crypto.tink:tink to v1.14.1 ([#3083](#3083)) ([c13b63e](c13b63e)) * update dependency com.google.errorprone:error_prone_annotations to v2.30.0 ([#3100](#3100)) ([a10ef54](a10ef54)) * update dependency com.google.errorprone:error_prone_annotations to v2.30.0 ([#3101](#3101)) ([9bff64f](9bff64f)) * update dependency lxml to v5.3.0 ([#3102](#3102)) ([4e145b1](4e145b1)) * update dependency org.apache.commons:commons-lang3 to v3.16.0 ([#3103](#3103)) ([95c9508](95c9508)) * update dependency org.checkerframework:checker-qual to v3.46.0 ([#3081](#3081)) ([2431920](2431920)) * update dependency org.easymock:easymock to v5.4.0 ([#3079](#3079)) ([182ae50](182ae50)) * update dependency pyyaml to v6.0.2 ([#3086](#3086)) ([f847e45](f847e45)) * update dependency watchdog to v4.0.2 ([#3094](#3094)) ([f1c75a1](f1c75a1)) * update google api dependencies ([#3071](#3071)) ([c5abe90](c5abe90)) * update google auth library dependencies to v1.24.1 ([#3109](#3109)) ([62acdd6](62acdd6)) * update googleapis/java-cloud-bom digest to a98202d ([#3097](#3097)) ([bb216ae](bb216ae)) * update googleapis/java-cloud-bom digest to ad905cc ([#3080](#3080)) ([250b26c](250b26c)) * update grpc dependencies to v1.66.0 ([#3104](#3104)) ([b63b643](b63b643)) * update opentelemetry-java monorepo to v1.41.0 ([#3105](#3105)) ([7364916](7364916)) * update protobuf to 3.25.4 ([#3113](#3113)) ([2b271fc](2b271fc)) * update slf4j monorepo to v2.0.16 ([#3098](#3098)) ([c13f932](c13f932)) ### Documentation * Update the Javadoc of ObsoleteApi.java ([#3088](#3088)) ([0ef6619](0ef6619)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

lqiu96 added 2 commits July 30, 2024 00:16

chore: Add unit tests for the generator

dfe72f9

chore: Clean up logic

a558237

product-auto-label bot added the size: l Pull request size is large. label Jul 30, 2024

lqiu96 commented Jul 30, 2024

View reviewed changes

lqiu96 marked this pull request as ready for review July 30, 2024 19:43

chore: Fix lint issues

173ef47

lqiu96 requested review from zhumin8 and blakeli0 July 30, 2024 20:00

blakeli0 reviewed Jul 31, 2024

View reviewed changes

lqiu96 added 4 commits July 30, 2024 21:34

chore: Resolve merge conflicts

ade43f8

chore: Address PR comments

86abe40

chore: Remove old constants

69bf136

chore: Update to specific test

fe1c25c

lqiu96 requested a review from blakeli0 August 1, 2024 20:29

Merge branch 'main' into fix-callable-generation-logic

f77823d

zhumin8 approved these changes Aug 2, 2024

View reviewed changes

blakeli0 approved these changes Aug 5, 2024

View reviewed changes

lqiu96 merged commit c21a013 into main Aug 5, 2024
34 checks passed

lqiu96 deleted the fix-callable-generation-logic branch August 5, 2024 19:23

release-please bot mentioned this pull request Aug 5, 2024

chore(main): release 2.44.0 #3074

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Generator callable generation is based on method type #3075

fix: Generator callable generation is based on method type #3075

lqiu96 commented Jul 30, 2024 •

edited

Loading

lqiu96 Jul 30, 2024

zhumin8 Jul 30, 2024

blakeli0 Jul 31, 2024

lqiu96 Jul 31, 2024

lqiu96 Jul 31, 2024

blakeli0 Jul 31, 2024

lqiu96 Jul 30, 2024

snippet-bot bot commented Jul 30, 2024 •

edited

Loading

blakeli0 Jul 31, 2024

blakeli0 Jul 31, 2024

lqiu96 Jul 31, 2024

blakeli0 Jul 31, 2024

lqiu96 Jul 31, 2024

sonarcloud bot commented Aug 1, 2024

sonarcloud bot commented Aug 1, 2024

zhumin8 left a comment

zhumin8 Aug 2, 2024

	Map<String, VariableExpr> classMemberVarExprs,
	Map<String, VariableExpr> callableClassMemberVarExprs,
	Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,

fix: Generator callable generation is based on method type #3075

fix: Generator callable generation is based on method type #3075

Conversation

lqiu96 commented Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 1, 2024

Quality Gate passed for 'gapic-generator-java-root'

sonarcloud bot commented Aug 1, 2024

Quality Gate passed for 'java_showcase_integration_tests'

zhumin8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 commented Jul 30, 2024 •

edited

Loading

snippet-bot bot commented Jul 30, 2024 •

edited

Loading