From 984ce67a6bb79a1759b8763cae39b03a8e671c3a Mon Sep 17 00:00:00 2001 From: yibole Date: Mon, 24 Aug 2026 00:05:22 -0700 Subject: [PATCH] Apply model sharing and name validation in Smithy codegen --- .../naming/DefaultSmithyNamingStrategy.java | 158 +++++++++++++++-- .../DefaultSmithyNamingStrategyTest.java | 165 +++++++++++++++++- 2 files changed, 305 insertions(+), 18 deletions(-) diff --git a/codegen/src/main/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategy.java b/codegen/src/main/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategy.java index 170b40a8a4dd..d5c21af01a9f 100644 --- a/codegen/src/main/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategy.java +++ b/codegen/src/main/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategy.java @@ -25,16 +25,26 @@ import static software.amazon.awssdk.utils.internal.CodegenNamingUtils.pascalCase; import static software.amazon.awssdk.utils.internal.CodegenNamingUtils.splitOnWordBoundaries; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; +import java.util.List; import java.util.Locale; +import java.util.Optional; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Stream; import software.amazon.awssdk.codegen.internal.Constant; import software.amazon.awssdk.codegen.internal.Utils; import software.amazon.awssdk.codegen.model.config.customization.CustomizationConfig; +import software.amazon.awssdk.codegen.model.config.customization.UnderscoresInNameBehavior; import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; import software.amazon.awssdk.codegen.model.intermediate.MemberModel; +import software.amazon.awssdk.codegen.model.intermediate.Metadata; +import software.amazon.awssdk.codegen.validation.ModelInvalidException; +import software.amazon.awssdk.codegen.validation.ValidationEntry; +import software.amazon.awssdk.codegen.validation.ValidationErrorId; +import software.amazon.awssdk.codegen.validation.ValidationErrorSeverity; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.StringUtils; import software.amazon.smithy.aws.traits.ServiceTrait; @@ -52,6 +62,9 @@ public class DefaultSmithyNamingStrategy implements NamingStrategy { private static final Logger log = Logger.loggerFor(DefaultSmithyNamingStrategy.class); private static final String COLLISION_DISAMBIGUATION_PREFIX = "Default"; + private static final Pattern VALID_IDENTIFIER_NAME = + Pattern.compile("\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*"); + private static final Set RESERVED_KEYWORDS; private static final Set RESERVED_EXCEPTION_METHOD_NAMES; private static final Set RESERVED_STRUCTURE_METHOD_NAMES; @@ -96,7 +109,7 @@ public DefaultSmithyNamingStrategy(Model smithyModel, ServiceShape service, CustomizationConfig customizationConfig) { this.smithyModel = smithyModel; this.service = service; - this.customizationConfig = customizationConfig; + this.customizationConfig = customizationConfig == null ? CustomizationConfig.create() : customizationConfig; } @Override @@ -127,62 +140,72 @@ public String getServiceNameForProfileFile() { @Override public String getClientPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_CLIENT_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_CLIENT_PATTERN); } @Override public String getModelPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_MODEL_PATTERN); + return getCustomizedPackageName(shareModelWithOrDefault(serviceName), Constant.PACKAGE_NAME_MODEL_PATTERN); } @Override public String getTransformPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_TRANSFORM_PATTERN); + return getCustomizedPackageName(shareModelWithOrDefault(serviceName), Constant.PACKAGE_NAME_TRANSFORM_PATTERN); } @Override public String getRequestTransformPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_TRANSFORM_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_TRANSFORM_PATTERN); } @Override public String getPaginatorsPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_PAGINATORS_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_PAGINATORS_PATTERN); } @Override public String getWaitersPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_WAITERS_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_WAITERS_PATTERN); } @Override public String getEndpointRulesPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_RULES_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_RULES_PATTERN); } @Override public String getPresignedUrlPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_PRESIGNEDURL_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_PRESIGNEDURL_PATTERN); } @Override public String getAuthSchemePackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_AUTH_SCHEME_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_AUTH_SCHEME_PATTERN); } @Override public String getJmesPathPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_JMESPATH_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_JMESPATH_PATTERN); } @Override public String getBatchManagerPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_BATCHMANAGER_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_BATCHMANAGER_PATTERN); } @Override public String getSmokeTestPackageName(String serviceName) { - return getCustomizedPackageName(serviceName, Constant.PACKAGE_NAME_SMOKE_TEST_PATTERN); + return getCustomizedPackageName(concatServiceNameIfShareModel(serviceName), + Constant.PACKAGE_NAME_SMOKE_TEST_PATTERN); } @Override @@ -367,8 +390,83 @@ public String getSigningNameForSystemProperties() { @Override public void validateCustomerVisibleNaming(IntermediateModel trimmedModel) { - // TODO(smithy-migration): port DefaultNamingStrategy.validateCustomerVisibleNaming so Smithy-derived - // IntermediateModels get the same customer-visible name checks. Tracked with the next AddSmithyShapes PR. + Metadata metadata = trimmedModel.getMetadata(); + validateCustomerVisibleName(metadata.getSyncInterface(), "metadata-derived interface name"); + validateCustomerVisibleName(metadata.getSyncBuilderInterface(), "metadata-derived builder interface name"); + validateCustomerVisibleName(metadata.getAsyncInterface(), "metadata-derived async interface name"); + validateCustomerVisibleName(metadata.getAsyncBuilderInterface(), "metadata-derived async builder interface name"); + validateCustomerVisibleName(metadata.getBaseBuilderInterface(), "metadata-derived builder interface name"); + validateCustomerVisibleName(metadata.getBaseExceptionName(), "metadata-derived exception name"); + validateCustomerVisibleName(metadata.getBaseRequestName(), "metadata-derived request name"); + validateCustomerVisibleName(metadata.getBaseResponseName(), "metadata-derived response name"); + + trimmedModel.getOperations().values().forEach(operation -> { + validateCustomerVisibleName(operation.getOperationName(), "operations"); + }); + + trimmedModel.getWaiters().forEach((name, waiter) -> { + validateCustomerVisibleName(name, "waiters"); + }); + + trimmedModel.getShapes().values().forEach(shape -> { + String shapeName = shape.getShapeName(); + validateCustomerVisibleName(shapeName, "shapes"); + shape.getMembers().forEach(member -> { + validateCustomerVisibleName(member.getFluentGetterMethodName(), shapeName + " shape"); + validateCustomerVisibleName(member.getFluentSetterMethodName(), shapeName + " shape"); + validateCustomerVisibleName(member.getFluentEnumGetterMethodName(), shapeName + " shape"); + validateCustomerVisibleName(member.getFluentEnumSetterMethodName(), shapeName + " shape"); + validateCustomerVisibleName(member.getExistenceCheckMethodName(), shapeName + " shape"); + validateCustomerVisibleName(member.getBeanStyleGetterMethodName(), shapeName + " shape"); + validateCustomerVisibleName(member.getBeanStyleSetterMethodName(), shapeName + " shape"); + validateCustomerVisibleName(member.getEnumType(), shapeName + " shape"); + }); + }); + } + + private void validateCustomerVisibleName(String name, String location) { + if (name == null) { + return; + } + + if (name.contains("_")) { + UnderscoresInNameBehavior behavior = customizationConfig.getUnderscoresInNameBehavior(); + List allowedNames = customizationConfig.getAllowedUnderscoreNames(); + if (allowedNames != null && allowedNames.contains(name)) { + return; + } + + String supportedBehaviors = Arrays.toString(UnderscoresInNameBehavior.values()); + if (behavior == null) { + throw ModelInvalidException.fromEntry(ValidationEntry.create( + ValidationErrorId.INVALID_IDENTIFIER_NAME, + ValidationErrorSeverity.DANGER, + String.format( + "Encountered a name or identifier that the customer will see (%s in the %s) with an underscore. " + + "This isn't idiomatic in Java. Please remove the underscores.", + name, location) + )); + } + if (behavior != UnderscoresInNameBehavior.ALLOW) { + throw ModelInvalidException.fromEntry(ValidationEntry.create( + ValidationErrorId.INVALID_CODEGEN_CUSTOMIZATION, + ValidationErrorSeverity.DANGER, + String.format( + "Unsupported underscoresInShapeNameBehavior: %s. Supported values: %s", + behavior, supportedBehaviors) + )); + } + } + + if (!VALID_IDENTIFIER_NAME.matcher(name).matches()) { + throw ModelInvalidException.fromEntry(ValidationEntry.create( + ValidationErrorId.INVALID_IDENTIFIER_NAME, + ValidationErrorSeverity.DANGER, + String.format( + "Encountered a name or identifier that is invalid within Java (%s in %s). Please remove invalid " + + "characters.", name, location) + )); + } } private String serviceId() { @@ -389,6 +487,36 @@ private String getCustomizedPackageName(String serviceName, String defaultPatter return String.format(defaultPattern, StringUtils.lowerCase(serviceName)); } + /** + * Nests the package under the service whose models are being shared, using the configured package + * name when the service sets one and the given service name otherwise. + * + *

Keys off the presence of {@code shareModelConfig} rather than of {@code shareModelWith}, + * matching {@link DefaultNamingStrategy}. Deliberate: diverging from C2J here would change + * generated package names. + */ + private String concatServiceNameIfShareModel(String serviceName) { + if (customizationConfig.getShareModelConfig() != null) { + return customizationConfig.getShareModelConfig().getShareModelWith() + "." + + Optional.ofNullable(customizationConfig.getShareModelConfig().getPackageName()) + .orElse(serviceName); + } + return serviceName; + } + + /** + * Returns the name of the service whose models are being shared, or the given service name when + * this service does not share models. The model and transform packages are shared outright rather + * than nested under the sharing service. + */ + private String shareModelWithOrDefault(String serviceName) { + if (customizationConfig.getShareModelConfig() != null + && customizationConfig.getShareModelConfig().getShareModelWith() != null) { + return customizationConfig.getShareModelConfig().getShareModelWith(); + } + return serviceName; + } + private static boolean isJavaKeyword(String word) { return RESERVED_KEYWORDS.contains(word) || RESERVED_KEYWORDS.contains(StringUtils.lowerCase(word)); diff --git a/codegen/src/test/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategyTest.java b/codegen/src/test/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategyTest.java index 59ec35517923..abdfdfb9fe62 100644 --- a/codegen/src/test/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategyTest.java +++ b/codegen/src/test/java/software/amazon/awssdk/codegen/naming/DefaultSmithyNamingStrategyTest.java @@ -16,7 +16,9 @@ package software.amazon.awssdk.codegen.naming; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.Arrays; import java.util.function.Function; import java.util.stream.Stream; import org.junit.jupiter.api.Test; @@ -24,6 +26,10 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import software.amazon.awssdk.codegen.model.config.customization.CustomizationConfig; +import software.amazon.awssdk.codegen.model.config.customization.ShareModelConfig; +import software.amazon.awssdk.codegen.model.config.customization.UnderscoresInNameBehavior; +import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel; +import software.amazon.awssdk.codegen.model.intermediate.Metadata; import software.amazon.awssdk.codegen.model.service.ServiceMetadata; import software.amazon.awssdk.codegen.model.service.ServiceModel; import software.amazon.smithy.model.Model; @@ -40,17 +46,28 @@ */ class DefaultSmithyNamingStrategyTest { + private static final String SHARING_SERVICE = "dynamodbstreams"; + private static DefaultNamingStrategy c2j(String serviceId, String signingName) { + return c2j(serviceId, signingName, CustomizationConfig.create()); + } + + private static DefaultNamingStrategy c2j(String serviceId, String signingName, CustomizationConfig customization) { ServiceMetadata metadata = new ServiceMetadata(); metadata.setServiceId(serviceId); metadata.setSigningName(signingName); metadata.setEndpointPrefix(signingName); ServiceModel model = new ServiceModel(); model.setMetadata(metadata); - return new DefaultNamingStrategy(model, CustomizationConfig.create()); + return new DefaultNamingStrategy(model, customization); } private static DefaultSmithyNamingStrategy smithy(String serviceId, String signingName) { + return smithy(serviceId, signingName, CustomizationConfig.create()); + } + + private static DefaultSmithyNamingStrategy smithy(String serviceId, String signingName, + CustomizationConfig customization) { String idl = "$version: \"2\"\n" + "namespace com.example\n" @@ -67,11 +84,33 @@ private static DefaultSmithyNamingStrategy smithy(String serviceId, String signi .assemble() .unwrap(); ServiceShape service = model.expectShape(ShapeId.from("com.example#Widgets"), ServiceShape.class); - return new DefaultSmithyNamingStrategy(model, service, CustomizationConfig.create()); + return new DefaultSmithyNamingStrategy(model, service, customization); } private static NamingPair pair(String serviceId, String signingName) { - return new NamingPair(c2j(serviceId, signingName), smithy(serviceId, signingName)); + return pair(serviceId, signingName, CustomizationConfig.create()); + } + + private static NamingPair pair(String serviceId, String signingName, CustomizationConfig customization) { + return new NamingPair(c2j(serviceId, signingName, customization), + smithy(serviceId, signingName, customization)); + } + + private static CustomizationConfig shareModelsWith(String shareModelWith, String packageName) { + ShareModelConfig shareModelConfig = new ShareModelConfig(); + shareModelConfig.setShareModelWith(shareModelWith); + shareModelConfig.setPackageName(packageName); + CustomizationConfig customization = CustomizationConfig.create(); + customization.setShareModelConfig(shareModelConfig); + return customization; + } + + private static IntermediateModel modelWithAsyncBuilderInterface(String name) { + Metadata metadata = new Metadata(); + metadata.setAsyncBuilderInterface(name); + IntermediateModel model = new IntermediateModel(); + model.setMetadata(metadata); + return model; } private static final class NamingPair { @@ -125,4 +164,124 @@ void getServiceName_stripsServiceSuffixForGrandfatheredIds() { // still exercises the trailing-"service" strip in the naming strategy. assertThat(pair("Directory Service", "ds").smithy.getServiceName()).isEqualTo("Directory"); } + + /** + * Lists all twelve package-name methods so that one forgetting the {@code shareModelConfig} + * redirect is caught, even though only two services in the SDK share models. + */ + static Stream packageNamers() { + return Stream.of( + Arguments.of("getClientPackageName", + (Function) s -> s.getClientPackageName(SHARING_SERVICE)), + Arguments.of("getModelPackageName", + (Function) s -> s.getModelPackageName(SHARING_SERVICE)), + Arguments.of("getTransformPackageName", + (Function) s -> s.getTransformPackageName(SHARING_SERVICE)), + Arguments.of("getRequestTransformPackageName", + (Function) s -> s.getRequestTransformPackageName(SHARING_SERVICE)), + Arguments.of("getPaginatorsPackageName", + (Function) s -> s.getPaginatorsPackageName(SHARING_SERVICE)), + Arguments.of("getWaitersPackageName", + (Function) s -> s.getWaitersPackageName(SHARING_SERVICE)), + Arguments.of("getEndpointRulesPackageName", + (Function) s -> s.getEndpointRulesPackageName(SHARING_SERVICE)), + Arguments.of("getPresignedUrlPackageName", + (Function) s -> s.getPresignedUrlPackageName(SHARING_SERVICE)), + Arguments.of("getAuthSchemePackageName", + (Function) s -> s.getAuthSchemePackageName(SHARING_SERVICE)), + Arguments.of("getJmesPathPackageName", + (Function) s -> s.getJmesPathPackageName(SHARING_SERVICE)), + Arguments.of("getBatchManagerPackageName", + (Function) s -> s.getBatchManagerPackageName(SHARING_SERVICE)), + Arguments.of("getSmokeTestPackageName", + (Function) s -> s.getSmokeTestPackageName(SHARING_SERVICE)) + ); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("packageNamers") + void packageName_whenSharingModels_matchesC2j(String label, Function extract) { + NamingPair p = pair("DynamoDB Streams", "dynamodb", shareModelsWith("dynamodb", "streams")); + assertThat(extract.apply(p.smithy)).isEqualTo(extract.apply(p.c2j)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("packageNamers") + void packageName_whenNotSharingModels_matchesC2j(String label, Function extract) { + NamingPair p = pair("DynamoDB Streams", "dynamodb"); + assertThat(extract.apply(p.smithy)).isEqualTo(extract.apply(p.c2j)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("packageNamers") + void packageName_whenSharingModelsWithoutPackageName_matchesC2j(String label, + Function extract) { + NamingPair p = pair("DynamoDB Streams", "dynamodb", shareModelsWith("dynamodb", null)); + assertThat(extract.apply(p.smithy)).isEqualTo(extract.apply(p.c2j)); + } + + /** + * Pins both mechanisms to concrete values, so a change breaking both strategies in the same way is + * still caught. The other assertions in this class are differential and would not be. + */ + @Test + void packageName_whenSharingModels_nestsAllButModelAndTransform() { + NamingStrategy strategy = smithy("DynamoDB Streams", "dynamodb", shareModelsWith("dynamodb", "streams")); + + assertThat(strategy.getClientPackageName(SHARING_SERVICE)).isEqualTo("dynamodb.streams"); + assertThat(strategy.getRequestTransformPackageName(SHARING_SERVICE)).isEqualTo("dynamodb.streams.transform"); + assertThat(strategy.getModelPackageName(SHARING_SERVICE)).isEqualTo("dynamodb.model"); + assertThat(strategy.getTransformPackageName(SHARING_SERVICE)).isEqualTo("dynamodb.transform"); + } + + @Test + void validateCustomerVisibleNaming_underscoreWithNoBehaviorSet_throws() { + NamingStrategy strategy = smithy("DynamoDB", "dynamodb"); + + assertThatThrownBy(() -> strategy.validateCustomerVisibleNaming(modelWithAsyncBuilderInterface("foo_bar"))) + .isInstanceOf(RuntimeException.class); + } + + @Test + void validateCustomerVisibleNaming_underscoreWithAllowBehavior_passes() { + CustomizationConfig customization = + CustomizationConfig.create().withUnderscoresInShapeNameBehavior(UnderscoresInNameBehavior.ALLOW); + NamingStrategy strategy = smithy("DynamoDB", "dynamodb", customization); + + strategy.validateCustomerVisibleNaming(modelWithAsyncBuilderInterface("foo_bar")); + } + + @Test + void validateCustomerVisibleNaming_underscoreOnAllowlist_passes() { + CustomizationConfig customization = CustomizationConfig.create(); + customization.setAllowedUnderscoreNames(Arrays.asList("checksumXXHASH3_64", "foo_bar")); + NamingStrategy strategy = smithy("DynamoDB", "dynamodb", customization); + + strategy.validateCustomerVisibleNaming(modelWithAsyncBuilderInterface("foo_bar")); + } + + @Test + void validateCustomerVisibleNaming_underscoreOffAllowlist_throws() { + CustomizationConfig customization = CustomizationConfig.create(); + customization.setAllowedUnderscoreNames(Arrays.asList("checksumXXHASH3_64", "foo_bar")); + NamingStrategy strategy = smithy("DynamoDB", "dynamodb", customization); + + assertThatThrownBy(() -> strategy.validateCustomerVisibleNaming(modelWithAsyncBuilderInterface("fizz_buzz"))) + .isInstanceOf(RuntimeException.class); + } + + @Test + void validateCustomerVisibleNaming_nameIsNotALegalJavaIdentifier_throws() { + NamingStrategy strategy = smithy("DynamoDB", "dynamodb"); + + assertThatThrownBy(() -> strategy.validateCustomerVisibleNaming(modelWithAsyncBuilderInterface("foo-bar"))) + .isInstanceOf(RuntimeException.class); + } + + @Test + void validateCustomerVisibleNaming_idiomaticName_passes() { + NamingStrategy strategy = smithy("DynamoDB", "dynamodb"); + + strategy.validateCustomerVisibleNaming(modelWithAsyncBuilderInterface("DynamoDbAsyncClientBuilder")); + } }