From 08eca7de7e80c6f242a23a9d76659e0a413c896f Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Tue, 18 Jul 2023 11:34:43 -0400 Subject: [PATCH] fix: resource name class deduplication (#1854) * chore: add resource name golden from collisions.proto * chore: improve deduplication logic * chore: finish test case * chore: unnecessary proto definitions * chore: format * chore: remove commented line in collisions.proto * chore: simplify deduplication logic * chore: revert changes in TypeStore.java * chore: improve javadoc of `createImplementsTypes` --- .../ResourceNameHelperClassComposer.java | 25 +- .../ResourceNameHelperClassComposerTest.java | 30 ++ .../goldens/CollisionResourceName.golden | 281 ++++++++++++++++++ .../src/test/proto/collisions.proto | 19 +- 4 files changed, 351 insertions(+), 4 deletions(-) create mode 100644 gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/goldens/CollisionResourceName.golden diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposer.java b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposer.java index f8c961ccab..b9dc386a9e 100644 --- a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposer.java +++ b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposer.java @@ -131,7 +131,7 @@ public GapicClass generate(ResourceName resourceName, GapicContext context) { .setAnnotations(createClassAnnotations()) .setScope(ScopeNode.PUBLIC) .setName(className) - .setImplementsTypes(createImplementsTypes()) + .setImplementsTypes(createImplementsTypes(className)) .setStatements( createClassStatements( templateFinalVarExprs, @@ -160,8 +160,27 @@ private static List createClassAnnotations() { .build()); } - private static List createImplementsTypes() { - return Arrays.asList(FIXED_TYPESTORE.get("ResourceName")); + /** + * Returns a singleton list with {@code ResourceName} as its only member. Checks for collisions + * + * @param implementingClassName class that is implementing the resulting list + */ + private static List createImplementsTypes(String implementingClassName) { + // the original resource name reference has useFullName == false + TypeNode originalResourceName = FIXED_TYPESTORE.get("ResourceName"); + if (implementingClassName.equals(originalResourceName.reference().name())) { + // we create a copy with useFullName == true + return Arrays.asList( + TypeNode.withReference( + ConcreteReference.builder() + .setUseFullName(true) + .setClazz(com.google.api.resourcenames.ResourceName.class) + .setGenerics(originalResourceName.reference().generics()) + .setIsStaticImport(originalResourceName.reference().isStaticImport()) + .setWildcardUpperBound(originalResourceName.reference().wildcardUpperBound()) + .build())); + } + return Arrays.asList(originalResourceName); } private static List createTemplateClassMembers( diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposerTest.java b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposerTest.java index f1441f5f78..fa471f0ed2 100644 --- a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposerTest.java +++ b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/ResourceNameHelperClassComposerTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import com.google.api.generator.engine.writer.JavaWriterVisitor; import com.google.api.generator.gapic.model.GapicClass; @@ -34,6 +35,7 @@ import com.google.protobuf.Descriptors.ServiceDescriptor; import com.google.showcase.v1beta1.EchoOuterClass; import com.google.showcase.v1beta1.TestingOuterClass; +import com.google.test.collisions.CollisionsOuterClass; import google.cloud.CommonResources; import java.nio.file.Path; import java.nio.file.Paths; @@ -50,6 +52,9 @@ import org.junit.Test; public class ResourceNameHelperClassComposerTest { + + private final String COLLIDING_RESOURCE_NAME_KEY = "config.googleapis.com/Resource"; + private ServiceDescriptor echoService; private FileDescriptor echoFileDescriptor; @@ -238,4 +243,29 @@ public void generateResourceNameClass_childSingleton() { Paths.get(GoldenFileWriter.getGoldenDir(this.getClass()), "AgentName.golden"); Assert.assertCodeEquals(goldenFilePath, visitor.write()); } + + @Test + public void generateResourceNameClass_resourceNameCollisionIsAvoided() { + ResourceName collidingResourceName = + Parser.parseResourceNames(CollisionsOuterClass.getDescriptor()) + .get(COLLIDING_RESOURCE_NAME_KEY); + + GapicContext irrelevantContext = TestProtoLoader.instance().parseShowcaseEcho(); + GapicClass clazz = + ResourceNameHelperClassComposer.instance() + .generate(collidingResourceName, irrelevantContext); + JavaWriterVisitor visitor = new JavaWriterVisitor(); + clazz.classDefinition().accept(visitor); + GoldenFileWriter.saveCodegenToFile( + this.getClass(), "CollisionResourceName.golden", visitor.write()); + Path goldenFilePath = + Paths.get(GoldenFileWriter.getGoldenDir(this.getClass()), "CollisionResourceName.golden"); + Assert.assertCodeEquals(goldenFilePath, visitor.write()); + + assertEquals(1, clazz.classDefinition().implementsTypes().size()); + assertTrue(clazz.classDefinition().implementsTypes().get(0).reference().useFullName()); + assertEquals( + clazz.classDefinition().classIdentifier().name(), + clazz.classDefinition().implementsTypes().get(0).reference().name()); + } } diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/goldens/CollisionResourceName.golden b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/goldens/CollisionResourceName.golden new file mode 100644 index 0000000000..d28798a6c2 --- /dev/null +++ b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/resourcename/goldens/CollisionResourceName.golden @@ -0,0 +1,281 @@ +package com.google.test.collisions; + +import com.google.api.pathtemplate.PathTemplate; +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 ResourceName implements com.google.api.resourcenames.ResourceName { + private static final PathTemplate PROJECT_LOCATION_DEPLOYMENT_REVISION_RESOURCE = + PathTemplate.createWithoutUrlEncoding( + "projects/{project}/locations/{location}/deployments/{deployment}/revisions/{revision}/resources/{resource}"); + private volatile Map fieldValuesMap; + private final String project; + private final String location; + private final String deployment; + private final String revision; + private final String resource; + + @Deprecated + protected ResourceName() { + project = null; + location = null; + deployment = null; + revision = null; + resource = null; + } + + private ResourceName(Builder builder) { + project = Preconditions.checkNotNull(builder.getProject()); + location = Preconditions.checkNotNull(builder.getLocation()); + deployment = Preconditions.checkNotNull(builder.getDeployment()); + revision = Preconditions.checkNotNull(builder.getRevision()); + resource = Preconditions.checkNotNull(builder.getResource()); + } + + public String getProject() { + return project; + } + + public String getLocation() { + return location; + } + + public String getDeployment() { + return deployment; + } + + public String getRevision() { + return revision; + } + + public String getResource() { + return resource; + } + + public static Builder newBuilder() { + return new Builder(); + } + + public Builder toBuilder() { + return new Builder(this); + } + + public static ResourceName of( + String project, String location, String deployment, String revision, String resource) { + return newBuilder() + .setProject(project) + .setLocation(location) + .setDeployment(deployment) + .setRevision(revision) + .setResource(resource) + .build(); + } + + public static String format( + String project, String location, String deployment, String revision, String resource) { + return newBuilder() + .setProject(project) + .setLocation(location) + .setDeployment(deployment) + .setRevision(revision) + .setResource(resource) + .build() + .toString(); + } + + public static ResourceName parse(String formattedString) { + if (formattedString.isEmpty()) { + return null; + } + Map matchMap = + PROJECT_LOCATION_DEPLOYMENT_REVISION_RESOURCE.validatedMatch( + formattedString, "ResourceName.parse: formattedString not in valid format"); + return of( + matchMap.get("project"), + matchMap.get("location"), + matchMap.get("deployment"), + matchMap.get("revision"), + matchMap.get("resource")); + } + + 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 (ResourceName value : values) { + if (value == null) { + list.add(""); + } else { + list.add(value.toString()); + } + } + return list; + } + + public static boolean isParsableFrom(String formattedString) { + return PROJECT_LOCATION_DEPLOYMENT_REVISION_RESOURCE.matches(formattedString); + } + + @Override + public Map getFieldValuesMap() { + if (fieldValuesMap == null) { + synchronized (this) { + if (fieldValuesMap == null) { + ImmutableMap.Builder fieldMapBuilder = ImmutableMap.builder(); + if (project != null) { + fieldMapBuilder.put("project", project); + } + if (location != null) { + fieldMapBuilder.put("location", location); + } + if (deployment != null) { + fieldMapBuilder.put("deployment", deployment); + } + if (revision != null) { + fieldMapBuilder.put("revision", revision); + } + if (resource != null) { + fieldMapBuilder.put("resource", resource); + } + fieldValuesMap = fieldMapBuilder.build(); + } + } + } + return fieldValuesMap; + } + + public String getFieldValue(String fieldName) { + return getFieldValuesMap().get(fieldName); + } + + @Override + public String toString() { + return PROJECT_LOCATION_DEPLOYMENT_REVISION_RESOURCE.instantiate( + "project", + project, + "location", + location, + "deployment", + deployment, + "revision", + revision, + "resource", + resource); + } + + @Override + public boolean equals(java.lang.Object o) { + if (o == this) { + return true; + } + if (o != null || getClass() == o.getClass()) { + ResourceName that = ((ResourceName) o); + return Objects.equals(this.project, that.project) + && Objects.equals(this.location, that.location) + && Objects.equals(this.deployment, that.deployment) + && Objects.equals(this.revision, that.revision) + && Objects.equals(this.resource, that.resource); + } + return false; + } + + @Override + public int hashCode() { + int h = 1; + h *= 1000003; + h ^= Objects.hashCode(project); + h *= 1000003; + h ^= Objects.hashCode(location); + h *= 1000003; + h ^= Objects.hashCode(deployment); + h *= 1000003; + h ^= Objects.hashCode(revision); + h *= 1000003; + h ^= Objects.hashCode(resource); + return h; + } + + /** + * Builder for + * projects/{project}/locations/{location}/deployments/{deployment}/revisions/{revision}/resources/{resource}. + */ + public static class Builder { + private String project; + private String location; + private String deployment; + private String revision; + private String resource; + + protected Builder() {} + + public String getProject() { + return project; + } + + public String getLocation() { + return location; + } + + public String getDeployment() { + return deployment; + } + + public String getRevision() { + return revision; + } + + public String getResource() { + return resource; + } + + public Builder setProject(String project) { + this.project = project; + return this; + } + + public Builder setLocation(String location) { + this.location = location; + return this; + } + + public Builder setDeployment(String deployment) { + this.deployment = deployment; + return this; + } + + public Builder setRevision(String revision) { + this.revision = revision; + return this; + } + + public Builder setResource(String resource) { + this.resource = resource; + return this; + } + + private Builder(ResourceName resourceName) { + this.project = resourceName.project; + this.location = resourceName.location; + this.deployment = resourceName.deployment; + this.revision = resourceName.revision; + this.resource = resourceName.resource; + } + + public ResourceName build() { + return new ResourceName(this); + } + } +} diff --git a/gapic-generator-java/src/test/proto/collisions.proto b/gapic-generator-java/src/test/proto/collisions.proto index 03c1060d8e..296469c19d 100644 --- a/gapic-generator-java/src/test/proto/collisions.proto +++ b/gapic-generator-java/src/test/proto/collisions.proto @@ -14,8 +14,11 @@ syntax = "proto3"; +import "google/api/annotations.proto"; import "google/api/client.proto"; import "google/longrunning/operations.proto"; +import "google/api/field_behavior.proto"; +import "google/api/resource.proto"; package google.showcase.v1beta1; @@ -69,4 +72,18 @@ message Annotation { message Location { string name = 1; -} \ No newline at end of file +} + +message Resource { + option (google.api.resource) = { + type: "config.googleapis.com/Resource" + pattern: "projects/{project}/locations/{location}/deployments/{deployment}/revisions/{revision}/resources/{resource}" + }; + + // Resource name of the Resource. + // Format: + // `projects/{project}/locations/{location}/deployments/{deployment}/revisions/{revision}/resources/{resource}` + string name = 1 [ + (google.api.field_behavior) = OUTPUT_ONLY + ]; +}