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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dfe72f9
chore: Add unit tests for the generator
lqiu96 Jul 30, 2024
a558237
chore: Clean up logic
lqiu96 Jul 30, 2024
173ef47
chore: Fix lint issues
lqiu96 Jul 30, 2024
ade43f8
chore: Resolve merge conflicts
lqiu96 Jul 31, 2024
86abe40
chore: Address PR comments
lqiu96 Jul 31, 2024
69bf136
chore: Remove old constants
lqiu96 Jul 31, 2024
fe1c25c
chore: Update to specific test
lqiu96 Aug 1, 2024
f77823d
Merge branch 'main' into fix-callable-generation-logic
lqiu96 Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
256 changes: 159 additions & 97 deletions ...google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,59 +460,63 @@ private Map<String, VariableExpr> createCallableClassMembers(
}
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
String callableName = String.format(CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName);
callableClassMembers.put(
callableName,
VariableExpr.withVariable(
Variable.builder()
.setName(callableName)
.setType(getCallableType(protoMethod))
.build()));
callableClassMembers.put(callableName, getCallableExpr(protoMethod, callableName));
if (protoMethod.hasLro()) {
callableName =
String.format(OPERATION_CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName);
callableClassMembers.put(
callableName,
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()));
callableClassMembers.put(callableName, getOperationCallableExpr(protoMethod, callableName));
}
if (protoMethod.isPaged()) {
callableName = String.format(PAGED_CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName);
callableClassMembers.put(
callableName,
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()));
callableName, getPagedCallableExpr(typeStore, protoMethod, callableName));
}
}
return callableClassMembers;
}

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());
}
Comment on lines +474 to +514
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. 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,

  2. 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.

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.


protected List<AnnotationNode> createClassAnnotations(Service service) {
List<AnnotationNode> annotations = new ArrayList<>();
if (!PackageChecker.isGaApi(service.pakkage())) {
Expand Down Expand Up @@ -547,7 +551,6 @@ protected List<MethodDefinition> createClassMethods(
service,
typeStore,
classMemberVarExprs,
callableClassMemberVarExprs,
protoMethodNameToDescriptorVarExprs,
classStatements));
javaMethods.addAll(
Expand Down Expand Up @@ -646,7 +649,6 @@ protected List<MethodDefinition> createConstructorMethods(
Service service,
TypeStore typeStore,
Map<String, VariableExpr> classMemberVarExprs,
Map<String, VariableExpr> callableClassMemberVarExprs,
Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,
List<Statement> classStatements) {
TypeNode stubSettingsType =
Expand Down Expand Up @@ -786,22 +788,33 @@ protected List<MethodDefinition> createConstructorMethods(
secondCtorStatements.add(EMPTY_LINE_STATEMENT);

// Initialize <method>Callable variables.
secondCtorExprs.addAll(
callableClassMemberVarExprs.entrySet().stream()
.map(
e ->
createCallableInitExpr(
context,
service,
e.getKey(),
e.getValue(),
callableFactoryVarExpr,
settingsVarExpr,
clientContextVarExpr,
operationsStubClassVarExpr,
thisExpr,
javaStyleMethodNameToTransportSettingsVarExprs))
.collect(Collectors.toList()));
// The logic inside createCallableInitExprs() is very similar to createCallableClassMembers().
// It is mostly duplicated because `createCallableClassMembers` returns a heuristic to
// determine the RPC type. The RPCs are mapped by name and the types are determined by the
// generated name and was problematic for certain RPC names. For example, the GetApiOperation
// RPC name would have a mapping of GetApiOperationCallable, and the `createCallableInitExprs`
// method would attempt to generate LRO code because of the `OperationCallable` suffix.
// Instead, we now pass the method object which is the SoT for the type of the method and not
// based on heuristics/ suffix.
for (Method method : service.methods()) {
// Do not generate callables for non supported RPCs (i.e. Bidi-Streaming and Client Streaming
// for HttpJson)
if (!method.isSupportedByTransport(getTransportContext().transport())) {
continue;
}
secondCtorExprs.addAll(
createCallableInitExprs(
context,
service,
method,
typeStore,
callableFactoryVarExpr,
settingsVarExpr,
clientContextVarExpr,
operationsStubClassVarExpr,
thisExpr,
javaStyleMethodNameToTransportSettingsVarExprs));
}
secondCtorStatements.addAll(
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();
Expand Down Expand Up @@ -871,67 +884,116 @@ protected VariableExpr declareLongRunningClient() {
return null;
}

private Expr createCallableInitExpr(
// 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(
Comment on lines +883 to +887
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!

GapicContext context,
Service service,
String callableVarName,
VariableExpr callableVarExpr,
Comment on lines -877 to -878
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.

Method method,
TypeStore typeStore,
VariableExpr callableFactoryVarExpr,
VariableExpr settingsVarExpr,
VariableExpr clientContextVarExpr,
VariableExpr operationsStubClassVarExpr,
Expr thisExpr,
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs) {
boolean isOperation = callableVarName.endsWith(OPERATION_CALLABLE_NAME);
boolean isPaged = callableVarName.endsWith(PAGED_CALLABLE_NAME);
int sublength;
if (isOperation) {
sublength = OPERATION_CALLABLE_NAME.length();
} else if (isPaged) {
sublength = PAGED_CALLABLE_NAME.length();
} else {
sublength = CALLABLE_NAME.length();
}
String javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
List<Expr> creatorMethodArgVarExprs;
List<Expr> callableInitExprs = new ArrayList<>();
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(method.name());

Expr transportSettingsVarExpr =
javaStyleMethodNameToTransportSettingsVarExprs.get(javaStyleMethodName);
if (transportSettingsVarExpr == null && isOperation) {
// Try again, in case the name detection above was inaccurate.
isOperation = false;
sublength = CALLABLE_NAME.length();
javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
transportSettingsVarExpr =
javaStyleMethodNameToTransportSettingsVarExprs.get(javaStyleMethodName);
}
javaStyleMethodNameToTransportSettingsVarExprs.get(javaStyleProtoMethodName);
Preconditions.checkNotNull(
transportSettingsVarExpr,
String.format(
"No transport settings variable found for method name %s", javaStyleMethodName));
if (isOperation) {
"No transport settings variable found for method name %s", javaStyleProtoMethodName));

// Build the normal callable which will be generated for every RPC
VariableExpr callableVarExpr =
getCallableExpr(
method, String.format(CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName));
List<Expr> creatorMethodArgVarExprs =
Arrays.asList(
transportSettingsVarExpr,
MethodInvocationExpr.builder()
.setExprReferenceExpr(settingsVarExpr)
.setMethodName(String.format("%sSettings", javaStyleProtoMethodName))
.build(),
clientContextVarExpr);
AssignmentExpr callableExpr =
buildCallableTransportExpr(
context,
service,
callableFactoryVarExpr,
thisExpr,
javaStyleProtoMethodName,
callableVarExpr,
creatorMethodArgVarExprs);
callableInitExprs.add(callableExpr);

// Build an additional paged callable if the RPC is paged. The creatorMethodArgVarExprs is the
// same as the normal callable
if (method.isPaged()) {
callableVarExpr =
getPagedCallableExpr(
typeStore,
method,
String.format(PAGED_CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName));
callableExpr =
buildCallableTransportExpr(
context,
service,
callableFactoryVarExpr,
thisExpr,
javaStyleProtoMethodName,
callableVarExpr,
creatorMethodArgVarExprs);
callableInitExprs.add(callableExpr);
}

// Build an additional operation callable if the RPC is an LRO. Rebuild the
// creatorMethodArgVarExprs as LROs have a special OperationSettings
if (method.hasLro()) {
callableVarExpr =
getOperationCallableExpr(
method,
String.format(OPERATION_CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName));
creatorMethodArgVarExprs =
Arrays.asList(
transportSettingsVarExpr,
MethodInvocationExpr.builder()
.setExprReferenceExpr(settingsVarExpr)
.setMethodName(String.format("%sOperationSettings", javaStyleMethodName))
.setMethodName(String.format("%sOperationSettings", javaStyleProtoMethodName))
.build(),
clientContextVarExpr,
operationsStubClassVarExpr);
} else {
creatorMethodArgVarExprs =
Arrays.asList(
transportSettingsVarExpr,
MethodInvocationExpr.builder()
.setExprReferenceExpr(settingsVarExpr)
.setMethodName(String.format("%sSettings", javaStyleMethodName))
.build(),
clientContextVarExpr);
callableExpr =
buildCallableTransportExpr(
context,
service,
callableFactoryVarExpr,
thisExpr,
javaStyleProtoMethodName,
callableVarExpr,
creatorMethodArgVarExprs);
callableInitExprs.add(callableExpr);
}

String methodName = JavaStyle.toUpperCamelCase(javaStyleMethodName);
return callableInitExprs;
}

private AssignmentExpr buildCallableTransportExpr(
GapicContext context,
Service service,
VariableExpr callableFactoryVarExpr,
Expr thisExpr,
String methodName,
VariableExpr callableVarExpr,
List<Expr> creatorMethodArgVarExprs) {
Optional<String> callableCreatorMethodName =
getCallableCreatorMethodName(context, service, callableVarExpr.type(), methodName);
getCallableCreatorMethodName(
context, service, callableVarExpr.type(), JavaStyle.toUpperCamelCase(methodName));

Expr initExpr;
if (callableCreatorMethodName.isPresent()) {
Expand Down
10 changes: 10 additions & 0 deletions ...t/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.api.generator.test.framework.Assert;
import com.google.api.generator.test.protoloader.GrpcTestProtoLoader;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.provider.Arguments;

class GrpcServiceStubClassComposerTest {
@Test
Expand Down Expand Up @@ -92,4 +93,13 @@ void generateGrpcServiceStubClass_autopopulateField() {
Assert.assertGoldenClass(this.getClass(), clazz, "GrpcAutoPopulateFieldStub.golden");
Assert.assertEmptySamples(clazz.samples());
}

@Test
void generateGrpcServiceStubClass_namingField() {
GapicContext context = GrpcTestProtoLoader.instance().parseNaming();
Service service = context.services().get(0);
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);
Assert.assertGoldenClass(this.getClass(), clazz, "GrpcNamingStub.golden");
Assert.assertEmptySamples(clazz.samples());
}
}
Loading
Loading