From 8733038a9678cbf70f99290ded63a7957c35ca5c Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Wed, 14 Aug 2024 06:22:11 -0400 Subject: [PATCH] Include error shapes in member constraint transformation (#3787) Constrained members are extracted into standalone shapes with constraints on them, and the original shape's targets are replaced by the newly created shapes in the model. This PR adds support for including error shapes in this transformation. Co-authored-by: Fahad Zubair --- .../ConstrainedMemberTransform.kt | 13 ++-- .../smithy/ConstraintsMemberShapeTest.kt | 66 +++++++++++-------- 2 files changed, 43 insertions(+), 36 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/transformers/ConstrainedMemberTransform.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/transformers/ConstrainedMemberTransform.kt index c3540b273..354547980 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/transformers/ConstrainedMemberTransform.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/transformers/ConstrainedMemberTransform.kt @@ -18,8 +18,6 @@ import software.amazon.smithy.model.traits.RequiredTrait import software.amazon.smithy.model.traits.Trait import software.amazon.smithy.model.transform.ModelTransformer import software.amazon.smithy.rust.codegen.core.smithy.DirectedWalker -import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait -import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticOutputTrait import software.amazon.smithy.rust.codegen.core.util.UNREACHABLE import software.amazon.smithy.rust.codegen.core.util.orNull import software.amazon.smithy.rust.codegen.server.smithy.allConstraintTraits @@ -75,16 +73,15 @@ object ConstrainedMemberTransform { val additionalNames = HashSet() val walker = DirectedWalker(model) - // Find all synthetic input / output structures that have been added by - // the OperationNormalizer, get constrained members out of those structures, - // convert them into non-constrained members and then pass them to the transformer. - // The transformer will add new shapes, and will replace existing member shapes' target - // with the newly added shapes. + // Begin with the operations listed in the service and traverse the inputs, outputs, and errors specified + // in each operation. Extract the target shapes of constrained members from structures, lists, maps, and + // unions into standalone shapes with constraints. Update the model by incorporating these extracted shapes + // and adjust the target shapes of the original constrained members to reference the newly created + // constrained types. val transformations = model.operationShapes .flatMap { listOfNotNull(it.input.orNull(), it.output.orNull()) + it.errors } .mapNotNull { model.expectShape(it).asStructureShape().orElse(null) } - .filter { it.hasTrait(SyntheticInputTrait.ID) || it.hasTrait(SyntheticOutputTrait.ID) } .flatMap { walker.walkShapes(it) } .filter { it is StructureShape || it is ListShape || it is UnionShape || it is MapShape } .flatMap { it.constrainedMembers() } diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsMemberShapeTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsMemberShapeTest.kt index b51333859..f5f524a52 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsMemberShapeTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsMemberShapeTest.kt @@ -24,25 +24,29 @@ import software.amazon.smithy.rust.codegen.core.util.toPascalCase import software.amazon.smithy.rust.codegen.core.util.toSnakeCase import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverIntegrationTest import software.amazon.smithy.rust.codegen.server.smithy.transformers.ConstrainedMemberTransform +import kotlin.streams.toList class ConstraintsMemberShapeTest { - private val outputModelOnly = + private val sampleModel = """ namespace constrainedMemberShape + use smithy.framework#ValidationException use aws.protocols#restJson1 use aws.api#data @restJson1 service ConstrainedService { - operations: [OperationUsingGet] + operations: [SampleOperation] } - @http(uri: "/anOperation", method: "GET") - operation OperationUsingGet { - output : OperationUsingGetOutput + @http(uri: "/anOperation", method: "POST") + operation SampleOperation { + output: SampleInputOutput + input: SampleInputOutput + errors: [ValidationException, ErrorWithMemberConstraint] } - structure OperationUsingGetOutput { + structure SampleInputOutput { plainLong : Long plainInteger : Integer plainShort : Short @@ -148,6 +152,12 @@ class ConstraintsMemberShapeTest { string PatternString @range(min: 0, max:1000) integer RangedInteger + + @error("server") + structure ErrorWithMemberConstraint { + @range(min: 100, max: 999) + statusCode: Integer + } """.asSmithyModel() private fun loadModel(model: Model): Model = @@ -155,16 +165,16 @@ class ConstraintsMemberShapeTest { @Test fun `non constrained fields should not be changed`() { - val transformedModel = loadModel(outputModelOnly) + val transformedModel = loadModel(sampleModel) fun checkFieldTargetRemainsSame(fieldName: String) { checkMemberShapeIsSame( transformedModel, - outputModelOnly, - "constrainedMemberShape.synthetic#OperationUsingGetOutput\$$fieldName", - "constrainedMemberShape#OperationUsingGetOutput\$$fieldName", + sampleModel, + "constrainedMemberShape.synthetic#SampleOperationOutput\$$fieldName", + "constrainedMemberShape#SampleInputOutput\$$fieldName", ) { - "OperationUsingGetOutput$fieldName has changed whereas it is not constrained and should have remained same" + "SampleInputOutput$fieldName has changed whereas it is not constrained and should have remained same" } } @@ -189,7 +199,7 @@ class ConstraintsMemberShapeTest { checkMemberShapeIsSame( transformedModel, - outputModelOnly, + sampleModel, "constrainedMemberShape#StructWithConstrainedMember\$long", "constrainedMemberShape#StructWithConstrainedMember\$long", ) @@ -197,10 +207,10 @@ class ConstraintsMemberShapeTest { @Test fun `constrained members should have a different target now`() { - val transformedModel = loadModel(outputModelOnly) + val transformedModel = loadModel(sampleModel) checkMemberShapeChanged( transformedModel, - outputModelOnly, + sampleModel, "constrainedMemberShape#PatternStringListOverride\$member", "constrainedMemberShape#PatternStringListOverride\$member", ) @@ -208,9 +218,9 @@ class ConstraintsMemberShapeTest { fun checkSyntheticFieldTargetChanged(fieldName: String) { checkMemberShapeChanged( transformedModel, - outputModelOnly, - "constrainedMemberShape.synthetic#OperationUsingGetOutput\$$fieldName", - "constrainedMemberShape#OperationUsingGetOutput\$$fieldName", + sampleModel, + "constrainedMemberShape.synthetic#SampleOperationOutput\$$fieldName", + "constrainedMemberShape#SampleInputOutput\$$fieldName", ) { "constrained member $fieldName should have been changed into a new type." } @@ -219,7 +229,7 @@ class ConstraintsMemberShapeTest { fun checkFieldTargetChanged(memberNameWithContainer: String) { checkMemberShapeChanged( transformedModel, - outputModelOnly, + sampleModel, "constrainedMemberShape#$memberNameWithContainer", "constrainedMemberShape#$memberNameWithContainer", ) { @@ -251,36 +261,36 @@ class ConstraintsMemberShapeTest { @Test fun `extra trait on a constrained member should remain on it`() { - val transformedModel = loadModel(outputModelOnly) + val transformedModel = loadModel(sampleModel) checkShapeHasTrait( transformedModel, - outputModelOnly, - "constrainedMemberShape.synthetic#OperationUsingGetOutput\$constrainedPatternString", - "constrainedMemberShape#OperationUsingGetOutput\$constrainedPatternString", + sampleModel, + "constrainedMemberShape.synthetic#SampleOperationOutput\$constrainedPatternString", + "constrainedMemberShape#SampleInputOutput\$constrainedPatternString", DataTrait("content", SourceLocation.NONE), ) } @Test fun `required remains on constrained member shape`() { - val transformedModel = loadModel(outputModelOnly) + val transformedModel = loadModel(sampleModel) checkShapeHasTrait( transformedModel, - outputModelOnly, - "constrainedMemberShape.synthetic#OperationUsingGetOutput\$requiredConstrainedString", - "constrainedMemberShape#OperationUsingGetOutput\$requiredConstrainedString", + sampleModel, + "constrainedMemberShape.synthetic#SampleOperationOutput\$requiredConstrainedString", + "constrainedMemberShape#SampleInputOutput\$requiredConstrainedString", RequiredTrait(), ) } @Test fun `generate code and check member constrained shapes are in the right modules`() { - serverIntegrationTest(outputModelOnly, IntegrationTestParams(service = "constrainedMemberShape#ConstrainedService")) { codegenContext, rustCrate -> + serverIntegrationTest(sampleModel, IntegrationTestParams(service = "constrainedMemberShape#ConstrainedService")) { codegenContext, rustCrate -> fun RustWriter.testTypeExistsInBuilderModule(typeName: String) { unitTest( "builder_module_has_${typeName.toSnakeCase()}", """ - #[allow(unused_imports)] use crate::output::operation_using_get_output::$typeName; + #[allow(unused_imports)] use crate::output::sample_operation_output::$typeName; """, ) } -- GitLab