fix: sort method signatures at proto parse time (#385)

googleapis · Oct 9, 2020 · c789364 · c789364
1 parent a3ebfec
commit c789364
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 62 deletions.
diff --git a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java
@@ -503,27 +503,7 @@ private static List<MethodDefinition> createMethodVariants(
     }
 
     String methodInputTypeName = methodInputType.reference().name();
-
-    // Make the method signature order deterministic, which helps with unit testing and per-version
-    // diffs.
-    List<List<MethodArgument>> sortedMethodSignatures =
-        method.methodSignatures().stream()
-            .sorted(
-                (s1, s2) -> {
-                  if (s1.size() != s2.size()) {
-                    return s1.size() - s2.size();
-                  }
-                  for (int i = 0; i < s1.size(); i++) {
-                    int compareVal = s1.get(i).compareTo(s2.get(i));
-                    if (compareVal != 0) {
-                      return compareVal;
-                    }
-                  }
-                  return 0;
-                })
-            .collect(Collectors.toList());
-
-    for (List<MethodArgument> signature : sortedMethodSignatures) {
+    for (List<MethodArgument> signature : method.methodSignatures()) {
       // Get the argument list.
       List<VariableExpr> arguments =
           signature.stream()

diff --git a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientTestClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientTestClassComposer.java
@@ -437,31 +437,11 @@ private static List<MethodDefinition> createTestMethods(
                 resourceNames,
                 messageTypes));
       } else {
-        // Make the method signature order deterministic, which helps with unit testing and
-        // per-version
-        // diffs.
-        List<List<MethodArgument>> sortedMethodSignatures =
-            method.methodSignatures().stream()
-                .sorted(
-                    (s1, s2) -> {
-                      if (s1.size() != s2.size()) {
-                        return s1.size() - s2.size();
-                      }
-                      for (int i = 0; i < s1.size(); i++) {
-                        int compareVal = s1.get(i).compareTo(s2.get(i));
-                        if (compareVal != 0) {
-                          return compareVal;
-                        }
-                      }
-                      return 0;
-                    })
-                .collect(Collectors.toList());
-
-        for (int i = 0; i < sortedMethodSignatures.size(); i++) {
+        for (int i = 0; i < method.methodSignatures().size(); i++) {
           javaMethods.add(
               createRpcTestMethod(
                   method,
-                  sortedMethodSignatures.get(i),
+                  method.methodSignatures().get(i),
                   i,
                   service.name(),
                   classMemberVarExprs,
@@ -470,7 +450,7 @@ private static List<MethodDefinition> createTestMethods(
           javaMethods.add(
               createRpcExceptionTestMethod(
                   method,
-                  sortedMethodSignatures.get(i),
+                  method.methodSignatures().get(i),
                   i,
                   service.name(),
                   classMemberVarExprs,

diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/MethodSignatureParser.java b/src/main/java/com/google/api/generator/gapic/protoparser/MethodSignatureParser.java
@@ -98,7 +98,29 @@ public static List<List<MethodArgument>> parseMethodSignatures(
       }
       signatures.addAll(flattenMethodSignatureVariants(argumentNames, argumentNameToOverloads));
     }
-    return signatures;
+
+    // Make the method signature order deterministic, which helps with unit testing and per-version
+    // diffs.
+    List<List<MethodArgument>> sortedMethodSignatures =
+        signatures.stream()
+            .sorted(
+                (s1, s2) -> {
+                  // Sort by number of arguments first.
+                  if (s1.size() != s2.size()) {
+                    return s1.size() - s2.size();
+                  }
+                  // Then by MethodSignature properties.
+                  for (int i = 0; i < s1.size(); i++) {
+                    int compareVal = s1.get(i).compareTo(s2.get(i));
+                    if (compareVal != 0) {
+                      return compareVal;
+                    }
+                  }
+                  return 0;
+                })
+            .collect(Collectors.toList());
+
+    return sortedMethodSignatures;
   }
 
   @VisibleForTesting

diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java
@@ -224,11 +224,14 @@ public void parseMethodSignatures_basic() {
             outputResourceNames);
     assertEquals(7, methodSignatures.size());
 
-    // Signature contents: ["content"].
+    // Signature contents: ["parent"].
     List<MethodArgument> methodArgs = methodSignatures.get(0);
     assertEquals(1, methodArgs.size());
     MethodArgument argument = methodArgs.get(0);
-    assertMethodArgumentEquals("content", TypeNode.STRING, ImmutableList.of(), argument);
+    TypeNode resourceNameType =
+        TypeNode.withReference(
+            ConcreteReference.withClazz(com.google.api.resourcenames.ResourceName.class));
+    assertMethodArgumentEquals("parent", resourceNameType, ImmutableList.of(), argument);
 
     // Signature contents: ["error"].
     methodArgs = methodSignatures.get(1);
@@ -237,33 +240,45 @@ public void parseMethodSignatures_basic() {
     assertMethodArgumentEquals(
         "error", TypeNode.withReference(createStatusReference()), ImmutableList.of(), argument);
 
-    // Signature contents: ["content", "severity"].
+    // Signature contents: ["name"], resource helper variant.
     methodArgs = methodSignatures.get(2);
-    assertEquals(2, methodArgs.size());
+    assertEquals(1, methodArgs.size());
     argument = methodArgs.get(0);
-    assertMethodArgumentEquals("content", TypeNode.STRING, ImmutableList.of(), argument);
-    argument = methodArgs.get(1);
-    assertMethodArgumentEquals(
-        "severity",
+    TypeNode foobarNameType =
         TypeNode.withReference(
-            VaporReference.builder().setName("Severity").setPakkage(ECHO_PACKAGE).build()),
-        ImmutableList.of(),
-        argument);
+            VaporReference.builder().setName("FoobarName").setPakkage(ECHO_PACKAGE).build());
+    assertMethodArgumentEquals("name", foobarNameType, ImmutableList.of(), argument);
 
-    // Signature contents: ["name"], String variant.
+    // Signature contents: ["content"].
     methodArgs = methodSignatures.get(3);
     assertEquals(1, methodArgs.size());
     argument = methodArgs.get(0);
-    assertMethodArgumentEquals("name", TypeNode.STRING, ImmutableList.of(), argument);
+    assertMethodArgumentEquals("content", TypeNode.STRING, ImmutableList.of(), argument);
 
-    // Signature contents: ["name"], resource helper variant.
+    // Signature contents: ["name"], String variant.
     methodArgs = methodSignatures.get(4);
     assertEquals(1, methodArgs.size());
     argument = methodArgs.get(0);
-    TypeNode foobarNameType =
+    assertMethodArgumentEquals("name", TypeNode.STRING, ImmutableList.of(), argument);
+
+    // Signature contents: ["parent"], String variant.
+    methodArgs = methodSignatures.get(5);
+    assertEquals(1, methodArgs.size());
+    argument = methodArgs.get(0);
+    assertMethodArgumentEquals("parent", TypeNode.STRING, ImmutableList.of(), argument);
+
+    // Signature contents: ["content", "severity"].
+    methodArgs = methodSignatures.get(6);
+    assertEquals(2, methodArgs.size());
+    argument = methodArgs.get(0);
+    assertMethodArgumentEquals("content", TypeNode.STRING, ImmutableList.of(), argument);
+    argument = methodArgs.get(1);
+    assertMethodArgumentEquals(
+        "severity",
         TypeNode.withReference(
-            VaporReference.builder().setName("FoobarName").setPakkage(ECHO_PACKAGE).build());
-    assertMethodArgumentEquals("name", foobarNameType, ImmutableList.of(), argument);
+            VaporReference.builder().setName("Severity").setPakkage(ECHO_PACKAGE).build()),
+        ImmutableList.of(),
+        argument);
   }
 
   @Test