Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix directly constrained List shape with indirectly constrained aggregate type member shape #3894

Merged
merged 6 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather move all these to constraints.smithy, since, as noted here, simply compiling the crate is sufficient. Each of these is generating and compiling a brand new crate, which adds time. Instead, constraints.smithy constitues a "kitchen-sink"-style test for all constraints-related things.

Copy link
Contributor Author

@drganjoo drganjoo Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agreed. I should have added a TODO here to clarify why these tests are included. There’s an earlier commit in this PR that adds them to constraints.smithy, but our pubConstrainedTypes = false implementation is currently incomplete and has several edge cases, causing the PR to fail. I’ve created a new issue (#3895) to document all related edge cases. I initially tried to address these within this PR but encountered cases requiring more refactoring than is feasible here.

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)
}
}
Loading