From 46ad4363c0b0b3df345c0830b24cc703be8ad139 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Fri, 9 Oct 2020 18:37:46 -0700 Subject: [PATCH] [ggj][codegen] fix: space out codegen in GrpcServiceStub (#376) * fix: refactor requestBuilder into separate method in ServiceClientClassComposer * feat: add EmptyLineStatement * fix: space out lines in ServiceStubSettings * fix: space out codegen in GrpcServiceStub * fix: privately-scoped vars in ServiceClientTest codegen (#377) --- .../GrpcServiceStubClassComposer.java | 53 ++++++++++++++++--- .../ServiceClientTestClassComposer.java | 2 +- .../composer/goldens/EchoClientTest.golden | 8 +-- .../composer/goldens/GrpcEchoStub.golden | 12 +++++ 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/api/generator/gapic/composer/GrpcServiceStubClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/GrpcServiceStubClassComposer.java index 30b05204f9..73f0780209 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/GrpcServiceStubClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/GrpcServiceStubClassComposer.java @@ -28,6 +28,7 @@ import com.google.api.generator.engine.ast.AssignmentExpr; import com.google.api.generator.engine.ast.ClassDefinition; import com.google.api.generator.engine.ast.ConcreteReference; +import com.google.api.generator.engine.ast.EmptyLineStatement; import com.google.api.generator.engine.ast.EnumRefExpr; import com.google.api.generator.engine.ast.Expr; import com.google.api.generator.engine.ast.ExprStatement; @@ -68,6 +69,8 @@ import javax.annotation.Generated; public class GrpcServiceStubClassComposer implements ClassComposer { + private static final Statement EMPTY_LINE_STATEMENT = EmptyLineStatement.create(); + private static final String CLASS_NAME_PATTERN = "Grpc%sStub"; private static final String GRPC_SERVICE_CALLABLE_FACTORY_PATTERN = "Grpc%sCallableFactory"; private static final String METHOD_DESCRIPTOR_NAME_PATTERN = "%sMethodDescriptor"; @@ -131,11 +134,12 @@ public GapicClass generate(Service service, Map ignore) { .setType(staticTypes.get("GrpcStubCallableFactory")) .build())); - List classStatements = new ArrayList<>(); - classStatements.addAll( - createMethodDescriptorVariableDecls(service, protoMethodNameToDescriptorVarExprs)); - classStatements.addAll(createClassMemberFieldDeclarations(callableClassMemberVarExprs)); - classStatements.addAll(createClassMemberFieldDeclarations(classMemberVarExprs)); + List classStatements = + createClassStatements( + service, + protoMethodNameToDescriptorVarExprs, + callableClassMemberVarExprs, + classMemberVarExprs); ClassDefinition classDef = ClassDefinition.builder() @@ -158,6 +162,25 @@ public GapicClass generate(Service service, Map ignore) { return GapicClass.create(kind, classDef); } + private static List createClassStatements( + Service service, + Map protoMethodNameToDescriptorVarExprs, + Map callableClassMemberVarExprs, + Map classMemberVarExprs) { + List classStatements = new ArrayList<>(); + for (Statement statement : + createMethodDescriptorVariableDecls(service, protoMethodNameToDescriptorVarExprs)) { + classStatements.add(statement); + classStatements.add(EMPTY_LINE_STATEMENT); + } + + classStatements.addAll(createClassMemberFieldDeclarations(callableClassMemberVarExprs)); + classStatements.add(EMPTY_LINE_STATEMENT); + + classStatements.addAll(createClassMemberFieldDeclarations(classMemberVarExprs)); + return classStatements; + } + private static List createMethodDescriptorVariableDecls( Service service, Map protoMethodNameToDescriptorVarExprs) { return service.methods().stream() @@ -520,6 +543,7 @@ private static List createConstructorMethods( Expr thisExpr = ValueExpr.withValue(ThisObjectValue.withType(types.get(getThisClassName(service.name())))); // Body of the second constructor method. + List secondCtorStatements = new ArrayList<>(); List secondCtorExprs = new ArrayList<>(); secondCtorExprs.add( AssignmentExpr.builder() @@ -544,6 +568,10 @@ private static List createConstructorMethods( .setReturnType(operationsStubClassVarExpr.type()) .build()) .build()); + secondCtorStatements.addAll( + secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList())); + secondCtorExprs.clear(); + secondCtorStatements.add(EMPTY_LINE_STATEMENT); // Transport settings local variables. Map javaStyleMethodNameToTransportSettingsVarExprs = @@ -578,6 +606,10 @@ private static List createConstructorMethods( JavaStyle.toLowerCamelCase(m.name())), protoMethodNameToDescriptorVarExprs.get(m.name()))) .collect(Collectors.toList())); + secondCtorStatements.addAll( + secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList())); + secondCtorExprs.clear(); + secondCtorStatements.add(EMPTY_LINE_STATEMENT); // Initialize Callable variables. secondCtorExprs.addAll( @@ -594,6 +626,10 @@ private static List createConstructorMethods( thisExpr, javaStyleMethodNameToTransportSettingsVarExprs)) .collect(Collectors.toList())); + secondCtorStatements.addAll( + secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList())); + secondCtorExprs.clear(); + secondCtorStatements.add(EMPTY_LINE_STATEMENT); // Instantiate backgroundResources. MethodInvocationExpr getBackgroundResourcesMethodExpr = @@ -612,14 +648,15 @@ private static List createConstructorMethods( .setArguments(Arrays.asList(getBackgroundResourcesMethodExpr)) .build()) .build()); + secondCtorStatements.addAll( + secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList())); + secondCtorExprs.clear(); // Second constructor method. MethodDefinition secondCtor = ctorMakerFn.apply( Arrays.asList(settingsVarExpr, clientContextVarExpr, callableFactoryVarExpr), - secondCtorExprs.stream() - .map(e -> ExprStatement.withExpr(e)) - .collect(Collectors.toList())); + secondCtorStatements); return Arrays.asList(firstCtor, secondCtor); } diff --git a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientTestClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientTestClassComposer.java index 39c8836c6c..b200f74b5b 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientTestClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientTestClassComposer.java @@ -186,7 +186,7 @@ private static List createClassMemberFieldDecls( ExprStatement.withExpr( v.toBuilder() .setIsDecl(true) - .setScope(ScopeNode.PUBLIC) + .setScope(ScopeNode.PRIVATE) .setIsStatic(v.type().reference().name().startsWith("Mock")) .build())) .collect(Collectors.toList()); diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/EchoClientTest.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/EchoClientTest.golden index c1cf340662..1c026587a5 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/EchoClientTest.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/EchoClientTest.golden @@ -37,10 +37,10 @@ import org.junit.Test; @Generated("by gapic-generator-java") public class EchoClientTest { - public static MockServiceHelper mockServiceHelper; - public static MockEcho mockEcho; - public EchoClient client; - public LocalChannelProvider channelProvider; + private static MockServiceHelper mockServiceHelper; + private static MockEcho mockEcho; + private EchoClient client; + private LocalChannelProvider channelProvider; @BeforeClass public static void startStaticServer() { diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcEchoStub.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcEchoStub.golden index fdc6daf449..6cfd500bc0 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcEchoStub.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcEchoStub.golden @@ -45,6 +45,7 @@ public class GrpcEchoStub extends EchoStub { .setRequestMarshaller(ProtoUtils.marshaller(EchoRequest.getDefaultInstance())) .setResponseMarshaller(ProtoUtils.marshaller(EchoResponse.getDefaultInstance())) .build(); + private static final MethodDescriptor expandMethodDescriptor = MethodDescriptor.newBuilder() .setType(MethodDescriptor.MethodType.SERVER_STREAMING) @@ -52,6 +53,7 @@ public class GrpcEchoStub extends EchoStub { .setRequestMarshaller(ProtoUtils.marshaller(ExpandRequest.getDefaultInstance())) .setResponseMarshaller(ProtoUtils.marshaller(EchoResponse.getDefaultInstance())) .build(); + private static final MethodDescriptor collectMethodDescriptor = MethodDescriptor.newBuilder() .setType(MethodDescriptor.MethodType.CLIENT_STREAMING) @@ -59,6 +61,7 @@ public class GrpcEchoStub extends EchoStub { .setRequestMarshaller(ProtoUtils.marshaller(EchoRequest.getDefaultInstance())) .setResponseMarshaller(ProtoUtils.marshaller(EchoResponse.getDefaultInstance())) .build(); + private static final MethodDescriptor chatMethodDescriptor = MethodDescriptor.newBuilder() .setType(MethodDescriptor.MethodType.BIDI_STREAMING) @@ -66,6 +69,7 @@ public class GrpcEchoStub extends EchoStub { .setRequestMarshaller(ProtoUtils.marshaller(EchoRequest.getDefaultInstance())) .setResponseMarshaller(ProtoUtils.marshaller(EchoResponse.getDefaultInstance())) .build(); + private static final MethodDescriptor chatAgainMethodDescriptor = MethodDescriptor.newBuilder() .setType(MethodDescriptor.MethodType.BIDI_STREAMING) @@ -73,6 +77,7 @@ public class GrpcEchoStub extends EchoStub { .setRequestMarshaller(ProtoUtils.marshaller(EchoRequest.getDefaultInstance())) .setResponseMarshaller(ProtoUtils.marshaller(EchoResponse.getDefaultInstance())) .build(); + private static final MethodDescriptor pagedExpandMethodDescriptor = MethodDescriptor.newBuilder() @@ -82,6 +87,7 @@ public class GrpcEchoStub extends EchoStub { .setResponseMarshaller( ProtoUtils.marshaller(PagedExpandResponse.getDefaultInstance())) .build(); + private static final MethodDescriptor waitMethodDescriptor = MethodDescriptor.newBuilder() .setType(MethodDescriptor.MethodType.UNARY) @@ -89,6 +95,7 @@ public class GrpcEchoStub extends EchoStub { .setRequestMarshaller(ProtoUtils.marshaller(WaitRequest.getDefaultInstance())) .setResponseMarshaller(ProtoUtils.marshaller(Operation.getDefaultInstance())) .build(); + private static final MethodDescriptor blockMethodDescriptor = MethodDescriptor.newBuilder() .setType(MethodDescriptor.MethodType.UNARY) @@ -96,6 +103,7 @@ public class GrpcEchoStub extends EchoStub { .setRequestMarshaller(ProtoUtils.marshaller(BlockRequest.getDefaultInstance())) .setResponseMarshaller(ProtoUtils.marshaller(BlockResponse.getDefaultInstance())) .build(); + private final UnaryCallable echoCallable; private final ServerStreamingCallable expandCallable; private final ClientStreamingCallable collectCallable; @@ -107,6 +115,7 @@ public class GrpcEchoStub extends EchoStub { private final UnaryCallable waitCallable; private final OperationCallable waitOperationCallable; private final UnaryCallable blockCallable; + private final BackgroundResource backgroundResources; private final GrpcOperationsStub operationsStub; private final GrpcStubCallableFactory callableFactory; @@ -136,6 +145,7 @@ public class GrpcEchoStub extends EchoStub { throws IOException { this.callableFactory = callableFactory; this.operationsStub = GrpcOperationsStub.create(clientContext, callableFactory); + GrpcCallSettings echoTransportSettings = GrpcCallSettings.newBuilder() .setMethodDescriptor(echoMethodDescriptor) @@ -168,6 +178,7 @@ public class GrpcEchoStub extends EchoStub { GrpcCallSettings.newBuilder() .setMethodDescriptor(blockMethodDescriptor) .build(); + this.echoCallable = callableFactory.createUnaryCallable( echoTransportSettings, settings.echoSettings(), clientContext); @@ -198,6 +209,7 @@ public class GrpcEchoStub extends EchoStub { this.blockCallable = callableFactory.createUnaryCallable( blockTransportSettings, settings.blockSettings(), clientContext); + this.backgroundResources = new BackgroundResourceAggregation(clientContext.getBackgroundResources()); }