From 7e9abbb8b7a7b75e6a7cee1a3a832fe503086f4a Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 25 Nov 2020 13:58:21 -0800 Subject: [PATCH 1/3] fix: handle empty method_signature in parsing protos --- .../protoparser/MethodSignatureParser.java | 5 +++ .../gapic/protoparser/ParserTest.java | 31 +++++++++++++++++-- .../api/generator/gapic/testdata/echo.proto | 1 + 3 files changed, 34 insertions(+), 3 deletions(-) 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 ebf877d133..06fcb0b357 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 @@ -23,6 +23,7 @@ import com.google.api.generator.gapic.model.ResourceReference; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.protobuf.Descriptors.MethodDescriptor; import java.util.ArrayList; @@ -59,6 +60,10 @@ public static List> parseMethodSignatures( // Example from Expand in echo.proto: // stringSigs: ["content,error", "content,error,info"]. for (String stringSig : stringSigs) { + if (Strings.isNullOrEmpty(stringSig)) { + continue; + } + List argumentNames = new ArrayList<>(); Map> argumentNameToOverloads = new HashMap<>(); diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java index f3c696b9f1..c56a38ed2c 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java +++ b/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java @@ -187,8 +187,9 @@ public void parseLro_missingMetadataType() { @Test public void parseMethodSignatures_empty() { // TODO(miraleung): Move this to MethodSignatureParserTest. - MethodDescriptor chatWithInfoMethodDescriptor = echoService.getMethods().get(5); - TypeNode inputType = TypeParser.parseType(chatWithInfoMethodDescriptor.getInputType()); + MethodDescriptor methodDescriptor = echoService.getMethods().get(3); + assertEquals("Chat", methodDescriptor.getName()); + TypeNode inputType = TypeParser.parseType(methodDescriptor.getInputType()); Map messageTypes = Parser.parseMessages(echoFileDescriptor); Map resourceNames = Parser.parseResourceNames(echoFileDescriptor); Set outputResourceNames = new HashSet<>(); @@ -198,7 +199,31 @@ public void parseMethodSignatures_empty() { echoService, ECHO_PACKAGE, messageTypes, resourceNames, outputResourceNames); assertThat( MethodSignatureParser.parseMethodSignatures( - chatWithInfoMethodDescriptor, + methodDescriptor, + ECHO_PACKAGE, + inputType, + messageTypes, + resourceNames, + outputResourceNames)) + .isEmpty(); + } + + @Test + public void parseMethodSignatures_emptyString() { + // TODO(miraleung): Move this to MethodSignatureParserTest. + MethodDescriptor methodDescriptor = echoService.getMethods().get(5); + assertEquals("PagedExpand", methodDescriptor.getName()); + TypeNode inputType = TypeParser.parseType(methodDescriptor.getInputType()); + Map messageTypes = Parser.parseMessages(echoFileDescriptor); + Map resourceNames = Parser.parseResourceNames(echoFileDescriptor); + Set outputResourceNames = new HashSet<>(); + + List methods = + Parser.parseMethods( + echoService, ECHO_PACKAGE, messageTypes, resourceNames, outputResourceNames); + assertThat( + MethodSignatureParser.parseMethodSignatures( + methodDescriptor, ECHO_PACKAGE, inputType, messageTypes, diff --git a/src/test/java/com/google/api/generator/gapic/testdata/echo.proto b/src/test/java/com/google/api/generator/gapic/testdata/echo.proto index 19058ab680..854956ffcc 100644 --- a/src/test/java/com/google/api/generator/gapic/testdata/echo.proto +++ b/src/test/java/com/google/api/generator/gapic/testdata/echo.proto @@ -96,6 +96,7 @@ service Echo { post: "/v1beta1/echo:pagedExpand" body: "*" }; + option (google.api.method_signature) = ""; } // This method will wait the requested amount of and then return. From 3e21503a4ae5c1ef2059d34e4e30ed1b62978a88 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 25 Nov 2020 14:01:15 -0800 Subject: [PATCH 2/3] fix: add google/type:type_java_proto as default test dep --- rules_java_gapic/java_gapic.bzl | 1 + test/integration/BUILD.bazel | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/rules_java_gapic/java_gapic.bzl b/rules_java_gapic/java_gapic.bzl index e76ecd4682..b178c397e8 100644 --- a/rules_java_gapic/java_gapic.bzl +++ b/rules_java_gapic/java_gapic.bzl @@ -197,6 +197,7 @@ def java_gapic_library( # Test deps. actual_test_deps = test_deps + actual_deps + [ + "@com_google_googleapis//google/type:type_java_proto", # Commonly used. "@com_google_api_gax_java//gax-grpc:gax_grpc_testlib", "@com_google_api_gax_java//gax:gax_testlib", "@com_google_code_gson_gson//jar", diff --git a/test/integration/BUILD.bazel b/test/integration/BUILD.bazel index 597e389b17..59b65ac274 100644 --- a/test/integration/BUILD.bazel +++ b/test/integration/BUILD.bazel @@ -55,7 +55,6 @@ java_gapic_library( "@com_google_googleapis//google/cloud/asset/v1:asset_java_proto", "@com_google_googleapis//google/iam/v1:iam_java_proto", "@com_google_googleapis//google/identity/accesscontextmanager/v1:accesscontextmanager_proto", - "@com_google_googleapis//google/type:type_java_proto", ], ) @@ -75,7 +74,6 @@ java_gapic_assembly_gradle_pkg( "@com_google_googleapis//google/cloud/asset/v1:asset_java_proto", "@com_google_googleapis//google/cloud/asset/v1:asset_proto", "@com_google_googleapis//google/cloud/orgpolicy/v1:orgpolicy_java_proto", - "@com_google_googleapis//google/identity/accesscontextmanager/type:type_java_proto", "@com_google_googleapis//google/identity/accesscontextmanager/v1:accesscontextmanager_java_proto", ], ) From bcecb1f345399329d85cb12c6fe6c1d04d881c62 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Wed, 25 Nov 2020 17:41:43 -0800 Subject: [PATCH 3/3] fix: support singleton resource names in parsing and codegen --- .../protoparser/ResourceReferenceParser.java | 23 +- .../ResourceNameHelperClassComposerTest.java | 22 ++ .../gapic/composer/goldens/AgentName.golden | 249 ++++++++++++++++++ .../ResourceReferenceParserTest.java | 10 +- 4 files changed, 289 insertions(+), 15 deletions(-) create mode 100644 src/test/java/com/google/api/generator/gapic/composer/goldens/AgentName.golden 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 68233dc83e..a32b61e318 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 @@ -70,10 +70,8 @@ public static List parseResourceNames( Preconditions.checkNotNull( resourceName, String.format( - "No resource definition found for reference with type %s", - resourceReference.resourceTypeString()) - + "\nDEL: " - + resourceNames.keySet()); + "No resource definition found for reference with type %s", + resourceReference.resourceTypeString())); if (!resourceReference.isChildType() || resourceName.isOnlyWildcard()) { return Arrays.asList(resourceName); } @@ -189,17 +187,20 @@ static Optional parseParentPattern(String pattern) { return Optional.empty(); } + int lastTokenIndex = tokens.length - 2; + int minLengthWithParent = 4; + // Singleton patterns, e.g. projects/{project}/agent. + if (!lastToken.contains("{")) { + minLengthWithParent = 3; + lastTokenIndex = tokens.length - 1; + } + // No fully-formed parent. Expected: ancestors/{ancestor}/childNodes/{child_node}. - if (tokens.length < 4) { + if (tokens.length < minLengthWithParent) { return Optional.empty(); } - Preconditions.checkState( - lastToken.contains("{"), - String.format( - "Pattern %s must end with a brace-encapsulated variable, e.g. {foobar}", pattern)); - - return Optional.of(String.join(SLASH, Arrays.asList(tokens).subList(0, tokens.length - 2))); + return Optional.of(String.join(SLASH, Arrays.asList(tokens).subList(0, lastTokenIndex))); } @VisibleForTesting diff --git a/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java index 5532738d73..acb47db274 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposerTest.java @@ -216,4 +216,26 @@ public void generateResourceNameClass_testingBlueprintPatternWithNonSlashSeparat Path goldenFilePath = Paths.get(ComposerConstants.GOLDENFILES_DIRECTORY, "TestName.golden"); Assert.assertCodeEquals(goldenFilePath, visitor.write()); } + + @Test + public void generateResourceNameClass_childSingleton() { + ResourceName agentResname = + ResourceName.builder() + .setVariableName("agent") + .setPakkage("com.google.cloud.dialogflow.v2beta1") + .setResourceTypeString("dialogflow.googleapis.com/Agent") + .setPatterns( + Arrays.asList( + "projects/{project}/locations/{location}/agent", "projects/{project}/agent")) + .setParentMessageName("Agent") + .setDescription("This is a description") + .build(); + + GapicClass clazz = ResourceNameHelperClassComposer.instance().generate(agentResname); + JavaWriterVisitor visitor = new JavaWriterVisitor(); + clazz.classDefinition().accept(visitor); + Utils.saveCodegenToFile(this.getClass(), "AgentName.golden", visitor.write()); + Path goldenFilePath = Paths.get(ComposerConstants.GOLDENFILES_DIRECTORY, "AgentName.golden"); + Assert.assertCodeEquals(goldenFilePath, visitor.write()); + } } diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/AgentName.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/AgentName.golden new file mode 100644 index 0000000000..c722cdd4f7 --- /dev/null +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/AgentName.golden @@ -0,0 +1,249 @@ +package com.google.cloud.dialogflow.v2beta1; + +import com.google.api.core.BetaApi; +import com.google.api.pathtemplate.PathTemplate; +import com.google.api.pathtemplate.ValidationException; +import com.google.api.resourcenames.ResourceName; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import javax.annotation.Generated; + +// AUTO-GENERATED DOCUMENTATION AND CLASS. +@Generated("by gapic-generator-java") +public class AgentName implements ResourceName { + private static final PathTemplate PROJECT_LOCATION = + PathTemplate.createWithoutUrlEncoding("projects/{project}/locations/{location}/agent"); + private static final PathTemplate PROJECT = + PathTemplate.createWithoutUrlEncoding("projects/{project}/agent"); + private volatile Map fieldValuesMap; + private PathTemplate pathTemplate; + private String fixedValue; + private final String project; + private final String location; + + @Deprecated + protected AgentName() { + project = null; + location = null; + } + + private AgentName(Builder builder) { + project = Preconditions.checkNotNull(builder.getProject()); + location = Preconditions.checkNotNull(builder.getLocation()); + pathTemplate = PROJECT_LOCATION; + } + + private AgentName(ProjectBuilder builder) { + project = Preconditions.checkNotNull(builder.getProject()); + location = null; + pathTemplate = PROJECT; + } + + public String getProject() { + return project; + } + + public String getLocation() { + return location; + } + + public static Builder newBuilder() { + return new Builder(); + } + + @BetaApi("The per-pattern Builders are not stable yet and may be changed in the future.") + public static Builder newProjectLocationBuilder() { + return new Builder(); + } + + @BetaApi("The per-pattern Builders are not stable yet and may be changed in the future.") + public static ProjectBuilder newProjectBuilder() { + return new ProjectBuilder(); + } + + public Builder toBuilder() { + return new Builder(this); + } + + public static AgentName of(String project, String location) { + return newBuilder().setProject(project).setLocation(location).build(); + } + + @BetaApi("The static create methods are not stable yet and may be changed in the future.") + public static AgentName ofProjectLocationName(String project, String location) { + return newBuilder().setProject(project).setLocation(location).build(); + } + + @BetaApi("The static create methods are not stable yet and may be changed in the future.") + public static AgentName ofProjectName(String project) { + return newProjectBuilder().setProject(project).build(); + } + + public static String format(String project, String location) { + return newBuilder().setProject(project).setLocation(location).build().toString(); + } + + @BetaApi("The static format methods are not stable yet and may be changed in the future.") + public static String formatProjectLocationName(String project, String location) { + return newBuilder().setProject(project).setLocation(location).build().toString(); + } + + @BetaApi("The static format methods are not stable yet and may be changed in the future.") + public static String formatProjectName(String project) { + return newProjectBuilder().setProject(project).build().toString(); + } + + public static AgentName parse(String formattedString) { + if (formattedString.isEmpty()) { + return null; + } + if (PROJECT_LOCATION.matches(formattedString)) { + Map matchMap = PROJECT_LOCATION.match(formattedString); + return ofProjectLocationName(matchMap.get("project"), matchMap.get("location")); + } else if (PROJECT.matches(formattedString)) { + Map matchMap = PROJECT.match(formattedString); + return ofProjectName(matchMap.get("project")); + } + throw new ValidationException("AgentName.parse: formattedString not in valid format"); + } + + public static List parseList(List formattedStrings) { + List list = new ArrayList<>(formattedStrings.size()); + for (String formattedString : formattedStrings) { + list.add(parse(formattedString)); + } + return list; + } + + public static List toStringList(List values) { + List list = new ArrayList<>(values.size()); + for (AgentName value : values) { + if (Objects.isNull(value)) { + list.add(""); + } else { + list.add(value.toString()); + } + } + return list; + } + + public static boolean isParsableFrom(String formattedString) { + return PROJECT_LOCATION.matches(formattedString) || PROJECT.matches(formattedString); + } + + @Override + public Map getFieldValuesMap() { + if (Objects.isNull(fieldValuesMap)) { + synchronized (this) { + if (Objects.isNull(fieldValuesMap)) { + ImmutableMap.Builder fieldMapBuilder = ImmutableMap.builder(); + if (!Objects.isNull(project)) { + fieldMapBuilder.put("project", project); + } + if (!Objects.isNull(location)) { + fieldMapBuilder.put("location", location); + } + fieldValuesMap = fieldMapBuilder.build(); + } + } + } + return fieldValuesMap; + } + + public String getFieldValue(String fieldName) { + return getFieldValuesMap().get(fieldName); + } + + @Override + public String toString() { + return !Objects.isNull(fixedValue) ? fixedValue : pathTemplate.instantiate(getFieldValuesMap()); + } + + @Override + public boolean equals(Object o) { + if (o == this) { + return true; + } + if (o != null || getClass() == o.getClass()) { + AgentName that = ((AgentName) o); + return Objects.equals(this.project, that.project) + && Objects.equals(this.location, that.location); + } + return false; + } + + @Override + public int hashCode() { + int h = 1; + h *= 1000003; + h ^= Objects.hashCode(fixedValue); + h *= 1000003; + h ^= Objects.hashCode(project); + h *= 1000003; + h ^= Objects.hashCode(location); + return h; + } + + /** Builder for projects/{project}/locations/{location}/agent. */ + public static class Builder { + private String project; + private String location; + + protected Builder() {} + + public String getProject() { + return project; + } + + public String getLocation() { + return location; + } + + public Builder setProject(String project) { + this.project = project; + return this; + } + + public Builder setLocation(String location) { + this.location = location; + return this; + } + + private Builder(AgentName agentName) { + Preconditions.checkArgument( + Objects.equals(agentName.pathTemplate, PROJECT_LOCATION), + "toBuilder is only supported when AgentName has the pattern of projects/{project}/locations/{location}/agent"); + project = agentName.project; + location = agentName.location; + } + + public AgentName build() { + return new AgentName(this); + } + } + + /** Builder for projects/{project}/agent. */ + @BetaApi("The per-pattern Builders are not stable yet and may be changed in the future.") + public static class ProjectBuilder { + private String project; + + protected ProjectBuilder() {} + + public String getProject() { + return project; + } + + public ProjectBuilder setProject(String project) { + this.project = project; + return this; + } + + public AgentName build() { + return new AgentName(this); + } + } +} 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 c2cae61f88..346431d2c5 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 @@ -113,11 +113,12 @@ public void parseParentResourceName_badPattern() { ResourceReferenceParser.parseParentResourceName( "projects/{project}/billingAccounts", MAIN_PACKAGE, - null, - MAIN_PACKAGE, + "com.google.cloud.billing.v1", "cloudbilling.googleapis.com/Feature", + null, new HashMap()); - assertFalse(parentResourceNameOpt.isPresent()); + assertTrue(parentResourceNameOpt.isPresent()); + assertEquals("projects/{project}", parentResourceNameOpt.get().patterns().get(0)); } @Test @@ -159,7 +160,8 @@ public void parseParentPattern_insufficientPathComponents() { public void parseParentPattern_lastComponentIsNotAVariable() { Optional parentPatternOpt = ResourceReferenceParser.parseParentPattern("projects/{project}/foobars"); - assertFalse(parentPatternOpt.isPresent()); + assertTrue(parentPatternOpt.isPresent()); + assertEquals("projects/{project}", parentPatternOpt.get()); } @Test