From ae7b403ea096455e57d4f104e654241c4a8d2326 Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Tue, 12 Nov 2024 11:02:18 +0000 Subject: [PATCH] Fix directly constrained `List` shape with indirectly constrained aggregate type member shape (#3894) This PR fixes a bug in the code generation for a directly constrained List shape that has an indirectly constrained aggregate type as a member shape. For example, in the following model: ```smithy @http(uri: "/sample", method: "POST") operation SampleOp { input := { a: ItemList } errors: [ValidationException] } @length(min: 1 max: 100) list ItemList { member: Item } map Item { key: ItemName value: ItemDescription } @length(min: 0 max: 65535) string ItemName string ItemDescription ``` A `TryFrom` implementation is generated that converts from `ItemListUnconstrained` to `ItemList` by converting each member of the inner list of `ItemUnconstrained` to `ItemConstrained` and then converting it to `Vec>`: ```rust impl std::convert::TryFrom for crate::model::ItemList { type Error = crate::model::item_list::ConstraintViolation; fn try_from(value: ItemListUnconstrained) -> Result { let res: Result< ::std::vec::Vec< ::std::collections::HashMap, >, (usize, crate::model::item::ConstraintViolation), > = value .0 .into_iter() .enumerate() .map(|(idx, inner)| { inner.try_into() .map(|ic: crate::constrained::item_constrained::ItemConstrained| ic.into()) .map_err(|inner_violation| (idx, inner_violation)) }) .collect(); let inner = res.map_err(|(idx, inner_violation)| Self::Error::Member(idx, inner_violation))?; Self::try_from(inner) } } ``` Partially fixes issue: #3885 for publicly constrained shapes only. --------- Co-authored-by: Fahad Zubair --- .../UnconstrainedCollectionGenerator.kt | 37 +++++ .../codegen/server/smithy/ConstraintsTest.kt | 134 ++++++++++++++++++ 2 files changed, 171 insertions(+) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt index 041b5a1381..3c47ef2128 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt @@ -143,6 +143,43 @@ class UnconstrainedCollectionGenerator( "InnerConstraintViolationSymbol" to innerConstraintViolationSymbol, "ConstrainValueWritable" to constrainValueWritable, ) + + val constrainedValueTypeIsNotFinalType = + resolvesToNonPublicConstrainedValueType && shape.isDirectlyConstrained(symbolProvider) + if (constrainedValueTypeIsNotFinalType) { + // Refer to the comments under `UnconstrainedMapGenerator` for a more in-depth explanation + // of this process. Consider the following Smithy model where a constrained list contains + // an indirectly constrained shape as a member: + // + // ```smithy + // @length(min: 1, max: 100) + // list ItemList { + // member: Item + // } + // + // list Item { + // member: ItemName + // } + // + // @length(min: 1, max: 100) + // string ItemName + // ``` + // + // The final type exposed to the user is `ItemList>>`. However, the + // intermediate representation generated is `Vec`. This needs to be + // transformed into `Vec>` to satisfy the `TryFrom` implementation and + // successfully construct an `ItemList` instance. + // + // This transformation is necessary due to the nested nature of the constraints and + // the difference between the internal constrained representation and the external + // user-facing type. + rustTemplate( + """ + let inner: Vec<#{FinalType}> = inner.into_iter().map(|value| value.into()).collect(); + """, + "FinalType" to symbolProvider.toSymbol(shape.member), + ) + } } else { rust("let inner = value.0;") } diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsTest.kt index 31123ef811..b0b4a01d28 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsTest.kt @@ -25,8 +25,11 @@ import software.amazon.smithy.model.shapes.StructureShape import software.amazon.smithy.model.traits.AbstractTrait import software.amazon.smithy.model.transform.ModelTransformer import software.amazon.smithy.protocol.traits.Rpcv2CborTrait +import software.amazon.smithy.rust.codegen.core.testutil.IntegrationTestParams +import software.amazon.smithy.rust.codegen.core.testutil.ServerAdditionalSettings import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel import software.amazon.smithy.rust.codegen.core.util.lookup +import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverIntegrationTest import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestSymbolProvider import java.io.File @@ -219,4 +222,135 @@ class ConstraintsTest { structWithInnerDefault.canReachConstrainedShape(model, symbolProvider) shouldBe false primitiveBoolean.isDirectlyConstrained(symbolProvider) shouldBe false } + + // TODO(#3895): Move tests that use `generateAndCompileServer` into `constraints.smithy` once issue is resolved + private fun generateAndCompileServer( + model: Model, + pubConstraints: Boolean = true, + dir: File? = null, + ) { + if (dir?.exists() == true) { + dir.deleteRecursively() + } + + // Simply compiling the crate is sufficient as a test. + serverIntegrationTest( + model, + IntegrationTestParams( + service = "test#SampleService", + additionalSettings = + ServerAdditionalSettings.builder() + .publicConstrainedTypes(pubConstraints) + .toObjectNode(), + overrideTestDir = dir, + ), + ) { _, _ -> + } + } + + private fun createModel( + inputMemberShape: String, + additionalShapes: () -> String, + ) = """ + namespace test + use aws.protocols#restJson1 + use smithy.framework#ValidationException + + @restJson1 + service SampleService { + operations: [SampleOp] + } + + @http(uri: "/sample", method: "POST") + operation SampleOp { + input := { + items : $inputMemberShape + } + errors: [ValidationException] + } + @length(min: 0 max: 65535) + string ItemName + string ItemDescription + ${additionalShapes()} + """.asSmithyModel(smithyVersion = "2") + + @Test + fun `constrained map with an indirectly constrained nested list should compile`() { + val model = + createModel("ItemMap") { + """ + @length(min: 1 max: 100) + map ItemMap { + key: ItemName, + value: ItemListA + } + list ItemListA { + member: ItemListB + } + list ItemListB { + member: ItemDescription + } + """ + } + generateAndCompileServer(model) + } + + @Test + fun `constrained list with an indirectly constrained map should compile`() { + val model = + createModel("ItemList") { + """ + @length(min: 1 max: 100) + list ItemList { + member: Item + } + map Item { + key: ItemName + value: ItemDescription + } + """ + } + generateAndCompileServer(model) + } + + @Test + fun `constrained list with an indirectly constrained nested list should compile`() { + val model = + createModel("ItemList") { + """ + @length(min: 1 max: 100) + list ItemList { + member: ItemA + } + list ItemA { + member: ItemB + } + list ItemB { + member: ItemName + } + """ + } + generateAndCompileServer(model) + } + + @Test + fun `constrained list with an indirectly constrained list that has an indirectly constrained map should compile`() { + val model = + createModel("ItemList") { + """ + @length(min: 1 max: 100) + list ItemList { + member: NestedItemList + } + list NestedItemList { + member: Item + } + map Item { + key: ItemName + value: ItemDescription + } + """ + } + generateAndCompileServer(model) + } }