Skip to content

Commit

Permalink
Fix directly constrained List shape with indirectly constrained agg…
Browse files Browse the repository at this point in the history 
…regate 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: Fahad Zubair <fahadzub@amazon.com>
  • Loading branch information
@drganjoo
drganjoo and Fahad Zubair authored Nov 12, 2024
1 parent fd10a16 commit ae7b403
Show file tree
Hide file tree
Showing 2 changed files with 171 additions and 0 deletions.
37 changes: 37 additions & 0 deletions ...e/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;")
}
Expand Down
134 changes: 134 additions & 0 deletions ...rver/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
}
}

0 comments on commit ae7b403

Please sign in to comment.