From bb8f2f2d59d121864b2ded15dbc74f71e0478bf0 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Thu, 19 Nov 2020 12:09:40 -0800 Subject: [PATCH 1/3] fix: align resname ctors with superclass variants --- .../ResourceNameHelperClassComposer.java | 32 +++++++++++++++---- .../goldens/BillingAccountLocationName.golden | 8 ++++- .../gapic/composer/goldens/FoobarName.golden | 16 +++++++--- .../gapic/composer/goldens/SessionName.golden | 7 +++- .../gapic/composer/goldens/TestName.golden | 9 +++++- test/integration/goldens/asset/FeedName.java | 14 ++++++-- .../logging/BillingAccountLocationName.java | 8 ++++- .../goldens/logging/BillingAccountName.java | 7 +++- .../goldens/logging/CmekSettingsName.java | 16 +++++++--- .../goldens/logging/FolderLocationName.java | 8 ++++- .../goldens/logging/FolderName.java | 7 +++- .../goldens/logging/LocationName.java | 8 ++++- .../goldens/logging/LogBucketName.java | 18 ++++++++--- .../goldens/logging/LogExclusionName.java | 17 +++++++--- .../goldens/logging/LogMetricName.java | 8 ++++- test/integration/goldens/logging/LogName.java | 17 +++++++--- .../goldens/logging/LogSinkName.java | 17 +++++++--- .../logging/OrganizationLocationName.java | 8 ++++- .../goldens/logging/OrganizationName.java | 7 +++- .../goldens/logging/ProjectName.java | 7 +++- .../goldens/redis/InstanceName.java | 9 +++++- .../goldens/redis/LocationName.java | 8 ++++- 22 files changed, 208 insertions(+), 48 deletions(-) diff --git a/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java index 00c8f03c34..7469af4ec2 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/ResourceNameHelperClassComposer.java @@ -289,6 +289,29 @@ private static List createConstructorMethods( boolean hasVariants = tokenHierarchies.size() > 1; List javaMethods = new ArrayList<>(); + final ValueExpr nullExpr = ValueExpr.withValue(NullObjectValue.create()); + Function assignTokenToNullExpr = + t -> + AssignmentExpr.builder() + .setVariableExpr(patternTokenVarExprs.get(t)) + .setValueExpr(nullExpr) + .build(); + + // First deprecated constructor. + javaMethods.add( + MethodDefinition.constructorBuilder() + .setAnnotations( + Arrays.asList( + AnnotationNode.withType( + TypeNode.withReference(ConcreteReference.withClazz(Deprecated.class))))) + .setScope(ScopeNode.PROTECTED) + .setReturnType(thisClassType) + .setBody( + getTokenSet(tokenHierarchies).stream() + .map(t -> ExprStatement.withExpr(assignTokenToNullExpr.apply(t))) + .collect(Collectors.toList())) + .build()); + for (int i = 0; i < tokenHierarchies.size(); i++) { List tokens = tokenHierarchies.get(i); List bodyExprs = new ArrayList<>(); @@ -316,16 +339,11 @@ private static List createConstructorMethods( .build()); } // Initialize the rest to null. - ValueExpr nullExpr = ValueExpr.withValue(NullObjectValue.create()); for (String token : getTokenSet(tokenHierarchies)) { if (tokens.contains(token)) { continue; } - bodyExprs.add( - AssignmentExpr.builder() - .setVariableExpr(patternTokenVarExprs.get(token)) - .setValueExpr(nullExpr) - .build()); + bodyExprs.add(assignTokenToNullExpr.apply(token)); } if (hasVariants) { @@ -1389,7 +1407,7 @@ private static ClassDefinition createNestedBuilderClass( TypeNode thisClassType = types.get(className); MethodDefinition ctor = MethodDefinition.constructorBuilder() - .setScope(ScopeNode.PRIVATE) + .setScope(ScopeNode.PROTECTED) .setReturnType(thisClassType) .build(); nestedClassMethods.add(ctor); diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/BillingAccountLocationName.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/BillingAccountLocationName.golden index 94fd7c738d..7500be4b9d 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/BillingAccountLocationName.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/BillingAccountLocationName.golden @@ -20,6 +20,12 @@ public class BillingAccountLocationName implements ResourceName { private final String billingAccount; private final String location; + @Deprecated + protected BillingAccountLocationName() { + billingAccount = null; + location = null; + } + private BillingAccountLocationName(Builder builder) { billingAccount = Preconditions.checkNotNull(builder.getBillingAccount()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -141,7 +147,7 @@ public class BillingAccountLocationName implements ResourceName { private String billingAccount; private String location; - private Builder() {} + protected Builder() {} public String getBillingAccount() { return billingAccount; diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/FoobarName.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/FoobarName.golden index 7f693a446a..9823f297c5 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/FoobarName.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/FoobarName.golden @@ -32,6 +32,14 @@ public class FoobarName implements ResourceName { private final String variant; private final String barFoo; + @Deprecated + protected FoobarName() { + project = null; + foobar = null; + variant = null; + barFoo = null; + } + private FoobarName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); foobar = Preconditions.checkNotNull(builder.getFoobar()); @@ -285,7 +293,7 @@ public class FoobarName implements ResourceName { private String project; private String foobar; - private Builder() {} + protected Builder() {} public String getProject() { return project; @@ -325,7 +333,7 @@ public class FoobarName implements ResourceName { private String variant; private String foobar; - private ProjectVariantFoobarBuilder() {} + protected ProjectVariantFoobarBuilder() {} public String getProject() { return project; @@ -364,7 +372,7 @@ public class FoobarName implements ResourceName { public static class FoobarBuilder { private String foobar; - private FoobarBuilder() {} + protected FoobarBuilder() {} public String getFoobar() { return foobar; @@ -386,7 +394,7 @@ public class FoobarName implements ResourceName { private String barFoo; private String foobar; - private BarFooFoobarBuilder() {} + protected BarFooFoobarBuilder() {} public String getBarFoo() { return barFoo; diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/SessionName.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/SessionName.golden index b17c2718c1..e0ef56bae0 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/SessionName.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/SessionName.golden @@ -18,6 +18,11 @@ public class SessionName implements ResourceName { private volatile Map fieldValuesMap; private final String session; + @Deprecated + protected SessionName() { + session = null; + } + private SessionName(Builder builder) { session = Preconditions.checkNotNull(builder.getSession()); } @@ -125,7 +130,7 @@ public class SessionName implements ResourceName { public static class Builder { private String session; - private Builder() {} + protected Builder() {} public String getSession() { return session; diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestName.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestName.golden index f4a72ec1ad..af723a66c3 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestName.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestName.golden @@ -20,6 +20,13 @@ public class TestName implements ResourceName { private final String shardId; private final String testId; + @Deprecated + protected TestName() { + session = null; + shardId = null; + testId = null; + } + private TestName(Builder builder) { session = Preconditions.checkNotNull(builder.getSession()); shardId = Preconditions.checkNotNull(builder.getShardId()); @@ -157,7 +164,7 @@ public class TestName implements ResourceName { private String shardId; private String testId; - private Builder() {} + protected Builder() {} public String getSession() { return session; diff --git a/test/integration/goldens/asset/FeedName.java b/test/integration/goldens/asset/FeedName.java index f05af8f2e6..3d76da97b4 100644 --- a/test/integration/goldens/asset/FeedName.java +++ b/test/integration/goldens/asset/FeedName.java @@ -45,6 +45,14 @@ public class FeedName implements ResourceName { private final String folder; private final String organization; + @Deprecated + protected FeedName() { + project = null; + feed = null; + folder = null; + organization = null; + } + private FeedName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); feed = Preconditions.checkNotNull(builder.getFeed()); @@ -263,7 +271,7 @@ public static class Builder { private String project; private String feed; - private Builder() {} + protected Builder() {} public String getProject() { return project; @@ -302,7 +310,7 @@ public static class FolderFeedBuilder { private String folder; private String feed; - private FolderFeedBuilder() {} + protected FolderFeedBuilder() {} public String getFolder() { return folder; @@ -333,7 +341,7 @@ public static class OrganizationFeedBuilder { private String organization; private String feed; - private OrganizationFeedBuilder() {} + protected OrganizationFeedBuilder() {} public String getOrganization() { return organization; diff --git a/test/integration/goldens/logging/BillingAccountLocationName.java b/test/integration/goldens/logging/BillingAccountLocationName.java index ead5f6cdec..deaf554a9f 100644 --- a/test/integration/goldens/logging/BillingAccountLocationName.java +++ b/test/integration/goldens/logging/BillingAccountLocationName.java @@ -36,6 +36,12 @@ public class BillingAccountLocationName implements ResourceName { private final String billingAccount; private final String location; + @Deprecated + protected BillingAccountLocationName() { + billingAccount = null; + location = null; + } + private BillingAccountLocationName(Builder builder) { billingAccount = Preconditions.checkNotNull(builder.getBillingAccount()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -157,7 +163,7 @@ public static class Builder { private String billingAccount; private String location; - private Builder() {} + protected Builder() {} public String getBillingAccount() { return billingAccount; diff --git a/test/integration/goldens/logging/BillingAccountName.java b/test/integration/goldens/logging/BillingAccountName.java index aac4c70071..d9667ac9c6 100644 --- a/test/integration/goldens/logging/BillingAccountName.java +++ b/test/integration/goldens/logging/BillingAccountName.java @@ -34,6 +34,11 @@ public class BillingAccountName implements ResourceName { private volatile Map fieldValuesMap; private final String billingAccount; + @Deprecated + protected BillingAccountName() { + billingAccount = null; + } + private BillingAccountName(Builder builder) { billingAccount = Preconditions.checkNotNull(builder.getBillingAccount()); } @@ -141,7 +146,7 @@ public int hashCode() { public static class Builder { private String billingAccount; - private Builder() {} + protected Builder() {} public String getBillingAccount() { return billingAccount; diff --git a/test/integration/goldens/logging/CmekSettingsName.java b/test/integration/goldens/logging/CmekSettingsName.java index e90b6428a1..a3125e93e9 100644 --- a/test/integration/goldens/logging/CmekSettingsName.java +++ b/test/integration/goldens/logging/CmekSettingsName.java @@ -47,6 +47,14 @@ public class CmekSettingsName implements ResourceName { private final String folder; private final String billingAccount; + @Deprecated + protected CmekSettingsName() { + project = null; + organization = null; + folder = null; + billingAccount = null; + } + private CmekSettingsName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); organization = null; @@ -287,7 +295,7 @@ public int hashCode() { public static class Builder { private String project; - private Builder() {} + protected Builder() {} public String getProject() { return project; @@ -315,7 +323,7 @@ public CmekSettingsName build() { public static class OrganizationBuilder { private String organization; - private OrganizationBuilder() {} + protected OrganizationBuilder() {} public String getOrganization() { return organization; @@ -336,7 +344,7 @@ public CmekSettingsName build() { public static class FolderBuilder { private String folder; - private FolderBuilder() {} + protected FolderBuilder() {} public String getFolder() { return folder; @@ -357,7 +365,7 @@ public CmekSettingsName build() { public static class BillingAccountBuilder { private String billingAccount; - private BillingAccountBuilder() {} + protected BillingAccountBuilder() {} public String getBillingAccount() { return billingAccount; diff --git a/test/integration/goldens/logging/FolderLocationName.java b/test/integration/goldens/logging/FolderLocationName.java index 24d56ee303..afde415244 100644 --- a/test/integration/goldens/logging/FolderLocationName.java +++ b/test/integration/goldens/logging/FolderLocationName.java @@ -35,6 +35,12 @@ public class FolderLocationName implements ResourceName { private final String folder; private final String location; + @Deprecated + protected FolderLocationName() { + folder = null; + location = null; + } + private FolderLocationName(Builder builder) { folder = Preconditions.checkNotNull(builder.getFolder()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -154,7 +160,7 @@ public static class Builder { private String folder; private String location; - private Builder() {} + protected Builder() {} public String getFolder() { return folder; diff --git a/test/integration/goldens/logging/FolderName.java b/test/integration/goldens/logging/FolderName.java index 8f0c022439..20940167a0 100644 --- a/test/integration/goldens/logging/FolderName.java +++ b/test/integration/goldens/logging/FolderName.java @@ -34,6 +34,11 @@ public class FolderName implements ResourceName { private volatile Map fieldValuesMap; private final String folder; + @Deprecated + protected FolderName() { + folder = null; + } + private FolderName(Builder builder) { folder = Preconditions.checkNotNull(builder.getFolder()); } @@ -141,7 +146,7 @@ public int hashCode() { public static class Builder { private String folder; - private Builder() {} + protected Builder() {} public String getFolder() { return folder; diff --git a/test/integration/goldens/logging/LocationName.java b/test/integration/goldens/logging/LocationName.java index 9bc33a8940..ab2a8319ea 100644 --- a/test/integration/goldens/logging/LocationName.java +++ b/test/integration/goldens/logging/LocationName.java @@ -35,6 +35,12 @@ public class LocationName implements ResourceName { private final String project; private final String location; + @Deprecated + protected LocationName() { + project = null; + location = null; + } + private LocationName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -154,7 +160,7 @@ public static class Builder { private String project; private String location; - private Builder() {} + protected Builder() {} public String getProject() { return project; diff --git a/test/integration/goldens/logging/LogBucketName.java b/test/integration/goldens/logging/LogBucketName.java index cb94fab601..32d0b934c4 100644 --- a/test/integration/goldens/logging/LogBucketName.java +++ b/test/integration/goldens/logging/LogBucketName.java @@ -53,6 +53,16 @@ public class LogBucketName implements ResourceName { private final String folder; private final String billingAccount; + @Deprecated + protected LogBucketName() { + project = null; + location = null; + bucket = null; + organization = null; + folder = null; + billingAccount = null; + } + private LogBucketName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -372,7 +382,7 @@ public static class Builder { private String location; private String bucket; - private Builder() {} + protected Builder() {} public String getProject() { return project; @@ -422,7 +432,7 @@ public static class OrganizationLocationBucketBuilder { private String location; private String bucket; - private OrganizationLocationBucketBuilder() {} + protected OrganizationLocationBucketBuilder() {} public String getOrganization() { return organization; @@ -463,7 +473,7 @@ public static class FolderLocationBucketBuilder { private String location; private String bucket; - private FolderLocationBucketBuilder() {} + protected FolderLocationBucketBuilder() {} public String getFolder() { return folder; @@ -504,7 +514,7 @@ public static class BillingAccountLocationBucketBuilder { private String location; private String bucket; - private BillingAccountLocationBucketBuilder() {} + protected BillingAccountLocationBucketBuilder() {} public String getBillingAccount() { return billingAccount; diff --git a/test/integration/goldens/logging/LogExclusionName.java b/test/integration/goldens/logging/LogExclusionName.java index 8c0bc46049..037561fda3 100644 --- a/test/integration/goldens/logging/LogExclusionName.java +++ b/test/integration/goldens/logging/LogExclusionName.java @@ -49,6 +49,15 @@ public class LogExclusionName implements ResourceName { private final String folder; private final String billingAccount; + @Deprecated + protected LogExclusionName() { + project = null; + exclusion = null; + organization = null; + folder = null; + billingAccount = null; + } + private LogExclusionName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); exclusion = Preconditions.checkNotNull(builder.getExclusion()); @@ -321,7 +330,7 @@ public static class Builder { private String project; private String exclusion; - private Builder() {} + protected Builder() {} public String getProject() { return project; @@ -360,7 +369,7 @@ public static class OrganizationExclusionBuilder { private String organization; private String exclusion; - private OrganizationExclusionBuilder() {} + protected OrganizationExclusionBuilder() {} public String getOrganization() { return organization; @@ -391,7 +400,7 @@ public static class FolderExclusionBuilder { private String folder; private String exclusion; - private FolderExclusionBuilder() {} + protected FolderExclusionBuilder() {} public String getFolder() { return folder; @@ -422,7 +431,7 @@ public static class BillingAccountExclusionBuilder { private String billingAccount; private String exclusion; - private BillingAccountExclusionBuilder() {} + protected BillingAccountExclusionBuilder() {} public String getBillingAccount() { return billingAccount; diff --git a/test/integration/goldens/logging/LogMetricName.java b/test/integration/goldens/logging/LogMetricName.java index 1d7150abad..2b50476244 100644 --- a/test/integration/goldens/logging/LogMetricName.java +++ b/test/integration/goldens/logging/LogMetricName.java @@ -35,6 +35,12 @@ public class LogMetricName implements ResourceName { private final String project; private final String metric; + @Deprecated + protected LogMetricName() { + project = null; + metric = null; + } + private LogMetricName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); metric = Preconditions.checkNotNull(builder.getMetric()); @@ -153,7 +159,7 @@ public static class Builder { private String project; private String metric; - private Builder() {} + protected Builder() {} public String getProject() { return project; diff --git a/test/integration/goldens/logging/LogName.java b/test/integration/goldens/logging/LogName.java index b4f4cea34e..7a1e540e8f 100644 --- a/test/integration/goldens/logging/LogName.java +++ b/test/integration/goldens/logging/LogName.java @@ -48,6 +48,15 @@ public class LogName implements ResourceName { private final String folder; private final String billingAccount; + @Deprecated + protected LogName() { + project = null; + log = null; + organization = null; + folder = null; + billingAccount = null; + } + private LogName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); log = Preconditions.checkNotNull(builder.getLog()); @@ -307,7 +316,7 @@ public static class Builder { private String project; private String log; - private Builder() {} + protected Builder() {} public String getProject() { return project; @@ -346,7 +355,7 @@ public static class OrganizationLogBuilder { private String organization; private String log; - private OrganizationLogBuilder() {} + protected OrganizationLogBuilder() {} public String getOrganization() { return organization; @@ -377,7 +386,7 @@ public static class FolderLogBuilder { private String folder; private String log; - private FolderLogBuilder() {} + protected FolderLogBuilder() {} public String getFolder() { return folder; @@ -408,7 +417,7 @@ public static class BillingAccountLogBuilder { private String billingAccount; private String log; - private BillingAccountLogBuilder() {} + protected BillingAccountLogBuilder() {} public String getBillingAccount() { return billingAccount; diff --git a/test/integration/goldens/logging/LogSinkName.java b/test/integration/goldens/logging/LogSinkName.java index b2211390dc..c078226190 100644 --- a/test/integration/goldens/logging/LogSinkName.java +++ b/test/integration/goldens/logging/LogSinkName.java @@ -48,6 +48,15 @@ public class LogSinkName implements ResourceName { private final String folder; private final String billingAccount; + @Deprecated + protected LogSinkName() { + project = null; + sink = null; + organization = null; + folder = null; + billingAccount = null; + } + private LogSinkName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); sink = Preconditions.checkNotNull(builder.getSink()); @@ -311,7 +320,7 @@ public static class Builder { private String project; private String sink; - private Builder() {} + protected Builder() {} public String getProject() { return project; @@ -350,7 +359,7 @@ public static class OrganizationSinkBuilder { private String organization; private String sink; - private OrganizationSinkBuilder() {} + protected OrganizationSinkBuilder() {} public String getOrganization() { return organization; @@ -381,7 +390,7 @@ public static class FolderSinkBuilder { private String folder; private String sink; - private FolderSinkBuilder() {} + protected FolderSinkBuilder() {} public String getFolder() { return folder; @@ -412,7 +421,7 @@ public static class BillingAccountSinkBuilder { private String billingAccount; private String sink; - private BillingAccountSinkBuilder() {} + protected BillingAccountSinkBuilder() {} public String getBillingAccount() { return billingAccount; diff --git a/test/integration/goldens/logging/OrganizationLocationName.java b/test/integration/goldens/logging/OrganizationLocationName.java index 2f9cde61d8..f01aea1f66 100644 --- a/test/integration/goldens/logging/OrganizationLocationName.java +++ b/test/integration/goldens/logging/OrganizationLocationName.java @@ -35,6 +35,12 @@ public class OrganizationLocationName implements ResourceName { private final String organization; private final String location; + @Deprecated + protected OrganizationLocationName() { + organization = null; + location = null; + } + private OrganizationLocationName(Builder builder) { organization = Preconditions.checkNotNull(builder.getOrganization()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -154,7 +160,7 @@ public static class Builder { private String organization; private String location; - private Builder() {} + protected Builder() {} public String getOrganization() { return organization; diff --git a/test/integration/goldens/logging/OrganizationName.java b/test/integration/goldens/logging/OrganizationName.java index a8860f3e9c..fdc5f8eaa3 100644 --- a/test/integration/goldens/logging/OrganizationName.java +++ b/test/integration/goldens/logging/OrganizationName.java @@ -34,6 +34,11 @@ public class OrganizationName implements ResourceName { private volatile Map fieldValuesMap; private final String organization; + @Deprecated + protected OrganizationName() { + organization = null; + } + private OrganizationName(Builder builder) { organization = Preconditions.checkNotNull(builder.getOrganization()); } @@ -141,7 +146,7 @@ public int hashCode() { public static class Builder { private String organization; - private Builder() {} + protected Builder() {} public String getOrganization() { return organization; diff --git a/test/integration/goldens/logging/ProjectName.java b/test/integration/goldens/logging/ProjectName.java index 2003259874..58101d3f7a 100644 --- a/test/integration/goldens/logging/ProjectName.java +++ b/test/integration/goldens/logging/ProjectName.java @@ -34,6 +34,11 @@ public class ProjectName implements ResourceName { private volatile Map fieldValuesMap; private final String project; + @Deprecated + protected ProjectName() { + project = null; + } + private ProjectName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); } @@ -141,7 +146,7 @@ public int hashCode() { public static class Builder { private String project; - private Builder() {} + protected Builder() {} public String getProject() { return project; diff --git a/test/integration/goldens/redis/InstanceName.java b/test/integration/goldens/redis/InstanceName.java index ac6e5b1ce5..56f2d728b1 100644 --- a/test/integration/goldens/redis/InstanceName.java +++ b/test/integration/goldens/redis/InstanceName.java @@ -37,6 +37,13 @@ public class InstanceName implements ResourceName { private final String location; private final String instance; + @Deprecated + protected InstanceName() { + project = null; + location = null; + instance = null; + } + private InstanceName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -174,7 +181,7 @@ public static class Builder { private String location; private String instance; - private Builder() {} + protected Builder() {} public String getProject() { return project; diff --git a/test/integration/goldens/redis/LocationName.java b/test/integration/goldens/redis/LocationName.java index 89cad0d585..4f2c953ed7 100644 --- a/test/integration/goldens/redis/LocationName.java +++ b/test/integration/goldens/redis/LocationName.java @@ -35,6 +35,12 @@ public class LocationName implements ResourceName { private final String project; private final String location; + @Deprecated + protected LocationName() { + project = null; + location = null; + } + private LocationName(Builder builder) { project = Preconditions.checkNotNull(builder.getProject()); location = Preconditions.checkNotNull(builder.getLocation()); @@ -154,7 +160,7 @@ public static class Builder { private String project; private String location; - private Builder() {} + protected Builder() {} public String getProject() { return project; From 7336db7fa27488e24642506705aa9c4d3dc510a6 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Thu, 19 Nov 2020 17:06:47 -0800 Subject: [PATCH 2/3] fix: avoid type collisions - refactor {Concrete,Vapor}Ref into AstNodes --- .../generator/engine/ast/AstNodeVisitor.java | 4 + .../engine/ast/ConcreteReference.java | 9 ++ .../api/generator/engine/ast/Reference.java | 4 +- .../generator/engine/ast/VaporReference.java | 5 + .../engine/writer/ImportWriterVisitor.java | 101 ++++++++++++------ .../engine/writer/JavaWriterVisitor.java | 80 ++++++++++++-- .../composer/goldens/TestingClientTest.golden | 18 ++-- 7 files changed, 170 insertions(+), 51 deletions(-) diff --git a/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java b/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java index 14f75eb452..b13213a311 100644 --- a/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java +++ b/src/main/java/com/google/api/generator/engine/ast/AstNodeVisitor.java @@ -24,6 +24,10 @@ public interface AstNodeVisitor { public void visit(AnnotationNode annotation); + public void visit(ConcreteReference reference); + + public void visit(VaporReference reference); + /** =============================== EXPRESSIONS =============================== */ public void visit(ValueExpr valueExpr); diff --git a/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java b/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java index 64f34d1927..4cf5728648 100644 --- a/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java +++ b/src/main/java/com/google/api/generator/engine/ast/ConcreteReference.java @@ -37,6 +37,11 @@ public abstract class ConcreteReference implements Reference { // Private. abstract Class clazz(); + @Override + public void accept(AstNodeVisitor visitor) { + visitor.visit(this); + } + @Nullable @Override public abstract Reference wildcardUpperBound(); @@ -188,6 +193,10 @@ public Reference copyAndSetGenerics(List generics) { return toBuilder().setGenerics(generics).build(); } + public String simpleName() { + return clazz().getSimpleName(); + } + public static ConcreteReference withClazz(Class clazz) { return builder().setClazz(clazz).build(); } diff --git a/src/main/java/com/google/api/generator/engine/ast/Reference.java b/src/main/java/com/google/api/generator/engine/ast/Reference.java index 7031b5cbe6..a0b9325f5d 100644 --- a/src/main/java/com/google/api/generator/engine/ast/Reference.java +++ b/src/main/java/com/google/api/generator/engine/ast/Reference.java @@ -18,7 +18,9 @@ import java.util.List; import javax.annotation.Nullable; -public interface Reference { +public interface Reference extends AstNode { + void accept(AstNodeVisitor visitor); + ImmutableList generics(); String name(); diff --git a/src/main/java/com/google/api/generator/engine/ast/VaporReference.java b/src/main/java/com/google/api/generator/engine/ast/VaporReference.java index 0461fabab4..a522582734 100644 --- a/src/main/java/com/google/api/generator/engine/ast/VaporReference.java +++ b/src/main/java/com/google/api/generator/engine/ast/VaporReference.java @@ -29,6 +29,11 @@ public abstract class VaporReference implements Reference { private static final String RIGHT_ANGLE = ">"; private static final String COMMA = ", "; + @Override + public void accept(AstNodeVisitor visitor) { + visitor.visit(this); + } + @Override public abstract ImmutableList generics(); diff --git a/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java b/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java index 557c43c447..a6a5b1bc8f 100644 --- a/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java +++ b/src/main/java/com/google/api/generator/engine/writer/ImportWriterVisitor.java @@ -26,6 +26,7 @@ import com.google.api.generator.engine.ast.CastExpr; import com.google.api.generator.engine.ast.ClassDefinition; import com.google.api.generator.engine.ast.CommentStatement; +import com.google.api.generator.engine.ast.ConcreteReference; import com.google.api.generator.engine.ast.EmptyLineStatement; import com.google.api.generator.engine.ast.EnumRefExpr; import com.google.api.generator.engine.ast.Expr; @@ -55,6 +56,7 @@ import com.google.api.generator.engine.ast.TypeNode; import com.google.api.generator.engine.ast.UnaryOperationExpr; import com.google.api.generator.engine.ast.ValueExpr; +import com.google.api.generator.engine.ast.VaporReference; import com.google.api.generator.engine.ast.VariableExpr; import com.google.api.generator.engine.ast.WhileStatement; import com.google.common.base.Preconditions; @@ -65,6 +67,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -76,6 +79,9 @@ public class ImportWriterVisitor implements AstNodeVisitor { private final Set staticImports = new TreeSet<>(); private final Set imports = new TreeSet<>(); + // Cache the list of short names, since it will be used relatively often. + private final Set importShortNames = new TreeSet<>(); + private String currentPackage; @Nullable private String currentClassName; @@ -109,6 +115,24 @@ public String write() { return sb.toString(); } + public boolean collidesWithImport(String pakkage, String shortName) { + // This is a sufficiently-good heuristic since it's unlikely that the AST structure has changed + // if the size is the same. + if (importShortNames.size() != imports.size()) { + importShortNames.clear(); + importShortNames.addAll( + imports.stream() + .map(s -> s.substring(s.lastIndexOf(DOT) + 1)) + .collect(Collectors.toSet())); + } + return importShortNames.contains(shortName) + && imports.stream() + .filter(s -> s.equals(String.format("%s.%s", pakkage, shortName))) + .findFirst() + .orElse("") + .isEmpty(); + } + @Override public void visit(IdentifierNode identifier) { // Nothing to do. @@ -127,6 +151,16 @@ public void visit(TypeNode type) { references(refs); } + @Override + public void visit(ConcreteReference reference) { + handleReference(reference); + } + + @Override + public void visit(VaporReference reference) { + handleReference(reference); + } + @Override public void visit(ScopeNode scope) { // Nothing to do. @@ -410,44 +444,49 @@ private void variableExpressions(List expressions) { } } - private void references(List refs) { - for (Reference ref : refs) { - // Don't need to import this. - if (ref.useFullName()) { - continue; - } - if (!ref.isStaticImport() - && (ref.isFromPackage(PKG_JAVA_LANG) || ref.isFromPackage(currentPackage))) { - continue; - } + private void handleReference(Reference reference) { + // Don't need to import this. + if (reference.useFullName()) { + return; + } + if (!reference.isStaticImport() + && (reference.isFromPackage(PKG_JAVA_LANG) || reference.isFromPackage(currentPackage))) { + return; + } - if (ref.isWildcard()) { - if (ref.wildcardUpperBound() != null) { - references(Arrays.asList(ref.wildcardUpperBound())); - } - continue; + if (reference.isWildcard()) { + if (reference.wildcardUpperBound() != null) { + references(Arrays.asList(reference.wildcardUpperBound())); } + return; + } - if (ref.isStaticImport() - && !Strings.isNullOrEmpty(currentClassName) - && !ref.enclosingClassNames().isEmpty() - && ref.enclosingClassNames().contains(currentClassName)) { - continue; - } + if (reference.isStaticImport() + && !Strings.isNullOrEmpty(currentClassName) + && !reference.enclosingClassNames().isEmpty() + && reference.enclosingClassNames().contains(currentClassName)) { + return; + } - if (ref.isStaticImport()) { - // This is a static import. - staticImports.add(ref.fullName()); + if (reference.isStaticImport()) { + // This is a static import. + staticImports.add(reference.fullName()); + } else { + if (reference.hasEnclosingClass()) { + imports.add( + String.format( + "%s.%s", reference.pakkage(), String.join(DOT, reference.enclosingClassNames()))); } else { - if (ref.hasEnclosingClass()) { - imports.add( - String.format("%s.%s", ref.pakkage(), String.join(DOT, ref.enclosingClassNames()))); - } else { - imports.add(ref.fullName()); - } + imports.add(reference.fullName()); } + } - references(ref.generics()); + references(reference.generics()); + } + + private void references(List refs) { + for (Reference ref : refs) { + ref.accept(this); } } diff --git a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java index 77801d69cd..f2984f36ad 100644 --- a/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java +++ b/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java @@ -26,6 +26,7 @@ import com.google.api.generator.engine.ast.CastExpr; import com.google.api.generator.engine.ast.ClassDefinition; import com.google.api.generator.engine.ast.CommentStatement; +import com.google.api.generator.engine.ast.ConcreteReference; import com.google.api.generator.engine.ast.EmptyLineStatement; import com.google.api.generator.engine.ast.EnumRefExpr; import com.google.api.generator.engine.ast.Expr; @@ -43,6 +44,7 @@ import com.google.api.generator.engine.ast.NewObjectExpr; import com.google.api.generator.engine.ast.OperatorKind; import com.google.api.generator.engine.ast.PackageInfoDefinition; +import com.google.api.generator.engine.ast.Reference; import com.google.api.generator.engine.ast.ReferenceConstructorExpr; import com.google.api.generator.engine.ast.RelationalOperationExpr; import com.google.api.generator.engine.ast.ReturnExpr; @@ -56,6 +58,7 @@ import com.google.api.generator.engine.ast.TypeNode.TypeKind; import com.google.api.generator.engine.ast.UnaryOperationExpr; import com.google.api.generator.engine.ast.ValueExpr; +import com.google.api.generator.engine.ast.VaporReference; import com.google.api.generator.engine.ast.Variable; import com.google.api.generator.engine.ast.VariableExpr; import com.google.api.generator.engine.ast.WhileStatement; @@ -145,23 +148,15 @@ public void visit(IdentifierNode identifier) { @Override public void visit(TypeNode type) { TypeKind typeKind = type.typeKind(); - StringBuilder generatedCodeBuilder = new StringBuilder(); if (type.isPrimitiveType()) { - generatedCodeBuilder.append(typeKind.toString().toLowerCase()); + buffer.append(typeKind.toString().toLowerCase()); } else { - if (type.reference().useFullName()) { - generatedCodeBuilder.append(type.reference().pakkage()); - generatedCodeBuilder.append(DOT); - } - // A null pointer exception will be thrown if reference is null, which is WAI. - generatedCodeBuilder.append(type.reference().name()); + type.reference().accept(this); } if (type.isArray()) { - generatedCodeBuilder.append("[]"); + buffer.append("[]"); } - - buffer.append(generatedCodeBuilder.toString()); } @Override @@ -181,6 +176,68 @@ public void visit(AnnotationNode annotation) { newline(); } + @Override + public void visit(ConcreteReference reference) { + if (reference.isWildcard()) { + buffer.append(QUESTION_MARK); + if (reference.wildcardUpperBound() != null) { + // Handle the upper bound. + buffer.append(SPACE); + buffer.append(EXTENDS); + buffer.append(SPACE); + reference.wildcardUpperBound().accept(this); + } + return; + } + String pakkage = reference.pakkage(); + String shortName = reference.name(); + if (reference.useFullName() || importWriterVisitor.collidesWithImport(pakkage, shortName)) { + buffer.append(pakkage); + buffer.append(DOT); + } + + if (reference.hasEnclosingClass() && !reference.isStaticImport()) { + buffer.append(String.join(DOT, reference.enclosingClassNames())); + buffer.append(DOT); + } + + buffer.append(reference.simpleName()); + + if (!reference.generics().isEmpty()) { + buffer.append(LEFT_ANGLE); + for (int i = 0; i < reference.generics().size(); i++) { + Reference r = reference.generics().get(i); + r.accept(this); + if (i < reference.generics().size() - 1) { + buffer.append(COMMA); + buffer.append(SPACE); + } + } + buffer.append(RIGHT_ANGLE); + } + } + + @Override + public void visit(VaporReference reference) { + // This implementation should be more extensive, but there are no existing use cases that + // exercise the edge cases. + // TODO(miraleung): Give this behavioral parity with ConcreteReference. + String pakkage = reference.pakkage(); + String shortName = reference.name(); + + if (reference.useFullName() || importWriterVisitor.collidesWithImport(pakkage, shortName)) { + buffer.append(pakkage); + buffer.append(DOT); + if (reference.hasEnclosingClass()) { + buffer.append(String.join(DOT, reference.enclosingClassNames())); + buffer.append(DOT); + } + } + + // A null pointer exception will be thrown if reference is null, which is WAI. + buffer.append(shortName); + } + /** =============================== EXPRESSIONS =============================== */ @Override public void visit(ValueExpr valueExpr) { @@ -798,6 +855,7 @@ public void visit(ClassDefinition classDefinition) { newline(); } + // This must go first, so that we can check for type collisions. classDefinition.accept(importWriterVisitor); if (!classDefinition.isNested()) { buffer.append(importWriterVisitor.write()); diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden index f1bdaeeff7..cae284d0c9 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden @@ -281,8 +281,8 @@ public class TestingClientTest { @Test public void getTestTest() throws Exception { - Test expectedResponse = - Test.newBuilder() + com.google.showcase.v1beta1.Test expectedResponse = + com.google.showcase.v1beta1.Test.newBuilder() .setName(TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]").toString()) .setDescription("description-1724546052") .build(); @@ -290,7 +290,7 @@ public class TestingClientTest { TestName name = TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]"); - Test actualResponse = client.getTest(name); + com.google.showcase.v1beta1.Test actualResponse = client.getTest(name); Assert.assertEquals(expectedResponse, actualResponse); List actualRequests = mockTesting.getRequests(); @@ -320,8 +320,8 @@ public class TestingClientTest { @Test public void getTestTest2() throws Exception { - Test expectedResponse = - Test.newBuilder() + com.google.showcase.v1beta1.Test expectedResponse = + com.google.showcase.v1beta1.Test.newBuilder() .setName(TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]").toString()) .setDescription("description-1724546052") .build(); @@ -329,7 +329,7 @@ public class TestingClientTest { String name = "name3373707"; - Test actualResponse = client.getTest(name); + com.google.showcase.v1beta1.Test actualResponse = client.getTest(name); Assert.assertEquals(expectedResponse, actualResponse); List actualRequests = mockTesting.getRequests(); @@ -359,7 +359,8 @@ public class TestingClientTest { @Test public void listTestsTest() throws Exception { - Test responsesElement = Test.newBuilder().build(); + com.google.showcase.v1beta1.Test responsesElement = + com.google.showcase.v1beta1.Test.newBuilder().build(); ListTestsResponse expectedResponse = ListTestsResponse.newBuilder() .setNextPageToken("") @@ -376,7 +377,8 @@ public class TestingClientTest { ListTestsPagedResponse pagedListResponse = client.listTests(request); - List resources = Lists.newArrayList(pagedListResponse.iterateAll()); + List resources = + Lists.newArrayList(pagedListResponse.iterateAll()); Assert.assertEquals(1, resources.size()); Assert.assertEquals(expectedResponse.getTestsList().get(0), resources.get(0)); From 2f7c8571cac663c92bb0b7800d5b264e54dda3e0 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Thu, 19 Nov 2020 17:46:13 -0800 Subject: [PATCH 3/3] fix: better message null checks in Parser --- .../google/api/generator/gapic/protoparser/Parser.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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 eea185b777..861504b14d 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 @@ -344,9 +344,11 @@ static List parseMethods( } } + Message inputMessage = messageTypes.get(inputType.reference().name()); + Preconditions.checkNotNull( + inputMessage, String.format("No message found for %s", inputType.reference().name())); Optional> httpBindingsOpt = - HttpRuleParser.parseHttpBindings( - protoMethod, messageTypes.get(inputType.reference().name()), messageTypes); + HttpRuleParser.parseHttpBindings(protoMethod, inputMessage, messageTypes); List httpBindings = httpBindingsOpt.isPresent() ? httpBindingsOpt.get() : Collections.emptyList(); @@ -370,8 +372,7 @@ static List parseMethods( .setIsPaged(parseIsPaged(protoMethod, messageTypes)) .build()); - // Any input type that has a resource reference will need a resource name helper class. - Message inputMessage = messageTypes.get(inputType.reference().name()); + // Any input type that has a resource reference will need a resource name helper calss. for (Field field : inputMessage.fields()) { if (field.hasResourceReference()) { String resourceTypeString = field.resourceReference().resourceTypeString();