fix: prevent name collisions between messages and services (#107)

Previously, a Discovery `schema` and a `resource` that differed only in casing would result in a proto `message` and `service`, respectively, with the same upper-camel-case name in the same namespace. This would cause downstream problems when attempting to parse the proto.

We now disambiguate such cases by suffixing the names of such conflicting services with `Service`.

We explicitly do NOT deal with recursive disambiguation. That is, if the first disambiguation attempt between a message and a service fails, we do not attempt to find a different suffix to continue the disambiguation. If needed, we can add this later, but for the moment it seems a good idea to flag such occurrences.

src/main/java/com/google/cloud/discotoproto3converter/ConverterApp.java

@@ -48,6 +48,9 @@ protected ConverterApp(ConverterWriter writer) {
     this.writer = writer;
   }
 
+  // Note that serviceIgnoreList should contain the names of services as they would be naively
+  // derived from the Discovery document (i.e. before disambiguation if they conflict with any of
+  // the messages).
   public void convert(
       String discoveryDocPath,
       String previousProtoPath,

src/main/java/com/google/cloud/discotoproto3converter/proto3/DocumentToProtoConverter.java

@@ -44,11 +44,15 @@ public class DocumentToProtoConverter {
   private final Set<String> messageIgnoreSet;
   private final String relativeLinkPrefix;
   private final boolean enumsAsStrings;
+  private boolean schemaRead;
 
   // Set this to "true" to get some tracing output on stderr during development. Leave this as
   // "false" for production code.
   private final boolean trace = false;
 
+  // Note that serviceIgnoreSet should contain the names of services as they would be naively
+  // derived from the Discovery document (i.e. before disambiguation if they conflict with any of
+  // the messages).
   public DocumentToProtoConverter(
       Document document,
       String documentFileName,
@@ -91,6 +95,7 @@ private void readSchema(Document document) {
     for (Message message : protoFile.getMessages().values()) {
       resolveReferences(message);
     }
+    schemaRead = true;
   }
 
   private void resolveReferences(Message message) {
@@ -109,7 +114,7 @@ private boolean checkForAllowedAnyFields(Message message) {
   }
 
   private boolean checkForAllowedAnyFields(Message message, String previousFieldPath) {
-    // We want to we recursively check every child and don't short-circuit when haveAny becomes
+    // We want to recursively check every child so we don't short-circuit when haveAny becomes
     // true, as we rely on the side effect (exception) to signal a google.protobuf.Any in an
     // unsupported location.
     boolean haveAny = false;
@@ -127,15 +132,68 @@ private boolean checkForAllowedAnyFields(Message message, String previousFieldPa
               "illegal ANY type not under \"*.error.details\": " + currentFieldPath);
         }
       } else {
-        // Check for Any fields in this field's children, even if we already determined
-        // that its siblings contain Any fields.
+        // Check for Any fields in this field's children, even if we already determined that its
+        // siblings contain Any fields. This allows us to raise an exception if we have an Any in an
+        // unsupported location.
         boolean childrenHaveAny = checkForAllowedAnyFields(field.getValueType(), currentFieldPath);
         haveAny = childrenHaveAny || haveAny;
       }
     }
     return haveAny;
   }
 
+  // Tries to resolve name collisions between an intended service name and already-registered
+  // messages. Returns a non-conflicting service name to use, or throws an exception if the
+  // collision could not be resolved.
+  private String avoidNameCollisions(String originalServiceName) {
+    String newServiceName = originalServiceName;
+    Map<String, Message> messages = protoFile.getMessages();
+    if (messages.containsKey(originalServiceName)) {
+      newServiceName = originalServiceName + "Service";
+      if (messages.containsKey(newServiceName)) {
+        throw new IllegalArgumentException(
+            "could not resolve name collision for service \""
+                + originalServiceName
+                + "\": "
+                + "messages \""
+                + originalServiceName
+                + "\" and \""
+                + newServiceName
+                + "\" both exist");
+      }
+    }
+
+    // We need to verify that newServiceName, whether it was modified above or not, does not
+    // conflict with a previously registered service name. This could happen if either this service
+    // or a previously registered service were modified to avoid a name collision.
+    if (protoFile.getServices().containsKey(newServiceName)) {
+      if (newServiceName.endsWith("Service")) {
+        int index = newServiceName.lastIndexOf("Service");
+        String possibleMessage = newServiceName.substring(0, index);
+        if (messages.containsKey(possibleMessage)) {
+          throw new IllegalArgumentException(
+              "could not resolve name collision for service \""
+                  + originalServiceName
+                  + "\": "
+                  + "message \""
+                  + possibleMessage
+                  + "\" "
+                  + "and service \""
+                  + newServiceName
+                  + "\" both exist");
+        }
+      }
+
+      if (messages.containsKey(newServiceName)) {
+        // We should never reach here because the Discovery document should never have two
+        // identically named services (resources), but we have this code to verify our assumptions.
+        throw new IllegalArgumentException(
+            "multiple definitions of services named \"" + newServiceName + "\"");
+      }
+    }
+    return newServiceName;
+  }
+
   // If there is a naming conflict between two or more enums in the same message, convert all
   // enum types to strings (happens rarely, but happens).
   private void cleanupEnumNamingConflicts() {
@@ -363,7 +421,7 @@ private boolean applyLroConfiguration() {
           }
         }
 
-        // A temprorary workaround to detect polling service to use if there is no match.
+        // A temporary workaround to detect polling service to use if there is no match.
         if (pollingServiceMessageFields.size() == 1
             && pollingServiceMessageFields.containsKey("parent_id")) {
           noMatchPollingServiceName = service.getName();
@@ -659,20 +717,26 @@ private String getMessageName(Schema sch, Boolean isEnum) {
   }
 
   private void readResources(Document document) {
+    if (!schemaRead) {
+      throw new IllegalStateException(
+          "schema should be read in before resources in order to avoid name collisions");
+    }
+
     String endpointSuffix = document.baseUrl().substring(document.rootUrl().length());
     endpointSuffix = endpointSuffix.startsWith("/") ? endpointSuffix : '/' + endpointSuffix;
     endpointSuffix = endpointSuffix.replaceAll("/$", "");
     String endpoint = document.rootUrl().replaceAll("(^https://)|(/$)", "");
 
     for (Map.Entry<String, List<Method>> entry : document.resources().entrySet()) {
-      String grpcServiceName = Name.anyCamel(entry.getKey()).toUpperCamel();
-      GrpcService service =
-          new GrpcService(grpcServiceName, getServiceDescription(grpcServiceName));
-      if (serviceIgnoreSet.contains(service.getName())) {
+      String originalGrpcServiceName = Name.anyCamel(entry.getKey()).toUpperCamel();
+      if (serviceIgnoreSet.contains(originalGrpcServiceName)) {
         // Ignore the service (as early as possible to avoid dependency failures on previously
         // ignored request messages used in this service).
         continue;
       }
+      String grpcServiceName = avoidNameCollisions(originalGrpcServiceName);
+      GrpcService service =
+          new GrpcService(grpcServiceName, getServiceDescription(originalGrpcServiceName));
       service.getOptions().add(createOption("google.api.default_host", endpoint));
 
       Set<String> authScopes = new HashSet<>();

src/test/java/com/google/cloud/discotoproto3converter/DiscoToProto3ConverterAppTest.java

@@ -118,6 +118,107 @@ public void convertEnumsAsStrings() throws IOException {
     assertEquals(baselineBody, actualBody);
   }
 
+  @Test
+  public void nameCollisionAvoidanceSuccessOneMessageOneService() throws IOException {
+    DiscoToProto3ConverterApp app = new DiscoToProto3ConverterApp();
+    Path prefix = Paths.get("google", "cloud", "compute", "v1small");
+    Path discoveryDocPath =
+        Paths.get(
+            "src",
+            "test",
+            "resources",
+            prefix.toString(),
+            "compute.v1small.collision.message-1.service-1.json");
+    Path generatedFilePath =
+        Paths.get(
+            outputDir.toString(),
+            prefix.toString(),
+            "compute.v1small.collision.message-1.service-1.proto");
+
+    app.convert(
+        discoveryDocPath.toString(),
+        null,
+        generatedFilePath.toString(),
+        "",
+        "",
+        "https://cloud.google.com",
+        "true",
+        "true");
+
+    String actualBody = readFile(generatedFilePath);
+    Path baselineFilePath =
+        Paths.get(
+            "src",
+            "test",
+            "resources",
+            prefix.toString(),
+            "compute.v1small.collision.message-1.service-1.proto.baseline");
+    String baselineBody = readFile(baselineFilePath);
+    assertEquals(baselineBody, actualBody);
+  }
+
+  @Test
+  public void nameCollisionAvoidanceFailureTwoMessagesOneService() throws IOException {
+    DiscoToProto3ConverterApp app = new DiscoToProto3ConverterApp();
+    Path prefix = Paths.get("google", "cloud", "compute", "v1small");
+    Path discoveryDocPath =
+        Paths.get(
+            "src",
+            "test",
+            "resources",
+            prefix.toString(),
+            "compute.v1small.collision.message-2.service-1.json");
+    Path generatedFilePath =
+        Paths.get(
+            outputDir.toString(),
+            prefix.toString(),
+            "compute.v1small.collision.message-2.service-1.proto");
+
+    assertThrows(
+        java.lang.IllegalArgumentException.class,
+        () ->
+            app.convert(
+                discoveryDocPath.toString(),
+                null,
+                generatedFilePath.toString(),
+                "",
+                "",
+                "https://cloud.google.com",
+                "true",
+                "true"));
+  }
+
+  @Test
+  public void nameCollisionAvoidanceFailureOneMessageTwoServices() throws IOException {
+    DiscoToProto3ConverterApp app = new DiscoToProto3ConverterApp();
+    Path prefix = Paths.get("google", "cloud", "compute", "v1small");
+    Path discoveryDocPath =
+        Paths.get(
+            "src",
+            "test",
+            "resources",
+            prefix.toString(),
+            "compute.v1small.collision.message-1.service-2.json");
+    Path generatedFilePath =
+        Paths.get(
+            outputDir.toString(),
+            prefix.toString(),
+            "compute.v1small.collision.message-1.service-2.proto");
+
+    assertThrows(
+        java.lang.IllegalArgumentException.class,
+        () ->
+            app.convert(
+                discoveryDocPath.toString(),
+                null,
+                generatedFilePath.toString(),
+                "",
+                "",
+                "https://cloud.google.com",
+                "true",
+                "true"));
+  }
+
   @Test
   public void convertAnyFieldInError() throws IOException {
     DiscoToProto3ConverterApp app = new DiscoToProto3ConverterApp();