Merge branch 'main' into renovate/com.google.code.gson-gson-2.x

pom.xml

@@ -27,12 +27,12 @@
     <dependency>
       <groupId>com.fasterxml.jackson.core</groupId>
       <artifactId>jackson-annotations</artifactId>
-      <version>2.13.2</version>
+      <version>2.16.1</version>
     </dependency>
     <dependency>
       <groupId>com.fasterxml.jackson.core</groupId>
       <artifactId>jackson-core</artifactId>
-      <version>2.13.2</version>
+      <version>2.16.1</version>
     </dependency>
     <dependency>
       <groupId>com.fasterxml.jackson.core</groupId>

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,7 +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,
@@ -61,6 +69,7 @@ public DocumentToProtoConverter(
     readResources(document);
     cleanupEnumNamingConflicts();
     this.protoFile.setHasLroDefinitions(applyLroConfiguration());
+    this.protoFile.setHasAnyFields(checkAnyFields());
     convertEnumFieldsToStrings();
   }
 
@@ -81,24 +90,110 @@ private ProtoFileMetadata readDocumentMetadata(Document document, String documen
 
   private void readSchema(Document document) {
     for (Map.Entry<String, Schema> entry : document.schemas().entrySet()) {
-      schemaToField(entry.getValue(), true);
+      schemaToField(entry.getValue(), true, "*** readSchema\n");
     }
     for (Message message : protoFile.getMessages().values()) {
       resolveReferences(message);
     }
+    schemaRead = true;
   }
 
   private void resolveReferences(Message message) {
     for (Field field : message.getFields()) {
       Message valueType = field.getValueType();
-      if (valueType.isRef()) {
+      if (valueType.isRef()) { // replace the field object with a link to the message it references
         field.setValueType(protoFile.getMessages().get(valueType.getName()));
       } else {
         resolveReferences(valueType);
       }
     }
   }
 
+  private boolean checkForAllowedAnyFields(Message message) {
+    return checkForAllowedAnyFields(message, message.getName());
+  }
+
+  private boolean checkForAllowedAnyFields(Message message, String previousFieldPath) {
+    // 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;
+    for (Field field : message.getFields()) {
+      Message valueType = field.getValueType();
+      String currentFieldPath = previousFieldPath + "." + field.getName();
+      if (valueType.getName() == Message.PRIMITIVES.get("google.protobuf.Any").getName()) {
+        if (currentFieldPath.endsWith(".error.details")) {
+          haveAny = true;
+          if (trace) {
+            System.err.printf("Found ANY field at %s\n", currentFieldPath);
+          }
+        } else {
+          throw new IllegalArgumentException(
+              "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. 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() {
@@ -209,6 +304,26 @@ private void convertEnumFieldsToStrings() {
     }
   }
 
+  private boolean checkAnyFields() {
+    boolean haveAny = false;
+    // Note that we only check for Any fields for messages rooted in requests and responses. We
+    // don't want to initiate the check in sub-messages that will be included in those, because then
+    // the path to the Any field may incorrectly fail to match where it's actually included and
+    // we'll get an erroneous exception about incorrect usage of Any
+    for (GrpcService service : protoFile.getServices().values()) {
+      for (GrpcMethod method : service.getMethods()) {
+        // It's important these checks are not short-circuited!
+
+        // TODO: Decide whether should we disallow error.details.Any on inputs. The only use case
+        // would seem to be somehow echoing the error message back to the server?
+        boolean inInput = checkForAllowedAnyFields(method.getInput());
+        boolean inOutput = checkForAllowedAnyFields(method.getOutput());
+        haveAny = haveAny || inInput || inOutput;
+      }
+    }
+    return haveAny;
+  }
+
   private boolean applyLroConfiguration() {
     //
     // 1. Set `operation_field` annotations (Operation fields essential for LRO).
@@ -306,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();
@@ -405,16 +520,23 @@ private Option createOption(String optionName, Object scalarValue) {
     return option;
   }
 
-  private Field schemaToField(Schema sch, boolean optional) {
+  private Field schemaToField(Schema sch, boolean optional, String debugPreviousPath) {
     String name = Name.anyCamel(sch.key()).toCapitalizedLowerUnderscore();
     String description = sch.description();
     Message valueType = null;
     boolean repeated = false;
     Message keyType = null;
+    String debugCurrentPath =
+        debugPreviousPath + String.format("SCHEMA: %s\n%s\n----\n", name, description);
+
+    if (trace) {
+      System.err.printf("*** schemaToField: \n%s", debugCurrentPath);
+    }
 
     switch (sch.type()) {
       case ANY:
-        throw new IllegalArgumentException("Any type detected in schema: " + sch);
+        valueType = Message.PRIMITIVES.get("google.protobuf.Any");
+        break;
       case ARRAY:
         repeated = true;
         break;
@@ -489,7 +611,8 @@ private Field schemaToField(Schema sch, boolean optional) {
 
     if (repeated) {
       Field subField =
-          schemaToField(keyType == null ? sch.items() : sch.additionalProperties(), true);
+          schemaToField(
+              keyType == null ? sch.items() : sch.additionalProperties(), true, debugCurrentPath);
       valueType = subField.getValueType();
     }
 
@@ -500,8 +623,9 @@ private Field schemaToField(Schema sch, boolean optional) {
       return field;
     }
 
+    // Recurse for nested messages
     for (Map.Entry<String, Schema> entry : sch.properties().entrySet()) {
-      Field valueTypeField = schemaToField(entry.getValue(), true);
+      Field valueTypeField = schemaToField(entry.getValue(), true, debugCurrentPath);
       valueType.getFields().add(valueTypeField);
       if (valueTypeField.getValueType().isEnum()) {
         valueType.getEnums().add(valueTypeField.getValueType());
@@ -520,6 +644,7 @@ private Field schemaToField(Schema sch, boolean optional) {
     } else if (!valueType.isRef()) {
       if (valueType.getDescription() != null
           && existingMessage.getDescription() != null
+          // TODO: not clear on the reason this was originally put in
           && valueType.getDescription().length() < existingMessage.getDescription().length()) {
         putAllMessages(valueType.getName(), valueType);
       }
@@ -592,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<>();
@@ -633,7 +764,7 @@ private void readResources(Document document) {
 
         for (Schema pathParam : method.pathParams().values()) {
           boolean required = methodSignatureParamNames.containsKey(pathParam.getIdentifier());
-          Field pathField = schemaToField(pathParam, !required);
+          Field pathField = schemaToField(pathParam, !required, "readResources(A):)  ");
           if (required) {
             Option opt = createOption("google.api.field_behavior", ProtoOptionValues.REQUIRED);
             pathField.getOptions().add(opt);
@@ -647,7 +778,7 @@ private void readResources(Document document) {
 
         for (Schema queryParam : method.queryParams().values()) {
           boolean required = methodSignatureParamNames.containsKey(queryParam.getIdentifier());
-          Field queryField = schemaToField(queryParam, !required);
+          Field queryField = schemaToField(queryParam, !required, "readResources(B):  ");
           if (required) {
             Option opt = createOption("google.api.field_behavior", ProtoOptionValues.REQUIRED);
             queryField.getOptions().add(opt);

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

@@ -37,6 +37,13 @@ public class Message extends ProtoElement<Message> {
     PRIMITIVES.put("float", new Message("float", false, false, null));
     PRIMITIVES.put("double", new Message("double", false, false, null));
     PRIMITIVES.put("", new Message("", false, true, null));
+
+    // This isn't technically a primitive, but it is a fundamental well-known-type with no a priori
+    // structure.
+    //
+    // TODO: If we start accepting additional well-known types, create a specific data structure for
+    // those rather than overloading "PRIMITIVES".
+    PRIMITIVES.put("google.protobuf.Any", new Message("google.protobuf.Any", false, false, null));
   }
 
   private final SortedSet<Field> fields = new TreeSet<>();