[ggj][codegen][ast] fix: solidify codegen method order with AST Compa…

…rables (#311)

* feat: add protobuf comment parser util

* fix: add basic proto build rules

* feat: add header comments to ServiceClient

* fix: build protoc at test time

* fix!: wrap protobuf location and process comments

* fix: solidify codegen method order with TypeNode/MethodArg and Comparable

* fix: clean up tests

src/main/java/com/google/api/generator/engine/ast/TypeNode.java

@@ -23,7 +23,7 @@
 import javax.annotation.Nullable;
 
 @AutoValue
-public abstract class TypeNode implements AstNode {
+public abstract class TypeNode implements AstNode, Comparable<TypeNode> {
   static final Reference EXCEPTION_REFERENCE = ConcreteReference.withClazz(Exception.class);
   public static final Reference WILDCARD_REFERENCE = ConcreteReference.wildcard();
 
@@ -88,6 +88,31 @@ public enum TypeKind {
   @Nullable
   public abstract Reference reference();
 
+  @Override
+  public int compareTo(TypeNode other) {
+    // Ascending order of name.
+    if (isPrimitiveType()) {
+      if (other.isPrimitiveType()) {
+        return typeKind().name().compareTo(other.typeKind().name());
+      }
+      // b is a reference type or null, so a < b.
+      return -1;
+    }
+
+    if (this.equals(TypeNode.NULL)) {
+      // Can't self-compare, so we don't need to check whether the other one is TypeNode.NULL.
+      return other.isPrimitiveType() ? 1 : -1;
+    }
+
+    if (other.isPrimitiveType() || other.equals(TypeNode.NULL)) {
+      return 1;
+    }
+
+    // Both are reference types.
+    // TODO(miraleung): Replace this with a proper reference Comaparator.
+    return reference().fullName().compareTo(other.reference().fullName());
+  }
+
   public static Builder builder() {
     return new AutoValue_TypeNode.Builder().setIsArray(false);
   }

src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java

@@ -476,7 +476,26 @@ private static List<MethodDefinition> createMethodVariants(
     Preconditions.checkNotNull(
         inputMessage, String.format("Message %s not found", methodInputTypeName));
 
-    for (List<MethodArgument> signature : method.methodSignatures()) {
+    // 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) {
       // Get the argument list.
       List<VariableExpr> arguments =
           signature.stream()

src/main/java/com/google/api/generator/gapic/model/MethodArgument.java

@@ -20,7 +20,7 @@
 import java.util.List;
 
 @AutoValue
-public abstract class MethodArgument {
+public abstract class MethodArgument implements Comparable<MethodArgument> {
   public abstract String name();
 
   public abstract TypeNode type();
@@ -32,6 +32,15 @@ public abstract class MethodArgument {
   // Returns true if this is a resource name helper tyep.
   public abstract boolean isResourceNameHelper();
 
+  @Override
+  public int compareTo(MethodArgument other) {
+    int compareVal = type().compareTo(other.type());
+    if (compareVal == 0) {
+      compareVal = name().compareTo(other.name());
+    }
+    return compareVal;
+  }
+
   public static Builder builder() {
     return new AutoValue_MethodArgument.Builder()
         .setNestedTypes(ImmutableList.of())

src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java

@@ -14,6 +14,7 @@
 
 package com.google.api.generator.engine.ast;
 
+import static com.google.common.truth.Truth.assertThat;
 import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertTrue;
 import static org.junit.Assert.assertThrows;
@@ -112,6 +113,36 @@ public void type_wildcardUpperBoundGenerics() {
             .build());
   }
 
+  @Test
+  public void compareTypes() {
+    // Primitive and primitive.
+    // Can't compare objects to themselves, so this test is omitted.
+    assertThat(TypeNode.INT.compareTo(TypeNode.BOOLEAN)).isGreaterThan(0);
+    assertThat(TypeNode.BOOLEAN.compareTo(TypeNode.INT)).isLessThan(0);
+
+    // Primitive and null.
+    assertThat(TypeNode.INT.compareTo(TypeNode.NULL)).isLessThan(0);
+    assertThat(TypeNode.NULL.compareTo(TypeNode.INT)).isGreaterThan(0);
+
+    // Primitive and reference.
+    assertThat(TypeNode.INT.compareTo(TypeNode.INT_OBJECT)).isLessThan(0);
+    assertThat(TypeNode.INT.compareTo(TypeNode.STRING)).isLessThan(0);
+    assertThat(TypeNode.INT_OBJECT.compareTo(TypeNode.INT)).isGreaterThan(0);
+
+    // Reference and null.
+    // No test for null against null because we can't compare objects to themselves.
+    assertThat(TypeNode.INT_OBJECT.compareTo(TypeNode.NULL)).isGreaterThan(0);
+    assertThat(TypeNode.NULL.compareTo(TypeNode.BOOLEAN_OBJECT)).isLessThan(0);
+
+    // Reference and reference. Sorted alphabetically by package.
+    assertThat(TypeNode.BOOLEAN_OBJECT.compareTo(TypeNode.INT_OBJECT)).isLessThan(0);
+    assertThat(TypeNode.BOOLEAN_OBJECT.compareTo(TypeNode.STRING)).isLessThan(0);
+    assertThat(
+            TypeNode.BOOLEAN_OBJECT.compareTo(
+                TypeNode.withReference(ConcreteReference.withClazz(Arrays.class))))
+        .isLessThan(0);
+  }
+
   @Test
   public void invalidType_topLevelWildcard() {
     assertThrows(

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

@@ -203,8 +203,11 @@ public void generateServiceClasses() {
           + "    return operationsClient;\n"
           + "  }\n"
           + "\n"
-          + "  public final EchoResponse echo(String content) {\n"
-          + "    EchoRequest request = EchoRequest.newBuilder().setContent(content).build();\n"
+          + "  public final EchoResponse echo(ResourceName parent) {\n"
+          + "    EchoRequest request =\n"
+          + "        EchoRequest.newBuilder()\n"
+          + "            .setParent(Strings.isNullOrEmpty(parent) ? null : parent.toString())\n"
+          + "            .build();\n"
           + "    return echo(request);\n"
           + "  }\n"
           + "\n"
@@ -213,22 +216,21 @@ public void generateServiceClasses() {
           + "    return echo(request);\n"
           + "  }\n"
           + "\n"
-          + "  public final EchoResponse echo(String content, Severity severity) {\n"
+          + "  public final EchoResponse echo(FoobarName name) {\n"
           + "    EchoRequest request =\n"
-          + "        EchoRequest.newBuilder().setContent(content).setSeverity(severity).build();\n"
+          + "        EchoRequest.newBuilder()\n"
+          + "            .setName(Strings.isNullOrEmpty(name) ? null : name.toString())\n"
+          + "            .build();\n"
           + "    return echo(request);\n"
           + "  }\n"
           + "\n"
-          + "  public final EchoResponse echo(String name) {\n"
-          + "    EchoRequest request = EchoRequest.newBuilder().setName(name).build();\n"
+          + "  public final EchoResponse echo(String content) {\n"
+          + "    EchoRequest request = EchoRequest.newBuilder().setContent(content).build();\n"
           + "    return echo(request);\n"
           + "  }\n"
           + "\n"
-          + "  public final EchoResponse echo(FoobarName name) {\n"
-          + "    EchoRequest request =\n"
-          + "        EchoRequest.newBuilder()\n"
-          + "            .setName(Strings.isNullOrEmpty(name) ? null : name.toString())\n"
-          + "            .build();\n"
+          + "  public final EchoResponse echo(String name) {\n"
+          + "    EchoRequest request = EchoRequest.newBuilder().setName(name).build();\n"
           + "    return echo(request);\n"
           + "  }\n"
           + "\n"
@@ -237,11 +239,9 @@ public void generateServiceClasses() {
           + "    return echo(request);\n"
           + "  }\n"
           + "\n"
-          + "  public final EchoResponse echo(ResourceName parent) {\n"
+          + "  public final EchoResponse echo(String content, Severity severity) {\n"
           + "    EchoRequest request =\n"
-          + "        EchoRequest.newBuilder()\n"
-          + "            .setParent(Strings.isNullOrEmpty(parent) ? null : parent.toString())\n"
-          + "            .build();\n"
+          + "        EchoRequest.newBuilder().setContent(content).setSeverity(severity).build();\n"
           + "    return echo(request);\n"
           + "  }\n"
           + "\n"

src/test/java/com/google/api/generator/gapic/model/BUILD.bazel

@@ -2,6 +2,7 @@ package(default_visibility = ["//visibility:public"])
 
 TESTS = [
     "GapicServiceConfigTest",
+    "MethodArgumentTest",
     "MethodTest",
 ]
 
@@ -20,6 +21,7 @@ filegroup(
     deps = [
         "//:service_config_java_proto",
         "//src/main/java/com/google/api/generator:autovalue",
+        "//src/main/java/com/google/api/generator/engine/ast",
         "//src/main/java/com/google/api/generator/gapic/model",
         "//src/main/java/com/google/api/generator/gapic/protoparser",
         "//src/test/java/com/google/api/generator/gapic/testdata:showcase_java_proto",

src/test/java/com/google/api/generator/gapic/model/MethodArgumentTest.java

@@ -0,0 +1,51 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.api.generator.gapic.model;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.api.generator.engine.ast.TypeNode;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+import org.junit.Test;
+
+public class MethodArgumentTest {
+  @Test
+  public void compareMethodArguments() {
+    BiFunction<String, TypeNode, MethodArgument> methodArgFn =
+        (name, type) -> MethodArgument.builder().setName(name).setType(type).build();
+
+    // Cursory sanity-check of type-only differences, since these are already covered in the
+    // TypeNode test.
+    assertThat(
+            methodArgFn
+                .apply("foo", TypeNode.INT)
+                .compareTo(methodArgFn.apply("foo", TypeNode.BOOLEAN)))
+        .isGreaterThan(0);
+    assertThat(
+            methodArgFn
+                .apply("foo", TypeNode.INT)
+                .compareTo(methodArgFn.apply("foo", TypeNode.INT_OBJECT)))
+        .isLessThan(0);
+
+    // Non-type-based differences.
+    Function<String, MethodArgument> simpleMethodArgFn =
+        (name) -> methodArgFn.apply(name, TypeNode.INT);
+    assertThat(simpleMethodArgFn.apply("foo").compareTo(simpleMethodArgFn.apply("bar")))
+        .isGreaterThan(0);
+    assertThat(simpleMethodArgFn.apply("bar").compareTo(simpleMethodArgFn.apply("foo")))
+        .isLessThan(0);
+  }
+}