From 1ace49499175f2509062edb2fa5098e6cf8035d6 Mon Sep 17 00:00:00 2001 From: Emily Wang Date: Wed, 5 Jul 2023 11:45:02 -0400 Subject: [PATCH] fix: [gapic-generator-java] handle response and metadata type ambiguity in LRO parsing (#1726) --- .../engine/writer/JavaWriterVisitor.java | 3 +- .../generator/gapic/protoparser/Parser.java | 57 +++++------- .../engine/writer/JavaWriterVisitorTest.java | 65 ++++++++++++++ .../gapic/protoparser/TypeParserTest.java | 87 +++++++++++++++++++ .../src/test/proto/collisions.proto | 72 +++++++++++++++ 5 files changed, 250 insertions(+), 34 deletions(-) create mode 100644 gapic-generator-java/src/test/proto/collisions.proto diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java b/gapic-generator-java/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java index e092f1c69c..e956d86949 100644 --- a/gapic-generator-java/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java +++ b/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(); diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java index 35ba86dcc6..43f5f830fc 100644 --- a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java +++ b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java @@ -780,6 +780,21 @@ static List 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 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 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; } } 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 index 71ae78ae05..f3896901e1 100644 --- 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 @@ -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 statements = Arrays.asList(ExprStatement.withExpr(genericMethodInvocation)); + + MethodDefinition methodOne = + MethodDefinition.builder() + .setName("doSomething") + .setScope(ScopeNode.PRIVATE) + .setBody(statements) + .setReturnType(TypeNode.VOID) + .build(); + + List 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.barMethod(anotherServiceVar);\n" + + " }\n" + + "}\n"); + + assertThat(writerVisitor.write()).isEqualTo(expected); + } + @Test public void writeReferenceConstructorExpr_thisConstructorWithArguments() { VaporReference ref = diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/protoparser/TypeParserTest.java b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/protoparser/TypeParserTest.java index a79460e915..455f04bcae 100644 --- a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/protoparser/TypeParserTest.java +++ b/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 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 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 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"); + } } diff --git a/gapic-generator-java/src/test/proto/collisions.proto b/gapic-generator-java/src/test/proto/collisions.proto new file mode 100644 index 0000000000..03c1060d8e --- /dev/null +++ b/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; +} \ No newline at end of file