From e4f7374b03091ab03c82a9aa521714791cdfecb6 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 22 Sep 2020 13:04:07 -0700 Subject: [PATCH 1/5] fix!: refactor field into MethodArgument, add enum/msg flags --- .../ServiceClientCommentComposer.java | 5 ++- .../api/generator/gapic/model/Field.java | 14 ++++++- .../generator/gapic/model/MethodArgument.java | 27 ++++++------ .../protoparser/MethodSignatureParser.java | 42 +++++++++---------- .../generator/gapic/protoparser/Parser.java | 2 + .../gapic/model/MethodArgumentTest.java | 7 +++- .../MethodSignatureParserTest.java | 29 +++++++++++-- .../gapic/protoparser/ParserTest.java | 8 ++-- 8 files changed, 85 insertions(+), 49 deletions(-) diff --git a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientCommentComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientCommentComposer.java index d79bf7f03e..336f2e8e34 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientCommentComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientCommentComposer.java @@ -157,8 +157,9 @@ static List createRpcMethodHeaderComment( "request", "The request object containing all of the parameters for the API call."); } else { for (MethodArgument argument : methodArguments) { - methodJavadocBuilder.addParam( - argument.name(), argument.hasDescription() ? argument.description() : EMPTY_STRING); + String description = + argument.field().hasDescription() ? argument.field().description() : EMPTY_STRING; + methodJavadocBuilder.addParam(argument.name(), description); } } diff --git a/src/main/java/com/google/api/generator/gapic/model/Field.java b/src/main/java/com/google/api/generator/gapic/model/Field.java index 57d836979e..2008d8ab6d 100644 --- a/src/main/java/com/google/api/generator/gapic/model/Field.java +++ b/src/main/java/com/google/api/generator/gapic/model/Field.java @@ -24,6 +24,10 @@ public abstract class Field { public abstract TypeNode type(); + public abstract boolean isMessage(); + + public abstract boolean isEnum(); + public abstract boolean isRepeated(); public abstract boolean isMap(); @@ -43,7 +47,11 @@ public boolean hasResourceReference() { } public static Builder builder() { - return new AutoValue_Field.Builder().setIsRepeated(false).setIsMap(false); + return new AutoValue_Field.Builder() + .setIsMessage(false) + .setIsEnum(false) + .setIsRepeated(false) + .setIsMap(false); } @AutoValue.Builder @@ -52,6 +60,10 @@ public abstract static class Builder { public abstract Builder setType(TypeNode type); + public abstract Builder setIsMessage(boolean isMessage); + + public abstract Builder setIsEnum(boolean isEnum); + public abstract Builder setIsRepeated(boolean isRepeated); public abstract Builder setIsMap(boolean isMap); diff --git a/src/main/java/com/google/api/generator/gapic/model/MethodArgument.java b/src/main/java/com/google/api/generator/gapic/model/MethodArgument.java index f7ce9ad2b9..f5776aa730 100644 --- a/src/main/java/com/google/api/generator/gapic/model/MethodArgument.java +++ b/src/main/java/com/google/api/generator/gapic/model/MethodArgument.java @@ -18,28 +18,25 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import java.util.List; -import javax.annotation.Nullable; @AutoValue public abstract class MethodArgument implements Comparable { + // The method argument name. public abstract String name(); + // The type. This can be different from the associated field (e.g. for resource references). public abstract TypeNode type(); - // Records the path of nested types in descending order, excluding type() (which would have + // Additional metadata. + public abstract Field field(); + + // Records the path of nested fields in descending order, excluding type() (which would have // appeared as the last element). - public abstract ImmutableList nestedTypes(); + public abstract ImmutableList nestedFields(); - // Returns true if this is a resource name helper tyep. + // Returns true if this is a resource name helper method argument. public abstract boolean isResourceNameHelper(); - @Nullable - public abstract String description(); - - public boolean hasDescription() { - return description() != null; - } - @Override public int compareTo(MethodArgument other) { int compareVal = type().compareTo(other.type()); @@ -51,7 +48,7 @@ public int compareTo(MethodArgument other) { public static Builder builder() { return new AutoValue_MethodArgument.Builder() - .setNestedTypes(ImmutableList.of()) + .setNestedFields(ImmutableList.of()) .setIsResourceNameHelper(false); } @@ -61,11 +58,11 @@ public abstract static class Builder { public abstract Builder setType(TypeNode type); - public abstract Builder setNestedTypes(List nestedTypes); + public abstract Builder setField(Field field); - public abstract Builder setIsResourceNameHelper(boolean isResourceNameHelper); + public abstract Builder setNestedFields(List nestedFields); - public abstract Builder setDescription(String description); + public abstract Builder setIsResourceNameHelper(boolean isResourceNameHelper); public abstract MethodArgument build(); } 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 index baef34d268..ec218d8768 100644 --- a/src/main/java/com/google/api/generator/gapic/protoparser/MethodSignatureParser.java +++ b/src/main/java/com/google/api/generator/gapic/protoparser/MethodSignatureParser.java @@ -65,17 +65,17 @@ public static List> parseMethodSignatures( // stringSig.split: ["content", "error"]. for (String argumentName : stringSig.split(METHOD_SIGNATURE_DELIMITER)) { // For resource names, this will be empty. - List argumentTypePathAcc = new ArrayList<>(); + List argumentFieldPathAcc = new ArrayList<>(); // There should be more than one type returned only when we encounter a reousrce name. - Map argumentTypes = - parseTypeAndCommentFromArgumentName( + Map argumentTypes = + parseTypeFromArgumentName( argumentName, servicePackage, inputMessage, messageTypes, resourceNames, patternsToResourceNames, - argumentTypePathAcc, + argumentFieldPathAcc, outputArgResourceNames); int dotLastIndex = argumentName.lastIndexOf(DOT); String actualArgumentName = @@ -88,11 +88,11 @@ public static List> parseMethodSignatures( e -> MethodArgument.builder() .setName(actualArgumentName) - .setDescription(e.getValue()) // May be null. .setType(e.getKey()) + .setField(e.getValue()) .setIsResourceNameHelper( argumentTypes.size() > 1 && !e.getKey().equals(TypeNode.STRING)) - .setNestedTypes(argumentTypePathAcc) + .setNestedFields(argumentFieldPathAcc) .build()) .collect(Collectors.toList())); } @@ -143,18 +143,17 @@ private static List> flattenMethodSignatureVariants( return methodArgs; } - private static Map parseTypeAndCommentFromArgumentName( + private static Map parseTypeFromArgumentName( String argumentName, String servicePackage, Message inputMessage, Map messageTypes, Map resourceNames, Map patternsToResourceNames, - List argumentTypePathAcc, + List argumentFieldPathAcc, Set outputArgResourceNames) { - // Comment values may be null. - Map typeToComment = new HashMap<>(); + Map typeToField = new HashMap<>(); int dotIndex = argumentName.indexOf(DOT); if (dotIndex < 1) { Field field = inputMessage.fieldMap().get(argumentName); @@ -164,8 +163,8 @@ private static Map parseTypeAndCommentFromArgumentName( "Field %s not found from input message %s values %s", argumentName, inputMessage.name(), inputMessage.fieldMap().keySet())); if (!field.hasResourceReference()) { - typeToComment.put(field.type(), field.description()); - return typeToComment; + typeToField.put(field.type(), field); + return typeToField; } // Parse the resource name tyeps. @@ -177,14 +176,10 @@ private static Map parseTypeAndCommentFromArgumentName( resourceNames, patternsToResourceNames); outputArgResourceNames.addAll(resourceNameArgs); - typeToComment.put( - TypeNode.STRING, - resourceNameArgs.isEmpty() ? null : resourceNameArgs.get(0).description()); - typeToComment.putAll( - resourceNameArgs.stream() - .collect( - Collectors.toMap(r -> r.type(), r -> r.hasDescription() ? r.description() : ""))); - return typeToComment; + typeToField.put(TypeNode.STRING, field); + typeToField.putAll( + resourceNameArgs.stream().collect(Collectors.toMap(r -> r.type(), r -> field))); + return typeToField; } Preconditions.checkState( @@ -197,6 +192,7 @@ private static Map parseTypeAndCommentFromArgumentName( // Must be a sub-message for a type's subfield to be valid. Field firstField = inputMessage.fieldMap().get(firstFieldName); + // Validate the field into which we're descending. Preconditions.checkState( !firstField.isRepeated(), String.format("Cannot descend into repeated field %s", firstField.name())); @@ -213,15 +209,15 @@ private static Map parseTypeAndCommentFromArgumentName( String.format( "Message type %s for field reference %s invalid", firstFieldTypeName, firstFieldName)); - argumentTypePathAcc.add(firstFieldType); - return parseTypeAndCommentFromArgumentName( + argumentFieldPathAcc.add(firstField); + return parseTypeFromArgumentName( remainingArgumentName, servicePackage, firstFieldMessage, messageTypes, resourceNames, patternsToResourceNames, - argumentTypePathAcc, + argumentFieldPathAcc, outputArgResourceNames); } diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java b/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java index 340716d3de..5c9813198d 100644 --- a/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java +++ b/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java @@ -402,6 +402,8 @@ private static Field parseField(FieldDescriptor fieldDescriptor, Descriptor mess return fieldBuilder .setName(fieldDescriptor.getName()) .setType(TypeParser.parseType(fieldDescriptor)) + .setIsMessage(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.MESSAGE) + .setIsEnum(fieldDescriptor.getJavaType() == FieldDescriptor.JavaType.ENUM) .setIsRepeated(fieldDescriptor.isRepeated()) .setIsMap(fieldDescriptor.isMapField()) .setResourceReference(resourceReference) diff --git a/src/test/java/com/google/api/generator/gapic/model/MethodArgumentTest.java b/src/test/java/com/google/api/generator/gapic/model/MethodArgumentTest.java index 810758267a..691700cb3e 100644 --- a/src/test/java/com/google/api/generator/gapic/model/MethodArgumentTest.java +++ b/src/test/java/com/google/api/generator/gapic/model/MethodArgumentTest.java @@ -25,7 +25,12 @@ public class MethodArgumentTest { @Test public void compareMethodArguments() { BiFunction methodArgFn = - (name, type) -> MethodArgument.builder().setName(name).setType(type).build(); + (name, type) -> + MethodArgument.builder() + .setName(name) + .setType(type) + .setField(Field.builder().setName("foobar").setType(type).build()) + .build(); // Cursory sanity-check of type-only differences, since these are already covered in the // TypeNode test. diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/MethodSignatureParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/MethodSignatureParserTest.java index b8ccb29d08..4f141f6125 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/MethodSignatureParserTest.java +++ b/src/test/java/com/google/api/generator/gapic/protoparser/MethodSignatureParserTest.java @@ -19,6 +19,7 @@ import com.google.api.generator.engine.ast.TypeNode; import com.google.api.generator.engine.ast.VaporReference; +import com.google.api.generator.gapic.model.Field; import com.google.api.generator.gapic.model.MethodArgument; import com.google.protobuf.Descriptors.FileDescriptor; import com.google.protobuf.Descriptors.ServiceDescriptor; @@ -58,7 +59,12 @@ public void flattenMethodSignatures_basic() { List argumentNames = Arrays.asList(fooName); BiFunction methodArgFn = - (name, type) -> MethodArgument.builder().setName(name).setType(type).build(); + (name, type) -> + MethodArgument.builder() + .setName(name) + .setType(type) + .setField(Field.builder().setName(name).setType(type).build()) + .build(); List fooArgs = Arrays.asList(TypeNode.STRING, fooTypeOne, fooTypeTwo).stream() .map(t -> methodArgFn.apply(fooName, t)) @@ -92,7 +98,12 @@ public void flattenMethodSignatures_oneToMany() { List argumentNames = Arrays.asList(anInt, fooName); BiFunction methodArgFn = - (name, type) -> MethodArgument.builder().setName(name).setType(type).build(); + (name, type) -> + MethodArgument.builder() + .setName(name) + .setType(type) + .setField(Field.builder().setName(name).setType(type).build()) + .build(); List fooArgs = Arrays.asList(TypeNode.STRING, fooTypeOne, fooTypeTwo).stream() .map(t -> methodArgFn.apply(fooName, t)) @@ -127,7 +138,12 @@ public void flattenMethodSignatures_manyToOne() { List argumentNames = Arrays.asList(fooName, anInt); BiFunction methodArgFn = - (name, type) -> MethodArgument.builder().setName(name).setType(type).build(); + (name, type) -> + MethodArgument.builder() + .setName(name) + .setType(type) + .setField(Field.builder().setName(name).setType(type).build()) + .build(); List fooArgs = Arrays.asList(TypeNode.STRING, fooTypeOne, fooTypeTwo).stream() .map(t -> methodArgFn.apply(fooName, t)) @@ -173,7 +189,12 @@ public void flattenMethodSignatures_manyToMany() { List argumentNames = Arrays.asList(fooName, anInt, barName, anotherInt); BiFunction methodArgFn = - (name, type) -> MethodArgument.builder().setName(name).setType(type).build(); + (name, type) -> + MethodArgument.builder() + .setName(name) + .setType(type) + .setField(Field.builder().setName(name).setType(type).build()) + .build(); List fooArgs = Arrays.asList(TypeNode.STRING, fooTypeOne, fooTypeTwo).stream() .map(t -> methodArgFn.apply(fooName, t)) 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 index 4557598770..e05c4203d6 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java +++ b/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java @@ -76,6 +76,7 @@ public void parseMessages_basic() { .setType( TypeNode.withReference( VaporReference.builder().setName("Severity").setPakkage(ECHO_PACKAGE).build())) + .setIsEnum(true) .build(); TypeNode echoResponseType = TypeNode.withReference( @@ -87,7 +88,8 @@ public void parseMessages_basic() { .setName(echoResponseName) .setFields(Arrays.asList(echoResponseContentField, echoResponseSeverityField)) .build(); - assertThat(messageTypes.get(echoResponseName)).isEqualTo(echoResponseMessage); + + assertEquals(echoResponseMessage, messageTypes.get(echoResponseName)); } @Test @@ -375,10 +377,10 @@ public void sanitizeDefaultHost_basic() { } private void assertMethodArgumentEquals( - String name, TypeNode type, List nestedTypes, MethodArgument argument) { + String name, TypeNode type, List nestedFields, MethodArgument argument) { assertEquals(name, argument.name()); assertEquals(type, argument.type()); - assertEquals(nestedTypes, argument.nestedTypes()); + assertEquals(nestedFields, argument.nestedFields()); } private static Reference createStatusReference() { From 64896254e772e609792b79d8557814d1a47f1765 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 23 Sep 2020 16:48:36 -0700 Subject: [PATCH 2/5] feat: partial isAssignableFrom VaporRef support, enable full-name type usage --- .../engine/ast/ConcreteReference.java | 19 +++++ .../api/generator/engine/ast/Reference.java | 2 + .../generator/engine/ast/VaporReference.java | 13 ++- .../engine/writer/ImportWriterVisitor.java | 7 +- .../engine/writer/JavaWriterVisitor.java | 4 + .../engine/ast/ConcreteReferenceTest.java | 85 +++++++++++++++++++ .../writer/ImportWriterVisitorTest.java | 30 +++++++ .../engine/writer/JavaWriterVisitorTest.java | 30 +++++++ 8 files changed, 188 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java b/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java index c95823fc13..7d37a02417 100644 --- a/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java +++ b/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java @@ -84,6 +84,9 @@ public String pakkage() { return clazz().getPackage().getName(); } + @Override + public abstract boolean useFullName(); + @Override public String enclosingClassName() { if (!hasEnclosingClass()) { @@ -129,9 +132,22 @@ public boolean isSupertypeOrEquals(Reference other) { @Override public boolean isAssignableFrom(Reference other) { + if (other instanceof VaporReference && ((VaporReference) other).supertypeReference() != null) { + return isAssignableFrom(((VaporReference) other).supertypeReference()); + } + if (!(other instanceof ConcreteReference)) { return false; } + + if (generics().size() == other.generics().size()) { + for (int i = 0; i < generics().size(); i++) { + if (!generics().get(i).isSupertypeOrEquals(other.generics().get(i))) { + return false; + } + } + } + return clazz().isAssignableFrom(((ConcreteReference) other).clazz()); } @@ -178,6 +194,7 @@ public static ConcreteReference wildcardWithUpperBound(Reference upperBoundRefer public static Builder builder() { return new AutoValue_ConcreteReference.Builder() + .setUseFullName(false) .setGenerics(ImmutableList.of()) .setIsStaticImport(false); } @@ -189,6 +206,8 @@ public static Builder builder() { public abstract static class Builder { public abstract Builder setClazz(Class clazz); + public abstract Builder setUseFullName(boolean useFullName); + public abstract Builder setWildcardUpperBound(Reference reference); public abstract Builder setGenerics(List clazzes); diff --git a/src/main/java/com/google/api/generator/engine/ast/Reference.java b/src/main/java/com/google/api/generator/engine/ast/Reference.java index 6fd856c78f..ed26731f0f 100644 --- a/src/main/java/com/google/api/generator/engine/ast/Reference.java +++ b/src/main/java/com/google/api/generator/engine/ast/Reference.java @@ -27,6 +27,8 @@ public interface Reference { String pakkage(); + boolean useFullName(); + @Nullable String enclosingClassName(); diff --git a/src/main/java/com/google/api/generator/engine/ast/VaporReference.java b/src/main/java/com/google/api/generator/engine/ast/VaporReference.java index 4ba17aa356..fbd75afc2e 100644 --- a/src/main/java/com/google/api/generator/engine/ast/VaporReference.java +++ b/src/main/java/com/google/api/generator/engine/ast/VaporReference.java @@ -37,10 +37,16 @@ public abstract class VaporReference implements Reference { @Override public abstract String pakkage(); + @Override + public abstract boolean useFullName(); + @Nullable @Override public abstract String enclosingClassName(); + @Nullable + public abstract Reference supertypeReference(); + @Nullable @Override public Reference wildcardUpperBound() { @@ -77,7 +83,7 @@ public boolean isSupertypeOrEquals(Reference other) { @Override public boolean isAssignableFrom(Reference other) { - // Not handling this for VaporReference. + // Not handling this yet for VaporReference. return false; } @@ -117,6 +123,7 @@ public Reference copyAndSetGenerics(List generics) { public static Builder builder() { return new AutoValue_VaporReference.Builder() + .setUseFullName(false) .setGenerics(ImmutableList.of()) .setIsStaticImport(false); } @@ -130,12 +137,16 @@ public abstract static class Builder { public abstract Builder setPakkage(String pakkage); + public abstract Builder setUseFullName(boolean useFullName); + public abstract Builder setGenerics(List clazzes); public abstract Builder setEnclosingClassName(String enclosingClassName); public abstract Builder setIsStaticImport(boolean isStaticImport); + public abstract Builder setSupertypeReference(Reference supertypeReference); + // Private. abstract Builder setPlainName(String plainName); diff --git a/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java b/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java index e59067ec0c..f95ded9df5 100644 --- a/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java +++ b/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java @@ -108,7 +108,9 @@ public void visit(TypeNode type) { } List refs = new ArrayList<>(type.reference().generics()); - refs.add(type.reference()); + if (!type.reference().useFullName()) { + refs.add(type.reference()); + } references(refs); } @@ -377,6 +379,9 @@ private void variableExpressions(List expressions) { private void references(List refs) { for (Reference ref : refs) { // Don't need to import this. + if (ref.useFullName()) { + continue; + } if (!ref.isStaticImport() && (ref.isFromPackage(PKG_JAVA_LANG) || ref.isFromPackage(currentPackage))) { continue; diff --git a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java index b770de98bc..6d7ef45ee5 100644 --- a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java +++ b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java @@ -142,6 +142,10 @@ public void visit(TypeNode type) { if (type.isPrimitiveType()) { generatedCodeBuilder.append(typeKind.toString().toLowerCase()); } else { + if (type.reference().useFullName()) { + generatedCodeBuilder.append(type.reference().pakkage()); + generatedCodeBuilder.append(DOT); + } // A null pointer exception will be thrown if reference is null, which is WAI. generatedCodeBuilder.append(type.reference().name()); } diff --git a/src/test/java/com/google/api/generator/engine/ast/ConcreteReferenceTest.java b/src/test/java/com/google/api/generator/engine/ast/ConcreteReferenceTest.java index b284b1bb3a..7083cc280b 100644 --- a/src/test/java/com/google/api/generator/engine/ast/ConcreteReferenceTest.java +++ b/src/test/java/com/google/api/generator/engine/ast/ConcreteReferenceTest.java @@ -148,4 +148,89 @@ public void wildcards() { "? extends String", ConcreteReference.wildcardWithUpperBound(TypeNode.STRING.reference()).name()); } + + @Test + public void isAssignableFrom_concreteRef() { + assertFalse( + ConcreteReference.withClazz(List.class) + .isAssignableFrom(ConcreteReference.withClazz(Map.class))); + + assertTrue( + ConcreteReference.withClazz(List.class) + .isAssignableFrom(ConcreteReference.withClazz(ArrayList.class))); + assertFalse( + ConcreteReference.withClazz(ArrayList.class) + .isAssignableFrom(ConcreteReference.withClazz(List.class))); + assertTrue( + ConcreteReference.withClazz(ArrayList.class) + .isAssignableFrom(ConcreteReference.withClazz(ArrayList.class))); + + assertTrue( + ConcreteReference.withClazz(List.class) + .isAssignableFrom( + ConcreteReference.builder() + .setClazz(ArrayList.class) + .setGenerics(Arrays.asList(ConcreteReference.withClazz(String.class))) + .build())); + assertTrue( + ConcreteReference.builder() + .setClazz(List.class) + .setGenerics(Arrays.asList(ConcreteReference.withClazz(String.class))) + .build() + .isAssignableFrom( + ConcreteReference.builder() + .setClazz(ArrayList.class) + .setGenerics(Arrays.asList(ConcreteReference.withClazz(String.class))) + .build())); + assertFalse( + ConcreteReference.builder() + .setClazz(List.class) + .setGenerics(Arrays.asList(ConcreteReference.withClazz(Integer.class))) + .build() + .isAssignableFrom( + ConcreteReference.builder() + .setClazz(ArrayList.class) + .setGenerics(Arrays.asList(ConcreteReference.withClazz(String.class))) + .build())); + } + + @Test + public void isAssignableFrom_vaporRef() { + assertFalse( + ConcreteReference.withClazz(List.class) + .isAssignableFrom( + VaporReference.builder().setName("ArrayList").setPakkage("java.util").build())); + assertFalse( + ConcreteReference.withClazz(ArrayList.class) + .isAssignableFrom( + VaporReference.builder().setName("ArrayList").setPakkage("java.util").build())); + } + + @Test + public void isAssignableFrom_vaporRefWithConcreteRefSupertype() { + assertTrue( + ConcreteReference.withClazz(List.class) + .isAssignableFrom( + VaporReference.builder() + .setName("ArrayList") + .setPakkage("java.util") + .setSupertypeReference(ConcreteReference.withClazz(List.class)) + .build())); + assertTrue( + ConcreteReference.withClazz(List.class) + .isAssignableFrom( + VaporReference.builder() + .setName("SpecialArrayList") + .setPakkage("com.foo.bar") + .setSupertypeReference(ConcreteReference.withClazz(ArrayList.class)) + .build())); + assertFalse( + ConcreteReference.withClazz(List.class) + .isAssignableFrom( + VaporReference.builder() + .setName("HashMap") + .setPakkage("java.util") + .setSupertypeReference(ConcreteReference.withClazz(Map.class)) + .build())); + } } diff --git a/src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java b/src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java index 352b52379e..5a936b61b3 100644 --- a/src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java +++ b/src/test/java/com/google/api/generator/engine/writer/ImportWriterVisitorTest.java @@ -76,6 +76,36 @@ public void setUp() { writerVisitor.initialize(CURRENT_PACKAGE, CURRENT_CLASS); } + @Test + public void writeReferenceTypeImports_basic() { + TypeNode.withReference(ConcreteReference.withClazz(List.class)).accept(writerVisitor); + assertEquals("import java.util.List;\n\n", writerVisitor.write()); + + writerVisitor.clear(); + TypeNode.withReference( + VaporReference.builder().setName("FooBar").setPakkage("com.foo.bar").build()) + .accept(writerVisitor); + assertEquals("import com.foo.bar.FooBar;\n\n", writerVisitor.write()); + } + + @Test + public void writeReferenceTypeImports_useFullName() { + TypeNode.withReference( + ConcreteReference.builder().setClazz(List.class).setUseFullName(true).build()) + .accept(writerVisitor); + assertEquals("", writerVisitor.write()); + + writerVisitor.clear(); + TypeNode.withReference( + VaporReference.builder() + .setName("FooBar") + .setPakkage("com.foo.bar") + .setUseFullName(true) + .build()) + .accept(writerVisitor); + assertEquals("", writerVisitor.write()); + } + @Test public void writeNewObjectExprImports_basic() { // [Constructing] `new ArrayList<>()` diff --git a/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java b/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java index 30632ea40c..abbf2f52ec 100644 --- a/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java +++ b/src/test/java/com/google/api/generator/engine/writer/JavaWriterVisitorTest.java @@ -106,6 +106,36 @@ public void writePrimitiveArrayType() { assertEquals(writerVisitor.write(), "byte[]"); } + @Test + public void writeReferenceType_basic() { + TypeNode.withReference(ConcreteReference.withClazz(List.class)).accept(writerVisitor); + assertEquals("List", writerVisitor.write()); + + writerVisitor.clear(); + TypeNode.withReference( + VaporReference.builder().setName("FooBar").setPakkage("com.foo.bar").build()) + .accept(writerVisitor); + assertEquals("FooBar", writerVisitor.write()); + } + + @Test + public void writeReferenceType_useFullName() { + TypeNode.withReference( + ConcreteReference.builder().setClazz(List.class).setUseFullName(true).build()) + .accept(writerVisitor); + assertEquals("java.util.List", writerVisitor.write()); + + writerVisitor.clear(); + TypeNode.withReference( + VaporReference.builder() + .setName("FooBar") + .setPakkage("com.foo.bar") + .setUseFullName(true) + .build()) + .accept(writerVisitor); + assertEquals("com.foo.bar.FooBar", writerVisitor.write()); + } + @Test public void writeAnnotation_simple() { AnnotationNode annotation = AnnotationNode.OVERRIDE; From fb565b616576dbb32af462c31738ecab25ea6a7b Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 23 Sep 2020 23:08:04 -0700 Subject: [PATCH 3/5] feat: support negative numeric literals --- .../google/api/generator/engine/lexicon/Literal.java | 11 ++++++----- .../api/generator/engine/lexicon/LiteralTest.java | 6 ++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/api/generator/engine/lexicon/Literal.java b/src/main/java/com/google/api/generator/engine/lexicon/Literal.java index fb97f9fd0f..8ffd5ea989 100644 --- a/src/main/java/com/google/api/generator/engine/lexicon/Literal.java +++ b/src/main/java/com/google/api/generator/engine/lexicon/Literal.java @@ -22,13 +22,14 @@ public class Literal { private static final String BOOLEAN_FALSE = "false"; private static final String NULL_VALUE = "null"; - private static final Pattern INTEGER_PATTERN = Pattern.compile("^[0-9]+$"); - private static final Pattern LONG_PATTERN = Pattern.compile("^[0-9]+[Ll]?$"); + private static final Pattern INTEGER_PATTERN = Pattern.compile("^\\-?[0-9]+$"); + private static final Pattern LONG_PATTERN = Pattern.compile("^\\-?[0-9]+[Ll]?$"); private static final Pattern FLOAT_PATTERN = - Pattern.compile("^[0-9]+([fF]|(\\.(([0-9]+[fF])|[fF])))?$"); + Pattern.compile("^\\-?[0-9]+([fF]|(\\.(([0-9]+[fF])|[fF])))?$"); private static final Pattern DOUBLE_PATTERN_ONE = - Pattern.compile("^[0-9]+(\\.[0-9]+)?(\\.?[eE]\\-?[0-9]+)$"); - private static final Pattern DOUBLE_PATTERN_TWO = Pattern.compile("^\\d*\\.\\d+$|^\\d+\\.\\d*$"); + Pattern.compile("^\\-?[0-9]+(\\.[0-9]+)?(\\.?[eE]\\-?[0-9]+)$"); + private static final Pattern DOUBLE_PATTERN_TWO = + Pattern.compile("^\\-?\\d*\\.\\d+$|^\\d+\\.\\d*$"); public static boolean isBooleanLiteral(String str) { return str.equals(BOOLEAN_TRUE) || str.equals(BOOLEAN_FALSE); diff --git a/src/test/java/com/google/api/generator/engine/lexicon/LiteralTest.java b/src/test/java/com/google/api/generator/engine/lexicon/LiteralTest.java index ac22cbde6e..80b155045e 100644 --- a/src/test/java/com/google/api/generator/engine/lexicon/LiteralTest.java +++ b/src/test/java/com/google/api/generator/engine/lexicon/LiteralTest.java @@ -37,6 +37,7 @@ public void nullLDetected() { public void integerDetected() { assertThat(Literal.isIntegerLiteral("a123")).isFalse(); assertThat(Literal.isIntegerLiteral("123")).isTrue(); + assertThat(Literal.isIntegerLiteral("-123")).isTrue(); assertThat(Literal.isIntegerLiteral("123L")).isFalse(); assertThat(Literal.isIntegerLiteral("123r")).isFalse(); assertThat(Literal.isIntegerLiteral("123e2")).isFalse(); @@ -47,6 +48,7 @@ public void longDetected() { assertThat(Literal.isLongLiteral("123")).isTrue(); assertThat(Literal.isLongLiteral("123L")).isTrue(); assertThat(Literal.isLongLiteral("123l")).isTrue(); + assertThat(Literal.isLongLiteral("-123l")).isTrue(); assertThat(Literal.isLongLiteral("123e")).isFalse(); } @@ -58,6 +60,7 @@ public void floatDetected() { assertThat(Literal.isFloatLiteral("0.01")).isFalse(); assertThat(Literal.isFloatLiteral(".01")).isFalse(); assertThat(Literal.isFloatLiteral("123.f")).isTrue(); + assertThat(Literal.isFloatLiteral("-123.f")).isTrue(); assertThat(Literal.isFloatLiteral("123.F")).isTrue(); assertThat(Literal.isFloatLiteral("123.234F")).isTrue(); assertThat(Literal.isFloatLiteral("123.234Fe-3")).isFalse(); @@ -69,11 +72,14 @@ public void doubleDetected() { assertThat(Literal.isDoubleLiteral("123")).isTrue(); assertThat(Literal.isDoubleLiteral("0.01")).isTrue(); assertThat(Literal.isDoubleLiteral(".01")).isTrue(); + assertThat(Literal.isDoubleLiteral("-.01")).isTrue(); assertThat(Literal.isDoubleLiteral("123.0")).isTrue(); + assertThat(Literal.isDoubleLiteral("-123.0")).isTrue(); assertThat(Literal.isDoubleLiteral("123f")).isTrue(); assertThat(Literal.isDoubleLiteral("123E-2")).isTrue(); assertThat(Literal.isDoubleLiteral("123.134E-2")).isTrue(); assertThat(Literal.isDoubleLiteral("123.E-2")).isTrue(); + assertThat(Literal.isDoubleLiteral("-123.E-2")).isTrue(); assertThat(Literal.isDoubleLiteral("123e2")).isTrue(); assertThat(Literal.isDoubleLiteral("123e")).isFalse(); assertThat(Literal.isDoubleLiteral("123E-")).isFalse(); From a4381286d182df2921951740a3f8575852e0f16a Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 23 Sep 2020 23:15:22 -0700 Subject: [PATCH 4/5] fix: separate resname tokenizing from class composer --- .../ResourceNameHelperClassComposer.java | 41 +------ .../gapic/composer/ResourceNameTokenizer.java | 61 ++++++++++ .../ResourceNameHelperClassComposerTest.java | 84 +------------ .../composer/ResourceNameTokenizerTest.java | 114 ++++++++++++++++++ 4 files changed, 178 insertions(+), 122 deletions(-) create mode 100644 src/main/java/com/google/api/generator/gapic/composer/ResourceNameTokenizer.java create mode 100644 src/test/java/com/google/api/generator/gapic/composer/ResourceNameTokenizerTest.java diff --git a/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java index 652c8488d6..f686469607 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java @@ -69,10 +69,6 @@ public class ResourceNameHelperClassComposer { private static final String CLASS_NAME_PATTERN = "%sName"; - private static final String SLASH = "/"; - private static final String LEFT_BRACE = "{"; - private static final String RIGHT_BRACE = "}"; - private static final String BUILDER_CLASS_HEADER_PATTERN = "Builder for %s."; private static final ResourceNameHelperClassComposer INSTANCE = @@ -89,7 +85,8 @@ public static ResourceNameHelperClassComposer instance() { } public GapicClass generate(ResourceName resourceName) { - List> tokenHierarchies = parseTokenHierarchy(resourceName.patterns()); + List> tokenHierarchies = + ResourceNameTokenizer.parseTokenHierarchy(resourceName.patterns()); Map types = createDynamicTypes(resourceName, tokenHierarchies); List templateFinalVarExprs = createTemplateClassMembers(tokenHierarchies); Map patternTokenVarExprs = @@ -1448,40 +1445,6 @@ private static TypeNode getBuilderType( : types.get(getBuilderTypeName(tokenHierarchies, index)); } - @VisibleForTesting - static List> parseTokenHierarchy(List patterns) { - List nonSlashSepStrings = Arrays.asList("}_{", "}-{", "}.{", "}~{"); - - List> tokenHierachies = new ArrayList<>(); - for (String pattern : patterns) { - List hierarchy = new ArrayList<>(); - Set vars = PathTemplate.create(pattern).vars(); - String[] patternTokens = pattern.split(SLASH); - for (String patternToken : patternTokens) { - if (patternToken.startsWith(LEFT_BRACE) && patternToken.endsWith(RIGHT_BRACE)) { - String processedPatternToken = patternToken; - - // Handle non-slash separators. - if (nonSlashSepStrings.stream().anyMatch(s -> patternToken.contains(s))) { - for (String str : nonSlashSepStrings) { - processedPatternToken = processedPatternToken.replace(str, "_"); - } - } else { - // Handles wildcards. - processedPatternToken = - vars.stream() - .filter(v -> patternToken.contains(v)) - .collect(Collectors.toList()) - .get(0); - } - hierarchy.add(processedPatternToken.replace("{", "").replace("}", "")); - } - } - tokenHierachies.add(hierarchy); - } - return tokenHierachies; - } - @VisibleForTesting static Set getTokenSet(List> tokenHierarchy) { return tokenHierarchy.stream() diff --git a/src/main/java/com/google/api/generator/gapic/composer/ResourceNameTokenizer.java b/src/main/java/com/google/api/generator/gapic/composer/ResourceNameTokenizer.java new file mode 100644 index 0000000000..671b5b6bcf --- /dev/null +++ b/src/main/java/com/google/api/generator/gapic/composer/ResourceNameTokenizer.java @@ -0,0 +1,61 @@ +// 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.composer; + +import com.google.api.pathtemplate.PathTemplate; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +public class ResourceNameTokenizer { + private static final String SLASH = "/"; + private static final String LEFT_BRACE = "{"; + private static final String RIGHT_BRACE = "}"; + + static List> parseTokenHierarchy(List patterns) { + List nonSlashSepStrings = Arrays.asList("}_{", "}-{", "}.{", "}~{"); + + List> tokenHierachies = new ArrayList<>(); + for (String pattern : patterns) { + List hierarchy = new ArrayList<>(); + Set vars = PathTemplate.create(pattern).vars(); + String[] patternTokens = pattern.split(SLASH); + for (String patternToken : patternTokens) { + if (patternToken.startsWith(LEFT_BRACE) && patternToken.endsWith(RIGHT_BRACE)) { + String processedPatternToken = patternToken; + + // Handle non-slash separators. + if (nonSlashSepStrings.stream().anyMatch(s -> patternToken.contains(s))) { + for (String str : nonSlashSepStrings) { + processedPatternToken = processedPatternToken.replace(str, "_"); + } + } else { + // Handles wildcards. + processedPatternToken = + vars.stream() + .filter(v -> patternToken.contains(v)) + .collect(Collectors.toList()) + .get(0); + } + hierarchy.add(processedPatternToken.replace("{", "").replace("}", "")); + } + } + tokenHierachies.add(hierarchy); + } + return tokenHierachies; + } +} diff --git a/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java index 75911fcffb..48d00a10cf 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java @@ -48,95 +48,13 @@ public void setUp() { assertEquals(echoService.getName(), "Echo"); } - @Test - public void parseTokenHierarchy_basic() { - List patterns = - Arrays.asList( - "projects/{project}/locations/{location}/autoscalingPolicies/{autoscaling_policy}", - "projects/{project}/regions/{region}/autoscalingPolicies/{autoscaling_policy}", - "projects/{project}/autoscalingPolicies/{autoscaling_policy}"); - List> tokenHierarchies = - ResourceNameHelperClassComposer.parseTokenHierarchy(patterns); - assertEquals(3, tokenHierarchies.size()); - assertThat(tokenHierarchies.get(0)) - .containsExactly("project", "location", "autoscaling_policy"); - assertThat(tokenHierarchies.get(1)).containsExactly("project", "region", "autoscaling_policy"); - assertThat(tokenHierarchies.get(2)).containsExactly("project", "autoscaling_policy"); - } - - @Test - public void parseTokenHierarchy_wildcards() { - List patterns = - Arrays.asList( - "projects/{project}/metricDescriptors/{metric_descriptor=**}", - "organizations/{organization}/metricDescriptors/{metric_descriptor=**}", - "folders/{folder=**}/metricDescriptors/{metric_descriptor}"); - List> tokenHierarchies = - ResourceNameHelperClassComposer.parseTokenHierarchy(patterns); - assertEquals(3, tokenHierarchies.size()); - assertThat(tokenHierarchies.get(0)).containsExactly("project", "metric_descriptor"); - assertThat(tokenHierarchies.get(1)).containsExactly("organization", "metric_descriptor"); - assertThat(tokenHierarchies.get(2)).containsExactly("folder", "metric_descriptor"); - } - - @Test - public void parseTokenHierarchy_singletonCollection() { - List patterns = - Arrays.asList( - "projects/{project}/agent/sessions/{session}", - "projects/{project}/agent/environments/{environment}/users/{user}/sessions/{session}"); - List> tokenHierarchies = - ResourceNameHelperClassComposer.parseTokenHierarchy(patterns); - assertEquals(2, tokenHierarchies.size()); - assertThat(tokenHierarchies.get(0)).containsExactly("project", "session"); - assertThat(tokenHierarchies.get(1)) - .containsExactly("project", "environment", "user", "session"); - } - - @Test - public void parseTokenHierarchy_singletonCollectionAndNonSlashSeparators() { - List patterns = - Arrays.asList( - "users/{user}/profile/blurbs/legacy/{legacy_user}~{blurb}", - "users/{user}/profile/blurbs/{blurb}", - "rooms/{room}/blurbs/{blurb}", - "users/{user}/profile/blurbs/legacy/{legacy_document}_{blurb}", - "users/{user}/profile/blurbs/legacy/{legacy_book}-{blurb}", - "rooms/{room}/blurbs/legacy/{legacy_room}.{blurb}"); - - List> tokenHierarchies = - ResourceNameHelperClassComposer.parseTokenHierarchy(patterns); - assertEquals(6, tokenHierarchies.size()); - assertThat(tokenHierarchies.get(0)).containsExactly("user", "legacy_user_blurb"); - assertThat(tokenHierarchies.get(1)).containsExactly("user", "blurb"); - assertThat(tokenHierarchies.get(2)).containsExactly("room", "blurb"); - assertThat(tokenHierarchies.get(3)).containsExactly("user", "legacy_document_blurb"); - assertThat(tokenHierarchies.get(4)).containsExactly("user", "legacy_book_blurb"); - assertThat(tokenHierarchies.get(5)).containsExactly("room", "legacy_room_blurb"); - } - - @Test - public void parseTokenHierarchy_invalidPatterns() { - List patterns = - Arrays.asList( - "projects/{project}/agent/sessions/{session}/anotherSingleton", - "{project}/agent/environments/{environment}/users/{user}/sessions/{session}"); - List> tokenHierarchies = - ResourceNameHelperClassComposer.parseTokenHierarchy(patterns); - assertEquals(2, tokenHierarchies.size()); - assertThat(tokenHierarchies.get(0)).containsExactly("project", "session"); - assertThat(tokenHierarchies.get(1)) - .containsExactly("project", "environment", "user", "session"); - } - @Test public void getTokenSet_basic() { List patterns = Arrays.asList( "projects/{project}/agent/sessions/{session}", "projects/{project}/agent/environments/{environment}/users/{user}/sessions/{session}"); - List> tokenHierarchies = - ResourceNameHelperClassComposer.parseTokenHierarchy(patterns); + List> tokenHierarchies = ResourceNameTokenizer.parseTokenHierarchy(patterns); Set tokenSet = ResourceNameHelperClassComposer.getTokenSet(tokenHierarchies); assertEquals( diff --git a/src/test/java/com/google/api/generator/gapic/composer/ResourceNameTokenizerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ResourceNameTokenizerTest.java new file mode 100644 index 0000000000..aed8624d92 --- /dev/null +++ b/src/test/java/com/google/api/generator/gapic/composer/ResourceNameTokenizerTest.java @@ -0,0 +1,114 @@ +// 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.composer; + +import static com.google.common.truth.Truth.assertThat; +import static junit.framework.Assert.assertEquals; + +import com.google.protobuf.Descriptors.FileDescriptor; +import com.google.protobuf.Descriptors.ServiceDescriptor; +import com.google.showcase.v1beta1.EchoOuterClass; +import java.util.Arrays; +import java.util.List; +import org.junit.Before; +import org.junit.Test; + +public class ResourceNameTokenizerTest { + private ServiceDescriptor echoService; + private FileDescriptor echoFileDescriptor; + + @Before + public void setUp() { + echoFileDescriptor = EchoOuterClass.getDescriptor(); + echoService = echoFileDescriptor.getServices().get(0); + assertEquals(echoService.getName(), "Echo"); + } + + @Test + public void parseTokenHierarchy_basic() { + List patterns = + Arrays.asList( + "projects/{project}/locations/{location}/autoscalingPolicies/{autoscaling_policy}", + "projects/{project}/regions/{region}/autoscalingPolicies/{autoscaling_policy}", + "projects/{project}/autoscalingPolicies/{autoscaling_policy}"); + List> tokenHierarchies = ResourceNameTokenizer.parseTokenHierarchy(patterns); + assertEquals(3, tokenHierarchies.size()); + assertThat(tokenHierarchies.get(0)) + .containsExactly("project", "location", "autoscaling_policy"); + assertThat(tokenHierarchies.get(1)).containsExactly("project", "region", "autoscaling_policy"); + assertThat(tokenHierarchies.get(2)).containsExactly("project", "autoscaling_policy"); + } + + @Test + public void parseTokenHierarchy_wildcards() { + List patterns = + Arrays.asList( + "projects/{project}/metricDescriptors/{metric_descriptor=**}", + "organizations/{organization}/metricDescriptors/{metric_descriptor=**}", + "folders/{folder=**}/metricDescriptors/{metric_descriptor}"); + List> tokenHierarchies = ResourceNameTokenizer.parseTokenHierarchy(patterns); + assertEquals(3, tokenHierarchies.size()); + assertThat(tokenHierarchies.get(0)).containsExactly("project", "metric_descriptor"); + assertThat(tokenHierarchies.get(1)).containsExactly("organization", "metric_descriptor"); + assertThat(tokenHierarchies.get(2)).containsExactly("folder", "metric_descriptor"); + } + + @Test + public void parseTokenHierarchy_singletonCollection() { + List patterns = + Arrays.asList( + "projects/{project}/agent/sessions/{session}", + "projects/{project}/agent/environments/{environment}/users/{user}/sessions/{session}"); + List> tokenHierarchies = ResourceNameTokenizer.parseTokenHierarchy(patterns); + assertEquals(2, tokenHierarchies.size()); + assertThat(tokenHierarchies.get(0)).containsExactly("project", "session"); + assertThat(tokenHierarchies.get(1)) + .containsExactly("project", "environment", "user", "session"); + } + + @Test + public void parseTokenHierarchy_singletonCollectionAndNonSlashSeparators() { + List patterns = + Arrays.asList( + "users/{user}/profile/blurbs/legacy/{legacy_user}~{blurb}", + "users/{user}/profile/blurbs/{blurb}", + "rooms/{room}/blurbs/{blurb}", + "users/{user}/profile/blurbs/legacy/{legacy_document}_{blurb}", + "users/{user}/profile/blurbs/legacy/{legacy_book}-{blurb}", + "rooms/{room}/blurbs/legacy/{legacy_room}.{blurb}"); + + List> tokenHierarchies = ResourceNameTokenizer.parseTokenHierarchy(patterns); + assertEquals(6, tokenHierarchies.size()); + assertThat(tokenHierarchies.get(0)).containsExactly("user", "legacy_user_blurb"); + assertThat(tokenHierarchies.get(1)).containsExactly("user", "blurb"); + assertThat(tokenHierarchies.get(2)).containsExactly("room", "blurb"); + assertThat(tokenHierarchies.get(3)).containsExactly("user", "legacy_document_blurb"); + assertThat(tokenHierarchies.get(4)).containsExactly("user", "legacy_book_blurb"); + assertThat(tokenHierarchies.get(5)).containsExactly("room", "legacy_room_blurb"); + } + + @Test + public void parseTokenHierarchy_invalidPatterns() { + List patterns = + Arrays.asList( + "projects/{project}/agent/sessions/{session}/anotherSingleton", + "{project}/agent/environments/{environment}/users/{user}/sessions/{session}"); + List> tokenHierarchies = ResourceNameTokenizer.parseTokenHierarchy(patterns); + assertEquals(2, tokenHierarchies.size()); + assertThat(tokenHierarchies.get(0)).containsExactly("project", "session"); + assertThat(tokenHierarchies.get(1)) + .containsExactly("project", "environment", "user", "session"); + } +} From 396a0cc9dbd00a63ee38d0644a8c828f8a16e1a2 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 23 Sep 2020 23:16:20 -0700 Subject: [PATCH 5/5] feat: add per-type default value composer --- .../gapic/composer/DefaultValueComposer.java | 189 ++++++++++++++++ .../composer/DefaultValueComposerTest.java | 211 ++++++++++++++++++ 2 files changed, 400 insertions(+) create mode 100644 src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java create mode 100644 src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java diff --git a/src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java b/src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java new file mode 100644 index 0000000000..f53123c4c0 --- /dev/null +++ b/src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java @@ -0,0 +1,189 @@ +// 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.composer; + +import com.google.api.generator.engine.ast.ConcreteReference; +import com.google.api.generator.engine.ast.Expr; +import com.google.api.generator.engine.ast.MethodInvocationExpr; +import com.google.api.generator.engine.ast.NewObjectExpr; +import com.google.api.generator.engine.ast.PrimitiveValue; +import com.google.api.generator.engine.ast.StringObjectValue; +import com.google.api.generator.engine.ast.TypeNode; +import com.google.api.generator.engine.ast.ValueExpr; +import com.google.api.generator.gapic.model.Field; +import com.google.api.generator.gapic.model.MethodArgument; +import com.google.api.generator.gapic.model.ResourceName; +import com.google.api.generator.gapic.utils.JavaStyle; +import com.google.api.generator.gapic.utils.ResourceNameConstants; +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +public class DefaultValueComposer { + static Expr createDefaultValue( + MethodArgument methodArg, Map resourceNames) { + if (methodArg.isResourceNameHelper()) { + Preconditions.checkState( + methodArg.field().hasResourceReference(), + String.format( + "No corresponding resource reference for argument %s found on field %s %s", + methodArg.name(), methodArg.field().type(), methodArg.field().name())); + ResourceName resourceName = + resourceNames.get(methodArg.field().resourceReference().resourceTypeString()); + Preconditions.checkNotNull( + resourceName, + String.format( + "No resource name found for reference %s", + methodArg.field().resourceReference().resourceTypeString())); + return createDefaultValue( + resourceName, resourceNames.values().stream().collect(Collectors.toList())); + } + + if (methodArg.type().equals(methodArg.field().type())) { + return createDefaultValue(methodArg.field()); + } + + return createDefaultValue( + Field.builder().setName(methodArg.name()).setType(methodArg.type()).build()); + } + + static Expr createDefaultValue(Field f) { + if (f.isRepeated()) { + TypeNode newType = + TypeNode.withReference( + ConcreteReference.withClazz(f.isMap() ? HashMap.class : ArrayList.class)); + return NewObjectExpr.builder().setType(newType).setIsGeneric(true).build(); + } + + if (f.isEnum()) { + return MethodInvocationExpr.builder() + .setStaticReferenceType(f.type()) + .setMethodName("forNumber") + .setArguments( + ValueExpr.withValue( + PrimitiveValue.builder().setType(TypeNode.INT).setValue("0").build())) + .setReturnType(f.type()) + .build(); + } + + if (f.isMessage()) { + MethodInvocationExpr newBuilderExpr = + MethodInvocationExpr.builder() + .setStaticReferenceType(f.type()) + .setMethodName("newBuilder") + .build(); + return MethodInvocationExpr.builder() + .setExprReferenceExpr(newBuilderExpr) + .setMethodName("build") + .setReturnType(f.type()) + .build(); + } + + if (f.type().equals(TypeNode.STRING)) { + return ValueExpr.withValue( + StringObjectValue.withValue(String.format("%s%s", f.name(), f.name().hashCode()))); + } + + if (TypeNode.isNumericType(f.type())) { + return ValueExpr.withValue( + PrimitiveValue.builder() + .setType(f.type()) + .setValue(String.format("%s", f.name().hashCode())) + .build()); + } + + if (f.type().equals(TypeNode.BOOLEAN)) { + return ValueExpr.withValue( + PrimitiveValue.builder().setType(f.type()).setValue("true").build()); + } + + throw new UnsupportedOperationException( + String.format( + "Default value for field %s with type %s not implemented yet.", f.name(), f.type())); + } + + static Expr createDefaultValue(ResourceName resourceName, List resnames) { + boolean hasOnePattern = resourceName.patterns().size() == 1; + if (resourceName.isOnlyWildcard()) { + List unexaminedResnames = new ArrayList<>(resnames); + for (ResourceName resname : resnames) { + if (resname.isOnlyWildcard()) { + unexaminedResnames.remove(resname); + continue; + } + unexaminedResnames.remove(resname); + return createDefaultValue(resname, unexaminedResnames); + } + // Should not get here. + Preconditions.checkState( + !unexaminedResnames.isEmpty(), + String.format( + "No default resource name found for wildcard %s", resourceName.resourceTypeString())); + } + + // The cost tradeoffs of new ctors versus distinct() don't really matter here, since this list + // will usually have a very small number of elements. + List patterns = new ArrayList<>(new HashSet<>(resourceName.patterns())); + boolean containsOnlyDeletedTopic = + patterns.size() == 1 && patterns.get(0).equals(ResourceNameConstants.DELETED_TOPIC_LITERAL); + String ofMethodName = "of"; + List patternPlaceholderTokens = new ArrayList<>(); + + if (containsOnlyDeletedTopic) { + ofMethodName = "ofDeletedTopic"; + } else { + for (String pattern : resourceName.patterns()) { + if (pattern.equals(ResourceNameConstants.WILDCARD_PATTERN) + || pattern.equals(ResourceNameConstants.DELETED_TOPIC_LITERAL)) { + continue; + } + patternPlaceholderTokens.addAll( + ResourceNameTokenizer.parseTokenHierarchy(Arrays.asList(pattern)).get(0)); + break; + } + } + + if (!hasOnePattern) { + ofMethodName = + String.format( + "of%sName", + String.join( + "", + patternPlaceholderTokens.stream() + .map(s -> JavaStyle.toUpperCamelCase(s)) + .collect(Collectors.toList()))); + } + + TypeNode resourceNameJavaType = resourceName.type(); + List argExprs = + patternPlaceholderTokens.stream() + .map( + s -> + ValueExpr.withValue( + StringObjectValue.withValue(String.format("[%s]", s.toUpperCase())))) + .collect(Collectors.toList()); + return MethodInvocationExpr.builder() + .setStaticReferenceType(resourceNameJavaType) + .setMethodName(ofMethodName) + .setArguments(argExprs) + .setReturnType(resourceNameJavaType) + .build(); + } +} diff --git a/src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java new file mode 100644 index 0000000000..bb0083f013 --- /dev/null +++ b/src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java @@ -0,0 +1,211 @@ +// 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.composer; + +import static junit.framework.Assert.assertEquals; +import static org.junit.Assert.assertThrows; + +import com.google.api.generator.engine.ast.Expr; +import com.google.api.generator.engine.ast.TypeNode; +import com.google.api.generator.engine.writer.JavaWriterVisitor; +import com.google.api.generator.gapic.model.Field; +import com.google.api.generator.gapic.model.ResourceName; +import com.google.api.generator.gapic.protoparser.Parser; +import com.google.protobuf.Descriptors.FileDescriptor; +import com.google.testgapic.v1beta1.LockerProto; +import java.util.Collections; +import java.util.Map; +import java.util.stream.Collectors; +import org.junit.Before; +import org.junit.Test; + +public class DefaultValueComposerTest { + private JavaWriterVisitor writerVisitor; + + @Before + public void setUp() { + writerVisitor = new JavaWriterVisitor(); + } + + @Test + public void defaultValue_mapField() { + // Incorrect and will never happen in real usage, but proves that map detection is based on + // isMap rather than type(). + Field field = + Field.builder() + .setName("foobar") + .setType(TypeNode.STRING) + .setIsMap(true) + .setIsRepeated(true) + .build(); + Expr expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("new HashMap<>()", writerVisitor.write()); + + writerVisitor.clear(); + + // isMap() and isRepeated() will be set by protoc simulataneoulsy, but we check this edge case + // for completeness. + field = Field.builder().setName("foobar").setType(TypeNode.STRING).setIsMap(true).build(); + expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("\"foobar-1268878963\"", writerVisitor.write()); + } + + @Test + public void defaultValue_listField() { + // Incorrect and will never happen in real usage, but proves that list detection is based on + // isRepeated rather than type(). + Field field = + Field.builder().setName("foobar").setType(TypeNode.STRING).setIsRepeated(true).build(); + Expr expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("new ArrayList<>()", writerVisitor.write()); + } + + @Test + public void defaultValue_enumField() { + // Incorrect and will never happen in real usage, but proves that enum detection is based on + // isEnum() rather than type(). + Field field = + Field.builder().setName("foobar").setType(TypeNode.STRING).setIsEnum(true).build(); + Expr expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("String.forNumber(0)", writerVisitor.write()); + } + + @Test + public void defaultValue_messageField() { + // Incorrect and will never happen in real usage, but proves that message detection is based on + // isMessage() rather than type(). + Field field = + Field.builder().setName("foobar").setType(TypeNode.STRING).setIsMessage(true).build(); + Expr expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("String.newBuilder().build()", writerVisitor.write()); + } + + @Test + public void defaultValue_stringField() { + Field field = Field.builder().setName("foobar").setType(TypeNode.STRING).build(); + Expr expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("\"foobar-1268878963\"", writerVisitor.write()); + } + + @Test + public void defaultValue_numericField() { + Field field = Field.builder().setName("foobar").setType(TypeNode.INT).build(); + Expr expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("-1268878963", writerVisitor.write()); + + writerVisitor.clear(); + field = Field.builder().setName("foobar").setType(TypeNode.DOUBLE).build(); + expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("-1268878963", writerVisitor.write()); + } + + @Test + public void defaultValue_booleanField() { + Field field = Field.builder().setName("foobar").setType(TypeNode.BOOLEAN).build(); + Expr expr = DefaultValueComposer.createDefaultValue(field); + expr.accept(writerVisitor); + assertEquals("true", writerVisitor.write()); + } + + @Test + public void defaultValue_resourceNameWithOnePattern() { + FileDescriptor lockerServiceFileDescriptor = LockerProto.getDescriptor(); + Map typeStringsToResourceNames = + Parser.parseResourceNames(lockerServiceFileDescriptor); + ResourceName resourceName = + typeStringsToResourceNames.get("cloudbilling.googleapis.com/BillingAccount"); + Expr expr = + DefaultValueComposer.createDefaultValue( + resourceName, + typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + expr.accept(writerVisitor); + assertEquals("BillingAccountName.of(\"[BILLING_ACCOUNT]\")", writerVisitor.write()); + } + + @Test + public void defaultValue_resourceNameWithMultiplePatterns() { + FileDescriptor lockerServiceFileDescriptor = LockerProto.getDescriptor(); + Map typeStringsToResourceNames = + Parser.parseResourceNames(lockerServiceFileDescriptor); + ResourceName resourceName = + typeStringsToResourceNames.get("cloudresourcemanager.googleapis.com/Folder"); + Expr expr = + DefaultValueComposer.createDefaultValue( + resourceName, + typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + expr.accept(writerVisitor); + assertEquals( + "FolderName.ofProjectFolderName(\"[PROJECT]\", \"[FOLDER]\")", writerVisitor.write()); + } + + @Test + public void defaultValue_resourceNameWithWildcardPattern() { + FileDescriptor lockerServiceFileDescriptor = LockerProto.getDescriptor(); + Map typeStringsToResourceNames = + Parser.parseResourceNames(lockerServiceFileDescriptor); + ResourceName resourceName = + typeStringsToResourceNames.get("cloudresourcemanager.googleapis.com/Anything"); + Expr expr = + DefaultValueComposer.createDefaultValue( + resourceName, + typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + expr.accept(writerVisitor); + assertEquals("DocumentName.ofDocumentName(\"[DOCUMENT]\")", writerVisitor.write()); + } + + @Test + public void invalidDefaultValue_wildcardResourceNameWithOnlyDeletedTopic() { + // Edge case that should never happen in practice. + // Wildcard, but the resource names map has only other names that contain only the deleted-topic + // constant. + FileDescriptor lockerServiceFileDescriptor = LockerProto.getDescriptor(); + Map typeStringsToResourceNames = + Parser.parseResourceNames(lockerServiceFileDescriptor); + ResourceName resourceName = + typeStringsToResourceNames.get("cloudresourcemanager.googleapis.com/Anything"); + ResourceName topicResourceName = typeStringsToResourceNames.get("pubsub.googleapis.com/Topic"); + typeStringsToResourceNames.clear(); + typeStringsToResourceNames.put(topicResourceName.resourceTypeString(), topicResourceName); + Expr expr = + DefaultValueComposer.createDefaultValue( + resourceName, + typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + expr.accept(writerVisitor); + assertEquals("TopicName.ofDeletedTopic()", writerVisitor.write()); + } + + @Test + public void invalidDefaultValue_resourceNameWithOnlyWildcards() { + // Edge case that should never happen in practice. + // Wildcard, but the resource names map has only other names that contain only the deleted-topic + // constant. + FileDescriptor lockerServiceFileDescriptor = LockerProto.getDescriptor(); + Map typeStringsToResourceNames = + Parser.parseResourceNames(lockerServiceFileDescriptor); + ResourceName resourceName = + typeStringsToResourceNames.get("cloudresourcemanager.googleapis.com/Anything"); + assertThrows( + IllegalStateException.class, + () -> DefaultValueComposer.createDefaultValue(resourceName, Collections.emptyList())); + } +}