fix: [gapic-generator-java] handle response and metadata type ambigui…

…ty in LRO parsing (#1726)

gapic-generator-java/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java

@@ -362,7 +362,8 @@ public void visit(MethodInvocationExpr methodInvocationExpr) {
       leftAngle();
       int numGenerics = methodInvocationExpr.generics().size();
       for (int i = 0; i < numGenerics; i++) {
-        buffer.append(methodInvocationExpr.generics().get(i).name());
+        Reference r = methodInvocationExpr.generics().get(i);
+        r.accept(this);
         if (i < numGenerics - 1) {
           buffer.append(COMMA);
           space();

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

@@ -780,6 +780,21 @@ static List<Method> parseMethods(
     return methods;
   }
 
+  private static String fetchTypeFullName(String typeName, MethodDescriptor methodDescriptor) {
+    // When provided type name is fully qualified, return as-is
+    // When only shortname is provided, assume same proto package as method (See
+    // https://aip.dev/151)
+    int lastDotIndex = typeName.lastIndexOf('.');
+    boolean isResponseTypeNameShortOnly = lastDotIndex < 0;
+    String responseTypeShortName =
+        lastDotIndex >= 0 ? typeName.substring(lastDotIndex + 1) : typeName;
+    String typeFullName =
+        isResponseTypeNameShortOnly
+            ? methodDescriptor.getFile().getPackage() + "." + responseTypeShortName
+            : typeName;
+    return typeFullName;
+  }
+
   @VisibleForTesting
   static LongrunningOperation parseLro(
       String servicePackage, MethodDescriptor methodDescriptor, Map<String, Message> messageTypes) {
@@ -820,43 +835,19 @@ static LongrunningOperation parseLro(
     Message responseMessage = null;
     Message metadataMessage = null;
 
-    int lastDotIndex = responseTypeName.lastIndexOf('.');
-    boolean isResponseTypeNameShortOnly = lastDotIndex < 0;
-    String responseTypeShortName =
-        lastDotIndex >= 0 ? responseTypeName.substring(lastDotIndex + 1) : responseTypeName;
-
-    lastDotIndex = metadataTypeName.lastIndexOf('.');
-    boolean isMetadataTypeNameShortOnly = lastDotIndex < 0;
-    String metadataTypeShortName =
-        lastDotIndex >= 0 ? metadataTypeName.substring(lastDotIndex + 1) : metadataTypeName;
+    String responseTypeFullName = fetchTypeFullName(responseTypeName, methodDescriptor);
+    String metadataTypeFullName = fetchTypeFullName(metadataTypeName, methodDescriptor);
 
     // The messageTypes map keys to the Java fully-qualified name.
     for (Map.Entry<String, Message> messageEntry : messageTypes.entrySet()) {
-      String messageKey = messageEntry.getKey();
-      int messageLastDotIndex = messageEntry.getKey().lastIndexOf('.');
-      String messageShortName =
-          messageLastDotIndex >= 0 ? messageKey.substring(messageLastDotIndex + 1) : messageKey;
-      if (responseMessage == null) {
-        if (isResponseTypeNameShortOnly && responseTypeName.equals(messageShortName)) {
-          responseMessage = messageEntry.getValue();
-        } else if (!isResponseTypeNameShortOnly && responseTypeShortName.equals(messageShortName)) {
-          // Ensure that the full proto name matches.
-          Message candidateMessage = messageEntry.getValue();
-          if (candidateMessage.fullProtoName().equals(responseTypeName)) {
-            responseMessage = candidateMessage;
-          }
-        }
+      Message candidateMessage = messageEntry.getValue();
+      if (responseMessage == null
+          && candidateMessage.fullProtoName().equals(responseTypeFullName)) {
+        responseMessage = candidateMessage;
       }
-      if (metadataMessage == null) {
-        if (isMetadataTypeNameShortOnly && metadataTypeName.equals(messageShortName)) {
-          metadataMessage = messageEntry.getValue();
-        } else if (!isMetadataTypeNameShortOnly && metadataTypeShortName.equals(messageShortName)) {
-          // Ensure that the full proto name matches.
-          Message candidateMessage = messageEntry.getValue();
-          if (candidateMessage.fullProtoName().equals(metadataTypeName)) {
-            metadataMessage = candidateMessage;
-          }
-        }
+      if (metadataMessage == null
+          && candidateMessage.fullProtoName().equals(metadataTypeFullName)) {
+        metadataMessage = candidateMessage;
       }
     }
 

gapic-generator-java/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java

@@ -2422,6 +2422,71 @@ public void writeClassDefinition_commentsStatementsAndMethods() {
     assertEquals(expected, writerVisitor.write());
   }
 
+  @Test
+  public void writeClassDefinition_withImportCollision() {
+
+    VaporReference firstType =
+        VaporReference.builder()
+            .setName("Service")
+            .setPakkage("com.google.api.generator.gapic.model")
+            .build();
+
+    VaporReference secondType =
+        VaporReference.builder().setName("Service").setPakkage("com.google.api").build();
+
+    Variable secondTypeVar =
+        Variable.builder()
+            .setName("anotherServiceVar")
+            .setType(TypeNode.withReference(secondType))
+            .build();
+
+    MethodInvocationExpr genericMethodInvocation =
+        MethodInvocationExpr.builder()
+            .setMethodName("barMethod")
+            .setStaticReferenceType(TypeNode.withReference(firstType))
+            .setGenerics(Arrays.asList(secondType))
+            .setArguments(VariableExpr.withVariable(secondTypeVar))
+            .setReturnType(TypeNode.STRING)
+            .build();
+
+    List<Statement> statements = Arrays.asList(ExprStatement.withExpr(genericMethodInvocation));
+
+    MethodDefinition methodOne =
+        MethodDefinition.builder()
+            .setName("doSomething")
+            .setScope(ScopeNode.PRIVATE)
+            .setBody(statements)
+            .setReturnType(TypeNode.VOID)
+            .build();
+
+    List<MethodDefinition> methods = Arrays.asList(methodOne);
+
+    ClassDefinition classDef =
+        ClassDefinition.builder()
+            .setPackageString("com.google.example")
+            .setName("FooService")
+            .setScope(ScopeNode.PUBLIC)
+            .setMethods(methods)
+            .build();
+
+    classDef.accept(writerVisitor);
+
+    String expected =
+        LineFormatter.lines(
+            "package com.google.example;\n"
+                + "\n"
+                + "import com.google.api.generator.gapic.model.Service;\n"
+                + "\n"
+                + "public class FooService {\n"
+                + "\n"
+                + "  private void doSomething() {\n"
+                + "    Service.<com.google.api.Service>barMethod(anotherServiceVar);\n"
+                + "  }\n"
+                + "}\n");
+
+    assertThat(writerVisitor.write()).isEqualTo(expected);
+  }
+
   @Test
   public void writeReferenceConstructorExpr_thisConstructorWithArguments() {
     VaporReference ref =

gapic-generator-java/src/test/java/com/google/api/generator/gapic/protoparser/TypeParserTest.java

@@ -14,19 +14,37 @@
 
 package com.google.api.generator.gapic.protoparser;
 
+import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
 
 import com.google.api.generator.engine.ast.Reference;
+import com.google.api.generator.gapic.model.LongrunningOperation;
+import com.google.api.generator.gapic.model.Message;
+import com.google.cloud.location.LocationsProto;
+import com.google.protobuf.DescriptorProtos;
 import com.google.protobuf.Descriptors.Descriptor;
 import com.google.protobuf.Descriptors.FileDescriptor;
 import com.google.protobuf.Descriptors.MethodDescriptor;
 import com.google.protobuf.Descriptors.ServiceDescriptor;
 import com.google.showcase.v1beta1.EchoOuterClass;
+import com.google.test.collisions.CollisionsOuterClass;
 import com.google.testgapic.v1beta1.NestedMessageProto;
+import java.util.Map;
 import org.junit.Test;
 
 public class TypeParserTest {
   // TODO(miraleung): Backfill with more tests (e.g. field, message, methods) for Parser.java.
+
+  private static final FileDescriptor COLLISIONS_FILE_DESCRIPTOR =
+      CollisionsOuterClass.getDescriptor();
+  private static final FileDescriptor DESCRIPTOR_PROTOS_FILE_DESCRIPTOR =
+      DescriptorProtos.getDescriptor();
+  private static final FileDescriptor LOCATION_PROTO_FILE_DESCRIPTOR =
+      LocationsProto.getDescriptor();
+  private static final ServiceDescriptor COLLISIONS_SERVICE =
+      COLLISIONS_FILE_DESCRIPTOR.getServices().get(0);
+
   @Test
   public void parseMessageType_basic() {
     FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
@@ -51,4 +69,73 @@ public void parseMessageType_nested() {
     Reference reference = TypeParser.parseMessageReference(messageDescriptor);
     assertEquals("com.google.testgapic.v1beta1.Outer.Middle.Inner", reference.fullName());
   }
+
+  @Test
+  public void parseLroResponseMetadataType_shortName_shouldMatchSamePackage() {
+    Map<String, Message> messageTypes = Parser.parseMessages(COLLISIONS_FILE_DESCRIPTOR);
+    messageTypes.putAll(Parser.parseMessages(DESCRIPTOR_PROTOS_FILE_DESCRIPTOR));
+    messageTypes.putAll(Parser.parseMessages(LOCATION_PROTO_FILE_DESCRIPTOR));
+    MethodDescriptor shouldUseSamePackageTypesLro = COLLISIONS_SERVICE.getMethods().get(0);
+
+    assertEquals(COLLISIONS_SERVICE.getName(), "Collisions");
+    assertThat(messageTypes)
+        .containsKey("com.google.protobuf.DescriptorProtos.GeneratedCodeInfo.Annotation");
+    assertThat(messageTypes)
+        .containsKey("com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location");
+    assertThat(messageTypes).containsKey("com.google.cloud.location.Location");
+
+    LongrunningOperation testLro =
+        Parser.parseLro(
+            TypeParser.getPackage(COLLISIONS_FILE_DESCRIPTOR),
+            shouldUseSamePackageTypesLro,
+            messageTypes);
+
+    assertThat(testLro.responseType().reference().fullName())
+        .isEqualTo("com.google.test.collisions.Annotation");
+    assertThat(testLro.metadataType().reference().fullName())
+        .isEqualTo("com.google.test.collisions.Location");
+  }
+
+  @Test
+  public void parseLroResponseMetadataType_shortName_shouldNotMatch() {
+    Map<String, Message> messageTypes = Parser.parseMessages(COLLISIONS_FILE_DESCRIPTOR);
+    messageTypes.putAll(Parser.parseMessages(DESCRIPTOR_PROTOS_FILE_DESCRIPTOR));
+    MethodDescriptor shortNameMatchShouldThrowLro = COLLISIONS_SERVICE.getMethods().get(1);
+
+    assertEquals(COLLISIONS_SERVICE.getName(), "Collisions");
+    assertThat(messageTypes)
+        .containsKey("com.google.protobuf.DescriptorProtos.ExtensionRangeOptions.Declaration");
+
+    assertThrows(
+        NullPointerException.class,
+        () ->
+            Parser.parseLro(
+                TypeParser.getPackage(COLLISIONS_FILE_DESCRIPTOR),
+                shortNameMatchShouldThrowLro,
+                messageTypes));
+  }
+
+  @Test
+  public void parseLroResponseMetadataType_shortName_withFullyQualifiedCollision() {
+    Map<String, Message> messageTypes = Parser.parseMessages(COLLISIONS_FILE_DESCRIPTOR);
+    messageTypes.putAll(Parser.parseMessages(DESCRIPTOR_PROTOS_FILE_DESCRIPTOR));
+    messageTypes.putAll(Parser.parseMessages(LOCATION_PROTO_FILE_DESCRIPTOR));
+    MethodDescriptor fullNameForDifferentPackageLro = COLLISIONS_SERVICE.getMethods().get(2);
+
+    assertEquals(COLLISIONS_SERVICE.getName(), "Collisions");
+    assertThat(messageTypes).containsKey("com.google.cloud.location.Location");
+    assertThat(messageTypes)
+        .containsKey("com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location");
+
+    LongrunningOperation testLro =
+        Parser.parseLro(
+            TypeParser.getPackage(COLLISIONS_FILE_DESCRIPTOR),
+            fullNameForDifferentPackageLro,
+            messageTypes);
+
+    assertThat(testLro.responseType().reference().fullName())
+        .isEqualTo("com.google.cloud.location.Location");
+    assertThat(testLro.metadataType().reference().fullName())
+        .isEqualTo("com.google.test.collisions.Location");
+  }
 }

gapic-generator-java/src/test/proto/collisions.proto

@@ -0,0 +1,72 @@
+// Copyright 2023 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+syntax = "proto3";
+
+import "google/api/client.proto";
+import "google/longrunning/operations.proto";
+
+package google.showcase.v1beta1;
+
+option java_package = "com.google.test.collisions";
+option java_multiple_files = true;
+
+// This service exercises scenarios where short names of types
+// exhibit ambiguity or collide with other types
+service Collisions {
+
+  option (google.api.default_host) = "localhost:7469";
+
+  rpc shouldUseSamePackageTypesLro(Request) returns (google.longrunning.Operation) {
+    option (google.longrunning.operation_info) = {
+      // collision with google.protobuf.DescriptorProtos.GeneratedCodeInfo.Annotation
+      response_type: "Annotation"
+      // collision with google.cloud.location.Location;
+      metadata_type: "Location"
+    };
+  }
+
+  rpc shortNameMatchShouldThrowLro(Request) returns (google.longrunning.Operation) {
+    option (google.longrunning.operation_info) = {
+      // collision with google.protobuf.DescriptorProtos.GeneratedCodeInfo.Annotation
+      response_type: "Annotation"
+      // collision with google.protobuf.DescriptorProtos.ExtensionRangeOptions.Declaration
+      // not a valid short name specification (no such type exist in same package)
+      metadata_type: "Declaration"
+    };
+  }
+
+  rpc fullNameForDifferentPackageLro(Request) returns (google.longrunning.Operation) {
+    option (google.longrunning.operation_info) = {
+      // fully qualified name should match google.cloud.location.Location
+      response_type: "google.cloud.location.Location"
+      // short name only should match Location message defined below
+      metadata_type: "Location"
+    };
+  }
+}
+
+message Request {
+  string name = 1;
+  Annotation annotation = 2;
+  Location location = 3;
+}
+
+message Annotation {
+  string name = 1;
+}
+
+message Location {
+  string name = 1;
+}