From 95ca83e7a14e99aceb86d2f845f47e6772f2aabe Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Fri, 18 Sep 2020 22:02:46 -0700 Subject: [PATCH] [ggj][codegen] feat: add comment parsing to methods and fields (#309) * 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 * feat: add comment parsing to methods and fields * fix: test * fix: solidify codegen method order with TypeNode/MethodArg and Comparable * fix: clean up tests * fix: merge --- .../api/generator/gapic/model/Field.java | 9 ++++ .../api/generator/gapic/model/Method.java | 9 +++- .../generator/gapic/model/MethodArgument.java | 10 +++++ .../generator/gapic/model/ResourceName.java | 10 +++++ .../protoparser/MethodSignatureParser.java | 42 ++++++++++++------- .../generator/gapic/protoparser/Parser.java | 41 ++++++++++++++++-- .../protoparser/ResourceReferenceParser.java | 5 +++ .../ResourceReferenceParserTest.java | 5 +++ 8 files changed, 111 insertions(+), 20 deletions(-) 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 8f6d10b2af..57d836979e 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 @@ -31,6 +31,13 @@ public abstract class Field { @Nullable public abstract ResourceReference resourceReference(); + @Nullable + public abstract String description(); + + public boolean hasDescription() { + return description() != null; + } + public boolean hasResourceReference() { return type().equals(TypeNode.STRING) && resourceReference() != null; } @@ -51,6 +58,8 @@ public abstract static class Builder { public abstract Builder setResourceReference(ResourceReference resourceReference); + public abstract Builder setDescription(String description); + public abstract Field build(); } } diff --git a/src/main/java/com/google/api/generator/gapic/model/Method.java b/src/main/java/com/google/api/generator/gapic/model/Method.java index eb77f15a1c..30e94c9ad5 100644 --- a/src/main/java/com/google/api/generator/gapic/model/Method.java +++ b/src/main/java/com/google/api/generator/gapic/model/Method.java @@ -42,6 +42,9 @@ public enum Stream { @Nullable public abstract LongrunningOperation lro(); + @Nullable + public abstract String description(); + // Example from Expand in echo.proto: Thet TypeNodes that map to // [["content", "error"], ["content", "error", "info"]]. public abstract ImmutableList> methodSignatures(); @@ -50,7 +53,9 @@ public boolean hasLro() { return lro() != null; } - // TODO(miraleung): Parse annotations, comments. + public boolean hasDescription() { + return description() != null; + } public static Builder builder() { return new AutoValue_Method.Builder() @@ -84,6 +89,8 @@ public abstract static class Builder { public abstract Builder setLro(LongrunningOperation lro); + public abstract Builder setDescription(String description); + public abstract Builder setMethodSignatures(List> methodSignature); public abstract Builder setIsPaged(boolean isPaged); 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 685f7b4685..f7ce9ad2b9 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,6 +18,7 @@ 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 { @@ -32,6 +33,13 @@ public abstract class MethodArgument implements Comparable { // Returns true if this is a resource name helper tyep. 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()); @@ -57,6 +65,8 @@ public abstract static class Builder { public abstract Builder setIsResourceNameHelper(boolean isResourceNameHelper); + public abstract Builder setDescription(String description); + public abstract MethodArgument build(); } } diff --git a/src/main/java/com/google/api/generator/gapic/model/ResourceName.java b/src/main/java/com/google/api/generator/gapic/model/ResourceName.java index 45e8e1c814..14770b7366 100644 --- a/src/main/java/com/google/api/generator/gapic/model/ResourceName.java +++ b/src/main/java/com/google/api/generator/gapic/model/ResourceName.java @@ -54,10 +54,17 @@ public abstract class ResourceName { @Nullable public abstract String parentMessageName(); + @Nullable + public abstract String description(); + public boolean hasParentMessageName() { return parentMessageName() != null; } + public boolean hasDescription() { + return description() != null; + } + public String resourceTypeName() { return resourceTypeString().substring(resourceTypeString().indexOf(SLASH) + 1); } @@ -92,6 +99,7 @@ public boolean equals(Object o) { } ResourceName other = (ResourceName) o; + // Exclude the description from the resource name because it's just a comment. return variableName().equals(other.variableName()) && pakkage().equals(other.pakkage()) && resourceTypeString().equals(other.resourceTypeString()) @@ -125,6 +133,8 @@ public abstract static class Builder { public abstract Builder setParentMessageName(String parentMessageName); + public abstract Builder setDescription(String description); + // Private setters. abstract Builder setType(TypeNode type); 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 d7d20e5265..baef34d268 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 @@ -25,7 +25,6 @@ import com.google.common.collect.Lists; import com.google.protobuf.Descriptors.MethodDescriptor; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -68,8 +67,8 @@ public static List> parseMethodSignatures( // For resource names, this will be empty. List argumentTypePathAcc = new ArrayList<>(); // There should be more than one type returned only when we encounter a reousrce name. - List argumentTypes = - parseTypeFromArgumentName( + Map argumentTypes = + parseTypeAndCommentFromArgumentName( argumentName, servicePackage, inputMessage, @@ -84,14 +83,15 @@ public static List> parseMethodSignatures( argumentNames.add(actualArgumentName); argumentNameToOverloads.put( actualArgumentName, - argumentTypes.stream() + argumentTypes.entrySet().stream() .map( - type -> + e -> MethodArgument.builder() .setName(actualArgumentName) - .setType(type) + .setDescription(e.getValue()) // May be null. + .setType(e.getKey()) .setIsResourceNameHelper( - argumentTypes.size() > 1 && !type.equals(TypeNode.STRING)) + argumentTypes.size() > 1 && !e.getKey().equals(TypeNode.STRING)) .setNestedTypes(argumentTypePathAcc) .build()) .collect(Collectors.toList())); @@ -143,7 +143,7 @@ private static List> flattenMethodSignatureVariants( return methodArgs; } - private static List parseTypeFromArgumentName( + private static Map parseTypeAndCommentFromArgumentName( String argumentName, String servicePackage, Message inputMessage, @@ -153,6 +153,8 @@ private static List parseTypeFromArgumentName( List argumentTypePathAcc, Set outputArgResourceNames) { + // Comment values may be null. + Map typeToComment = new HashMap<>(); int dotIndex = argumentName.indexOf(DOT); if (dotIndex < 1) { Field field = inputMessage.fieldMap().get(argumentName); @@ -162,19 +164,27 @@ private static List parseTypeFromArgumentName( "Field %s not found from input message %s values %s", argumentName, inputMessage.name(), inputMessage.fieldMap().keySet())); if (!field.hasResourceReference()) { - return Arrays.asList(field.type()); + typeToComment.put(field.type(), field.description()); + return typeToComment; } // Parse the resource name tyeps. List resourceNameArgs = ResourceReferenceParser.parseResourceNames( - field.resourceReference(), servicePackage, resourceNames, patternsToResourceNames); + field.resourceReference(), + servicePackage, + field.description(), + resourceNames, + patternsToResourceNames); outputArgResourceNames.addAll(resourceNameArgs); - List allFieldTypes = new ArrayList<>(); - allFieldTypes.add(TypeNode.STRING); - allFieldTypes.addAll( - resourceNameArgs.stream().map(r -> r.type()).collect(Collectors.toList())); - return allFieldTypes; + 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; } Preconditions.checkState( @@ -204,7 +214,7 @@ private static List parseTypeFromArgumentName( "Message type %s for field reference %s invalid", firstFieldTypeName, firstFieldName)); argumentTypePathAcc.add(firstFieldType); - return parseTypeFromArgumentName( + return parseTypeAndCommentFromArgumentName( remainingArgumentName, servicePackage, firstFieldMessage, 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 275ec8d5cb..c8485ad4f2 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 @@ -29,6 +29,7 @@ import com.google.api.generator.gapic.model.ResourceName; import com.google.api.generator.gapic.model.ResourceReference; import com.google.api.generator.gapic.model.Service; +import com.google.api.generator.gapic.model.SourceCodeInfoLocation; import com.google.api.generator.gapic.utils.ResourceNameConstants; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -55,6 +56,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -64,6 +66,9 @@ public class Parser { private static final String COLON = ":"; private static final String DEFAULT_PORT = "443"; + // Allow other parsers to access this. + protected static final SourceCodeInfoParser SOURCE_CODE_INFO_PARSER = new SourceCodeInfoParser(); + static class GapicParserException extends RuntimeException { public GapicParserException(String errorMessage) { super(errorMessage); @@ -147,7 +152,17 @@ public static List parseService( List oauthScopes = Arrays.asList(serviceOptions.getExtension(ClientProto.oauthScopes).split(COMMA)); - return Service.builder() + Service.Builder serviceBuilder = Service.builder(); + if (fileDescriptor.toProto().hasSourceCodeInfo()) { + SourceCodeInfoLocation protoServiceLocation = + SOURCE_CODE_INFO_PARSER.getLocation(s); + if (!Objects.isNull(protoServiceLocation) + && !Strings.isNullOrEmpty(protoServiceLocation.getLeadingComments())) { + serviceBuilder.setDescription(protoServiceLocation.getLeadingComments()); + } + } + + return serviceBuilder .setName(s.getName()) .setDefaultHost(defaultHost) .setOauthScopes(oauthScopes) @@ -244,8 +259,18 @@ static List parseMethods( for (MethodDescriptor protoMethod : serviceDescriptor.getMethods()) { // Parse the method. TypeNode inputType = TypeParser.parseType(protoMethod.getInputType()); + Method.Builder methodBuilder = Method.builder(); + if (protoMethod.getFile().toProto().hasSourceCodeInfo()) { + SourceCodeInfoLocation protoMethodLocation = + SOURCE_CODE_INFO_PARSER.getLocation(protoMethod); + if (!Objects.isNull(protoMethodLocation) + && !Strings.isNullOrEmpty(protoMethodLocation.getLeadingComments())) { + methodBuilder.setDescription(protoMethodLocation.getLeadingComments()); + } + } + methods.add( - Method.builder() + methodBuilder .setName(protoMethod.getName()) .setInputType(inputType) .setOutputType(TypeParser.parseType(protoMethod.getOutputType())) @@ -358,7 +383,17 @@ private static Field parseField(FieldDescriptor fieldDescriptor, Descriptor mess } } - return Field.builder() + Field.Builder fieldBuilder = Field.builder(); + if (fieldDescriptor.getFile().toProto().hasSourceCodeInfo()) { + SourceCodeInfoLocation protoFieldLocation = + SOURCE_CODE_INFO_PARSER.getLocation(fieldDescriptor); + if (!Objects.isNull(protoFieldLocation) + && !Strings.isNullOrEmpty(protoFieldLocation.getLeadingComments())) { + fieldBuilder.setDescription(protoFieldLocation.getLeadingComments()); + } + } + + return fieldBuilder .setName(fieldDescriptor.getName()) .setType(TypeParser.parseType(fieldDescriptor)) .setIsRepeated(fieldDescriptor.isRepeated()) diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParser.java b/src/main/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParser.java index f81d198260..a8731deea2 100644 --- a/src/main/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParser.java +++ b/src/main/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParser.java @@ -29,6 +29,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import javax.annotation.Nullable; public class ResourceReferenceParser { private static final String EMPTY_STRING = ""; @@ -39,6 +40,7 @@ public class ResourceReferenceParser { public static List parseResourceNames( ResourceReference resourceReference, String servicePackage, + @Nullable String description, Map resourceNames, Map patternsToResourceNames) { ResourceName resourceName = resourceNames.get(resourceReference.resourceTypeString()); @@ -62,6 +64,7 @@ public static List parseResourceNames( servicePackage, resourceName.pakkage(), resourceName.resourceTypeString(), + description, patternsToResourceNames); // Prevent duplicates. if (parentResourceNameOpt.isPresent() @@ -80,6 +83,7 @@ static Optional parseParentResourceName( String servicePackage, String resourcePackage, String resourceTypeString, + @Nullable String description, Map patternsToResourceNames) { Optional parentPatternOpt = parseParentPattern(pattern); if (!parentPatternOpt.isPresent()) { @@ -143,6 +147,7 @@ static Optional parseParentResourceName( .setPakkage(pakkage) .setResourceTypeString(parentResourceTypeString) .setPatterns(Arrays.asList(parentPattern)) + .setDescription(description) .build(); patternsToResourceNames.put(parentPattern, parentResourceName); diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParserTest.java index 990e7dc4a7..c2cae61f88 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParserTest.java +++ b/src/test/java/com/google/api/generator/gapic/protoparser/ResourceReferenceParserTest.java @@ -48,6 +48,7 @@ public void setUp() { public void parseParentResourceName_createFromPattern() { String resourceNamePackage = String.format("%s.common", MAIN_PACKAGE); String domainName = "cloudbilling.googleapis.com"; + String description = "This is the resource name description"; String resourceTypeString = String.format("%s/BillingAccount", domainName); String parentResourceTypeString = String.format("%s/Project", domainName); Map patternsToResourceNames = new HashMap<>(); @@ -59,6 +60,7 @@ public void parseParentResourceName_createFromPattern() { MAIN_PACKAGE, resourceNamePackage, resourceTypeString, + description, patternsToResourceNames); assertTrue(parentResourceNameOpt.isPresent()); @@ -67,6 +69,7 @@ public void parseParentResourceName_createFromPattern() { assertEquals(Arrays.asList(parentPattern), parentResourceName.patterns()); assertEquals(parentResourceTypeString, parentResourceName.resourceTypeString()); assertEquals(resourceNamePackage, parentResourceName.pakkage()); + assertEquals(description, parentResourceName.description()); assertEquals( TypeNode.withReference( VaporReference.builder() @@ -93,6 +96,7 @@ public void parseParentResourceName_parentResourceNameExists() { ResourceReferenceParser.parseParentResourceName( "projects/{project}/folders/{folder}/documents/{document}", MAIN_PACKAGE, + null, MAIN_PACKAGE, "cloudresourcemanager.googleapis.com/Document", patternsToResourceNames); @@ -109,6 +113,7 @@ public void parseParentResourceName_badPattern() { ResourceReferenceParser.parseParentResourceName( "projects/{project}/billingAccounts", MAIN_PACKAGE, + null, MAIN_PACKAGE, "cloudbilling.googleapis.com/Feature", new HashMap());