fix: refactor requestBuilder into separate method in ServiceClientCla…

…ssComposer

src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java

@@ -64,6 +64,7 @@
 import com.google.api.generator.gapic.model.MethodArgument;
 import com.google.api.generator.gapic.model.Service;
 import com.google.api.generator.gapic.utils.JavaStyle;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.util.concurrent.MoreExecutors;
@@ -90,6 +91,9 @@ public class ServiceClientClassComposer implements ClassComposer {
   private static final String PAGED_CALLABLE_NAME_PATTERN = "%sPagedCallable";
   private static final String OPERATION_CALLABLE_NAME_PATTERN = "%sOperationCallable";
 
+  private static final Reference LIST_REFERENCE = ConcreteReference.withClazz(List.class);
+  private static final Reference MAP_REFERENCE = ConcreteReference.withClazz(Map.class);
+
   private enum CallableMethodKind {
     REGULAR,
     LRO,
@@ -499,8 +503,6 @@ private static List<MethodDefinition> createMethodVariants(
     }
 
     String methodInputTypeName = methodInputType.reference().name();
-    Reference listRef = ConcreteReference.withClazz(List.class);
-    Reference mapRef = ConcreteReference.withClazz(Map.class);
 
     // Make the method signature order deterministic, which helps with unit testing and per-version
     // diffs.
@@ -544,71 +546,12 @@ private static List<MethodDefinition> createMethodVariants(
               .setIsDecl(true)
               .build();
 
-      MethodInvocationExpr newBuilderExpr =
-          MethodInvocationExpr.builder()
-              .setMethodName("newBuilder")
-              .setStaticReferenceType(methodInputType)
-              .build();
-      // TODO(miraleung): Handle nested arguments and descending setters here.
-      for (MethodArgument argument : signature) {
-        String argumentName = JavaStyle.toLowerCamelCase(argument.name());
-        TypeNode argumentType = argument.type();
-        String setterMethodVariantPattern = "set%s";
-        if (TypeNode.isReferenceType(argumentType)) {
-          if (listRef.isSupertypeOrEquals(argumentType.reference())) {
-            setterMethodVariantPattern = "addAll%s";
-          } else if (mapRef.isSupertypeOrEquals(argumentType.reference())) {
-            setterMethodVariantPattern = "putAll%s";
-          }
-        }
-        String setterMethodName =
-            String.format(setterMethodVariantPattern, JavaStyle.toUpperCamelCase(argumentName));
-
-        Expr argVarExpr =
-            VariableExpr.withVariable(
-                Variable.builder().setName(argumentName).setType(argumentType).build());
-
-        if (argument.isResourceNameHelper()) {
-          MethodInvocationExpr isNullCheckExpr =
-              MethodInvocationExpr.builder()
-                  .setStaticReferenceType(types.get("Objects"))
-                  .setMethodName("isNull")
-                  .setArguments(Arrays.asList(argVarExpr))
-                  .setReturnType(TypeNode.BOOLEAN)
-                  .build();
-          Expr nullExpr = ValueExpr.withValue(NullObjectValue.create());
-          MethodInvocationExpr toStringExpr =
-              MethodInvocationExpr.builder()
-                  .setExprReferenceExpr(argVarExpr)
-                  .setMethodName("toString")
-                  .setReturnType(TypeNode.STRING)
-                  .build();
-          argVarExpr =
-              TernaryExpr.builder()
-                  .setConditionExpr(isNullCheckExpr)
-                  .setThenExpr(nullExpr)
-                  .setElseExpr(toStringExpr)
-                  .build();
-        }
+      Expr requestBuilderExpr = createRequestBuilderExpr(method, signature, types);
 
-        newBuilderExpr =
-            MethodInvocationExpr.builder()
-                .setMethodName(setterMethodName)
-                .setArguments(Arrays.asList(argVarExpr))
-                .setExprReferenceExpr(newBuilderExpr)
-                .build();
-      }
-
-      MethodInvocationExpr builderExpr =
-          MethodInvocationExpr.builder()
-              .setMethodName("build")
-              .setExprReferenceExpr(newBuilderExpr)
-              .setReturnType(methodInputType)
-              .build();
       AssignmentExpr requestAssignmentExpr =
           AssignmentExpr.builder()
               .setVariableExpr(requestVarExpr)
-              .setValueExpr(builderExpr)
+              .setValueExpr(requestBuilderExpr)
               .build();
       List<Statement> statements = Arrays.asList(ExprStatement.withExpr(requestAssignmentExpr));
 
@@ -1373,6 +1316,75 @@ private static ClassDefinition createNestedRpcFixedSizeCollectionClass(
         .build();
   }
 
+  @VisibleForTesting
+  static Expr createRequestBuilderExpr(
+      Method method, List<MethodArgument> signature, Map<String, TypeNode> types) {
+    TypeNode methodInputType = method.inputType();
+    MethodInvocationExpr newBuilderExpr =
+        MethodInvocationExpr.builder()
+            .setMethodName("newBuilder")
+            .setStaticReferenceType(methodInputType)
+            .build();
+    // TODO(miraleung): Handle nested arguments and descending setters here.
+    for (MethodArgument argument : signature) {
+      String argumentName = JavaStyle.toLowerCamelCase(argument.name());
+      TypeNode argumentType = argument.type();
+      String setterMethodVariantPattern = "set%s";
+      if (TypeNode.isReferenceType(argumentType)) {
+        if (LIST_REFERENCE.isSupertypeOrEquals(argumentType.reference())) {
+          setterMethodVariantPattern = "addAll%s";
+        } else if (MAP_REFERENCE.isSupertypeOrEquals(argumentType.reference())) {
+          setterMethodVariantPattern = "putAll%s";
+        }
+      }
+      String setterMethodName =
+          String.format(setterMethodVariantPattern, JavaStyle.toUpperCamelCase(argumentName));
+
+      Expr argVarExpr =
+          VariableExpr.withVariable(
+              Variable.builder().setName(argumentName).setType(argumentType).build());
+
+      if (argument.isResourceNameHelper()) {
+        MethodInvocationExpr isNullCheckExpr =
+            MethodInvocationExpr.builder()
+                .setStaticReferenceType(types.get("Objects"))
+                .setMethodName("isNull")
+                .setArguments(Arrays.asList(argVarExpr))
+                .setReturnType(TypeNode.BOOLEAN)
+                .build();
+        Expr nullExpr = ValueExpr.withValue(NullObjectValue.create());
+        MethodInvocationExpr toStringExpr =
+            MethodInvocationExpr.builder()
+                .setExprReferenceExpr(argVarExpr)
+                .setMethodName("toString")
+                .setReturnType(TypeNode.STRING)
+                .build();
+        argVarExpr =
+            TernaryExpr.builder()
+                .setConditionExpr(isNullCheckExpr)
+                .setThenExpr(nullExpr)
+                .setElseExpr(toStringExpr)
+                .build();
+      }
+
+      newBuilderExpr =
+          MethodInvocationExpr.builder()
+              .setMethodName(setterMethodName)
+              .setArguments(Arrays.asList(argVarExpr))
+              .setExprReferenceExpr(newBuilderExpr)
+              .build();
+    }
+
+    MethodInvocationExpr builderExpr =
+        MethodInvocationExpr.builder()
+            .setMethodName("build")
+            .setExprReferenceExpr(newBuilderExpr)
+            .setReturnType(methodInputType)
+            .build();
+
+    return builderExpr;
+  }
+
   private static Map<String, TypeNode> createTypes(
       Service service, Map<String, Message> messageTypes) {
     Map<String, TypeNode> types = new HashMap<>();

src/test/java/com/google/api/generator/gapic/composer/BUILD.bazel

@@ -11,6 +11,8 @@ UPDATE_GOLDENS_TESTS = [
     "MockServiceClassComposerTest",
     "MockServiceImplClassComposerTest",
     "ResourceNameHelperClassComposerTest",
+    "ServiceClientClassComposerTest",
+    "ServiceClientTestClassComposerTest",
     "ServiceSettingsClassComposerTest",
     "ServiceStubSettingsClassComposerTest",
     "ServiceStubClassComposerTest",
@@ -20,8 +22,6 @@ TESTS = UPDATE_GOLDENS_TESTS + [
     "DefaultValueComposerTest",
     "ResourceNameTokenizerTest",
     "RetrySettingsComposerTest",
-    "ServiceClientClassComposerTest",
-    "ServiceClientTestClassComposerTest",
 ]
 
 TEST_DEPS = [

src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java

@@ -27,28 +27,22 @@
 import com.google.protobuf.Descriptors.FileDescriptor;
 import com.google.protobuf.Descriptors.ServiceDescriptor;
 import com.google.showcase.v1beta1.EchoOuterClass;
+import com.google.showcase.v1beta1.IdentityOuterClass;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import org.junit.Before;
 import org.junit.Test;
 
 public class ServiceClientClassComposerTest {
-  private ServiceDescriptor echoService;
-  private FileDescriptor echoFileDescriptor;
-
-  @Before
-  public void setUp() {
-    echoFileDescriptor = EchoOuterClass.getDescriptor();
-    echoService = echoFileDescriptor.getServices().get(0);
-    assertEquals(echoService.getName(), "Echo");
-  }
-
   @Test
   public void generateServiceClasses() {
+    FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
+    ServiceDescriptor echoService = echoFileDescriptor.getServices().get(0);
+    assertEquals(echoService.getName(), "Echo");
+
     Map<String, Message> messageTypes = Parser.parseMessages(echoFileDescriptor);
     Map<String, ResourceName> resourceNames = Parser.parseResourceNames(echoFileDescriptor);
     Set<ResourceName> outputResourceNames = new HashSet<>();
@@ -65,4 +59,27 @@ public void generateServiceClasses() {
     Path goldenFilePath = Paths.get(ComposerConstants.GOLDENFILES_DIRECTORY, "EchoClient.golden");
     assertCodeEquals(goldenFilePath, visitor.write());
   }
+
+  @Test
+  public void generateServiceClasses_methodSignatureHasNestedFields() {
+    FileDescriptor fileDescriptor = IdentityOuterClass.getDescriptor();
+    ServiceDescriptor identityService = fileDescriptor.getServices().get(0);
+    assertEquals(identityService.getName(), "Identity");
+
+    Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor);
+    Map<String, ResourceName> resourceNames = Parser.parseResourceNames(fileDescriptor);
+    Set<ResourceName> outputResourceNames = new HashSet<>();
+    List<Service> services =
+        Parser.parseService(fileDescriptor, messageTypes, resourceNames, outputResourceNames);
+
+    Service protoService = services.get(0);
+    GapicClass clazz = ServiceClientClassComposer.instance().generate(protoService, messageTypes);
+
+    JavaWriterVisitor visitor = new JavaWriterVisitor();
+    clazz.classDefinition().accept(visitor);
+    Utils.saveCodegenToFile(this.getClass(), "IdentityClient.golden", visitor.write());
+    Path goldenFilePath =
+        Paths.get(ComposerConstants.GOLDENFILES_DIRECTORY, "IdentityClient.golden");
+    assertCodeEquals(goldenFilePath, visitor.write());
+  }
 }