From 205514a7358a4b486d637224f84c037fbda8db9d Mon Sep 17 00:00:00 2001 From: vcjana Date: Mon, 13 Oct 2025 15:10:10 -0700 Subject: [PATCH 1/2] Add deprecation warning to S3 GetBucketLocation operation - Add DeprecatedTrait to GetBucketLocation operation in S3Decorator - Include message recommending HeadBucket as the preferred alternative - Add comprehensive unit tests to verify deprecation behavior - Add integration test to verify generated Rust code includes #[deprecated] attribute This addresses customer feedback that GetBucketLocation is marked as deprecated in AWS documentation but not in the Rust SDK. --- .../rustsdk/customize/s3/S3Decorator.kt | 29 +++ .../rustsdk/customize/s3/S3DecoratorTest.kt | 196 ++++++++++++++++++ 2 files changed, 225 insertions(+) create mode 100644 aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt diff --git a/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt b/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt index d43ae4ad560..65aa17e5790 100644 --- a/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt +++ b/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt @@ -13,6 +13,7 @@ import software.amazon.smithy.model.shapes.ServiceShape import software.amazon.smithy.model.shapes.Shape import software.amazon.smithy.model.shapes.ShapeId import software.amazon.smithy.model.shapes.StructureShape +import software.amazon.smithy.model.traits.DeprecatedTrait import software.amazon.smithy.model.transform.ModelTransformer import software.amazon.smithy.rulesengine.traits.EndpointTestCase import software.amazon.smithy.rulesengine.traits.EndpointTestOperationInput @@ -60,6 +61,13 @@ class S3Decorator : ClientCodegenDecorator { ShapeId.from("com.amazonaws.s3#ListDirectoryBucketsOutput"), ) + // GetBucketLocation is deprecated because AWS recommends using HeadBucket instead + // to determine a bucket's region + private val deprecatedOperations = + setOf( + ShapeId.from("com.amazonaws.s3#GetBucketLocation"), + ) + override fun protocols( serviceId: ShapeId, currentProtocols: ProtocolMap, @@ -81,6 +89,9 @@ class S3Decorator : ClientCodegenDecorator { shape.letIf(isInInvalidXmlRootAllowList(shape)) { logger.info("Adding AllowInvalidXmlRoot trait to $it") (it as StructureShape).toBuilder().addTrait(AllowInvalidXmlRoot()).build() + }.letIf(isDeprecatedOperation(shape)) { + logger.info("Adding DeprecatedTrait to $it") + (it as OperationShape).toBuilder().addTrait(createDeprecatedTrait()).build() } } // the model has the bucket in the path @@ -182,6 +193,24 @@ class S3Decorator : ClientCodegenDecorator { private fun isInInvalidXmlRootAllowList(shape: Shape): Boolean { return shape.isStructureShape && invalidXmlRootAllowList.contains(shape.id) } + + /** + * Checks if the given shape is an operation that should be marked as deprecated. + */ + private fun isDeprecatedOperation(shape: Shape): Boolean { + return shape.isOperationShape && deprecatedOperations.contains(shape.id) + } + + /** + * Creates a DeprecatedTrait with a message recommending HeadBucket as the preferred alternative. + * GetBucketLocation is deprecated because HeadBucket is more reliable and less prone to misuse + * when determining a bucket's region. + */ + private fun createDeprecatedTrait(): DeprecatedTrait { + return DeprecatedTrait.builder() + .message("Use HeadBucket operation instead to determine the bucket's region. For more information, see https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html") + .build() + } } class FilterEndpointTests( diff --git a/aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt b/aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt new file mode 100644 index 00000000000..ca66faf7ef5 --- /dev/null +++ b/aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt @@ -0,0 +1,196 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.rustsdk.customize.s3 + +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import software.amazon.smithy.model.shapes.OperationShape +import software.amazon.smithy.model.shapes.ServiceShape +import software.amazon.smithy.model.shapes.ShapeId +import software.amazon.smithy.model.traits.DeprecatedTrait +import software.amazon.smithy.rust.codegen.client.testutil.testClientRustSettings +import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel +import software.amazon.smithy.rust.codegen.core.testutil.integrationTest +import software.amazon.smithy.rust.codegen.core.util.hasTrait +import software.amazon.smithy.rustsdk.awsSdkIntegrationTest +import kotlin.jvm.optionals.getOrNull + +internal class S3DecoratorTest { + /** + * Base S3 model for testing. Includes GetBucketLocation, HeadBucket, and CreateBucket operations. + * This minimal model allows us to test deprecation behavior without loading the full S3 model. + */ + private val baseModel = + """ + namespace com.amazonaws.s3 + + use aws.protocols#restXml + use aws.auth#sigv4 + use aws.api#service + use smithy.rules#endpointRuleSet + + @restXml + @sigv4(name: "s3") + @service( + sdkId: "S3" + arnNamespace: "s3" + ) + @endpointRuleSet({ + "version": "1.0", + "rules": [{ "type": "endpoint", "conditions": [], "endpoint": { "url": "https://s3.amazonaws.com" } }], + "parameters": { + "Region": { "required": false, "type": "String", "builtIn": "AWS::Region" } + } + }) + service S3 { + version: "2006-03-01", + operations: [GetBucketLocation, HeadBucket, CreateBucket] + } + + @http(method: "GET", uri: "/{Bucket}?location") + operation GetBucketLocation { + input: GetBucketLocationRequest + output: GetBucketLocationOutput + } + + @http(method: "HEAD", uri: "/{Bucket}") + operation HeadBucket { + input: HeadBucketRequest + } + + @http(method: "PUT", uri: "/{Bucket}") + operation CreateBucket { + input: CreateBucketRequest + } + + structure GetBucketLocationRequest { + @required + @httpLabel + Bucket: String + } + + @output + structure GetBucketLocationOutput { + LocationConstraint: String + } + + structure HeadBucketRequest { + @required + @httpLabel + Bucket: String + } + + structure CreateBucketRequest { + @required + @httpLabel + Bucket: String + } + """.asSmithyModel() + + private val serviceShape = baseModel.expectShape(ShapeId.from("com.amazonaws.s3#S3"), ServiceShape::class.java) + private val settings = testClientRustSettings() + + /** + * Helper method to apply the S3Decorator transformation to the base model. + * This simulates what happens during code generation. + */ + private fun transformModel() = S3Decorator().transformModel(serviceShape, baseModel, settings) + + @Test + fun `GetBucketLocation operation has DeprecatedTrait`() { + // Apply the S3Decorator transformation + val transformedModel = transformModel() + + // Get the GetBucketLocation operation from the transformed model + val getBucketLocationId = ShapeId.from("com.amazonaws.s3#GetBucketLocation") + val operation = transformedModel.expectShape(getBucketLocationId, OperationShape::class.java) + + // Assert that the operation has the DeprecatedTrait + assertTrue(operation.hasTrait(), "GetBucketLocation should have DeprecatedTrait") + + // Get the trait and verify the message content + val trait = operation.expectTrait(DeprecatedTrait::class.java) + val message = trait.message.getOrNull() + + // Assert that the message contains "HeadBucket" + assertTrue( + message?.contains("HeadBucket") == true, + "Deprecation message should mention HeadBucket", + ) + + // Assert that the message contains the AWS documentation URL + assertTrue( + message?.contains("https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html") == true, + "Deprecation message should include AWS documentation URL", + ) + } + + @Test + fun `Other S3 operations do not have DeprecatedTrait`() { + // Apply the S3Decorator transformation + val transformedModel = transformModel() + + // Check HeadBucket operation does NOT have DeprecatedTrait + val headBucketId = ShapeId.from("com.amazonaws.s3#HeadBucket") + val headBucket = transformedModel.expectShape(headBucketId, OperationShape::class.java) + assertFalse( + headBucket.hasTrait(), + "HeadBucket should not have DeprecatedTrait", + ) + + // Check CreateBucket operation does NOT have DeprecatedTrait + val createBucketId = ShapeId.from("com.amazonaws.s3#CreateBucket") + val createBucket = transformedModel.expectShape(createBucketId, OperationShape::class.java) + assertFalse( + createBucket.hasTrait(), + "CreateBucket should not have DeprecatedTrait", + ) + } + + @Test + fun `Generated Rust code includes deprecated attribute for GetBucketLocation`() { + // This integration test generates Rust code for the S3 client with the modified decorator + // and verifies that the #[deprecated] attribute is present in the generated code by: + // 1. Generating the complete S3 client code with the S3Decorator applied + // 2. Compiling the generated code (which includes the client module with get_bucket_location) + // 3. Running a test that references the deprecated method + // + // The test passes if: + // - The code generation succeeds (DeprecatedTrait is properly converted to Rust #[deprecated]) + // - The generated code compiles successfully + // - The test that uses the method compiles (deprecation warnings are allowed) + // + // This verifies the end-to-end flow: Model transformation -> Code generation -> Compilation + awsSdkIntegrationTest(baseModel) { ctx, rustCrate -> + val moduleName = ctx.moduleUseName() + + // Create an integration test that uses the deprecated method + // This will trigger a deprecation warning during compilation if the attribute is present + rustCrate.integrationTest("verify_get_bucket_location_deprecated") { + writeWithNoFormatting( + """ + use $moduleName::Client; + + // This test verifies that the get_bucket_location method exists and is accessible. + // If the #[deprecated] attribute is present in the generated code, the Rust compiler + // will emit a deprecation warning when this code is compiled. + // + // The deprecation message should contain: + // - "HeadBucket" - the recommended alternative operation + // - "https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html" - documentation URL + #[test] + fn get_bucket_location_method_exists() { + // Reference the method to trigger deprecation warning + fn check_method_exists(_: T) {} + check_method_exists(Client::get_bucket_location); + } + """, + ) + } + } + } +} From bd84e3fb31d07556dea6c7766d2723bd8c01e781 Mon Sep 17 00:00:00 2001 From: vcjana Date: Tue, 14 Oct 2025 14:17:46 -0700 Subject: [PATCH 2/2] Refactor createDeprecatedTrait to accept message parameter - Changed createDeprecatedTrait() to take message as parameter - Updated deprecatedOperations from Set to Map for message storage - Removed integration test per code review feedback --- .../rustsdk/customize/s3/S3Decorator.kt | 18 +++---- .../customize/s3/S3ExpiresDecorator.kt | 47 ++++++++++++------- .../rustsdk/customize/s3/S3DecoratorTest.kt | 45 ------------------ 3 files changed, 40 insertions(+), 70 deletions(-) diff --git a/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt b/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt index 65aa17e5790..f91c0eaabd9 100644 --- a/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt +++ b/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt @@ -64,8 +64,9 @@ class S3Decorator : ClientCodegenDecorator { // GetBucketLocation is deprecated because AWS recommends using HeadBucket instead // to determine a bucket's region private val deprecatedOperations = - setOf( - ShapeId.from("com.amazonaws.s3#GetBucketLocation"), + mapOf( + ShapeId.from("com.amazonaws.s3#GetBucketLocation") to + "Use HeadBucket operation instead to determine the bucket's region. For more information, see https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html", ) override fun protocols( @@ -91,7 +92,8 @@ class S3Decorator : ClientCodegenDecorator { (it as StructureShape).toBuilder().addTrait(AllowInvalidXmlRoot()).build() }.letIf(isDeprecatedOperation(shape)) { logger.info("Adding DeprecatedTrait to $it") - (it as OperationShape).toBuilder().addTrait(createDeprecatedTrait()).build() + val message = deprecatedOperations[shape.id]!! + (it as OperationShape).toBuilder().addTrait(createDeprecatedTrait(message)).build() } } // the model has the bucket in the path @@ -198,17 +200,15 @@ class S3Decorator : ClientCodegenDecorator { * Checks if the given shape is an operation that should be marked as deprecated. */ private fun isDeprecatedOperation(shape: Shape): Boolean { - return shape.isOperationShape && deprecatedOperations.contains(shape.id) + return shape.isOperationShape && deprecatedOperations.containsKey(shape.id) } /** - * Creates a DeprecatedTrait with a message recommending HeadBucket as the preferred alternative. - * GetBucketLocation is deprecated because HeadBucket is more reliable and less prone to misuse - * when determining a bucket's region. + * Creates a DeprecatedTrait with the specified deprecation message. */ - private fun createDeprecatedTrait(): DeprecatedTrait { + private fun createDeprecatedTrait(message: String): DeprecatedTrait { return DeprecatedTrait.builder() - .message("Use HeadBucket operation instead to determine the bucket's region. For more information, see https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html") + .message(message) .build() } } diff --git a/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3ExpiresDecorator.kt b/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3ExpiresDecorator.kt index 3d89cfdbc43..49cd1d3ef98 100644 --- a/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3ExpiresDecorator.kt +++ b/aws/codegen-aws-sdk/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3ExpiresDecorator.kt @@ -33,8 +33,8 @@ import software.amazon.smithy.rustsdk.InlineAwsDependency import kotlin.streams.asSequence /** - * Enforces that Expires fields have the DateTime type (since in the future the model will change to model them as String), - * and add an ExpiresString field to maintain the raw string value sent. + * Enforces that Expires fields have the DateTime type (since in the future the model will change to + * model them as String), and add an ExpiresString field to maintain the raw string value sent. */ class S3ExpiresDecorator : ClientCodegenDecorator { override val name: String = "S3ExpiresDecorator" @@ -55,42 +55,55 @@ class S3ExpiresDecorator : ClientCodegenDecorator { .asSequence() .mapNotNull { shape -> shape.members() - .singleOrNull { member -> member.memberName.equals(expires, ignoreCase = true) } + .singleOrNull { member -> + member.memberName.equals(expires, ignoreCase = true) + } ?.target } .associateWith { ShapeType.TIMESTAMP } var transformedModel = transformer.changeShapeType(model, expiresShapeTimestampMap) // Add an `ExpiresString` string shape to the model - val expiresStringShape = StringShape.builder().id("aws.sdk.rust.s3.synthetic#$expiresString").build() + val expiresStringShape = + StringShape.builder().id("aws.sdk.rust.s3.synthetic#$expiresString").build() transformedModel = transformedModel.toBuilder().addShape(expiresStringShape).build() - // For output shapes only, deprecate `Expires` and add a synthetic member that targets `ExpiresString` + // For output shapes only, deprecate `Expires` and add a synthetic member that targets + // `ExpiresString` transformedModel = transformer.mapShapes(transformedModel) { shape -> - if (shape.hasTrait() && shape.memberNames.any { it.equals(expires, ignoreCase = true) }) { + if (shape.hasTrait() && + shape.memberNames.any { it.equals(expires, ignoreCase = true) } + ) { val builder = (shape as StructureShape).toBuilder() // Deprecate `Expires` - val expiresMember = shape.members().single { it.memberName.equals(expires, ignoreCase = true) } + val expiresMember = + shape.members().single { + it.memberName.equals(expires, ignoreCase = true) + } builder.removeMember(expiresMember.memberName) val deprecatedTrait = DeprecatedTrait.builder() - .message("Please use `expires_string` which contains the raw, unparsed value of this field.") + .message( + "Please use `expires_string` which contains the raw, unparsed value of this field.", + ) .build() builder.addMember( - expiresMember.toBuilder() - .addTrait(deprecatedTrait) - .build(), + expiresMember.toBuilder().addTrait(deprecatedTrait).build(), ) // Add a synthetic member targeting `ExpiresString` val expiresStringMember = MemberShape.builder() expiresStringMember.target(expiresStringShape.id) - expiresStringMember.id(expiresMember.id.toString() + "String") // i.e. com.amazonaws.s3.$ExpiresString - expiresStringMember.addTrait(HttpHeaderTrait(expiresString)) // Add HttpHeaderTrait to ensure the field is deserialized + expiresStringMember.id( + expiresMember.id.toString() + "String", + ) // i.e. com.amazonaws.s3.$ExpiresString + expiresStringMember.addTrait( + HttpHeaderTrait(expiresString), + ) // Add HttpHeaderTrait to ensure the field is deserialized expiresMember.getTrait()?.let { expiresStringMember.addTrait(it) // Copy documentation from `Expires` } @@ -132,8 +145,11 @@ class ParseExpiresFieldsCustomization( section.registerInterceptor(codegenContext.runtimeConfig, this) { val interceptor = RuntimeType.forInlineDependency( - InlineAwsDependency.forRustFile("s3_expires_interceptor"), - ).resolve("S3ExpiresInterceptor") + InlineAwsDependency.forRustFile( + "s3_expires_interceptor", + ), + ) + .resolve("S3ExpiresInterceptor") rustTemplate( """ #{S3ExpiresInterceptor} @@ -142,7 +158,6 @@ class ParseExpiresFieldsCustomization( ) } } - else -> {} } } diff --git a/aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt b/aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt index ca66faf7ef5..4172aff19a4 100644 --- a/aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt +++ b/aws/codegen-aws-sdk/src/test/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3DecoratorTest.kt @@ -14,9 +14,7 @@ import software.amazon.smithy.model.shapes.ShapeId import software.amazon.smithy.model.traits.DeprecatedTrait import software.amazon.smithy.rust.codegen.client.testutil.testClientRustSettings import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel -import software.amazon.smithy.rust.codegen.core.testutil.integrationTest import software.amazon.smithy.rust.codegen.core.util.hasTrait -import software.amazon.smithy.rustsdk.awsSdkIntegrationTest import kotlin.jvm.optionals.getOrNull internal class S3DecoratorTest { @@ -150,47 +148,4 @@ internal class S3DecoratorTest { "CreateBucket should not have DeprecatedTrait", ) } - - @Test - fun `Generated Rust code includes deprecated attribute for GetBucketLocation`() { - // This integration test generates Rust code for the S3 client with the modified decorator - // and verifies that the #[deprecated] attribute is present in the generated code by: - // 1. Generating the complete S3 client code with the S3Decorator applied - // 2. Compiling the generated code (which includes the client module with get_bucket_location) - // 3. Running a test that references the deprecated method - // - // The test passes if: - // - The code generation succeeds (DeprecatedTrait is properly converted to Rust #[deprecated]) - // - The generated code compiles successfully - // - The test that uses the method compiles (deprecation warnings are allowed) - // - // This verifies the end-to-end flow: Model transformation -> Code generation -> Compilation - awsSdkIntegrationTest(baseModel) { ctx, rustCrate -> - val moduleName = ctx.moduleUseName() - - // Create an integration test that uses the deprecated method - // This will trigger a deprecation warning during compilation if the attribute is present - rustCrate.integrationTest("verify_get_bucket_location_deprecated") { - writeWithNoFormatting( - """ - use $moduleName::Client; - - // This test verifies that the get_bucket_location method exists and is accessible. - // If the #[deprecated] attribute is present in the generated code, the Rust compiler - // will emit a deprecation warning when this code is compiled. - // - // The deprecation message should contain: - // - "HeadBucket" - the recommended alternative operation - // - "https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadBucket.html" - documentation URL - #[test] - fn get_bucket_location_method_exists() { - // Reference the method to trigger deprecation warning - fn check_method_exists(_: T) {} - check_method_exists(Client::get_bucket_location); - } - """, - ) - } - } - } }