From 109bc93ff84ae50da0f6f10ffe03bbd84f53ed8f Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 24 Nov 2020 01:20:08 -0800 Subject: [PATCH] [ggj][resnames][codegen] fix: add default value fallback for wildcard-only resnames (#562) * feat: add parsing for language_settings in gapic.yaml * fix: restructure GapicServiceConfig table-processing into ctor * fix: refactor {LRO,Batching} settings out of GapicServiceConfig's required creation params * build: support file deletion in integration test update Bazel rules * feat: support gapic.yaml Java name overrides for Service{Client,Settings} * fix: use gapic.yaml override names in comments * fix: Use the right file * fix: add default value fallback for wildcard-only resnames --- .../gapic/composer/DefaultValueComposer.java | 23 +++++++++------- .../composer/DefaultValueComposerTest.java | 27 ++++++++++++------- 2 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java b/src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java index 98143f5574..a8d89f3932 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/DefaultValueComposer.java @@ -65,7 +65,9 @@ static Expr createDefaultValue( "No resource name found for reference %s", methodArg.field().resourceReference().resourceTypeString())); return createDefaultValue( - resourceName, resourceNames.values().stream().collect(Collectors.toList())); + resourceName, + resourceNames.values().stream().collect(Collectors.toList()), + methodArg.field().name()); } if (methodArg.type().equals(methodArg.field().type())) { @@ -150,7 +152,8 @@ static Expr createDefaultValue(Field f, boolean useExplicitInitTypeInGenerics) { "Default value for field %s with type %s not implemented yet.", f.name(), f.type())); } - static Expr createDefaultValue(ResourceName resourceName, List resnames) { + static Expr createDefaultValue( + ResourceName resourceName, List resnames, String fieldOrMessageName) { boolean hasOnePattern = resourceName.patterns().size() == 1; if (resourceName.isOnlyWildcard()) { List unexaminedResnames = new ArrayList<>(resnames); @@ -160,13 +163,14 @@ static Expr createDefaultValue(ResourceName resourceName, List res continue; } unexaminedResnames.remove(resname); - return createDefaultValue(resname, unexaminedResnames); + return createDefaultValue(resname, unexaminedResnames, fieldOrMessageName); + } + + if (unexaminedResnames.isEmpty()) { + return ValueExpr.withValue( + StringObjectValue.withValue( + String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode()))); } - // Should not get here. - Preconditions.checkState( - !unexaminedResnames.isEmpty(), - String.format( - "No default resource name found for wildcard %s", resourceName.resourceTypeString())); } // The cost tradeoffs of new ctors versus distinct() don't really matter here, since this list @@ -242,7 +246,8 @@ static Expr createSimpleMessageBuilderExpr( defaultExpr = createDefaultValue( resourceNames.get(field.resourceReference().resourceTypeString()), - resourceNames.values().stream().collect(Collectors.toList())); + resourceNames.values().stream().collect(Collectors.toList()), + message.name()); defaultExpr = MethodInvocationExpr.builder() .setExprReferenceExpr(defaultExpr) diff --git a/src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java index c0f67e9946..5893f2f17f 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/DefaultValueComposerTest.java @@ -15,7 +15,6 @@ package com.google.api.generator.gapic.composer; import static junit.framework.Assert.assertEquals; -import static org.junit.Assert.assertThrows; import com.google.api.generator.engine.ast.ConcreteReference; import com.google.api.generator.engine.ast.Expr; @@ -153,7 +152,8 @@ public void defaultValue_resourceNameWithOnePattern() { Expr expr = DefaultValueComposer.createDefaultValue( resourceName, - typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + typeStringsToResourceNames.values().stream().collect(Collectors.toList()), + "ignored"); expr.accept(writerVisitor); assertEquals("BillingAccountName.of(\"[BILLING_ACCOUNT]\")", writerVisitor.write()); } @@ -168,7 +168,8 @@ public void defaultValue_resourceNameWithMultiplePatterns() { Expr expr = DefaultValueComposer.createDefaultValue( resourceName, - typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + typeStringsToResourceNames.values().stream().collect(Collectors.toList()), + "ignored"); expr.accept(writerVisitor); assertEquals( "FolderName.ofProjectFolderName(\"[PROJECT]\", \"[FOLDER]\")", writerVisitor.write()); @@ -184,13 +185,14 @@ public void defaultValue_resourceNameWithWildcardPattern() { Expr expr = DefaultValueComposer.createDefaultValue( resourceName, - typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + typeStringsToResourceNames.values().stream().collect(Collectors.toList()), + "ignored"); expr.accept(writerVisitor); assertEquals("DocumentName.ofDocumentName(\"[DOCUMENT]\")", writerVisitor.write()); } @Test - public void invalidDefaultValue_wildcardResourceNameWithOnlyDeletedTopic() { + public void defaultValue_wildcardResourceNameWithOnlyDeletedTopic() { // Edge case that should never happen in practice. // Wildcard, but the resource names map has only other names that contain only the deleted-topic // constant. @@ -205,13 +207,14 @@ public void invalidDefaultValue_wildcardResourceNameWithOnlyDeletedTopic() { Expr expr = DefaultValueComposer.createDefaultValue( resourceName, - typeStringsToResourceNames.values().stream().collect(Collectors.toList())); + typeStringsToResourceNames.values().stream().collect(Collectors.toList()), + "ignored"); expr.accept(writerVisitor); assertEquals("TopicName.ofDeletedTopic()", writerVisitor.write()); } @Test - public void invalidDefaultValue_resourceNameWithOnlyWildcards() { + public void defaultValue_resourceNameWithOnlyWildcards() { // Edge case that should never happen in practice. // Wildcard, but the resource names map has only other names that contain only the deleted-topic // constant. @@ -220,9 +223,13 @@ public void invalidDefaultValue_resourceNameWithOnlyWildcards() { Parser.parseResourceNames(lockerServiceFileDescriptor); ResourceName resourceName = typeStringsToResourceNames.get("cloudresourcemanager.googleapis.com/Anything"); - assertThrows( - IllegalStateException.class, - () -> DefaultValueComposer.createDefaultValue(resourceName, Collections.emptyList())); + String fallbackField = "foobar"; + Expr expr = + DefaultValueComposer.createDefaultValue( + resourceName, Collections.emptyList(), fallbackField); + expr.accept(writerVisitor); + assertEquals( + String.format("\"%s%s\"", fallbackField, fallbackField.hashCode()), writerVisitor.write()); } @Test