Skip to content

Commit 9432979

Browse files
miraleungmiraleung
authored
fix(protoc): Mirror protoc's field name conflict resolution logic in client generation [ggj] (#781)
* fix(protoc): Mirror protoc's field name conflict resolution logic in client gen * fix: update Field - set default value for originalName * fix: update googleapis-discovery hash with monolith removal * fix: post-conflict resolution goldens * fix(build): Update googleapis-discovery hash to fix compute integration test * fix(protoc): Mirror protoc's field name conflict resolution logic in client gen * fix: update Field - set default value for originalName * fix: post-conflict resolution goldens * Update Parser.java * Update Field.java
1 parent 46bb19a commit 9432979

12 files changed

Lines changed: 464 additions & 13 deletions

File tree

‎src/main/java/com/google/api/generator/gapic/model/Field.java‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,19 @@
1717
import com.google.api.generator.engine.ast.TypeNode;
1818
import com.google.auto.value.AutoValue;
1919
import java.util.Objects;
20+
import java.util.Optional;
2021
import javax.annotation.Nullable;
2122

2223
@AutoValue
2324
public abstract class Field {
25+
// The field's canonical name, potentially post-processed by conflict resolution logic.
2426
public abstract String name();
2527

28+
// The field's name as it appeared in the protobuf.
29+
// Not equal to name() only when there is a field name conflict, as per protoc's conflict
30+
// resolution behavior. For more context, please see the invocation site of the setter method.
31+
public abstract String originalName();
32+
2633
public abstract TypeNode type();
2734

2835
public abstract boolean isMessage();
@@ -43,6 +50,10 @@ public abstract class Field {
4350
@Nullable
4451
public abstract String description();
4552

53+
public boolean hasFieldNameConflict() {
54+
return !name().equals(originalName());
55+
}
56+
4657
public boolean hasDescription() {
4758
return description() != null;
4859
}
@@ -59,6 +70,7 @@ public boolean equals(Object o) {
5970

6071
Field other = (Field) o;
6172
return name().equals(other.name())
73+
&& originalName().equals(other.originalName())
6274
&& type().equals(other.type())
6375
&& isMessage() == other.isMessage()
6476
&& isEnum() == other.isEnum()
@@ -73,6 +85,7 @@ && isProto3Optional() == other.isProto3Optional()
7385
@Override
7486
public int hashCode() {
7587
return 17 * name().hashCode()
88+
+ 31 * originalName().hashCode()
7689
+ 19 * type().hashCode()
7790
+ (isMessage() ? 1 : 0) * 23
7891
+ (isEnum() ? 1 : 0) * 29
@@ -100,6 +113,8 @@ public static Builder builder() {
100113
public abstract static class Builder {
101114
public abstract Builder setName(String name);
102115

116+
public abstract Builder setOriginalName(String originalName);
117+
103118
public abstract Builder setType(TypeNode type);
104119

105120
public abstract Builder setIsMessage(boolean isMessage);
@@ -118,6 +133,18 @@ public abstract static class Builder {
118133

119134
public abstract Builder setDescription(String description);
120135

121-
public abstract Field build();
136+
// Private accessors.
137+
abstract String name();
138+
139+
abstract Optional<String> originalName();
140+
141+
abstract Field autoBuild();
142+
143+
public Field build() {
144+
if (!originalName().isPresent()) {
145+
setOriginalName(name());
146+
}
147+
return autoBuild();
148+
}
122149
}
123150
}

‎src/main/java/com/google/api/generator/gapic/model/Message.java‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,14 @@ public Builder setEnumValues(List<String> names, List<Integer> numbers) {
140140
public Message build() {
141141
Message message = autoBuild();
142142
if (!message.fields().isEmpty()) {
143-
message =
144-
message
145-
.toBuilder()
146-
.setFieldMap(fields().stream().collect(Collectors.toMap(f -> f.name(), f -> f)))
147-
.autoBuild();
143+
Map<String, Field> fieldMap =
144+
fields().stream().collect(Collectors.toMap(f -> f.name(), f -> f));
145+
// Handles string occurrences of a field's original name in a protobuf, such as
146+
// in the method signature annotaiton.
147+
fields().stream()
148+
.filter(f -> f.hasFieldNameConflict())
149+
.forEach(f -> fieldMap.put(f.originalName(), f));
150+
message = message.toBuilder().setFieldMap(fieldMap).autoBuild();
148151
}
149152
return message;
150153
}

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

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -830,14 +830,47 @@ private static List<Field> parseFields(
830830
// Sort by ascending field index order. This is important for paged responses, where the first
831831
// repeated type is taken.
832832
fields.sort((f1, f2) -> f1.getIndex() - f2.getIndex());
833+
834+
// Mirror protoc's name conflict resolution behavior for fields.
835+
// If a singular field's name equals that of a repeated field with "Count" or "List" suffixed,
836+
// append the protobuf's field number to both fields' names.
837+
// See:
838+
// https://github.com/protocolbuffers/protobuf/blob/9df42757f97da9f748a464deeda96427a8f7ade0/src/google/protobuf/compiler/java/java_context.cc#L60
839+
Map<String, Integer> repeatedFieldNamesToNumber =
840+
fields.stream()
841+
.filter(f -> f.isRepeated())
842+
.collect(Collectors.toMap(f -> f.getName(), f -> f.getNumber()));
843+
Set<Integer> fieldNumbersWithConflicts = new HashSet<>();
844+
for (FieldDescriptor field : fields) {
845+
Set<String> conflictingRepeatedFieldNames =
846+
repeatedFieldNamesToNumber.keySet().stream()
847+
.filter(
848+
n -> field.getName().equals(n + "_count") || field.getName().equals(n + "_list"))
849+
.collect(Collectors.toSet());
850+
if (!conflictingRepeatedFieldNames.isEmpty()) {
851+
fieldNumbersWithConflicts.addAll(
852+
conflictingRepeatedFieldNames.stream()
853+
.map(n -> repeatedFieldNamesToNumber.get(n))
854+
.collect(Collectors.toSet()));
855+
fieldNumbersWithConflicts.add(field.getNumber());
856+
}
857+
}
858+
833859
return fields.stream()
834-
.map(f -> parseField(f, messageDescriptor, outputResourceReferencesSeen))
860+
.map(
861+
f ->
862+
parseField(
863+
f,
864+
messageDescriptor,
865+
fieldNumbersWithConflicts.contains(f.getNumber()),
866+
outputResourceReferencesSeen))
835867
.collect(Collectors.toList());
836868
}
837869

838870
private static Field parseField(
839871
FieldDescriptor fieldDescriptor,
840872
Descriptor messageDescriptor,
873+
boolean hasFieldNameConflict,
841874
Set<ResourceReference> outputResourceReferencesSeen) {
842875
FieldOptions fieldOptions = fieldDescriptor.getOptions();
843876
MessageOptions messageOptions = messageDescriptor.getOptions();
@@ -882,8 +915,16 @@ private static Field parseField(
882915
}
883916
}
884917

918+
// Mirror protoc's name conflict resolution behavior for fields.
919+
// For more context, trace hasFieldNameConflict back to where it gets passed in above.
920+
String actualFieldName =
921+
hasFieldNameConflict
922+
? fieldDescriptor.getName() + fieldDescriptor.getNumber()
923+
: fieldDescriptor.getName();
924+
885925
return fieldBuilder
886-
.setName(fieldDescriptor.getName())
926+
.setName(actualFieldName)
927+
.setOriginalName(fieldDescriptor.getName())
887928
.setType(TypeParser.parseType(fieldDescriptor))
888929
.setIsMessage(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE)
889930
.setIsEnum(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.ENUM)

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ TEST_DEPS = [
2929
"//src/main/java/com/google/api/generator/gapic/protoparser",
3030
"//src/main/java/com/google/api/generator/gapic/composer/defaultvalue",
3131
"//src/test/java/com/google/api/generator/gapic/testdata:deprecated_service_java_proto",
32+
"//src/test/java/com/google/api/generator/gapic/testdata:bookshop_java_proto",
3233
"//src/test/java/com/google/api/generator/gapic/testdata:showcase_java_proto",
3334
"//src/test/java/com/google/api/generator/gapic/testdata:testgapic_java_proto",
3435
"@com_google_api_api_common//jar",

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,20 @@ public void generateServiceClasses_methodSignatureHasNestedFields() {
6262
JavaWriterVisitor visitor = new JavaWriterVisitor();
6363
clazz.classDefinition().accept(visitor);
6464
Utils.saveCodegenToFile(this.getClass(), "IdentityClient.golden", visitor.write());
65-
Path goldenFilePath =
66-
Paths.get(Utils.getGoldenDir(this.getClass()), "IdentityClient.golden");
65+
Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "IdentityClient.golden");
66+
assertCodeEquals(goldenFilePath, visitor.write());
67+
}
68+
69+
@Test
70+
public void generateServiceClasses_bookshopNameConflicts() {
71+
GapicContext context = TestProtoLoader.instance().parseBookshopService();
72+
Service protoService = context.services().get(0);
73+
GapicClass clazz = ServiceClientClassComposer.instance().generate(context, protoService);
74+
75+
JavaWriterVisitor visitor = new JavaWriterVisitor();
76+
clazz.classDefinition().accept(visitor);
77+
Utils.saveCodegenToFile(this.getClass(), "BookshopClient.golden", visitor.write());
78+
Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "BookshopClient.golden");
6779
assertCodeEquals(goldenFilePath, visitor.write());
6880
}
6981
}

‎src/test/java/com/google/api/generator/gapic/composer/common/TestProtoLoader.java‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import com.google.api.generator.gapic.protoparser.BatchingSettingsConfigParser;
2828
import com.google.api.generator.gapic.protoparser.Parser;
2929
import com.google.api.generator.gapic.protoparser.ServiceConfigParser;
30+
import com.google.bookshop.v1beta1.BookshopProto;
3031
import com.google.logging.v2.LogEntryProto;
3132
import com.google.logging.v2.LoggingConfigProto;
3233
import com.google.logging.v2.LoggingMetricsProto;
@@ -51,8 +52,7 @@
5152

5253
public class TestProtoLoader {
5354
private static final TestProtoLoader INSTANCE =
54-
new TestProtoLoader(
55-
Transport.GRPC, "src/test/java/com/google/api/generator/gapic/testdata/");
55+
new TestProtoLoader(Transport.GRPC, "src/test/java/com/google/api/generator/gapic/testdata/");
5656
private final String testFilesDirectory;
5757
private final Transport transport;
5858

@@ -93,6 +93,34 @@ public GapicContext parseDeprecatedService() {
9393
.build();
9494
}
9595

96+
public GapicContext parseBookshopService() {
97+
FileDescriptor fileDescriptor = BookshopProto.getDescriptor();
98+
ServiceDescriptor serviceDescriptor = fileDescriptor.getServices().get(0);
99+
assertEquals(serviceDescriptor.getName(), "Bookshop");
100+
101+
Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor);
102+
Map<String, ResourceName> resourceNames = new HashMap<>();
103+
Set<ResourceName> outputResourceNames = new HashSet<>();
104+
List<Service> services =
105+
Parser.parseService(
106+
fileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
107+
108+
String jsonFilename = "bookshop_grpc_service_config.json";
109+
Path jsonPath = Paths.get(testFilesDirectory, jsonFilename);
110+
Optional<GapicServiceConfig> configOpt = ServiceConfigParser.parse(jsonPath.toString());
111+
assertTrue(configOpt.isPresent());
112+
GapicServiceConfig config = configOpt.get();
113+
114+
return GapicContext.builder()
115+
.setMessages(messageTypes)
116+
.setResourceNames(resourceNames)
117+
.setServices(services)
118+
.setServiceConfig(config)
119+
.setHelperResourceNames(outputResourceNames)
120+
.setTransport(transport)
121+
.build();
122+
}
123+
96124
public GapicContext parseShowcaseEcho() {
97125
FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
98126
ServiceDescriptor echoServiceDescriptor = echoFileDescriptor.getServices().get(0);

0 commit comments

Comments
 (0)