Unverified Commit ae7b403e authored by Fahad Zubair's avatar Fahad Zubair Committed by GitHub
Browse files

Fix directly constrained `List` shape with indirectly constrained aggregate...


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<Vec<ItemName>>`:

```rust
    impl std::convert::TryFrom<ItemListUnconstrained> for crate::model::ItemList {
        type Error = crate::model::item_list::ConstraintViolation;
        fn try_from(value: ItemListUnconstrained) -> Result<Self, Self::Error> {
            let res: Result<
                ::std::vec::Vec<
                    ::std::collections::HashMap<crate::model::ItemName, ::std::string::String>,
                >,
                (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: default avatarFahad Zubair <fahadzub@amazon.com>
parent fd10a164
Loading
Loading
Loading
Loading
+37 −0
Original line number Diff line number Diff line
@@ -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<Vec<Vec<ItemName>>>`. However, the
                        // intermediate representation generated is `Vec<ItemConstrained>`. This needs to be
                        // transformed into `Vec<Vec<ItemName>>` 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;")
                }
+134 −0
Original line number Diff line number Diff line
@@ -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)
    }
}