Skip to content

Commit d4b2c54

Browse files
emmileaflqiu96
emmileaf
authored and
lqiu96
committed
fix: [gapic-generator-java] handle response and metadata type ambiguity in LRO parsing (#1726)
1 parent 23c8e9c commit d4b2c54

File tree

5 files changed

+250
-34
lines changed

5 files changed

+250
-34
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -362,7 +362,8 @@ public void visit(MethodInvocationExpr methodInvocationExpr) {
362362
leftAngle();
363363
int numGenerics = methodInvocationExpr.generics().size();
364364
for (int i = 0; i < numGenerics; i++) {
365-
buffer.append(methodInvocationExpr.generics().get(i).name());
365+
Reference r = methodInvocationExpr.generics().get(i);
366+
r.accept(this);
366367
if (i < numGenerics - 1) {
367368
buffer.append(COMMA);
368369
space();

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

Lines changed: 24 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -780,6 +780,21 @@ static List<Method> parseMethods(
780780
return methods;
781781
}
782782

783+
private static String fetchTypeFullName(String typeName, MethodDescriptor methodDescriptor) {
784+
// When provided type name is fully qualified, return as-is
785+
// When only shortname is provided, assume same proto package as method (See
786+
// https://aip.dev/151)
787+
int lastDotIndex = typeName.lastIndexOf('.');
788+
boolean isResponseTypeNameShortOnly = lastDotIndex < 0;
789+
String responseTypeShortName =
790+
lastDotIndex >= 0 ? typeName.substring(lastDotIndex + 1) : typeName;
791+
String typeFullName =
792+
isResponseTypeNameShortOnly
793+
? methodDescriptor.getFile().getPackage() + "." + responseTypeShortName
794+
: typeName;
795+
return typeFullName;
796+
}
797+
783798
@VisibleForTesting
784799
static LongrunningOperation parseLro(
785800
String servicePackage, MethodDescriptor methodDescriptor, Map<String, Message> messageTypes) {
@@ -820,43 +835,19 @@ static LongrunningOperation parseLro(
820835
Message responseMessage = null;
821836
Message metadataMessage = null;
822837

823-
int lastDotIndex = responseTypeName.lastIndexOf('.');
824-
boolean isResponseTypeNameShortOnly = lastDotIndex < 0;
825-
String responseTypeShortName =
826-
lastDotIndex >= 0 ? responseTypeName.substring(lastDotIndex + 1) : responseTypeName;
827-
828-
lastDotIndex = metadataTypeName.lastIndexOf('.');
829-
boolean isMetadataTypeNameShortOnly = lastDotIndex < 0;
830-
String metadataTypeShortName =
831-
lastDotIndex >= 0 ? metadataTypeName.substring(lastDotIndex + 1) : metadataTypeName;
838+
String responseTypeFullName = fetchTypeFullName(responseTypeName, methodDescriptor);
839+
String metadataTypeFullName = fetchTypeFullName(metadataTypeName, methodDescriptor);
832840

833841
// The messageTypes map keys to the Java fully-qualified name.
834842
for (Map.Entry<String, Message> messageEntry : messageTypes.entrySet()) {
835-
String messageKey = messageEntry.getKey();
836-
int messageLastDotIndex = messageEntry.getKey().lastIndexOf('.');
837-
String messageShortName =
838-
messageLastDotIndex >= 0 ? messageKey.substring(messageLastDotIndex + 1) : messageKey;
839-
if (responseMessage == null) {
840-
if (isResponseTypeNameShortOnly && responseTypeName.equals(messageShortName)) {
841-
responseMessage = messageEntry.getValue();
842-
} else if (!isResponseTypeNameShortOnly && responseTypeShortName.equals(messageShortName)) {
843-
// Ensure that the full proto name matches.
844-
Message candidateMessage = messageEntry.getValue();
845-
if (candidateMessage.fullProtoName().equals(responseTypeName)) {
846-
responseMessage = candidateMessage;
847-
}
848-
}
843+
Message candidateMessage = messageEntry.getValue();
844+
if (responseMessage == null
845+
&& candidateMessage.fullProtoName().equals(responseTypeFullName)) {
846+
responseMessage = candidateMessage;
849847
}
850-
if (metadataMessage == null) {
851-
if (isMetadataTypeNameShortOnly && metadataTypeName.equals(messageShortName)) {
852-
metadataMessage = messageEntry.getValue();
853-
} else if (!isMetadataTypeNameShortOnly && metadataTypeShortName.equals(messageShortName)) {
854-
// Ensure that the full proto name matches.
855-
Message candidateMessage = messageEntry.getValue();
856-
if (candidateMessage.fullProtoName().equals(metadataTypeName)) {
857-
metadataMessage = candidateMessage;
858-
}
859-
}
848+
if (metadataMessage == null
849+
&& candidateMessage.fullProtoName().equals(metadataTypeFullName)) {
850+
metadataMessage = candidateMessage;
860851
}
861852
}
862853

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2422,6 +2422,71 @@ public void writeClassDefinition_commentsStatementsAndMethods() {
24222422
assertEquals(expected, writerVisitor.write());
24232423
}
24242424

2425+
@Test
2426+
public void writeClassDefinition_withImportCollision() {
2427+
2428+
VaporReference firstType =
2429+
VaporReference.builder()
2430+
.setName("Service")
2431+
.setPakkage("com.google.api.generator.gapic.model")
2432+
.build();
2433+
2434+
VaporReference secondType =
2435+
VaporReference.builder().setName("Service").setPakkage("com.google.api").build();
2436+
2437+
Variable secondTypeVar =
2438+
Variable.builder()
2439+
.setName("anotherServiceVar")
2440+
.setType(TypeNode.withReference(secondType))
2441+
.build();
2442+
2443+
MethodInvocationExpr genericMethodInvocation =
2444+
MethodInvocationExpr.builder()
2445+
.setMethodName("barMethod")
2446+
.setStaticReferenceType(TypeNode.withReference(firstType))
2447+
.setGenerics(Arrays.asList(secondType))
2448+
.setArguments(VariableExpr.withVariable(secondTypeVar))
2449+
.setReturnType(TypeNode.STRING)
2450+
.build();
2451+
2452+
List<Statement> statements = Arrays.asList(ExprStatement.withExpr(genericMethodInvocation));
2453+
2454+
MethodDefinition methodOne =
2455+
MethodDefinition.builder()
2456+
.setName("doSomething")
2457+
.setScope(ScopeNode.PRIVATE)
2458+
.setBody(statements)
2459+
.setReturnType(TypeNode.VOID)
2460+
.build();
2461+
2462+
List<MethodDefinition> methods = Arrays.asList(methodOne);
2463+
2464+
ClassDefinition classDef =
2465+
ClassDefinition.builder()
2466+
.setPackageString("com.google.example")
2467+
.setName("FooService")
2468+
.setScope(ScopeNode.PUBLIC)
2469+
.setMethods(methods)
2470+
.build();
2471+
2472+
classDef.accept(writerVisitor);
2473+
2474+
String expected =
2475+
LineFormatter.lines(
2476+
"package com.google.example;\n"
2477+
+ "\n"
2478+
+ "import com.google.api.generator.gapic.model.Service;\n"
2479+
+ "\n"
2480+
+ "public class FooService {\n"
2481+
+ "\n"
2482+
+ " private void doSomething() {\n"
2483+
+ " Service.<com.google.api.Service>barMethod(anotherServiceVar);\n"
2484+
+ " }\n"
2485+
+ "}\n");
2486+
2487+
assertThat(writerVisitor.write()).isEqualTo(expected);
2488+
}
2489+
24252490
@Test
24262491
public void writeReferenceConstructorExpr_thisConstructorWithArguments() {
24272492
VaporReference ref =

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,37 @@
1414

1515
package com.google.api.generator.gapic.protoparser;
1616

17+
import static com.google.common.truth.Truth.assertThat;
1718
import static org.junit.Assert.assertEquals;
19+
import static org.junit.Assert.assertThrows;
1820

1921
import com.google.api.generator.engine.ast.Reference;
22+
import com.google.api.generator.gapic.model.LongrunningOperation;
23+
import com.google.api.generator.gapic.model.Message;
24+
import com.google.cloud.location.LocationsProto;
25+
import com.google.protobuf.DescriptorProtos;
2026
import com.google.protobuf.Descriptors.Descriptor;
2127
import com.google.protobuf.Descriptors.FileDescriptor;
2228
import com.google.protobuf.Descriptors.MethodDescriptor;
2329
import com.google.protobuf.Descriptors.ServiceDescriptor;
2430
import com.google.showcase.v1beta1.EchoOuterClass;
31+
import com.google.test.collisions.CollisionsOuterClass;
2532
import com.google.testgapic.v1beta1.NestedMessageProto;
33+
import java.util.Map;
2634
import org.junit.Test;
2735

2836
public class TypeParserTest {
2937
// TODO(miraleung): Backfill with more tests (e.g. field, message, methods) for Parser.java.
38+
39+
private static final FileDescriptor COLLISIONS_FILE_DESCRIPTOR =
40+
CollisionsOuterClass.getDescriptor();
41+
private static final FileDescriptor DESCRIPTOR_PROTOS_FILE_DESCRIPTOR =
42+
DescriptorProtos.getDescriptor();
43+
private static final FileDescriptor LOCATION_PROTO_FILE_DESCRIPTOR =
44+
LocationsProto.getDescriptor();
45+
private static final ServiceDescriptor COLLISIONS_SERVICE =
46+
COLLISIONS_FILE_DESCRIPTOR.getServices().get(0);
47+
3048
@Test
3149
public void parseMessageType_basic() {
3250
FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
@@ -51,4 +69,73 @@ public void parseMessageType_nested() {
5169
Reference reference = TypeParser.parseMessageReference(messageDescriptor);
5270
assertEquals("com.google.testgapic.v1beta1.Outer.Middle.Inner", reference.fullName());
5371
}
72+
73+
@Test
74+
public void parseLroResponseMetadataType_shortName_shouldMatchSamePackage() {
75+
Map<String, Message> messageTypes = Parser.parseMessages(COLLISIONS_FILE_DESCRIPTOR);
76+
messageTypes.putAll(Parser.parseMessages(DESCRIPTOR_PROTOS_FILE_DESCRIPTOR));
77+
messageTypes.putAll(Parser.parseMessages(LOCATION_PROTO_FILE_DESCRIPTOR));
78+
MethodDescriptor shouldUseSamePackageTypesLro = COLLISIONS_SERVICE.getMethods().get(0);
79+
80+
assertEquals(COLLISIONS_SERVICE.getName(), "Collisions");
81+
assertThat(messageTypes)
82+
.containsKey("com.google.protobuf.DescriptorProtos.GeneratedCodeInfo.Annotation");
83+
assertThat(messageTypes)
84+
.containsKey("com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location");
85+
assertThat(messageTypes).containsKey("com.google.cloud.location.Location");
86+
87+
LongrunningOperation testLro =
88+
Parser.parseLro(
89+
TypeParser.getPackage(COLLISIONS_FILE_DESCRIPTOR),
90+
shouldUseSamePackageTypesLro,
91+
messageTypes);
92+
93+
assertThat(testLro.responseType().reference().fullName())
94+
.isEqualTo("com.google.test.collisions.Annotation");
95+
assertThat(testLro.metadataType().reference().fullName())
96+
.isEqualTo("com.google.test.collisions.Location");
97+
}
98+
99+
@Test
100+
public void parseLroResponseMetadataType_shortName_shouldNotMatch() {
101+
Map<String, Message> messageTypes = Parser.parseMessages(COLLISIONS_FILE_DESCRIPTOR);
102+
messageTypes.putAll(Parser.parseMessages(DESCRIPTOR_PROTOS_FILE_DESCRIPTOR));
103+
MethodDescriptor shortNameMatchShouldThrowLro = COLLISIONS_SERVICE.getMethods().get(1);
104+
105+
assertEquals(COLLISIONS_SERVICE.getName(), "Collisions");
106+
assertThat(messageTypes)
107+
.containsKey("com.google.protobuf.DescriptorProtos.ExtensionRangeOptions.Declaration");
108+
109+
assertThrows(
110+
NullPointerException.class,
111+
() ->
112+
Parser.parseLro(
113+
TypeParser.getPackage(COLLISIONS_FILE_DESCRIPTOR),
114+
shortNameMatchShouldThrowLro,
115+
messageTypes));
116+
}
117+
118+
@Test
119+
public void parseLroResponseMetadataType_shortName_withFullyQualifiedCollision() {
120+
Map<String, Message> messageTypes = Parser.parseMessages(COLLISIONS_FILE_DESCRIPTOR);
121+
messageTypes.putAll(Parser.parseMessages(DESCRIPTOR_PROTOS_FILE_DESCRIPTOR));
122+
messageTypes.putAll(Parser.parseMessages(LOCATION_PROTO_FILE_DESCRIPTOR));
123+
MethodDescriptor fullNameForDifferentPackageLro = COLLISIONS_SERVICE.getMethods().get(2);
124+
125+
assertEquals(COLLISIONS_SERVICE.getName(), "Collisions");
126+
assertThat(messageTypes).containsKey("com.google.cloud.location.Location");
127+
assertThat(messageTypes)
128+
.containsKey("com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location");
129+
130+
LongrunningOperation testLro =
131+
Parser.parseLro(
132+
TypeParser.getPackage(COLLISIONS_FILE_DESCRIPTOR),
133+
fullNameForDifferentPackageLro,
134+
messageTypes);
135+
136+
assertThat(testLro.responseType().reference().fullName())
137+
.isEqualTo("com.google.cloud.location.Location");
138+
assertThat(testLro.metadataType().reference().fullName())
139+
.isEqualTo("com.google.test.collisions.Location");
140+
}
54141
}

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
// Copyright 2023 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
syntax = "proto3";
16+
17+
import "google/api/client.proto";
18+
import "google/longrunning/operations.proto";
19+
20+
package google.showcase.v1beta1;
21+
22+
option java_package = "com.google.test.collisions";
23+
option java_multiple_files = true;
24+
25+
// This service exercises scenarios where short names of types
26+
// exhibit ambiguity or collide with other types
27+
service Collisions {
28+
29+
option (google.api.default_host) = "localhost:7469";
30+
31+
rpc shouldUseSamePackageTypesLro(Request) returns (google.longrunning.Operation) {
32+
option (google.longrunning.operation_info) = {
33+
// collision with google.protobuf.DescriptorProtos.GeneratedCodeInfo.Annotation
34+
response_type: "Annotation"
35+
// collision with google.cloud.location.Location;
36+
metadata_type: "Location"
37+
};
38+
}
39+
40+
rpc shortNameMatchShouldThrowLro(Request) returns (google.longrunning.Operation) {
41+
option (google.longrunning.operation_info) = {
42+
// collision with google.protobuf.DescriptorProtos.GeneratedCodeInfo.Annotation
43+
response_type: "Annotation"
44+
// collision with google.protobuf.DescriptorProtos.ExtensionRangeOptions.Declaration
45+
// not a valid short name specification (no such type exist in same package)
46+
metadata_type: "Declaration"
47+
};
48+
}
49+
50+
rpc fullNameForDifferentPackageLro(Request) returns (google.longrunning.Operation) {
51+
option (google.longrunning.operation_info) = {
52+
// fully qualified name should match google.cloud.location.Location
53+
response_type: "google.cloud.location.Location"
54+
// short name only should match Location message defined below
55+
metadata_type: "Location"
56+
};
57+
}
58+
}
59+
60+
message Request {
61+
string name = 1;
62+
Annotation annotation = 2;
63+
Location location = 3;
64+
}
65+
66+
message Annotation {
67+
string name = 1;
68+
}
69+
70+
message Location {
71+
string name = 1;
72+
}

0 commit comments

Comments
 (0)