Skip to content

Commit

Permalink
fix: gax batching regression (#863)
Browse files Browse the repository at this point in the history 
* fix: gax batching regression
* Fix bug and add tests
* Update golden files to fix tests
* Revert some cleanups
  • Loading branch information
@chanseokoh
chanseokoh authored Oct 25, 2021
1 parent 6bc4417 commit 3a6f168
Show file tree
Hide file tree
Showing 9 changed files with 423 additions and 46 deletions.
3 changes: 1 addition & 2 deletions src/main/java/com/google/api/generator/gapic/Generator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ public static CodeGeneratorResponse generateGapic(CodeGeneratorRequest request)
List<GapicClass> clazzes = Composer.composeServiceClasses(context);
GapicPackageInfo packageInfo = Composer.composePackageInfo(context);
String outputFilename = "temp-codegen.srcjar";
CodeGeneratorResponse response = Writer.write(context, clazzes, packageInfo, outputFilename);
return response;
return Writer.write(context, clazzes, packageInfo, outputFilename);
}
}
2 changes: 1 addition & 1 deletion src/main/java/com/google/api/generator/gapic/composer/Composer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public static List<GapicClass> generateMockClasses(GapicContext context, List<Se
services.forEach(
s -> {
if (context.transport() == Transport.REST) {
// REST transport tests donot not use mock services.
// REST transport tests do not use mock services.
} else if (context.transport() == Transport.GRPC) {
clazzes.add(MockServiceClassComposer.instance().generate(context, s));
clazzes.add(MockServiceImplClassComposer.instance().generate(context, s));
Expand Down
95 changes: 60 additions & 35 deletions ...google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,12 @@
import com.google.api.generator.engine.ast.VariableExpr;
import com.google.api.generator.gapic.composer.comment.StubCommentComposer;
import com.google.api.generator.gapic.composer.store.TypeStore;
import com.google.api.generator.gapic.composer.utils.ClassNames;
import com.google.api.generator.gapic.composer.utils.PackageChecker;
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicClass.Kind;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.GapicServiceConfig;
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Service;
Expand Down Expand Up @@ -205,6 +207,7 @@ public GapicClass generate(GapicContext context, Service service) {
.setStatements(classStatements)
.setMethods(
createClassMethods(
context,
service,
typeStore,
classMemberVarExprs,
Expand Down Expand Up @@ -422,6 +425,7 @@ protected List<AnnotationNode> createClassAnnotations(Service service) {
}

protected List<MethodDefinition> createClassMethods(
GapicContext context,
Service service,
TypeStore typeStore,
Map<String, VariableExpr> classMemberVarExprs,
Expand All @@ -431,6 +435,7 @@ protected List<MethodDefinition> createClassMethods(
javaMethods.addAll(createStaticCreatorMethods(service, typeStore, "newBuilder"));
javaMethods.addAll(
createConstructorMethods(
context,
service,
typeStore,
classMemberVarExprs,
Expand Down Expand Up @@ -531,6 +536,7 @@ protected List<MethodDefinition> createStaticCreatorMethods(
}

protected List<MethodDefinition> createConstructorMethods(
GapicContext context,
Service service,
TypeStore typeStore,
Map<String, VariableExpr> classMemberVarExprs,
Expand Down Expand Up @@ -604,7 +610,9 @@ protected List<MethodDefinition> createConstructorMethods(
secondCtorExprs.add(
AssignmentExpr.builder()
.setVariableExpr(
classMemberVarExprs.get("callableFactory").toBuilder()
classMemberVarExprs
.get("callableFactory")
.toBuilder()
.setExprReferenceExpr(thisExpr)
.build())
.setValueExpr(callableFactoryVarExpr)
Expand All @@ -613,15 +621,16 @@ protected List<MethodDefinition> createConstructorMethods(
classMemberVarExprs.get(getTransportContext().transportOperationsStubNames().get(0));
// TODO: refactor this
if (generateOperationsStubLogic(service)) {
secondCtorExprs.addAll(createOperationsStubInitExpr(
service,
thisExpr,
operationsStubClassVarExpr,
clientContextVarExpr,
callableFactoryVarExpr));
secondCtorExprs.addAll(
createOperationsStubInitExpr(
service,
thisExpr,
operationsStubClassVarExpr,
clientContextVarExpr,
callableFactoryVarExpr));
}
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();
secondCtorStatements.add(EMPTY_LINE_STATEMENT);

Expand Down Expand Up @@ -660,7 +669,7 @@ protected List<MethodDefinition> createConstructorMethods(
protoMethodNameToDescriptorVarExprs.get(m.name())))
.collect(Collectors.toList()));
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();
secondCtorStatements.add(EMPTY_LINE_STATEMENT);

Expand All @@ -670,6 +679,8 @@ protected List<MethodDefinition> createConstructorMethods(
.map(
e ->
createCallableInitExpr(
context,
service,
e.getKey(),
e.getValue(),
callableFactoryVarExpr,
Expand All @@ -680,7 +691,7 @@ protected List<MethodDefinition> createConstructorMethods(
javaStyleMethodNameToTransportSettingsVarExprs))
.collect(Collectors.toList()));
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();
secondCtorStatements.add(EMPTY_LINE_STATEMENT);

Expand All @@ -705,7 +716,7 @@ protected List<MethodDefinition> createConstructorMethods(
.build())
.build());
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();

// Second constructor method.
Expand All @@ -723,14 +734,14 @@ protected List<Expr> createOperationsStubInitExpr(
VariableExpr operationsStubClassVarExpr,
VariableExpr clientContextVarExpr,
VariableExpr callableFactoryVarExpr) {
TypeNode opeationsStubType = getTransportOperationsStubType(service);
TypeNode operationsStubType = getTransportOperationsStubType(service);
return Collections.singletonList(
AssignmentExpr.builder()
.setVariableExpr(
operationsStubClassVarExpr.toBuilder().setExprReferenceExpr(thisExpr).build())
.setValueExpr(
MethodInvocationExpr.builder()
.setStaticReferenceType(opeationsStubType)
.setStaticReferenceType(operationsStubType)
.setMethodName("create")
.setArguments(Arrays.asList(clientContextVarExpr, callableFactoryVarExpr))
.setReturnType(operationsStubClassVarExpr.type())
Expand All @@ -748,6 +759,8 @@ protected VariableExpr declareLongRunningClient() {
}

private Expr createCallableInitExpr(
GapicContext context,
Service service,
String callableVarName,
VariableExpr callableVarExpr,
VariableExpr callableFactoryVarExpr,
Expand All @@ -758,7 +771,7 @@ private Expr createCallableInitExpr(
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs) {
boolean isOperation = callableVarName.endsWith(OPERATION_CALLABLE_NAME);
boolean isPaged = callableVarName.endsWith(PAGED_CALLABLE_NAME);
int sublength = 0;
int sublength;
if (isOperation) {
sublength = OPERATION_CALLABLE_NAME.length();
} else if (isPaged) {
Expand All @@ -767,11 +780,11 @@ private Expr createCallableInitExpr(
sublength = CALLABLE_NAME.length();
}
String javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
List<Expr> creatorMethodArgVarExprs = null;
List<Expr> creatorMethodArgVarExprs;
Expr transportSettingsVarExpr =
javaStyleMethodNameToTransportSettingsVarExprs.get(javaStyleMethodName);
if (transportSettingsVarExpr == null && isOperation) {
// Try again, in case the name dtection above was inaccurate.
// Try again, in case the name detection above was inaccurate.
isOperation = false;
sublength = CALLABLE_NAME.length();
javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
Expand Down Expand Up @@ -803,8 +816,9 @@ private Expr createCallableInitExpr(
clientContextVarExpr);
}

String methodName = JavaStyle.toUpperCamelCase(javaStyleMethodName);
Optional<String> callableCreatorMethodName =
getCallableCreatorMethodName(callableVarExpr.type());
getCallableCreatorMethodName(context, service, callableVarExpr.type(), methodName);

Expr initExpr;
if (callableCreatorMethodName.isPresent()) {
Expand All @@ -825,26 +839,37 @@ private Expr createCallableInitExpr(
.build();
}

protected Optional<String> getCallableCreatorMethodName(TypeNode callableVarExprType) {
final String typeName = callableVarExprType.reference().name();
String streamName = "Unary";

protected Optional<String> getCallableCreatorMethodName(
GapicContext context,
Service service,
TypeNode callableVarExprType,
String serviceMethodName) {
GapicServiceConfig serviceConfig = context.serviceConfig();
if (serviceConfig != null
&& serviceConfig.hasBatchingSetting(
service.protoPakkage(), service.name(), serviceMethodName)) {
return Optional.of("createBatchingCallable");
}
// Special handling for pagination methods.
if (callableVarExprType.reference().generics().size() == 2
&& callableVarExprType.reference().generics().get(1).name().endsWith("PagedResponse")) {
streamName = "Paged";
} else {
if (typeName.startsWith("Client")) {
streamName = "ClientStreaming";
} else if (typeName.startsWith("Server")) {
streamName = "ServerStreaming";
} else if (typeName.startsWith("Bidi")) {
streamName = "BidiStreaming";
} else if (typeName.startsWith("Operation")) {
streamName = "Operation";
}
return Optional.of("createPagedCallable");
}

String typeName = callableVarExprType.reference().name();
if (typeName.startsWith("Client")) {
return Optional.of("createClientStreamingCallable");
}
if (typeName.startsWith("Server")) {
return Optional.of("createServerStreamingCallable");
}
if (typeName.startsWith("Bidi")) {
return Optional.of("createBidiStreamingCallable");
}
if (typeName.startsWith("Operation")) {
return Optional.of("createOperationCallable");
}
return Optional.of(String.format("create%sCallable", streamName));
return Optional.of("createUnaryCallable");
}

private static List<MethodDefinition> createCallableGetterMethods(
Expand Down Expand Up @@ -960,7 +985,7 @@ private List<MethodDefinition> createStubOverrideMethods(
.setType(
TypeNode.withExceptionClazz(
IllegalStateException.class))
.setMessageExpr(String.format("Failed to close resource"))
.setMessageExpr("Failed to close resource")
.setCauseExpr(catchExceptionVarExpr)
.build())))
.build()))
Expand Down Expand Up @@ -1041,7 +1066,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
typeStore.putAll(
service.pakkage(),
service.methods().stream()
.filter(m -> m.isPaged())
.filter(Method::isPaged)
.map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
.collect(Collectors.toList()),
true,
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/api/generator/util/Trie.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
* A common-prefix trie. T represents the type of each "char" in a word (which is a T-typed list).
*/
public class Trie<T> {
private class Node<U> {
private static class Node<U> {
final U chr;
// Maintain insertion order to enable deterministic test output.
Map<U, Node<U>> children = new LinkedHashMap<>();
Expand All @@ -39,7 +39,7 @@ private class Node<U> {
}
}

private Node<T> root;
private final Node<T> root;

public Trie() {
root = new Node<>();
Expand Down
18 changes: 15 additions & 3 deletions ...t/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ public void generateGrpcServiceStubClass_deprecated() {
@Test
public void generateGrpcServiceStubClass_httpBindings() {
GapicContext context = GrpcTestProtoLoader.instance().parseShowcaseTesting();
Service testingProtoService = context.services().get(0);
GapicClass clazz =
GrpcServiceStubClassComposer.instance().generate(context, testingProtoService);
Service service = context.services().get(0);
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);

JavaWriterVisitor visitor = new JavaWriterVisitor();
clazz.classDefinition().accept(visitor);
Expand All @@ -79,4 +78,17 @@ public void generateGrpcServiceStubClass_httpBindingsWithSubMessageFields() {
Paths.get(Utils.getGoldenDir(this.getClass()), "GrpcPublisherStub.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}

@Test
public void generateGrpcServiceStubClass_createBatchingCallable() {
GapicContext context = GrpcTestProtoLoader.instance().parseLogging();
Service service = context.services().get(0);
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);

JavaWriterVisitor visitor = new JavaWriterVisitor();
clazz.classDefinition().accept(visitor);
Utils.saveCodegenToFile(this.getClass(), "GrpcLoggingStub.golden", visitor.write());
Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "GrpcLoggingStub.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}
}
Loading

0 comments on commit 3a6f168

Please sign in to comment.