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

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

Merged
merged 8 commits into from
Aug 5, 2024

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Jul 30, 2024

Fixes #3076

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Jul 30, 2024
Comment on lines +478 to +518
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());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>:

  1. Map<String, VariableExpr> classMemberVarExprs,
    Map<String, VariableExpr> callableClassMemberVarExprs,
    Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines -877 to -878
String callableVarName,
VariableExpr callableVarExpr,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

snippet-bot bot commented Jul 30, 2024

Here is the summary of possible violations 😱

There is a possible violation for not having product prefix.

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.

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

@lqiu96 lqiu96 requested review from zhumin8 and blakeli0 July 30, 2024 20:00
Comment on lines +478 to +518
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());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

  1. GetApiOperation would have name GetApiOperationCallable with the OperationCallable suffix
  2. ApiPaged would have name ApiPagedCallable with the PagedCallable suffix

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

sonarqubecloud bot commented Aug 1, 2024

Copy link

sonarqubecloud bot commented Aug 1, 2024

Quality Gate Passed 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

Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment on lines +883 to +887
// 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(
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment provided helpful context for understanding. Thank you!

@lqiu96 lqiu96 merged commit c21a013 into main Aug 5, 2024
34 checks passed
@lqiu96 lqiu96 deleted the fix-callable-generation-logic branch August 5, 2024 19:23
mpeddada1 pushed a commit that referenced this pull request Aug 16, 2024
🤖 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generator is creating Callables based on RPC suffix
3 participants