Skip to content

Commit 3a6f168

Browse files
chanseokohchanseokoh
authored
fix: gax batching regression (#863)
* fix: gax batching regression * Fix bug and add tests * Update golden files to fix tests * Revert some cleanups
1 parent 6bc4417 commit 3a6f168

File tree

9 files changed

+423
-46
lines changed

9 files changed

+423
-46
lines changed

‎src/main/java/com/google/api/generator/gapic/Generator.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ public static CodeGeneratorResponse generateGapic(CodeGeneratorRequest request)
3030
List<GapicClass> clazzes = Composer.composeServiceClasses(context);
3131
GapicPackageInfo packageInfo = Composer.composePackageInfo(context);
3232
String outputFilename = "temp-codegen.srcjar";
33-
CodeGeneratorResponse response = Writer.write(context, clazzes, packageInfo, outputFilename);
34-
return response;
33+
return Writer.write(context, clazzes, packageInfo, outputFilename);
3534
}
3635
}

‎src/main/java/com/google/api/generator/gapic/composer/Composer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ public static List<GapicClass> generateMockClasses(GapicContext context, List<Se
152152
services.forEach(
153153
s -> {
154154
if (context.transport() == Transport.REST) {
155-
// REST transport tests donot not use mock services.
155+
// REST transport tests do not use mock services.
156156
} else if (context.transport() == Transport.GRPC) {
157157
clazzes.add(MockServiceClassComposer.instance().generate(context, s));
158158
clazzes.add(MockServiceImplClassComposer.instance().generate(context, s));

‎src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java‎

Lines changed: 60 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,12 @@
5151
import com.google.api.generator.engine.ast.VariableExpr;
5252
import com.google.api.generator.gapic.composer.comment.StubCommentComposer;
5353
import com.google.api.generator.gapic.composer.store.TypeStore;
54+
import com.google.api.generator.gapic.composer.utils.ClassNames;
5455
import com.google.api.generator.gapic.composer.utils.PackageChecker;
5556
import com.google.api.generator.gapic.model.GapicClass;
5657
import com.google.api.generator.gapic.model.GapicClass.Kind;
5758
import com.google.api.generator.gapic.model.GapicContext;
59+
import com.google.api.generator.gapic.model.GapicServiceConfig;
5860
import com.google.api.generator.gapic.model.Message;
5961
import com.google.api.generator.gapic.model.Method;
6062
import com.google.api.generator.gapic.model.Service;
@@ -205,6 +207,7 @@ public GapicClass generate(GapicContext context, Service service) {
205207
.setStatements(classStatements)
206208
.setMethods(
207209
createClassMethods(
210+
context,
208211
service,
209212
typeStore,
210213
classMemberVarExprs,
@@ -422,6 +425,7 @@ protected List<AnnotationNode> createClassAnnotations(Service service) {
422425
}
423426

424427
protected List<MethodDefinition> createClassMethods(
428+
GapicContext context,
425429
Service service,
426430
TypeStore typeStore,
427431
Map<String, VariableExpr> classMemberVarExprs,
@@ -431,6 +435,7 @@ protected List<MethodDefinition> createClassMethods(
431435
javaMethods.addAll(createStaticCreatorMethods(service, typeStore, "newBuilder"));
432436
javaMethods.addAll(
433437
createConstructorMethods(
438+
context,
434439
service,
435440
typeStore,
436441
classMemberVarExprs,
@@ -531,6 +536,7 @@ protected List<MethodDefinition> createStaticCreatorMethods(
531536
}
532537

533538
protected List<MethodDefinition> createConstructorMethods(
539+
GapicContext context,
534540
Service service,
535541
TypeStore typeStore,
536542
Map<String, VariableExpr> classMemberVarExprs,
@@ -604,7 +610,9 @@ protected List<MethodDefinition> createConstructorMethods(
604610
secondCtorExprs.add(
605611
AssignmentExpr.builder()
606612
.setVariableExpr(
607-
classMemberVarExprs.get("callableFactory").toBuilder()
613+
classMemberVarExprs
614+
.get("callableFactory")
615+
.toBuilder()
608616
.setExprReferenceExpr(thisExpr)
609617
.build())
610618
.setValueExpr(callableFactoryVarExpr)
@@ -613,15 +621,16 @@ protected List<MethodDefinition> createConstructorMethods(
613621
classMemberVarExprs.get(getTransportContext().transportOperationsStubNames().get(0));
614622
// TODO: refactor this
615623
if (generateOperationsStubLogic(service)) {
616-
secondCtorExprs.addAll(createOperationsStubInitExpr(
617-
service,
618-
thisExpr,
619-
operationsStubClassVarExpr,
620-
clientContextVarExpr,
621-
callableFactoryVarExpr));
624+
secondCtorExprs.addAll(
625+
createOperationsStubInitExpr(
626+
service,
627+
thisExpr,
628+
operationsStubClassVarExpr,
629+
clientContextVarExpr,
630+
callableFactoryVarExpr));
622631
}
623632
secondCtorStatements.addAll(
624-
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
633+
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
625634
secondCtorExprs.clear();
626635
secondCtorStatements.add(EMPTY_LINE_STATEMENT);
627636

@@ -660,7 +669,7 @@ protected List<MethodDefinition> createConstructorMethods(
660669
protoMethodNameToDescriptorVarExprs.get(m.name())))
661670
.collect(Collectors.toList()));
662671
secondCtorStatements.addAll(
663-
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
672+
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
664673
secondCtorExprs.clear();
665674
secondCtorStatements.add(EMPTY_LINE_STATEMENT);
666675

@@ -670,6 +679,8 @@ protected List<MethodDefinition> createConstructorMethods(
670679
.map(
671680
e ->
672681
createCallableInitExpr(
682+
context,
683+
service,
673684
e.getKey(),
674685
e.getValue(),
675686
callableFactoryVarExpr,
@@ -680,7 +691,7 @@ protected List<MethodDefinition> createConstructorMethods(
680691
javaStyleMethodNameToTransportSettingsVarExprs))
681692
.collect(Collectors.toList()));
682693
secondCtorStatements.addAll(
683-
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
694+
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
684695
secondCtorExprs.clear();
685696
secondCtorStatements.add(EMPTY_LINE_STATEMENT);
686697

@@ -705,7 +716,7 @@ protected List<MethodDefinition> createConstructorMethods(
705716
.build())
706717
.build());
707718
secondCtorStatements.addAll(
708-
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
719+
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
709720
secondCtorExprs.clear();
710721

711722
// Second constructor method.
@@ -723,14 +734,14 @@ protected List<Expr> createOperationsStubInitExpr(
723734
VariableExpr operationsStubClassVarExpr,
724735
VariableExpr clientContextVarExpr,
725736
VariableExpr callableFactoryVarExpr) {
726-
TypeNode opeationsStubType = getTransportOperationsStubType(service);
737+
TypeNode operationsStubType = getTransportOperationsStubType(service);
727738
return Collections.singletonList(
728739
AssignmentExpr.builder()
729740
.setVariableExpr(
730741
operationsStubClassVarExpr.toBuilder().setExprReferenceExpr(thisExpr).build())
731742
.setValueExpr(
732743
MethodInvocationExpr.builder()
733-
.setStaticReferenceType(opeationsStubType)
744+
.setStaticReferenceType(operationsStubType)
734745
.setMethodName("create")
735746
.setArguments(Arrays.asList(clientContextVarExpr, callableFactoryVarExpr))
736747
.setReturnType(operationsStubClassVarExpr.type())
@@ -748,6 +759,8 @@ protected VariableExpr declareLongRunningClient() {
748759
}
749760

750761
private Expr createCallableInitExpr(
762+
GapicContext context,
763+
Service service,
751764
String callableVarName,
752765
VariableExpr callableVarExpr,
753766
VariableExpr callableFactoryVarExpr,
@@ -758,7 +771,7 @@ private Expr createCallableInitExpr(
758771
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs) {
759772
boolean isOperation = callableVarName.endsWith(OPERATION_CALLABLE_NAME);
760773
boolean isPaged = callableVarName.endsWith(PAGED_CALLABLE_NAME);
761-
int sublength = 0;
774+
int sublength;
762775
if (isOperation) {
763776
sublength = OPERATION_CALLABLE_NAME.length();
764777
} else if (isPaged) {
@@ -767,11 +780,11 @@ private Expr createCallableInitExpr(
767780
sublength = CALLABLE_NAME.length();
768781
}
769782
String javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
770-
List<Expr> creatorMethodArgVarExprs = null;
783+
List<Expr> creatorMethodArgVarExprs;
771784
Expr transportSettingsVarExpr =
772785
javaStyleMethodNameToTransportSettingsVarExprs.get(javaStyleMethodName);
773786
if (transportSettingsVarExpr == null && isOperation) {
774-
// Try again, in case the name dtection above was inaccurate.
787+
// Try again, in case the name detection above was inaccurate.
775788
isOperation = false;
776789
sublength = CALLABLE_NAME.length();
777790
javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
@@ -803,8 +816,9 @@ private Expr createCallableInitExpr(
803816
clientContextVarExpr);
804817
}
805818

819+
String methodName = JavaStyle.toUpperCamelCase(javaStyleMethodName);
806820
Optional<String> callableCreatorMethodName =
807-
getCallableCreatorMethodName(callableVarExpr.type());
821+
getCallableCreatorMethodName(context, service, callableVarExpr.type(), methodName);
808822

809823
Expr initExpr;
810824
if (callableCreatorMethodName.isPresent()) {
@@ -825,26 +839,37 @@ private Expr createCallableInitExpr(
825839
.build();
826840
}
827841

828-
protected Optional<String> getCallableCreatorMethodName(TypeNode callableVarExprType) {
829-
final String typeName = callableVarExprType.reference().name();
830-
String streamName = "Unary";
831-
842+
protected Optional<String> getCallableCreatorMethodName(
843+
GapicContext context,
844+
Service service,
845+
TypeNode callableVarExprType,
846+
String serviceMethodName) {
847+
GapicServiceConfig serviceConfig = context.serviceConfig();
848+
if (serviceConfig != null
849+
&& serviceConfig.hasBatchingSetting(
850+
service.protoPakkage(), service.name(), serviceMethodName)) {
851+
return Optional.of("createBatchingCallable");
852+
}
832853
// Special handling for pagination methods.
833854
if (callableVarExprType.reference().generics().size() == 2
834855
&& callableVarExprType.reference().generics().get(1).name().endsWith("PagedResponse")) {
835-
streamName = "Paged";
836-
} else {
837-
if (typeName.startsWith("Client")) {
838-
streamName = "ClientStreaming";
839-
} else if (typeName.startsWith("Server")) {
840-
streamName = "ServerStreaming";
841-
} else if (typeName.startsWith("Bidi")) {
842-
streamName = "BidiStreaming";
843-
} else if (typeName.startsWith("Operation")) {
844-
streamName = "Operation";
845-
}
856+
return Optional.of("createPagedCallable");
857+
}
858+
859+
String typeName = callableVarExprType.reference().name();
860+
if (typeName.startsWith("Client")) {
861+
return Optional.of("createClientStreamingCallable");
862+
}
863+
if (typeName.startsWith("Server")) {
864+
return Optional.of("createServerStreamingCallable");
865+
}
866+
if (typeName.startsWith("Bidi")) {
867+
return Optional.of("createBidiStreamingCallable");
868+
}
869+
if (typeName.startsWith("Operation")) {
870+
return Optional.of("createOperationCallable");
846871
}
847-
return Optional.of(String.format("create%sCallable", streamName));
872+
return Optional.of("createUnaryCallable");
848873
}
849874

850875
private static List<MethodDefinition> createCallableGetterMethods(
@@ -960,7 +985,7 @@ private List<MethodDefinition> createStubOverrideMethods(
960985
.setType(
961986
TypeNode.withExceptionClazz(
962987
IllegalStateException.class))
963-
.setMessageExpr(String.format("Failed to close resource"))
988+
.setMessageExpr("Failed to close resource")
964989
.setCauseExpr(catchExceptionVarExpr)
965990
.build())))
966991
.build()))
@@ -1041,7 +1066,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
10411066
typeStore.putAll(
10421067
service.pakkage(),
10431068
service.methods().stream()
1044-
.filter(m -> m.isPaged())
1069+
.filter(Method::isPaged)
10451070
.map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
10461071
.collect(Collectors.toList()),
10471072
true,

‎src/main/java/com/google/api/generator/util/Trie.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
* A common-prefix trie. T represents the type of each "char" in a word (which is a T-typed list).
2525
*/
2626
public class Trie<T> {
27-
private class Node<U> {
27+
private static class Node<U> {
2828
final U chr;
2929
// Maintain insertion order to enable deterministic test output.
3030
Map<U, Node<U>> children = new LinkedHashMap<>();
@@ -39,7 +39,7 @@ private class Node<U> {
3939
}
4040
}
4141

42-
private Node<T> root;
42+
private final Node<T> root;
4343

4444
public Trie() {
4545
root = new Node<>();

‎src/test/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposerTest.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,8 @@ public void generateGrpcServiceStubClass_deprecated() {
5555
@Test
5656
public void generateGrpcServiceStubClass_httpBindings() {
5757
GapicContext context = GrpcTestProtoLoader.instance().parseShowcaseTesting();
58-
Service testingProtoService = context.services().get(0);
59-
GapicClass clazz =
60-
GrpcServiceStubClassComposer.instance().generate(context, testingProtoService);
58+
Service service = context.services().get(0);
59+
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);
6160

6261
JavaWriterVisitor visitor = new JavaWriterVisitor();
6362
clazz.classDefinition().accept(visitor);
@@ -79,4 +78,17 @@ public void generateGrpcServiceStubClass_httpBindingsWithSubMessageFields() {
7978
Paths.get(Utils.getGoldenDir(this.getClass()), "GrpcPublisherStub.golden");
8079
Assert.assertCodeEquals(goldenFilePath, visitor.write());
8180
}
81+
82+
@Test
83+
public void generateGrpcServiceStubClass_createBatchingCallable() {
84+
GapicContext context = GrpcTestProtoLoader.instance().parseLogging();
85+
Service service = context.services().get(0);
86+
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);
87+
88+
JavaWriterVisitor visitor = new JavaWriterVisitor();
89+
clazz.classDefinition().accept(visitor);
90+
Utils.saveCodegenToFile(this.getClass(), "GrpcLoggingStub.golden", visitor.write());
91+
Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "GrpcLoggingStub.golden");
92+
Assert.assertCodeEquals(goldenFilePath, visitor.write());
93+
}
8294
}

0 commit comments

Comments
 (0)