[ggj][codegen][ast] fix: solidify codegen method order with AST Comparables (#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

Author: miraleung

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);
+ }
+}