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
googleapis · Jun 29, 2021 · 9432979 · 9432979
1 parent 46bb19a
commit 9432979
Show file tree

Hide file tree

Showing 12 changed files with 464 additions and 13 deletions.
diff --git a/src/main/java/com/google/api/generator/gapic/model/Field.java b/src/main/java/com/google/api/generator/gapic/model/Field.java
@@ -17,12 +17,19 @@
 import com.google.api.generator.engine.ast.TypeNode;
 import com.google.auto.value.AutoValue;
 import java.util.Objects;
+import java.util.Optional;
 import javax.annotation.Nullable;
 
 @AutoValue
 public abstract class Field {
+  // The field's canonical name, potentially post-processed by conflict resolution logic.
   public abstract String name();
 
+  // The field's name as it appeared in the protobuf.
+  // Not equal to name() only when there is a field name conflict, as per protoc's conflict
+  // resolution behavior. For more context, please see the invocation site of the setter method.
+  public abstract String originalName();
+
   public abstract TypeNode type();
 
   public abstract boolean isMessage();
@@ -43,6 +50,10 @@ public abstract class Field {
   @Nullable
   public abstract String description();
 
+  public boolean hasFieldNameConflict() {
+    return !name().equals(originalName());
+  }
+
   public boolean hasDescription() {
     return description() != null;
   }
@@ -59,6 +70,7 @@ public boolean equals(Object o) {
 
     Field other = (Field) o;
     return name().equals(other.name())
+        && originalName().equals(other.originalName())
         && type().equals(other.type())
         && isMessage() == other.isMessage()
         && isEnum() == other.isEnum()
@@ -73,6 +85,7 @@ && isProto3Optional() == other.isProto3Optional()
   @Override
   public int hashCode() {
     return 17 * name().hashCode()
+        + 31 * originalName().hashCode()
         + 19 * type().hashCode()
         + (isMessage() ? 1 : 0) * 23
         + (isEnum() ? 1 : 0) * 29
@@ -100,6 +113,8 @@ public static Builder builder() {
   public abstract static class Builder {
     public abstract Builder setName(String name);
 
+    public abstract Builder setOriginalName(String originalName);
+
     public abstract Builder setType(TypeNode type);
 
     public abstract Builder setIsMessage(boolean isMessage);
@@ -118,6 +133,18 @@ public abstract static class Builder {
 
     public abstract Builder setDescription(String description);
 
-    public abstract Field build();
+    // Private accessors.
+    abstract String name();
+
+    abstract Optional<String> originalName();
+
+    abstract Field autoBuild();
+
+    public Field build() {
+      if (!originalName().isPresent()) {
+        setOriginalName(name());
+      }
+      return autoBuild();
+    }
   }
 }
diff --git a/src/main/java/com/google/api/generator/gapic/model/Message.java b/src/main/java/com/google/api/generator/gapic/model/Message.java
@@ -140,11 +140,14 @@ public Builder setEnumValues(List<String> names, List<Integer> numbers) {
     public Message build() {
       Message message = autoBuild();
       if (!message.fields().isEmpty()) {
-        message =
-            message
-                .toBuilder()
-                .setFieldMap(fields().stream().collect(Collectors.toMap(f -> f.name(), f -> f)))
-                .autoBuild();
+        Map<String, Field> fieldMap =
+            fields().stream().collect(Collectors.toMap(f -> f.name(), f -> f));
+        // Handles string occurrences of a field's original name in a protobuf, such as
+        // in the method signature annotaiton.
+        fields().stream()
+            .filter(f -> f.hasFieldNameConflict())
+            .forEach(f -> fieldMap.put(f.originalName(), f));
+        message = message.toBuilder().setFieldMap(fieldMap).autoBuild();
       }
       return message;
     }

diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java b/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
@@ -830,14 +830,47 @@ private static List<Field> parseFields(
     // Sort by ascending field index order. This is important for paged responses, where the first
     // repeated type is taken.
     fields.sort((f1, f2) -> f1.getIndex() - f2.getIndex());
+
+    // Mirror protoc's name conflict resolution behavior for fields.
+    // If a singular field's name equals that of a repeated field with "Count" or "List" suffixed,
+    // append the protobuf's field number to both fields' names.
+    // See:
+    // https://github.com/protocolbuffers/protobuf/blob/9df42757f97da9f748a464deeda96427a8f7ade0/src/google/protobuf/compiler/java/java_context.cc#L60
+    Map<String, Integer> repeatedFieldNamesToNumber =
+        fields.stream()
+            .filter(f -> f.isRepeated())
+            .collect(Collectors.toMap(f -> f.getName(), f -> f.getNumber()));
+    Set<Integer> fieldNumbersWithConflicts = new HashSet<>();
+    for (FieldDescriptor field : fields) {
+      Set<String> conflictingRepeatedFieldNames =
+          repeatedFieldNamesToNumber.keySet().stream()
+              .filter(
+                  n -> field.getName().equals(n + "_count") || field.getName().equals(n + "_list"))
+              .collect(Collectors.toSet());
+      if (!conflictingRepeatedFieldNames.isEmpty()) {
+        fieldNumbersWithConflicts.addAll(
+            conflictingRepeatedFieldNames.stream()
+                .map(n -> repeatedFieldNamesToNumber.get(n))
+                .collect(Collectors.toSet()));
+        fieldNumbersWithConflicts.add(field.getNumber());
+      }
+    }
+
     return fields.stream()
-        .map(f -> parseField(f, messageDescriptor, outputResourceReferencesSeen))
+        .map(
+            f ->
+                parseField(
+                    f,
+                    messageDescriptor,
+                    fieldNumbersWithConflicts.contains(f.getNumber()),
+                    outputResourceReferencesSeen))
         .collect(Collectors.toList());
   }
 
   private static Field parseField(
       FieldDescriptor fieldDescriptor,
       Descriptor messageDescriptor,
+      boolean hasFieldNameConflict,
       Set<ResourceReference> outputResourceReferencesSeen) {
     FieldOptions fieldOptions = fieldDescriptor.getOptions();
     MessageOptions messageOptions = messageDescriptor.getOptions();
@@ -882,8 +915,16 @@ private static Field parseField(
       }
     }
 
+    // Mirror protoc's name conflict resolution behavior for fields.
+    // For more context, trace hasFieldNameConflict back to where it gets passed in above.
+    String actualFieldName =
+        hasFieldNameConflict
+            ? fieldDescriptor.getName() + fieldDescriptor.getNumber()
+            : fieldDescriptor.getName();
+
     return fieldBuilder
-        .setName(fieldDescriptor.getName())
+        .setName(actualFieldName)
+        .setOriginalName(fieldDescriptor.getName())
         .setType(TypeParser.parseType(fieldDescriptor))
         .setIsMessage(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE)
         .setIsEnum(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.ENUM)

diff --git a/src/test/java/com/google/api/generator/gapic/composer/common/BUILD.bazel b/src/test/java/com/google/api/generator/gapic/composer/common/BUILD.bazel
@@ -29,6 +29,7 @@ TEST_DEPS = [
     "//src/main/java/com/google/api/generator/gapic/protoparser",
     "//src/main/java/com/google/api/generator/gapic/composer/defaultvalue",
     "//src/test/java/com/google/api/generator/gapic/testdata:deprecated_service_java_proto",
+    "//src/test/java/com/google/api/generator/gapic/testdata:bookshop_java_proto",
     "//src/test/java/com/google/api/generator/gapic/testdata:showcase_java_proto",
     "//src/test/java/com/google/api/generator/gapic/testdata:testgapic_java_proto",
     "@com_google_api_api_common//jar",

diff --git a/...t/java/com/google/api/generator/gapic/composer/common/ServiceClientClassComposerTest.java b/...t/java/com/google/api/generator/gapic/composer/common/ServiceClientClassComposerTest.java
@@ -62,8 +62,20 @@ public void generateServiceClasses_methodSignatureHasNestedFields() {
     JavaWriterVisitor visitor = new JavaWriterVisitor();
     clazz.classDefinition().accept(visitor);
     Utils.saveCodegenToFile(this.getClass(), "IdentityClient.golden", visitor.write());
-    Path goldenFilePath =
-        Paths.get(Utils.getGoldenDir(this.getClass()), "IdentityClient.golden");
+    Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "IdentityClient.golden");
+    assertCodeEquals(goldenFilePath, visitor.write());
+  }
+
+  @Test
+  public void generateServiceClasses_bookshopNameConflicts() {
+    GapicContext context = TestProtoLoader.instance().parseBookshopService();
+    Service protoService = context.services().get(0);
+    GapicClass clazz = ServiceClientClassComposer.instance().generate(context, protoService);
+
+    JavaWriterVisitor visitor = new JavaWriterVisitor();
+    clazz.classDefinition().accept(visitor);
+    Utils.saveCodegenToFile(this.getClass(), "BookshopClient.golden", visitor.write());
+    Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "BookshopClient.golden");
     assertCodeEquals(goldenFilePath, visitor.write());
   }
 }
diff --git a/src/test/java/com/google/api/generator/gapic/composer/common/TestProtoLoader.java b/src/test/java/com/google/api/generator/gapic/composer/common/TestProtoLoader.java
@@ -27,6 +27,7 @@
 import com.google.api.generator.gapic.protoparser.BatchingSettingsConfigParser;
 import com.google.api.generator.gapic.protoparser.Parser;
 import com.google.api.generator.gapic.protoparser.ServiceConfigParser;
+import com.google.bookshop.v1beta1.BookshopProto;
 import com.google.logging.v2.LogEntryProto;
 import com.google.logging.v2.LoggingConfigProto;
 import com.google.logging.v2.LoggingMetricsProto;
@@ -51,8 +52,7 @@
 
 public class TestProtoLoader {
   private static final TestProtoLoader INSTANCE =
-      new TestProtoLoader(
-          Transport.GRPC, "src/test/java/com/google/api/generator/gapic/testdata/");
+      new TestProtoLoader(Transport.GRPC, "src/test/java/com/google/api/generator/gapic/testdata/");
   private final String testFilesDirectory;
   private final Transport transport;
 
@@ -93,6 +93,34 @@ public GapicContext parseDeprecatedService() {
         .build();
   }
 
+  public GapicContext parseBookshopService() {
+    FileDescriptor fileDescriptor = BookshopProto.getDescriptor();
+    ServiceDescriptor serviceDescriptor = fileDescriptor.getServices().get(0);
+    assertEquals(serviceDescriptor.getName(), "Bookshop");
+
+    Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor);
+    Map<String, ResourceName> resourceNames = new HashMap<>();
+    Set<ResourceName> outputResourceNames = new HashSet<>();
+    List<Service> services =
+        Parser.parseService(
+            fileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
+
+    String jsonFilename = "bookshop_grpc_service_config.json";
+    Path jsonPath = Paths.get(testFilesDirectory, jsonFilename);
+    Optional<GapicServiceConfig> configOpt = ServiceConfigParser.parse(jsonPath.toString());
+    assertTrue(configOpt.isPresent());
+    GapicServiceConfig config = configOpt.get();
+
+    return GapicContext.builder()
+        .setMessages(messageTypes)
+        .setResourceNames(resourceNames)
+        .setServices(services)
+        .setServiceConfig(config)
+        .setHelperResourceNames(outputResourceNames)
+        .setTransport(transport)
+        .build();
+  }
+
   public GapicContext parseShowcaseEcho() {
     FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
     ServiceDescriptor echoServiceDescriptor = echoFileDescriptor.getServices().get(0);