From fd218de18288d1ef48ffc9fd8cb33908eb886ae3 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 15 Sep 2020 15:13:10 -0700 Subject: [PATCH 01/11] feat: add protobuf comment parser util --- .../protoparser/SourceCodeInfoParser.java | 300 ++++++++++++++++++ .../generator/gapic/protoparser/BUILD.bazel | 2 + .../protoparser/SourceCodeInfoParserTest.java | 164 ++++++++++ .../api/generator/gapic/testdata/BUILD.bazel | 10 + .../api/generator/gapic/testdata/basic.proto | 80 +++++ 5 files changed, 556 insertions(+) create mode 100644 src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java create mode 100644 src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java create mode 100644 src/test/java/com/google/api/generator/gapic/testdata/basic.proto diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java b/src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java new file mode 100644 index 0000000000..ef1552c441 --- /dev/null +++ b/src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java @@ -0,0 +1,300 @@ +// 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.protoparser; + +import com.google.common.base.Function; +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.Maps; +import com.google.common.collect.Multimaps; +import com.google.protobuf.DescriptorProtos.DescriptorProto; +import com.google.protobuf.DescriptorProtos.EnumDescriptorProto; +import com.google.protobuf.DescriptorProtos.FileDescriptorProto; +import com.google.protobuf.DescriptorProtos.ServiceDescriptorProto; +import com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location; +import com.google.protobuf.Descriptors.Descriptor; +import com.google.protobuf.Descriptors.EnumDescriptor; +import com.google.protobuf.Descriptors.EnumValueDescriptor; +import com.google.protobuf.Descriptors.FieldDescriptor; +import com.google.protobuf.Descriptors.FileDescriptor; +import com.google.protobuf.Descriptors.MethodDescriptor; +import com.google.protobuf.Descriptors.OneofDescriptor; +import com.google.protobuf.Descriptors.ServiceDescriptor; +import java.util.Map; +import javax.annotation.Nullable; + +/** + * A helper class which provides protocol buffer source info for descriptors. + * + *

In order to make this work, the descriptors need to be produced using the flag {@code + * --include_source_info}. Note that descriptors taken from the generated java code have source info + * stripped, and won't work with this class. + * + *

This class uses internal caches to speed up access to the source info. It is not thread safe. + * If you think you need this functionality in a thread-safe context, feel free to suggest a + * refactor. + */ +public class SourceCodeInfoParser { + /** + * A map from file descriptors to the analyzed source info, stored as a multimap from a path of + * the form {@code n.m.l} to the location info. + */ + private final Map> fileToPathToLocation = + Maps.newHashMap(); + + /** A map from descriptor objects to the path to those objects in their proto file. */ + private final Map descriptorToPath = Maps.newHashMap(); + + /** Gets the location of a message, if available. */ + @Nullable + public Location getLocation(Descriptor message) { + FileDescriptor file = message.getFile(); + if (!file.toProto().hasSourceCodeInfo()) { + return null; + } + return getLocation(file, buildPath(message)); + } + + /** Gets the location of a field, if available. */ + @Nullable + public Location getLocation(FieldDescriptor field) { + FileDescriptor file = field.getFile(); + if (!file.toProto().hasSourceCodeInfo()) { + return null; + } + return getLocation(file, buildPath(field)); + } + + /** Gets the location of a service, if available. */ + @Nullable + public Location getLocation(ServiceDescriptor service) { + FileDescriptor file = service.getFile(); + if (!file.toProto().hasSourceCodeInfo()) { + return null; + } + return getLocation(file, buildPath(service)); + } + + /** Gets the location of a method, if available. */ + @Nullable + public Location getLocation(MethodDescriptor method) { + FileDescriptor file = method.getFile(); + if (!file.toProto().hasSourceCodeInfo()) { + return null; + } + return getLocation(file, buildPath(method)); + } + + /** Gets the location of an enum type, if available. */ + @Nullable + public Location getLocation(EnumDescriptor enumType) { + FileDescriptor file = enumType.getFile(); + if (!file.toProto().hasSourceCodeInfo()) { + return null; + } + return getLocation(file, buildPath(enumType)); + } + + /** Gets the location of an enum value, if available. */ + @Nullable + public Location getLocation(EnumValueDescriptor enumValue) { + FileDescriptor file = enumValue.getFile(); + if (!file.toProto().hasSourceCodeInfo()) { + return null; + } + return getLocation(file, buildPath(enumValue)); + } + + /** Gets the location of a oneof, if available. */ + @Nullable + public Location getLocation(OneofDescriptor oneof) { + FileDescriptor file = oneof.getFile(); + if (!file.toProto().hasSourceCodeInfo()) { + return null; + } + return getLocation(file, buildPath(oneof)); + } + + // ----------------------------------------------------------------------------- + // Helpers + + /** + * A helper to compute the location based on a file descriptor and a path into that descriptor. + */ + private Location getLocation(FileDescriptor file, String path) { + ImmutableList cands = getCandidateLocations(file, path); + if (cands != null && cands.isEmpty()) { + return null; + } else { + return cands.get(0); // We choose the first one. + } + } + + private ImmutableList getCandidateLocations(FileDescriptor file, String path) { + ImmutableListMultimap locationMap = fileToPathToLocation.get(file); + if (locationMap == null) { + locationMap = + Multimaps.index( + file.toProto().getSourceCodeInfo().getLocationList(), + new Function() { + @Override + public String apply(Location location) { + return Joiner.on('.').join(location.getPathList()); + } + }); + fileToPathToLocation.put(file, locationMap); + } + return locationMap.get(path); + } + + private String buildPath(Descriptor message) { + String path = descriptorToPath.get(message); + if (path != null) { + return path; + } + if (message.getContainingType() != null) { + path = + String.format( + "%s.%d.%d", + buildPath(message.getContainingType()), + DescriptorProto.NESTED_TYPE_FIELD_NUMBER, + message.getContainingType().getNestedTypes().indexOf(message)); + } else { + path = + String.format( + "%d.%d", + FileDescriptorProto.MESSAGE_TYPE_FIELD_NUMBER, + message.getFile().getMessageTypes().indexOf(message)); + } + descriptorToPath.put(message, path); + return path; + } + + private String buildPath(FieldDescriptor field) { + String path = descriptorToPath.get(field); + if (path != null) { + return path; + } + if (field.isExtension()) { + if (field.getExtensionScope() == null) { + path = + String.format( + "%d.%d", + FileDescriptorProto.EXTENSION_FIELD_NUMBER, + field.getFile().getExtensions().indexOf(field)); + } else { + path = + String.format( + "%s.%d.%d", + buildPath(field.getExtensionScope()), + DescriptorProto.EXTENSION_FIELD_NUMBER, + field.getExtensionScope().getExtensions().indexOf(field)); + } + } else { + path = + String.format( + "%s.%d.%d", + buildPath(field.getContainingType()), + DescriptorProto.FIELD_FIELD_NUMBER, + field.getContainingType().getFields().indexOf(field)); + } + descriptorToPath.put(field, path); + return path; + } + + private String buildPath(ServiceDescriptor service) { + String path = descriptorToPath.get(service); + if (path != null) { + return path; + } + path = + String.format( + "%d.%d", + FileDescriptorProto.SERVICE_FIELD_NUMBER, + service.getFile().getServices().indexOf(service)); + descriptorToPath.put(service, path); + return path; + } + + private String buildPath(MethodDescriptor method) { + String path = descriptorToPath.get(method); + if (path != null) { + return path; + } + path = + String.format( + "%s.%d.%d", + buildPath(method.getService()), + ServiceDescriptorProto.METHOD_FIELD_NUMBER, + method.getService().getMethods().indexOf(method)); + descriptorToPath.put(method, path); + return path; + } + + private String buildPath(EnumDescriptor enumType) { + String path = descriptorToPath.get(enumType); + if (path != null) { + return path; + } + if (enumType.getContainingType() != null) { + path = + String.format( + "%s.%d.%d", + buildPath(enumType.getContainingType()), + DescriptorProto.ENUM_TYPE_FIELD_NUMBER, + enumType.getContainingType().getEnumTypes().indexOf(enumType)); + } else { + path = + String.format( + "%d.%d", + FileDescriptorProto.ENUM_TYPE_FIELD_NUMBER, + enumType.getFile().getEnumTypes().indexOf(enumType)); + } + descriptorToPath.put(enumType, path); + return path; + } + + private String buildPath(EnumValueDescriptor enumValue) { + String path = descriptorToPath.get(enumValue); + if (path != null) { + return path; + } + path = + String.format( + "%s.%d.%d", + buildPath(enumValue.getType()), + EnumDescriptorProto.VALUE_FIELD_NUMBER, + enumValue.getType().getValues().indexOf(enumValue)); + descriptorToPath.put(enumValue, path); + return path; + } + + private String buildPath(OneofDescriptor oneof) { + String path = descriptorToPath.get(oneof); + if (path != null) { + return path; + } + path = + String.format( + "%s.%d.%d", + buildPath(oneof.getContainingType()), + DescriptorProto.ONEOF_DECL_FIELD_NUMBER, + oneof.getContainingType().getOneofs().indexOf(oneof)); + + descriptorToPath.put(oneof, path); + return path; + } +} diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/BUILD.bazel b/src/test/java/com/google/api/generator/gapic/protoparser/BUILD.bazel index d8bacd3937..cde28cbf97 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/BUILD.bazel +++ b/src/test/java/com/google/api/generator/gapic/protoparser/BUILD.bazel @@ -8,6 +8,7 @@ TESTS = [ "ResourceNameParserTest", "ResourceReferenceParserTest", "ServiceConfigParserTest", + "SourceCodeInfoParserTest", ] filegroup( @@ -19,6 +20,7 @@ filegroup( name = test_name, srcs = ["{0}.java".format(test_name)], data = [ + "//src/test/java/com/google/api/generator/gapic/testdata:basic_proto_descriptor", "//src/test/java/com/google/api/generator/gapic/testdata:gapic_config_files", "//src/test/java/com/google/api/generator/gapic/testdata:service_config_files", ], diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java new file mode 100644 index 0000000000..06d0d0c753 --- /dev/null +++ b/src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java @@ -0,0 +1,164 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this protoFile 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.protoparser; + +import static com.google.common.truth.Truth.assertThat; +import static junit.framework.Assert.assertEquals; + +import com.google.protobuf.DescriptorProtos.FileDescriptorProto; +import com.google.protobuf.DescriptorProtos.FileDescriptorSet; +import com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location; +import com.google.protobuf.Descriptors.Descriptor; +import com.google.protobuf.Descriptors.EnumDescriptor; +import com.google.protobuf.Descriptors.FileDescriptor; +import com.google.protobuf.Descriptors.OneofDescriptor; +import com.google.protobuf.Descriptors.ServiceDescriptor; +import java.io.FileInputStream; +import java.util.ArrayList; +import java.util.List; +import org.junit.Before; +import org.junit.Test; + +public class SourceCodeInfoParserTest { + private static final String TEST_PROTO_FILE = + "src/test/java/com/google/api/generator/gapic/testdata/basic_proto.descriptor"; + + private SourceCodeInfoParser parser; + private FileDescriptor protoFile; + + @Before + public void setUp() throws Exception { + parser = new SourceCodeInfoParser(); + protoFile = buildFileDescriptor(); + } + + @Test + public void getServiceInfo() { + Location location = parser.getLocation(protoFile.findServiceByName("FooService")); + assertEquals( + " This is a service description.\n It takes up multiple lines, like so.\n", + location.getLeadingComments()); + + location = parser.getLocation(protoFile.findServiceByName("BarService")); + assertEquals(" This is another service description.\n", location.getLeadingComments()); + } + + @Test + public void getMethodInfo() { + ServiceDescriptor service = protoFile.findServiceByName("FooService"); + Location location = parser.getLocation(service.findMethodByName("FooMethod")); + assertEquals( + " FooMethod does something.\n This comment also takes up multiple lines.\n", + location.getLeadingComments()); + + service = protoFile.findServiceByName("BarService"); + location = parser.getLocation(service.findMethodByName("BarMethod")); + assertEquals(" BarMethod does another thing.\n", location.getLeadingComments()); + } + + @Test + public void getOuterMessageInfo() { + Descriptor message = protoFile.findMessageTypeByName("FooMessage"); + Location location = parser.getLocation(message); + assertEquals( + " This is a message descxription.\n" + + " Lorum ipsum dolor sit amet consectetur adipiscing elit.\n", + location.getLeadingComments()); + + // Fields. + location = parser.getLocation(message.findFieldByName("field_one")); + assertEquals( + " This is a field description for field_one.\n" + + " And here is the second line of that description.\n", + location.getLeadingComments()); + assertEquals(" A field trailing comment.\n", location.getTrailingComments()); + + location = parser.getLocation(message.findFieldByName("field_two")); + assertEquals(" This is another field description.\n", location.getLeadingComments()); + assertEquals(" Another field trailing comment.\n", location.getTrailingComments()); + } + + @Test + public void getInnerMessageInfo() { + Descriptor message = protoFile.findMessageTypeByName("FooMessage"); + assertThat(message).isNotNull(); + message = message.findNestedTypeByName("BarMessage"); + + Location location = parser.getLocation(message); + assertEquals( + " This is an inner message description for BarMessage.\n", location.getLeadingComments()); + + // Fields. + location = parser.getLocation(message.findFieldByName("field_three")); + assertEquals(" A third leading comment for field_three.\n", location.getLeadingComments()); + + location = parser.getLocation(message.findFieldByName("field_two")); + assertEquals("\n This is a block comment for field_two.\n", location.getLeadingComments()); + } + + @Test + public void getOuterEnumInfo() { + EnumDescriptor protoEnum = protoFile.findEnumTypeByName("OuterEnum"); + Location location = parser.getLocation(protoEnum); + assertEquals(" This is an outer enum.\n", location.getLeadingComments()); + + // Enum fields. + location = parser.getLocation(protoEnum.findValueByName("VALUE_UNSPECIFIED")); + assertEquals(" Another unspecified value.\n", location.getLeadingComments()); + } + + @Test + public void getInnerEnumInfo() { + Descriptor message = protoFile.findMessageTypeByName("FooMessage"); + EnumDescriptor protoEnum = message.findEnumTypeByName("FoodEnum"); + Location location = parser.getLocation(protoEnum); + assertEquals(" An inner enum.\n", location.getLeadingComments()); + + // Enum fields. + location = parser.getLocation(protoEnum.findValueByName("RICE")); + assertEquals(" 😋 🍚.\n", location.getLeadingComments()); + location = parser.getLocation(protoEnum.findValueByName("CHOCOLATE")); + assertEquals(" 🤤 🍫.\n", location.getLeadingComments()); + } + + @Test + public void getOnoeofInfo() { + Descriptor message = protoFile.findMessageTypeByName("FooMessage"); + OneofDescriptor protoOneof = message.getOneofs().get(0); + Location location = parser.getLocation(protoOneof); + assertEquals(" An inner oneof.\n", location.getLeadingComments()); + + location = parser.getLocation(protoOneof.getField(0)); + assertEquals(" An InnerOneof comment for its field.\n", location.getLeadingComments()); + } + + /** + * Parses a {@link FileDescriptorSet} from the {@link TEST_PROTO_FILE} and converts the protos to + * {@link FileDescriptor} wrappers. + * + * @return the top level target protoFile descriptor + */ + private static FileDescriptor buildFileDescriptor() throws Exception { + FileDescriptor result = null; + List protoFileList = + FileDescriptorSet.parseFrom(new FileInputStream(TEST_PROTO_FILE)).getFileList(); + List deps = new ArrayList<>(); + for (FileDescriptorProto proto : protoFileList) { + result = FileDescriptor.buildFrom(proto, deps.toArray(new FileDescriptor[0])); + deps.add(result); + } + return result; + } +} diff --git a/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel b/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel index 44d2b3bf82..600b3cb18e 100644 --- a/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel +++ b/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel @@ -10,6 +10,16 @@ filegroup( srcs = glob(["*_gapic.yaml"]), ) +genrule( + name = "basic_proto_descriptor", + srcs = [ + "basic.proto", + ], + outs = ["basic_proto.descriptor"], + cmd = ("protoc --include_source_info --include_imports --descriptor_set_out=$@ $(SRCS)"), + message = "Generating proto descriptor", +) + proto_library( name = "showcase_proto", srcs = [ diff --git a/src/test/java/com/google/api/generator/gapic/testdata/basic.proto b/src/test/java/com/google/api/generator/gapic/testdata/basic.proto new file mode 100644 index 0000000000..bee4393336 --- /dev/null +++ b/src/test/java/com/google/api/generator/gapic/testdata/basic.proto @@ -0,0 +1,80 @@ +// Copyright 2018 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 +// +// https://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. + +syntax = "proto3"; + +package google.testdata; + +option java_package = "com.google.google.testdata"; + +// This is a service description. +// It takes up multiple lines, like so. +service FooService { + // FooMethod does something. + // This comment also takes up multiple lines. + rpc FooMethod(FooMessage) returns (FooMessage.BarMessage); +} + +// This is another service description. +service BarService { + // BarMethod does another thing. + rpc BarMethod(FooMessage) returns (FooMessage.BarMessage); +} + +// This is a message descxription. +// Lorum ipsum dolor sit amet consectetur adipiscing elit. +message FooMessage { + // This is a field description for field_one. + // And here is the second line of that description. + string field_one = 1; // A field trailing comment. + + // This is another field description. + string field_two = 2; + // Another field trailing comment. + + // This is an inner message description for BarMessage. + message BarMessage { + // A third leading comment for field_three. + string field_three = 1; + + /* + * This is a block comment for field_two. + */ + string field_two = 2; + } + + // An inner enum. + enum FoodEnum { + // Unspecified value. + FOOD_UNSPECIFIED = 0; + + // 😋 🍚. + RICE = 1; + + // 🤤 🍫. + CHOCOLATE = 2; + } + + // An inner oneof. + oneof InnerOneof { + // An InnerOneof comment for its field. + string field_four = 6; + } +} + +// This is an outer enum. +enum OuterEnum { + // Another unspecified value. + VALUE_UNSPECIFIED = 0; +} From 8c792e36e0e2b3a40719df1d29ab7f712655ade3 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 15 Sep 2020 15:16:29 -0700 Subject: [PATCH 02/11] fix: add basic proto build rules --- .../java/com/google/api/generator/gapic/testdata/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel b/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel index 600b3cb18e..e4569f7cb4 100644 --- a/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel +++ b/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel @@ -16,7 +16,7 @@ genrule( "basic.proto", ], outs = ["basic_proto.descriptor"], - cmd = ("protoc --include_source_info --include_imports --descriptor_set_out=$@ $(SRCS)"), + cmd = "protoc --include_source_info --include_imports --descriptor_set_out=$@ $(SRCS)", message = "Generating proto descriptor", ) From d6ee06813167937dcff91d721d998933573158c3 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 15 Sep 2020 15:16:58 -0700 Subject: [PATCH 03/11] feat: add header comments to ServiceClient --- .../composer/ServiceClientClassComposer.java | 2 + .../ServiceClientCommentComposer.java | 83 +++++++++++++++++++ .../api/generator/gapic/model/Service.java | 11 ++- .../ServiceClientClassComposerTest.java | 49 +++++++++++ 4 files changed, 144 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java index 74eaa5c6bd..cf0f2367bb 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java @@ -101,6 +101,8 @@ public GapicClass generate(Service service, Map messageTypes) { ClassDefinition classDef = ClassDefinition.builder() + .setHeaderCommentStatements( + ServiceClientCommentComposer.createClassHeaderComments(service)) .setPackageString(pakkage) .setAnnotations(createClassAnnotations(types)) .setImplementsTypes(createClassImplements(types)) 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 3ace86e832..46b7d61bbe 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 @@ -17,14 +17,62 @@ import com.google.api.generator.engine.ast.CommentStatement; import com.google.api.generator.engine.ast.JavaDocComment; import com.google.api.generator.engine.ast.TypeNode; +import com.google.api.generator.gapic.model.Service; +import com.google.api.generator.gapic.utils.JavaStyle; +import java.util.Arrays; +import java.util.List; class ServiceClientCommentComposer { + // Tokens. private static final String COLON = ":"; + // Constants. + private static final String SERVICE_DESCRIPTION_INTRO_STRING = + "This class provides the ability to make remote calls to the backing service through method " + + "calls that map to API methods. Sample code to get started:"; + private static final String SERVICE_DESCRIPTION_CLOSE_STRING = + "Note: close() needs to be called on the echoClient object to clean up resources such as " + + "threads. In the example above, try-with-resources is used, which automatically calls " + + "close()."; + private static final String SERVICE_DESCRIPTION_SURFACE_SUMMARY_STRING = + "The surface of this class includes several types of Java methods for each of the API's " + + "methods:"; + private static final String SERVICE_DESCRIPTION_SURFACE_CODA_STRING = + "See the individual methods for example code."; + private static final String SERVICE_DESCRIPTION_RESOURCE_NAMES_FORMATTING_STRING = + "Many parameters require resource names to be formatted in a particular way. To assist with" + + " these names, this class includes a format method for each type of name, and" + + " additionally a parse method to extract the individual identifiers contained within" + + " names that are returned."; + private static final String SERVICE_DESCRIPTION_CREDENTIALS_SUMMARY_STRING = + "To customize credentials:"; + private static final String SERVICE_DESCRIPTION_ENDPOINT_SUMMARY_STRING = + "To customize the endpoint:"; + + private static final List SERVICE_DESCRIPTION_SURFACE_DESCRIPTION = + Arrays.asList( + "A \"flattened\" method. With this type of method, the fields of the request type have" + + " been converted into function parameters. It may be the case that not all fields" + + " are available as parameters, and not every API method will have a flattened" + + " method entry point.", + "A \"request object\" method. This type of method only takes one parameter, a request" + + " object, which must be constructed before the call. Not every API method will" + + " have a request object method.", + "A \"callable\" method. This type of method takes no parameters and returns an immutable " + + "API callable object, which can be used to initiate calls to the service."); + + // Patterns. private static final String CREATE_METHOD_STUB_ARG_PATTERN = "Constructs an instance of EchoClient, using the given stub for making calls. This is for" + " advanced usage - prefer using create(%s)."; + private static final String SERVICE_DESCRIPTION_CUSTOMIZE_SUMMARY_PATTERN = + "This class can be customized by passing in a custom instance of %s to create(). For" + + " example:"; + + private static final String SERVICE_DESCRIPTION_SUMMARY_PATTERN = "Service Description: %s"; + + // Comments. static final CommentStatement CREATE_METHOD_NO_ARG_COMMENT = toSimpleComment("Constructs an instance of EchoClient with default settings."); @@ -45,6 +93,41 @@ class ServiceClientCommentComposer { "Returns the OperationsClient that can be used to query the status of a long-running" + " operation returned by another API method call."); + static List createClassHeaderComments(Service service) { + JavaDocComment.Builder classHeaderJavadocBuilder = JavaDocComment.builder(); + if (service.hasDescription()) { + classHeaderJavadocBuilder.addComment( + String.format(SERVICE_DESCRIPTION_SUMMARY_PATTERN, service.description())); + } + + // Service introduction. + classHeaderJavadocBuilder.addParagraph(SERVICE_DESCRIPTION_INTRO_STRING); + // TODO(summerji): Add sample code here. + + // API surface description. + classHeaderJavadocBuilder.addParagraph(SERVICE_DESCRIPTION_CLOSE_STRING); + classHeaderJavadocBuilder.addParagraph(SERVICE_DESCRIPTION_SURFACE_SUMMARY_STRING); + classHeaderJavadocBuilder.addOrderedList(SERVICE_DESCRIPTION_SURFACE_DESCRIPTION); + classHeaderJavadocBuilder.addParagraph(SERVICE_DESCRIPTION_SURFACE_CODA_STRING); + + // Formatting resource names. + classHeaderJavadocBuilder.addParagraph(SERVICE_DESCRIPTION_RESOURCE_NAMES_FORMATTING_STRING); + + // Customization examples. + classHeaderJavadocBuilder.addParagraph( + String.format( + SERVICE_DESCRIPTION_CUSTOMIZE_SUMMARY_PATTERN, + String.format("%sSettings", JavaStyle.toUpperCamelCase(service.name())))); + classHeaderJavadocBuilder.addParagraph(SERVICE_DESCRIPTION_CREDENTIALS_SUMMARY_STRING); + // TODO(summerji): Add credentials' customization sample code here. + classHeaderJavadocBuilder.addParagraph(SERVICE_DESCRIPTION_ENDPOINT_SUMMARY_STRING); + // TODO(summerji): Add endpoint customization sample code here. + + return Arrays.asList( + CommentComposer.AUTO_GENERATED_CLASS_COMMENT, + CommentStatement.withComment(classHeaderJavadocBuilder.build())); + } + static CommentStatement createCreateMethodStubArgComment(TypeNode settingsType) { return toSimpleComment( String.format(CREATE_METHOD_STUB_ARG_PATTERN, settingsType.reference().name())); diff --git a/src/main/java/com/google/api/generator/gapic/model/Service.java b/src/main/java/com/google/api/generator/gapic/model/Service.java index 22bf2e3c24..a56567af50 100644 --- a/src/main/java/com/google/api/generator/gapic/model/Service.java +++ b/src/main/java/com/google/api/generator/gapic/model/Service.java @@ -15,8 +15,10 @@ package com.google.api.generator.gapic.model; import com.google.auto.value.AutoValue; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import java.util.List; +import javax.annotation.Nullable; @AutoValue public abstract class Service { @@ -32,7 +34,12 @@ public abstract class Service { public abstract ImmutableList methods(); - // TODO(miraleung): Get comments. + @Nullable + public abstract String description(); + + public boolean hasDescription() { + return !Strings.isNullOrEmpty(description()); + } public static Builder builder() { return new AutoValue_Service.Builder().setMethods(ImmutableList.of()); @@ -52,6 +59,8 @@ public abstract static class Builder { public abstract Builder setMethods(List methods); + public abstract Builder setDescription(String description); + public abstract Service build(); } } diff --git a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java index b18cdbe0ea..27e33bfa2e 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java @@ -83,6 +83,55 @@ public void generateServiceClasses() { + "import java.util.concurrent.TimeUnit;\n" + "import javax.annotation.Generated;\n" + "\n" + + "// AUTO-GENERATED DOCUMENTATION AND CLASS.\n" + + "/**\n" + + " * This class provides the ability to make remote calls to the backing service" + + " through method calls\n" + + " * that map to API methods. Sample code to get started:\n" + + " *\n" + + " *

Note: close() needs to be called on the echoClient object to clean up resources" + + " such as\n" + + " * threads. In the example above, try-with-resources is used, which automatically" + + " calls close().\n" + + " *\n" + + " *

The surface of this class includes several types of Java methods for each of" + + " the API's\n" + + " * methods:\n" + + " *\n" + + " *

    \n" + + " *
  1. A \"flattened\" method. With this type of method, the fields of the request" + + " type have been\n" + + " * converted into function parameters. It may be the case that not all fields" + + " are available as\n" + + " * parameters, and not every API method will have a flattened method entry" + + " point.\n" + + " *
  2. A \"request object\" method. This type of method only takes one parameter, a" + + " request object,\n" + + " * which must be constructed before the call. Not every API method will have a" + + " request object\n" + + " * method.\n" + + " *
  3. A \"callable\" method. This type of method takes no parameters and returns" + + " an immutable API\n" + + " * callable object, which can be used to initiate calls to the service.\n" + + " *
\n" + + " *\n" + + " *

See the individual methods for example code.\n" + + " *\n" + + " *

Many parameters require resource names to be formatted in a particular way. To" + + " assist with\n" + + " * these names, this class includes a format method for each type of name, and" + + " additionally a parse\n" + + " * method to extract the individual identifiers contained within names that are" + + " returned.\n" + + " *\n" + + " *

This class can be customized by passing in a custom instance of EchoSettings to" + + " create(). For\n" + + " * example:\n" + + " *\n" + + " *

To customize credentials:\n" + + " *\n" + + " *

To customize the endpoint:\n" + + " */\n" + "@BetaApi\n" + "@Generated(\"by gapic-generator\")\n" + "public class EchoClient implements BackgroundResource {\n" From 82aa0b6e9d9e58c5c494c27a3c5209b837593049 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 15 Sep 2020 15:27:37 -0700 Subject: [PATCH 04/11] fix: build protoc at test time --- .../com/google/api/generator/gapic/testdata/BUILD.bazel | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel b/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel index e4569f7cb4..744138900f 100644 --- a/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel +++ b/src/test/java/com/google/api/generator/gapic/testdata/BUILD.bazel @@ -16,8 +16,13 @@ genrule( "basic.proto", ], outs = ["basic_proto.descriptor"], - cmd = "protoc --include_source_info --include_imports --descriptor_set_out=$@ $(SRCS)", + # CircleCI does not have protoc installed. + cmd = "$(location @com_google_protobuf//:protoc) " + + "--include_source_info --include_imports --descriptor_set_out=$@ $(SRCS)", message = "Generating proto descriptor", + tools = [ + "@com_google_protobuf//:protoc", + ], ) proto_library( From 2405b1ad3df5d5f8011a7c1c64c8ea2904e2f4c9 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 15 Sep 2020 16:19:50 -0700 Subject: [PATCH 05/11] fix!: wrap protobuf location and process comments --- .../gapic/model/SourceCodeInfoLocation.java | 64 +++++++++++++++++++ .../protoparser/SourceCodeInfoParser.java | 29 +++++---- .../protoparser/SourceCodeInfoParserTest.java | 57 ++++++++--------- 3 files changed, 107 insertions(+), 43 deletions(-) create mode 100644 src/main/java/com/google/api/generator/gapic/model/SourceCodeInfoLocation.java diff --git a/src/main/java/com/google/api/generator/gapic/model/SourceCodeInfoLocation.java b/src/main/java/com/google/api/generator/gapic/model/SourceCodeInfoLocation.java new file mode 100644 index 0000000000..fca6e7c791 --- /dev/null +++ b/src/main/java/com/google/api/generator/gapic/model/SourceCodeInfoLocation.java @@ -0,0 +1,64 @@ +// 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 com.google.common.escape.Escaper; +import com.google.common.escape.Escapers; +import com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location; +import javax.annotation.Nonnull; + +/** + * A light wrapper around SourceCodeInfo.Location to provide cleaner protobuf comments. Please see + * additional documentation on descriptor.proto. + */ +public class SourceCodeInfoLocation { + // Not a singleton because of nested-class instantiation mechanics. + private final NewlineEscaper ESCAPER = new NewlineEscaper(); + + @Nonnull private final Location location; + + private SourceCodeInfoLocation(Location location) { + this.location = location; + } + + public static SourceCodeInfoLocation create(@Nonnull Location location) { + return new SourceCodeInfoLocation(location); + } + + public String getLeadingComments() { + return processProtobufComment(location.getLeadingComments()); + } + + public String getTrailingComments() { + return processProtobufComment(location.getTrailingComments()); + } + + public String getLeadingDetachedComments(int index) { + return processProtobufComment(location.getLeadingDetachedComments(index)); + } + + private String processProtobufComment(String s) { + return ESCAPER.escape(s).trim(); + } + + private class NewlineEscaper extends Escaper { + private final Escaper charEscaper = Escapers.builder().addEscape('\n', "").build(); + + @Override + public String escape(String sourceString) { + return charEscaper.escape(sourceString); + } + } +} diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java b/src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java index ef1552c441..bfa4c62297 100644 --- a/src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java +++ b/src/main/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParser.java @@ -14,6 +14,7 @@ package com.google.api.generator.gapic.protoparser; +import com.google.api.generator.gapic.model.SourceCodeInfoLocation; import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; @@ -60,72 +61,72 @@ public class SourceCodeInfoParser { /** Gets the location of a message, if available. */ @Nullable - public Location getLocation(Descriptor message) { + public SourceCodeInfoLocation getLocation(Descriptor message) { FileDescriptor file = message.getFile(); if (!file.toProto().hasSourceCodeInfo()) { return null; } - return getLocation(file, buildPath(message)); + return SourceCodeInfoLocation.create(getLocation(file, buildPath(message))); } /** Gets the location of a field, if available. */ @Nullable - public Location getLocation(FieldDescriptor field) { + public SourceCodeInfoLocation getLocation(FieldDescriptor field) { FileDescriptor file = field.getFile(); if (!file.toProto().hasSourceCodeInfo()) { return null; } - return getLocation(file, buildPath(field)); + return SourceCodeInfoLocation.create(getLocation(file, buildPath(field))); } /** Gets the location of a service, if available. */ @Nullable - public Location getLocation(ServiceDescriptor service) { + public SourceCodeInfoLocation getLocation(ServiceDescriptor service) { FileDescriptor file = service.getFile(); if (!file.toProto().hasSourceCodeInfo()) { return null; } - return getLocation(file, buildPath(service)); + return SourceCodeInfoLocation.create(getLocation(file, buildPath(service))); } /** Gets the location of a method, if available. */ @Nullable - public Location getLocation(MethodDescriptor method) { + public SourceCodeInfoLocation getLocation(MethodDescriptor method) { FileDescriptor file = method.getFile(); if (!file.toProto().hasSourceCodeInfo()) { return null; } - return getLocation(file, buildPath(method)); + return SourceCodeInfoLocation.create(getLocation(file, buildPath(method))); } /** Gets the location of an enum type, if available. */ @Nullable - public Location getLocation(EnumDescriptor enumType) { + public SourceCodeInfoLocation getLocation(EnumDescriptor enumType) { FileDescriptor file = enumType.getFile(); if (!file.toProto().hasSourceCodeInfo()) { return null; } - return getLocation(file, buildPath(enumType)); + return SourceCodeInfoLocation.create(getLocation(file, buildPath(enumType))); } /** Gets the location of an enum value, if available. */ @Nullable - public Location getLocation(EnumValueDescriptor enumValue) { + public SourceCodeInfoLocation getLocation(EnumValueDescriptor enumValue) { FileDescriptor file = enumValue.getFile(); if (!file.toProto().hasSourceCodeInfo()) { return null; } - return getLocation(file, buildPath(enumValue)); + return SourceCodeInfoLocation.create(getLocation(file, buildPath(enumValue))); } /** Gets the location of a oneof, if available. */ @Nullable - public Location getLocation(OneofDescriptor oneof) { + public SourceCodeInfoLocation getLocation(OneofDescriptor oneof) { FileDescriptor file = oneof.getFile(); if (!file.toProto().hasSourceCodeInfo()) { return null; } - return getLocation(file, buildPath(oneof)); + return SourceCodeInfoLocation.create(getLocation(file, buildPath(oneof))); } // ----------------------------------------------------------------------------- diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java index 06d0d0c753..b8daabefeb 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java +++ b/src/test/java/com/google/api/generator/gapic/protoparser/SourceCodeInfoParserTest.java @@ -17,9 +17,9 @@ import static com.google.common.truth.Truth.assertThat; import static junit.framework.Assert.assertEquals; +import com.google.api.generator.gapic.model.SourceCodeInfoLocation; import com.google.protobuf.DescriptorProtos.FileDescriptorProto; import com.google.protobuf.DescriptorProtos.FileDescriptorSet; -import com.google.protobuf.DescriptorProtos.SourceCodeInfo.Location; import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Descriptors.EnumDescriptor; import com.google.protobuf.Descriptors.FileDescriptor; @@ -46,48 +46,47 @@ public void setUp() throws Exception { @Test public void getServiceInfo() { - Location location = parser.getLocation(protoFile.findServiceByName("FooService")); + SourceCodeInfoLocation location = parser.getLocation(protoFile.findServiceByName("FooService")); assertEquals( - " This is a service description.\n It takes up multiple lines, like so.\n", + "This is a service description. It takes up multiple lines, like so.", location.getLeadingComments()); location = parser.getLocation(protoFile.findServiceByName("BarService")); - assertEquals(" This is another service description.\n", location.getLeadingComments()); + assertEquals("This is another service description.", location.getLeadingComments()); } @Test public void getMethodInfo() { ServiceDescriptor service = protoFile.findServiceByName("FooService"); - Location location = parser.getLocation(service.findMethodByName("FooMethod")); + SourceCodeInfoLocation location = parser.getLocation(service.findMethodByName("FooMethod")); assertEquals( - " FooMethod does something.\n This comment also takes up multiple lines.\n", + "FooMethod does something. This comment also takes up multiple lines.", location.getLeadingComments()); service = protoFile.findServiceByName("BarService"); location = parser.getLocation(service.findMethodByName("BarMethod")); - assertEquals(" BarMethod does another thing.\n", location.getLeadingComments()); + assertEquals("BarMethod does another thing.", location.getLeadingComments()); } @Test public void getOuterMessageInfo() { Descriptor message = protoFile.findMessageTypeByName("FooMessage"); - Location location = parser.getLocation(message); + SourceCodeInfoLocation location = parser.getLocation(message); assertEquals( - " This is a message descxription.\n" - + " Lorum ipsum dolor sit amet consectetur adipiscing elit.\n", + "This is a message descxription. Lorum ipsum dolor sit amet consectetur adipiscing elit.", location.getLeadingComments()); // Fields. location = parser.getLocation(message.findFieldByName("field_one")); assertEquals( - " This is a field description for field_one.\n" - + " And here is the second line of that description.\n", + "This is a field description for field_one. And here is the second line of that" + + " description.", location.getLeadingComments()); - assertEquals(" A field trailing comment.\n", location.getTrailingComments()); + assertEquals("A field trailing comment.", location.getTrailingComments()); location = parser.getLocation(message.findFieldByName("field_two")); - assertEquals(" This is another field description.\n", location.getLeadingComments()); - assertEquals(" Another field trailing comment.\n", location.getTrailingComments()); + assertEquals("This is another field description.", location.getLeadingComments()); + assertEquals("Another field trailing comment.", location.getTrailingComments()); } @Test @@ -96,52 +95,52 @@ public void getInnerMessageInfo() { assertThat(message).isNotNull(); message = message.findNestedTypeByName("BarMessage"); - Location location = parser.getLocation(message); + SourceCodeInfoLocation location = parser.getLocation(message); assertEquals( - " This is an inner message description for BarMessage.\n", location.getLeadingComments()); + "This is an inner message description for BarMessage.", location.getLeadingComments()); // Fields. location = parser.getLocation(message.findFieldByName("field_three")); - assertEquals(" A third leading comment for field_three.\n", location.getLeadingComments()); + assertEquals("A third leading comment for field_three.", location.getLeadingComments()); location = parser.getLocation(message.findFieldByName("field_two")); - assertEquals("\n This is a block comment for field_two.\n", location.getLeadingComments()); + assertEquals("This is a block comment for field_two.", location.getLeadingComments()); } @Test public void getOuterEnumInfo() { EnumDescriptor protoEnum = protoFile.findEnumTypeByName("OuterEnum"); - Location location = parser.getLocation(protoEnum); - assertEquals(" This is an outer enum.\n", location.getLeadingComments()); + SourceCodeInfoLocation location = parser.getLocation(protoEnum); + assertEquals("This is an outer enum.", location.getLeadingComments()); // Enum fields. location = parser.getLocation(protoEnum.findValueByName("VALUE_UNSPECIFIED")); - assertEquals(" Another unspecified value.\n", location.getLeadingComments()); + assertEquals("Another unspecified value.", location.getLeadingComments()); } @Test public void getInnerEnumInfo() { Descriptor message = protoFile.findMessageTypeByName("FooMessage"); EnumDescriptor protoEnum = message.findEnumTypeByName("FoodEnum"); - Location location = parser.getLocation(protoEnum); - assertEquals(" An inner enum.\n", location.getLeadingComments()); + SourceCodeInfoLocation location = parser.getLocation(protoEnum); + assertEquals("An inner enum.", location.getLeadingComments()); // Enum fields. location = parser.getLocation(protoEnum.findValueByName("RICE")); - assertEquals(" 😋 🍚.\n", location.getLeadingComments()); + assertEquals("😋 🍚.", location.getLeadingComments()); location = parser.getLocation(protoEnum.findValueByName("CHOCOLATE")); - assertEquals(" 🤤 🍫.\n", location.getLeadingComments()); + assertEquals("🤤 🍫.", location.getLeadingComments()); } @Test public void getOnoeofInfo() { Descriptor message = protoFile.findMessageTypeByName("FooMessage"); OneofDescriptor protoOneof = message.getOneofs().get(0); - Location location = parser.getLocation(protoOneof); - assertEquals(" An inner oneof.\n", location.getLeadingComments()); + SourceCodeInfoLocation location = parser.getLocation(protoOneof); + assertEquals("An inner oneof.", location.getLeadingComments()); location = parser.getLocation(protoOneof.getField(0)); - assertEquals(" An InnerOneof comment for its field.\n", location.getLeadingComments()); + assertEquals("An InnerOneof comment for its field.", location.getLeadingComments()); } /** From a59fd3f354bca8845fac53dcbf99a3bbffd883fc Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 16 Sep 2020 13:22:12 -0700 Subject: [PATCH 06/11] feat: add comment parsing to methods and fields --- .../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 + .../ServiceClientClassComposerTest.java | 102 ++++++++++++++++++ .../ResourceReferenceParserTest.java | 5 + 9 files changed, 213 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 1a27957f17..33ecfb2cdc 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 { @@ -32,6 +33,13 @@ public abstract class MethodArgument { // 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; + } + public static Builder builder() { return new AutoValue_MethodArgument.Builder() .setNestedTypes(ImmutableList.of()) @@ -48,6 +56,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/composer/ServiceClientClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java index 27e33bfa2e..5a0a3f0eb1 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java @@ -203,27 +203,63 @@ public void generateServiceClasses() { + " return operationsClient;\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param content\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String content) {\n" + " EchoRequest request = EchoRequest.newBuilder().setContent(content).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param error\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(Status error) {\n" + " EchoRequest request = EchoRequest.newBuilder().setError(error).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param content\n" + + " * @param severity\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String content, Severity severity) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder().setContent(content).setSeverity(severity).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param name\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String name) {\n" + " EchoRequest request = EchoRequest.newBuilder().setName(name).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param name\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(FoobarName name) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder()\n" @@ -232,11 +268,25 @@ public void generateServiceClasses() { + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param parent\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String parent) {\n" + " EchoRequest request = EchoRequest.newBuilder().setParent(parent).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param parent\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(ResourceName parent) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder()\n" @@ -245,65 +295,117 @@ public void generateServiceClasses() { + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(EchoRequest request) {\n" + " return echoCallable().call(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable echoCallable() {\n" + " return stub.echoCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final ServerStreamingCallable expandCallable()" + " {\n" + " return stub.expandCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final ClientStreamingCallable collectCallable()" + " {\n" + " return stub.collectCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final BidiStreamingCallable chatCallable() {\n" + " return stub.chatCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final BidiStreamingCallable chatAgainCallable()" + " {\n" + " return stub.chatAgainCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final PagedExpandResponse pagedExpand(PagedExpandRequest request) {\n" + " return pagedExpandCallable().call(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable\n" + " pagedExpandPagedCallable() {\n" + " return stub.pagedExpandPagedCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable" + " pagedExpandCallable() {\n" + " return stub.pagedExpandCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final OperationFuture waitAsync(WaitRequest" + " request) {\n" + " return waitOperationCallable().futureCall(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final OperationCallable" + " waitOperationCallable() {\n" + " return stub.waitOperationCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable waitCallable() {\n" + " return stub.waitCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final BlockResponse block(BlockRequest request) {\n" + " return blockCallable().call(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable blockCallable() {\n" + " return stub.blockCallable();\n" + " }\n" 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()); From 66dec11135abf9c0f4bff1089c17178dc6d83b94 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 16 Sep 2020 13:25:13 -0700 Subject: [PATCH 07/11] fix: test --- .../ServiceClientClassComposerTest.java | 102 ------------------ 1 file changed, 102 deletions(-) diff --git a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java index 5a0a3f0eb1..27e33bfa2e 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java @@ -203,63 +203,27 @@ public void generateServiceClasses() { + " return operationsClient;\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param content\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(String content) {\n" + " EchoRequest request = EchoRequest.newBuilder().setContent(content).build();\n" + " return echo(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param error\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(Status error) {\n" + " EchoRequest request = EchoRequest.newBuilder().setError(error).build();\n" + " return echo(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param content\n" - + " * @param severity\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(String content, Severity severity) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder().setContent(content).setSeverity(severity).build();\n" + " return echo(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param name\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(String name) {\n" + " EchoRequest request = EchoRequest.newBuilder().setName(name).build();\n" + " return echo(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param name\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(FoobarName name) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder()\n" @@ -268,25 +232,11 @@ public void generateServiceClasses() { + " return echo(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param parent\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(String parent) {\n" + " EchoRequest request = EchoRequest.newBuilder().setParent(parent).build();\n" + " return echo(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param parent\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(ResourceName parent) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder()\n" @@ -295,117 +245,65 @@ public void generateServiceClasses() { + " return echo(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param request The request object containing all of the parameters for the API" - + " call.\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final EchoResponse echo(EchoRequest request) {\n" + " return echoCallable().call(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final UnaryCallable echoCallable() {\n" + " return stub.echoCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final ServerStreamingCallable expandCallable()" + " {\n" + " return stub.expandCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final ClientStreamingCallable collectCallable()" + " {\n" + " return stub.collectCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final BidiStreamingCallable chatCallable() {\n" + " return stub.chatCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final BidiStreamingCallable chatAgainCallable()" + " {\n" + " return stub.chatAgainCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param request The request object containing all of the parameters for the API" - + " call.\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final PagedExpandResponse pagedExpand(PagedExpandRequest request) {\n" + " return pagedExpandCallable().call(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final UnaryCallable\n" + " pagedExpandPagedCallable() {\n" + " return stub.pagedExpandPagedCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final UnaryCallable" + " pagedExpandCallable() {\n" + " return stub.pagedExpandCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param request The request object containing all of the parameters for the API" - + " call.\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final OperationFuture waitAsync(WaitRequest" + " request) {\n" + " return waitOperationCallable().futureCall(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final OperationCallable" + " waitOperationCallable() {\n" + " return stub.waitOperationCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final UnaryCallable waitCallable() {\n" + " return stub.waitCallable();\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /**\n" - + " * Sample code:\n" - + " *\n" - + " * @param request The request object containing all of the parameters for the API" - + " call.\n" - + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" - + " */\n" + " public final BlockResponse block(BlockRequest request) {\n" + " return blockCallable().call(request);\n" + " }\n" + "\n" - + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" - + " /** Sample code: */\n" + " public final UnaryCallable blockCallable() {\n" + " return stub.blockCallable();\n" + " }\n" From 83254508b44778f886888dcb70d0207347812b11 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 16 Sep 2020 13:26:22 -0700 Subject: [PATCH 08/11] feat: add protobuf comments to ServiceClient --- .../composer/ServiceClientClassComposer.java | 9 ++ .../ServiceClientCommentComposer.java | 55 ++++++++++ .../ServiceClientClassComposerTest.java | 102 ++++++++++++++++++ 3 files changed, 166 insertions(+) diff --git a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java index cf0f2367bb..84064723b9 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java @@ -565,6 +565,8 @@ private static List createMethodVariants( javaMethods.add( MethodDefinition.builder() + .setHeaderCommentStatements( + ServiceClientCommentComposer.createRpcMethodHeaderComment(method, signature)) .setScope(ScopeNode.PUBLIC) .setIsFinal(true) .setReturnType(methodOutputType) @@ -595,6 +597,8 @@ private static List createMethodVariants( .build(); javaMethods.add( MethodDefinition.builder() + .setHeaderCommentStatements( + ServiceClientCommentComposer.createRpcMethodHeaderComment(method)) .setScope(ScopeNode.PUBLIC) .setIsFinal(true) .setReturnType(methodOutputType) @@ -608,6 +612,7 @@ private static List createMethodVariants( private static MethodDefinition createLroAsyncMethod( String serviceName, Method method, Map types) { + // TODO(miraleung): Create variants as well. Preconditions.checkState( method.hasLro(), String.format("Method %s does not have an LRO", method.name())); String methodName = JavaStyle.toLowerCamelCase(method.name()); @@ -642,6 +647,8 @@ private static MethodDefinition createLroAsyncMethod( .build(); return MethodDefinition.builder() + .setHeaderCommentStatements( + ServiceClientCommentComposer.createRpcMethodHeaderComment(method)) .setScope(ScopeNode.PUBLIC) .setIsFinal(true) .setReturnType(returnType) @@ -713,6 +720,8 @@ private static MethodDefinition createCallableMethod( .build(); return MethodDefinition.builder() + .setHeaderCommentStatements( + ServiceClientCommentComposer.createRpcCallableMethodHeaderComment(method)) .setScope(ScopeNode.PUBLIC) .setIsFinal(true) .setName(methodName) 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 46b7d61bbe..d79bf7f03e 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 @@ -17,14 +17,20 @@ import com.google.api.generator.engine.ast.CommentStatement; import com.google.api.generator.engine.ast.JavaDocComment; import com.google.api.generator.engine.ast.TypeNode; +import com.google.api.generator.gapic.model.Method; +import com.google.api.generator.gapic.model.MethodArgument; import com.google.api.generator.gapic.model.Service; import com.google.api.generator.gapic.utils.JavaStyle; import java.util.Arrays; +import java.util.Collections; import java.util.List; class ServiceClientCommentComposer { // Tokens. private static final String COLON = ":"; + private static final String EMPTY_STRING = ""; + private static final String API_EXCEPTION_TYPE_NAME = "com.google.api.gax.rpc.ApiException"; + private static final String EXCEPTION_CONDITION = "if the remote call fails"; // Constants. private static final String SERVICE_DESCRIPTION_INTRO_STRING = @@ -49,6 +55,8 @@ class ServiceClientCommentComposer { private static final String SERVICE_DESCRIPTION_ENDPOINT_SUMMARY_STRING = "To customize the endpoint:"; + private static final String METHOD_DESCRIPTION_SAMPLE_CODE_SUMMARY_STRING = "Sample code:"; + private static final List SERVICE_DESCRIPTION_SURFACE_DESCRIPTION = Arrays.asList( "A \"flattened\" method. With this type of method, the fields of the request type have" @@ -133,6 +141,53 @@ static CommentStatement createCreateMethodStubArgComment(TypeNode settingsType) String.format(CREATE_METHOD_STUB_ARG_PATTERN, settingsType.reference().name())); } + static List createRpcMethodHeaderComment( + Method method, List methodArguments) { + JavaDocComment.Builder methodJavadocBuilder = JavaDocComment.builder(); + + if (method.hasDescription()) { + methodJavadocBuilder.addComment(method.description()); + } + + methodJavadocBuilder.addParagraph(METHOD_DESCRIPTION_SAMPLE_CODE_SUMMARY_STRING); + // TODO(summerji): Add sample code here. + + if (methodArguments.isEmpty()) { + methodJavadocBuilder.addParam( + "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); + } + } + + methodJavadocBuilder.setThrows(API_EXCEPTION_TYPE_NAME, EXCEPTION_CONDITION); + + return Arrays.asList( + CommentComposer.AUTO_GENERATED_METHOD_COMMENT, + CommentStatement.withComment(methodJavadocBuilder.build())); + } + + static List createRpcMethodHeaderComment(Method method) { + return createRpcMethodHeaderComment(method, Collections.emptyList()); + } + + static List createRpcCallableMethodHeaderComment(Method method) { + JavaDocComment.Builder methodJavadocBuilder = JavaDocComment.builder(); + + if (method.hasDescription()) { + methodJavadocBuilder.addComment(method.description()); + } + + methodJavadocBuilder.addParagraph(METHOD_DESCRIPTION_SAMPLE_CODE_SUMMARY_STRING); + // TODO(summerji): Add sample code here. + + return Arrays.asList( + CommentComposer.AUTO_GENERATED_METHOD_COMMENT, + CommentStatement.withComment(methodJavadocBuilder.build())); + } + private static CommentStatement toSimpleComment(String comment) { return CommentStatement.withComment(JavaDocComment.withComment(comment)); } diff --git a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java index 27e33bfa2e..5a0a3f0eb1 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java @@ -203,27 +203,63 @@ public void generateServiceClasses() { + " return operationsClient;\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param content\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String content) {\n" + " EchoRequest request = EchoRequest.newBuilder().setContent(content).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param error\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(Status error) {\n" + " EchoRequest request = EchoRequest.newBuilder().setError(error).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param content\n" + + " * @param severity\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String content, Severity severity) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder().setContent(content).setSeverity(severity).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param name\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String name) {\n" + " EchoRequest request = EchoRequest.newBuilder().setName(name).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param name\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(FoobarName name) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder()\n" @@ -232,11 +268,25 @@ public void generateServiceClasses() { + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param parent\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(String parent) {\n" + " EchoRequest request = EchoRequest.newBuilder().setParent(parent).build();\n" + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param parent\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(ResourceName parent) {\n" + " EchoRequest request =\n" + " EchoRequest.newBuilder()\n" @@ -245,65 +295,117 @@ public void generateServiceClasses() { + " return echo(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final EchoResponse echo(EchoRequest request) {\n" + " return echoCallable().call(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable echoCallable() {\n" + " return stub.echoCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final ServerStreamingCallable expandCallable()" + " {\n" + " return stub.expandCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final ClientStreamingCallable collectCallable()" + " {\n" + " return stub.collectCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final BidiStreamingCallable chatCallable() {\n" + " return stub.chatCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final BidiStreamingCallable chatAgainCallable()" + " {\n" + " return stub.chatAgainCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final PagedExpandResponse pagedExpand(PagedExpandRequest request) {\n" + " return pagedExpandCallable().call(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable\n" + " pagedExpandPagedCallable() {\n" + " return stub.pagedExpandPagedCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable" + " pagedExpandCallable() {\n" + " return stub.pagedExpandCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final OperationFuture waitAsync(WaitRequest" + " request) {\n" + " return waitOperationCallable().futureCall(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final OperationCallable" + " waitOperationCallable() {\n" + " return stub.waitOperationCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable waitCallable() {\n" + " return stub.waitCallable();\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /**\n" + + " * Sample code:\n" + + " *\n" + + " * @param request The request object containing all of the parameters for the API" + + " call.\n" + + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + + " */\n" + " public final BlockResponse block(BlockRequest request) {\n" + " return blockCallable().call(request);\n" + " }\n" + "\n" + + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n" + + " /** Sample code: */\n" + " public final UnaryCallable blockCallable() {\n" + " return stub.blockCallable();\n" + " }\n" From 5acc151d8fb0253a7224e16fe889f57479cc60b5 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 16 Sep 2020 16:45:49 -0700 Subject: [PATCH 09/11] fix: solidify codegen method order with TypeNode/MethodArg and Comparable --- .../api/generator/engine/ast/TypeNode.java | 34 ++++++++++++- .../composer/ServiceClientClassComposer.java | 21 +++++++- .../generator/gapic/model/MethodArgument.java | 11 +++- .../generator/engine/ast/TypeNodeTest.java | 30 +++++++++++ .../ServiceClientClassComposerTest.java | 30 +++++------ .../api/generator/gapic/model/BUILD.bazel | 2 + .../gapic/model/MethodArgumentTest.java | 51 +++++++++++++++++++ 7 files changed, 161 insertions(+), 18 deletions(-) create mode 100644 src/test/java/com/google/api/generator/gapic/model/MethodArgumentTest.java diff --git a/src/main/java/com/google/api/generator/engine/ast/TypeNode.java b/src/main/java/com/google/api/generator/engine/ast/TypeNode.java index adc16f1f26..d16c8801c7 100644 --- a/src/main/java/com/google/api/generator/engine/ast/TypeNode.java +++ b/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 { static final Reference EXCEPTION_REFERENCE = ConcreteReference.withClazz(Exception.class); public static final Reference WILDCARD_REFERENCE = ConcreteReference.wildcard(); @@ -88,6 +88,38 @@ 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. + System.out.println( + String.format( + "DEL: %s comapre to %s: %d", + reference().fullName(), + other.reference().fullName(), + reference().fullName().compareTo(other.reference().fullName()))); + + return reference().fullName().compareTo(other.reference().fullName()); + } + public static Builder builder() { return new AutoValue_TypeNode.Builder().setIsArray(false); } diff --git a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java index cf0f2367bb..4c7ed0987f 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/ServiceClientClassComposer.java @@ -473,7 +473,26 @@ private static List createMethodVariants( Preconditions.checkNotNull( inputMessage, String.format("Message %s not found", methodInputTypeName)); - for (List signature : method.methodSignatures()) { + // Make the method signature order deterministic, which helps with unit testing and per-version + // diffs. + List> 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 signature : sortedMethodSignatures) { // Get the argument list. List arguments = signature.stream() 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 1a27957f17..685f7b4685 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 @@ -20,7 +20,7 @@ import java.util.List; @AutoValue -public abstract class MethodArgument { +public abstract class MethodArgument implements Comparable { 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()) diff --git a/src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java b/src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java index 5e559db5a7..392ecc37f8 100644 --- a/src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java +++ b/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,35 @@ 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.withReference(ConcreteReference.withClazz(Arrays.class)))) + .isLessThan(0); + } + @Test public void invalidType_topLevelWildcard() { assertThrows( diff --git a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java index 27e33bfa2e..ca3c8fd44c 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java +++ b/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" diff --git a/src/test/java/com/google/api/generator/gapic/model/BUILD.bazel b/src/test/java/com/google/api/generator/gapic/model/BUILD.bazel index 705c6b9cc7..6c919246db 100644 --- a/src/test/java/com/google/api/generator/gapic/model/BUILD.bazel +++ b/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", 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 new file mode 100644 index 0000000000..810758267a --- /dev/null +++ b/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 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 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); + } +} From 97e780ab8b85acaf81c7bdb45dc2f5b04d5dea58 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 16 Sep 2020 16:48:28 -0700 Subject: [PATCH 10/11] fix: clean up tests --- .../java/com/google/api/generator/engine/ast/TypeNode.java | 7 ------- .../com/google/api/generator/engine/ast/TypeNodeTest.java | 1 + 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main/java/com/google/api/generator/engine/ast/TypeNode.java b/src/main/java/com/google/api/generator/engine/ast/TypeNode.java index d16c8801c7..26c9b658c5 100644 --- a/src/main/java/com/google/api/generator/engine/ast/TypeNode.java +++ b/src/main/java/com/google/api/generator/engine/ast/TypeNode.java @@ -110,13 +110,6 @@ public int compareTo(TypeNode other) { // Both are reference types. // TODO(miraleung): Replace this with a proper reference Comaparator. - System.out.println( - String.format( - "DEL: %s comapre to %s: %d", - reference().fullName(), - other.reference().fullName(), - reference().fullName().compareTo(other.reference().fullName()))); - return reference().fullName().compareTo(other.reference().fullName()); } diff --git a/src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java b/src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java index 392ecc37f8..982849447b 100644 --- a/src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java +++ b/src/test/java/com/google/api/generator/engine/ast/TypeNodeTest.java @@ -136,6 +136,7 @@ public void compareTypes() { // 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)))) From 05ac1c9753a66b4f75efe847032c13e32dcc55f7 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Fri, 18 Sep 2020 22:17:46 -0700 Subject: [PATCH 11/11] fix: merge --- .../gapic/composer/ServiceClientCommentComposer.java | 2 -- .../gapic/composer/ServiceClientClassComposerTest.java | 4 ++-- 2 files changed, 2 insertions(+), 4 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 0f66ebd697..d79bf7f03e 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 @@ -55,10 +55,8 @@ class ServiceClientCommentComposer { private static final String SERVICE_DESCRIPTION_ENDPOINT_SUMMARY_STRING = "To customize the endpoint:"; - private static final String METHOD_DESCRIPTION_SAMPLE_CODE_SUMMARY_STRING = "Sample code:"; - private static final List SERVICE_DESCRIPTION_SURFACE_DESCRIPTION = Arrays.asList( "A \"flattened\" method. With this type of method, the fields of the request type have" diff --git a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java index 64cb53b7a3..fc56cb6ced 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/ServiceClientClassComposerTest.java @@ -348,8 +348,8 @@ public void generateServiceClasses() { + " call.\n" + " * @throws com.google.api.gax.rpc.ApiException if the remote call fails\n" + " */\n" - + " public final PagedExpandResponse pagedExpand(PagedExpandRequest request) {\n" - + " return pagedExpandCallable().call(request);\n" + + " public final PagedExpandPagedResponse pagedExpand(PagedExpandRequest request) {\n" + + " return pagedExpandPagedCallable().call(request);\n" + " }\n" + "\n" + " // AUTO-GENERATED DOCUMENTATION AND METHOD.\n"