fix: [gapic-generator-java] align writer behavior for nested types (#1709)

emmileaf · web-flow · commit a21ffe88f880 · 2023-06-05T13:55:15.000-04:00
* Removes duplicate handling of enclosing classes in JavaWriterVisitor and ImportWriterVisitor - change to only import outermost class, e.g. com.example.Outer), while type references will be written as Outer.Middle.Inner
* Fixes issue in ConcreteReference where multiple levels of enclosed classes are written in reversed order
diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java b/gapic-generator-java/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java
@@ -103,14 +103,15 @@ public ImmutableList<String> enclosingClassNames() {
     if (!hasEnclosingClass()) {
       return ImmutableList.of();
     }
-    // The innermost type will be the last element in the list.
+    // Builds list in order of inner to outer.
+    // Return the reversed list, since the outermost type is expected to lie at index 0.
     ImmutableList.Builder<String> listBuilder = new ImmutableList.Builder<>();
     Class<?> currentClz = clazz();
     while (currentClz.getEnclosingClass() != null) {
       listBuilder.add(currentClz.getEnclosingClass().getSimpleName());
       currentClz = currentClz.getEnclosingClass();
     }
-    return listBuilder.build();
+    return listBuilder.build().reverse();
   }
 
   @Override
diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java b/gapic-generator-java/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java
@@ -508,9 +508,11 @@ private void handleReference(Reference reference) {
       staticImports.add(reference.fullName());
     } else {
       if (reference.hasEnclosingClass()) {
+        // Only import outermost enclosing class, e.g. import com.foo.bar.Outer
         addImport(
             String.format(
-                "%s.%s", reference.pakkage(), String.join(DOT, reference.enclosingClassNames())));
+                "%s.%s",
+                reference.pakkage(), String.join(DOT, reference.enclosingClassNames().get(0))));
       } else {
         addImport(reference.fullName());
       }
diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java b/gapic-generator-java/src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java
@@ -58,6 +58,7 @@
 import com.google.api.generator.test.utils.LineFormatter;
 import com.google.common.base.Function;
 import com.google.common.base.Strings;
+import com.google.testgapic.v1beta1.Outer;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
@@ -428,6 +429,27 @@ public void writeVariableExprImports_annotationsWithDescription() {
         writerVisitor.write());
   }
 
+  @Test
+  public void writeVaporReferenceImport_outermostForNestedClass() {
+    VaporReference nestedVaporReference =
+        VaporReference.builder()
+            .setName("Inner")
+            .setEnclosingClassNames(Arrays.asList("Outer", "Middle"))
+            .setPakkage("com.google.testgapic.v1beta1")
+            .build();
+
+    TypeNode.withReference(nestedVaporReference).accept(writerVisitor);
+    assertEquals("import com.google.testgapic.v1beta1.Outer;\n\n", writerVisitor.write());
+  }
+
+  @Test
+  public void writeConcreteReferenceImport_outermostForNestedClass() {
+    ConcreteReference nestedConcreteReference =
+        ConcreteReference.withClazz(Outer.Middle.Inner.class);
+    TypeNode.withReference(nestedConcreteReference).accept(writerVisitor);
+    assertEquals("import com.google.testgapic.v1beta1.Outer;\n\n", writerVisitor.write());
+  }
+
   @Test
   public void writeAnonymousClassExprImports() {
     // [Constructing] Function<List<IOException>, MethodDefinition>
diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java b/gapic-generator-java/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java
@@ -71,9 +71,16 @@
 import com.google.api.generator.engine.ast.Variable;
 import com.google.api.generator.engine.ast.VariableExpr;
 import com.google.api.generator.engine.ast.WhileStatement;
+import com.google.api.generator.gapic.composer.grpc.ServiceClientClassComposer;
+import com.google.api.generator.gapic.model.GapicClass;
+import com.google.api.generator.gapic.model.GapicContext;
+import com.google.api.generator.gapic.model.Service;
+import com.google.api.generator.test.framework.Assert;
+import com.google.api.generator.test.protoloader.TestProtoLoader;
 import com.google.api.generator.test.utils.LineFormatter;
 import com.google.api.generator.test.utils.TestExprBuilder;
 import com.google.common.base.Function;
+import com.google.testgapic.v1beta1.Outer;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
@@ -129,6 +136,26 @@ public void writeReferenceType_basic() {
     assertEquals("FooBar", writerVisitor.write());
   }
 
+  @Test
+  public void writeVaporReferenceType_nestedClasses() {
+    VaporReference nestedVaporReference =
+        VaporReference.builder()
+            .setName("Inner")
+            .setEnclosingClassNames(Arrays.asList("Outer", "Middle"))
+            .setPakkage("com.google.testgapic.v1beta1")
+            .build();
+    TypeNode.withReference(nestedVaporReference).accept(writerVisitor);
+    assertEquals("Outer.Middle.Inner", writerVisitor.write());
+  }
+
+  @Test
+  public void writeConcreteReferenceType_nestedClasses() {
+    ConcreteReference nestedConcreteReference =
+        ConcreteReference.withClazz(Outer.Middle.Inner.class);
+    TypeNode.withReference(nestedConcreteReference).accept(writerVisitor);
+    assertEquals("Outer.Middle.Inner", writerVisitor.write());
+  }
+
   @Test
   public void writeReferenceType_useFullName() {
     TypeNode.withReference(
@@ -2796,6 +2823,17 @@ public void writePackageInfoDefinition() {
         writerVisitor.write());
   }
 
+  /** =============================== GOLDEN TESTS =============================== */
+  @Test
+  public void writeSGrpcServiceClientWithNestedClassImport() {
+    GapicContext context = TestProtoLoader.instance().parseNestedMessage();
+    Service nestedService = context.services().get(0);
+    GapicClass clazz = ServiceClientClassComposer.instance().generate(context, nestedService);
+
+    Assert.assertGoldenClass(
+        this.getClass(), clazz, "GrpcServiceClientWithNestedClassImport.golden");
+  }
+
   /** =============================== HELPERS =============================== */
   private static AssignmentExpr createAssignmentExpr(
       String variableName, String value, TypeNode type) {
diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/engine/writer/goldens/GrpcServiceClientWithNestedClassImport.golden b/gapic-generator-java/src/test/java/com/google/api/generator/engine/writer/goldens/GrpcServiceClientWithNestedClassImport.golden
@@ -0,0 +1,216 @@
+package com.google.types.testing;
+
+import com.google.api.gax.core.BackgroundResource;
+import com.google.api.gax.rpc.UnaryCallable;
+import com.google.testgapic.v1beta1.Outer;
+import com.google.types.testing.stub.NestedMessageServiceStub;
+import com.google.types.testing.stub.NestedMessageServiceStubSettings;
+import java.io.IOException;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Generated;
+
+// AUTO-GENERATED DOCUMENTATION AND CLASS.
+/**
+ * This class provides the ability to make remote calls to the backing service through method calls
+ * that map to API methods. Sample code to get started:
+ *
+ * <pre>{@code
+ * // This snippet has been automatically generated and should be regarded as a code template only.
+ * // It will require modifications to work:
+ * // - It may require correct/in-range values for request initialization.
+ * // - It may require specifying regional endpoints when creating the service client as shown in
+ * // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
+ * try (NestedMessageServiceClient nestedMessageServiceClient =
+ *     NestedMessageServiceClient.create()) {
+ *   Outer.Middle request = Outer.Middle.newBuilder().setX(120).build();
+ *   Outer.Middle.Inner response = nestedMessageServiceClient.nestedMessageMethod(request);
+ * }
+ * }</pre>
+ *
+ * <p>Note: close() needs to be called on the NestedMessageServiceClient object to clean up
+ * resources such as threads. In the example above, try-with-resources is used, which automatically
+ * calls close().
+ *
+ * <p>The surface of this class includes several types of Java methods for each of the API's
+ * methods:
+ *
+ * <ol>
+ *   <li>A "flattened" method. With this type of method, the fields of the request type have been
+ *       converted into function parameters. It may be the case that not all fields are available as
+ *       parameters, and not every API method will have a flattened method entry point.
+ *   <li>A "request object" method. This type of method only takes one parameter, a request object,
+ *       which must be constructed before the call. Not every API method will have a request object
+ *       method.
+ *   <li>A "callable" method. This type of method takes no parameters and returns an immutable API
+ *       callable object, which can be used to initiate calls to the service.
+ * </ol>
+ *
+ * <p>See the individual methods for example code.
+ *
+ * <p>Many parameters require resource names to be formatted in a particular way. To assist with
+ * these names, this class includes a format method for each type of name, and additionally a parse
+ * method to extract the individual identifiers contained within names that are returned.
+ *
+ * <p>This class can be customized by passing in a custom instance of NestedMessageServiceSettings
+ * to create(). For example:
+ *
+ * <p>To customize credentials:
+ *
+ * <pre>{@code
+ * // This snippet has been automatically generated and should be regarded as a code template only.
+ * // It will require modifications to work:
+ * // - It may require correct/in-range values for request initialization.
+ * // - It may require specifying regional endpoints when creating the service client as shown in
+ * // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
+ * NestedMessageServiceSettings nestedMessageServiceSettings =
+ *     NestedMessageServiceSettings.newBuilder()
+ *         .setCredentialsProvider(FixedCredentialsProvider.create(myCredentials))
+ *         .build();
+ * NestedMessageServiceClient nestedMessageServiceClient =
+ *     NestedMessageServiceClient.create(nestedMessageServiceSettings);
+ * }</pre>
+ *
+ * <p>To customize the endpoint:
+ *
+ * <pre>{@code
+ * // This snippet has been automatically generated and should be regarded as a code template only.
+ * // It will require modifications to work:
+ * // - It may require correct/in-range values for request initialization.
+ * // - It may require specifying regional endpoints when creating the service client as shown in
+ * // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
+ * NestedMessageServiceSettings nestedMessageServiceSettings =
+ *     NestedMessageServiceSettings.newBuilder().setEndpoint(myEndpoint).build();
+ * NestedMessageServiceClient nestedMessageServiceClient =
+ *     NestedMessageServiceClient.create(nestedMessageServiceSettings);
+ * }</pre>
+ *
+ * <p>Please refer to the GitHub repository's samples for more quickstart code snippets.
+ */
+@Generated("by gapic-generator-java")
+public class NestedMessageServiceClient implements BackgroundResource {
+  private final NestedMessageServiceSettings settings;
+  private final NestedMessageServiceStub stub;
+
+  /** Constructs an instance of NestedMessageServiceClient with default settings. */
+  public static final NestedMessageServiceClient create() throws IOException {
+    return create(NestedMessageServiceSettings.newBuilder().build());
+  }
+
+  /**
+   * Constructs an instance of NestedMessageServiceClient, using the given settings. The channels
+   * are created based on the settings passed in, or defaults for any settings that are not set.
+   */
+  public static final NestedMessageServiceClient create(NestedMessageServiceSettings settings)
+      throws IOException {
+    return new NestedMessageServiceClient(settings);
+  }
+
+  /**
+   * Constructs an instance of NestedMessageServiceClient, using the given stub for making calls.
+   * This is for advanced usage - prefer using create(NestedMessageServiceSettings).
+   */
+  public static final NestedMessageServiceClient create(NestedMessageServiceStub stub) {
+    return new NestedMessageServiceClient(stub);
+  }
+
+  /**
+   * Constructs an instance of NestedMessageServiceClient, using the given settings. This is
+   * protected so that it is easy to make a subclass, but otherwise, the static factory methods
+   * should be preferred.
+   */
+  protected NestedMessageServiceClient(NestedMessageServiceSettings settings) throws IOException {
+    this.settings = settings;
+    this.stub = ((NestedMessageServiceStubSettings) settings.getStubSettings()).createStub();
+  }
+
+  protected NestedMessageServiceClient(NestedMessageServiceStub stub) {
+    this.settings = null;
+    this.stub = stub;
+  }
+
+  public final NestedMessageServiceSettings getSettings() {
+    return settings;
+  }
+
+  public NestedMessageServiceStub getStub() {
+    return stub;
+  }
+
+  // AUTO-GENERATED DOCUMENTATION AND METHOD.
+  /**
+   * Sample code:
+   *
+   * <pre>{@code
+   * // This snippet has been automatically generated and should be regarded as a code template only.
+   * // It will require modifications to work:
+   * // - It may require correct/in-range values for request initialization.
+   * // - It may require specifying regional endpoints when creating the service client as shown in
+   * // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
+   * try (NestedMessageServiceClient nestedMessageServiceClient =
+   *     NestedMessageServiceClient.create()) {
+   *   Outer.Middle request = Outer.Middle.newBuilder().setX(120).build();
+   *   Outer.Middle.Inner response = nestedMessageServiceClient.nestedMessageMethod(request);
+   * }
+   * }</pre>
+   *
+   * @param request The request object containing all of the parameters for the API call.
+   * @throws com.google.api.gax.rpc.ApiException if the remote call fails
+   */
+  public final Outer.Middle.Inner nestedMessageMethod(Outer.Middle request) {
+    return nestedMessageMethodCallable().call(request);
+  }
+
+  // AUTO-GENERATED DOCUMENTATION AND METHOD.
+  /**
+   * Sample code:
+   *
+   * <pre>{@code
+   * // This snippet has been automatically generated and should be regarded as a code template only.
+   * // It will require modifications to work:
+   * // - It may require correct/in-range values for request initialization.
+   * // - It may require specifying regional endpoints when creating the service client as shown in
+   * // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
+   * try (NestedMessageServiceClient nestedMessageServiceClient =
+   *     NestedMessageServiceClient.create()) {
+   *   Outer.Middle request = Outer.Middle.newBuilder().setX(120).build();
+   *   ApiFuture<Outer.Middle.Inner> future =
+   *       nestedMessageServiceClient.nestedMessageMethodCallable().futureCall(request);
+   *   // Do something.
+   *   Outer.Middle.Inner response = future.get();
+   * }
+   * }</pre>
+   */
+  public final UnaryCallable<Outer.Middle, Outer.Middle.Inner> nestedMessageMethodCallable() {
+    return stub.nestedMessageMethodCallable();
+  }
+
+  @Override
+  public final void close() {
+    stub.close();
+  }
+
+  @Override
+  public void shutdown() {
+    stub.shutdown();
+  }
+
+  @Override
+  public boolean isShutdown() {
+    return stub.isShutdown();
+  }
+
+  @Override
+  public boolean isTerminated() {
+    return stub.isTerminated();
+  }
+
+  @Override
+  public void shutdownNow() {
+    stub.shutdownNow();
+  }
+
+  @Override
+  public boolean awaitTermination(long duration, TimeUnit unit) throws InterruptedException {
+    return stub.awaitTermination(duration, unit);
+  }
+}
diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/test/protoloader/TestProtoLoader.java b/gapic-generator-java/src/test/java/com/google/api/generator/test/protoloader/TestProtoLoader.java
@@ -41,6 +41,8 @@
 import com.google.showcase.v1beta1.MessagingOuterClass;
 import com.google.showcase.v1beta1.TestingOuterClass;
 import com.google.testdata.v1.DeprecatedServiceOuterClass;
+import com.google.testgapic.v1beta1.NestedMessageProto;
+import com.google.types.testing.TypesTestingProto;
 import google.cloud.CommonResources;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -131,6 +133,30 @@ public GapicContext parseBookshopService() {
         .build();
   }
 
+  public GapicContext parseNestedMessage() {
+    FileDescriptor fileDescriptor = TypesTestingProto.getDescriptor();
+    ServiceDescriptor serviceDescriptor = fileDescriptor.getServices().get(0);
+    assertEquals(serviceDescriptor.getName(), "NestedMessageService");
+    Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor);
+
+    FileDescriptor messageFileDescriptor = NestedMessageProto.getDescriptor();
+    messageTypes.putAll(Parser.parseMessages(messageFileDescriptor));
+
+    Map<String, ResourceName> resourceNames = new HashMap<>();
+    Set<ResourceName> outputResourceNames = new HashSet<>();
+    List<Service> services =
+        Parser.parseService(
+            fileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
+
+    return GapicContext.builder()
+        .setMessages(messageTypes)
+        .setResourceNames(resourceNames)
+        .setServices(services)
+        .setHelperResourceNames(outputResourceNames)
+        .setTransport(transport)
+        .build();
+  }
+
   public GapicContext parseShowcaseEcho() {
     FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
     ServiceDescriptor echoServiceDescriptor = echoFileDescriptor.getServices().get(0);
diff --git a/gapic-generator-java/src/test/proto/types_testing.proto b/gapic-generator-java/src/test/proto/types_testing.proto

Original file line number	Diff line number	Diff line change
`@@ -103,14 +103,15 @@ public ImmutableList<String> enclosingClassNames() {`
`103`	`103`	`if (!hasEnclosingClass()) {`
`104`	`104`	`return ImmutableList.of();`
`105`	`105`	`}`
`106`		`- // The innermost type will be the last element in the list.`
	`106`	`+ // Builds list in order of inner to outer.`
	`107`	`+ // Return the reversed list, since the outermost type is expected to lie at index 0.`
`107`	`108`	`ImmutableList.Builder<String> listBuilder = new ImmutableList.Builder<>();`
`108`	`109`	`Class<?> currentClz = clazz();`
`109`	`110`	`while (currentClz.getEnclosingClass() != null) {`
`110`	`111`	`listBuilder.add(currentClz.getEnclosingClass().getSimpleName());`
`111`	`112`	`currentClz = currentClz.getEnclosingClass();`
`112`	`113`	`}`
`113`		`- return listBuilder.build();`
	`114`	`+ return listBuilder.build().reverse();`
`114`	`115`	`}`
`115`	`116`
`116`	`117`	`@Override`