Skip to content

Commit 356444b

Browse files
david-perezdavid-perez
authored
Fix server SDK bug with directly constrained list/map shapes in operation output (#2761)
Fixes #2760. ## Testing I verified that the updated `constraints.smithy` integration test does not compile without the fix applied. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
1 parent 62dca85 commit 356444b

File tree

5 files changed

+140
-154
lines changed

5 files changed

+140
-154
lines changed

‎codegen-core/common-test-models/constraints.smithy

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@ use smithy.framework#ValidationException
1111
service ConstraintsService {
1212
operations: [
1313
ConstrainedShapesOperation,
14+
// See https://github.com/awslabs/smithy-rs/issues/2760 for why testing operations reaching
15+
// constrained shapes that only lie in the output is important.
16+
ConstrainedShapesOnlyInOutputOperation,
1417
ConstrainedHttpBoundShapesOperation,
1518
ConstrainedHttpPayloadBoundShapeOperation,
1619
ConstrainedRecursiveShapesOperation,
@@ -51,6 +54,11 @@ operation ConstrainedShapesOperation {
5154
errors: [ValidationException]
5255
}
5356

57+
@http(uri: "/constrained-shapes-only-in-output-operation", method: "POST")
58+
operation ConstrainedShapesOnlyInOutputOperation {
59+
output: ConstrainedShapesOnlyInOutputOperationOutput,
60+
}
61+
5462
@http(
5563
uri: "/constrained-http-bound-shapes-operation/{rangeIntegerLabel}/{rangeShortLabel}/{rangeLongLabel}/{rangeByteLabel}/{lengthStringLabel}/{enumStringLabel}",
5664
method: "POST"
@@ -935,3 +943,31 @@ map MapOfListOfListOfConB {
935943
key: String,
936944
value: ConBList
937945
}
946+
947+
structure ConstrainedShapesOnlyInOutputOperationOutput {
948+
list: ConstrainedListInOutput
949+
map: ConstrainedMapInOutput
950+
// Unions were not affected by
951+
// https://github.com/awslabs/smithy-rs/issues/2760, but testing anyway for
952+
// good measure.
953+
union: ConstrainedUnionInOutput
954+
}
955+
956+
@length(min: 69)
957+
list ConstrainedListInOutput {
958+
member: ConstrainedUnionInOutput
959+
}
960+
961+
@length(min: 69)
962+
map ConstrainedMapInOutput {
963+
key: String
964+
value: TransitivelyConstrainedStructureInOutput
965+
}
966+
967+
union ConstrainedUnionInOutput {
968+
structure: TransitivelyConstrainedStructureInOutput
969+
}
970+
971+
structure TransitivelyConstrainedStructureInOutput {
972+
lengthString: LengthString
973+
}

‎codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/CollectionConstraintViolationGenerator.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class CollectionConstraintViolationGenerator(
4848

4949
inlineModuleCreator(constraintViolationSymbol) {
5050
val constraintViolationVariants = constraintsInfo.map { it.constraintViolationVariant }.toMutableList()
51-
if (isMemberConstrained) {
51+
if (shape.isReachableFromOperationInput() && isMemberConstrained) {
5252
constraintViolationVariants += {
5353
val memberConstraintViolationSymbol =
5454
constraintViolationSymbolProvider.toSymbol(targetShape).letIf(

‎codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/MapConstraintViolationGenerator.kt

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,12 @@ class MapConstraintViolationGenerator(
4545
val constraintViolationName = constraintViolationSymbol.name
4646

4747
val constraintViolationCodegenScopeMutableList: MutableList<Pair<String, Any>> = mutableListOf()
48-
if (isKeyConstrained(keyShape, symbolProvider)) {
48+
val keyConstraintViolationExists = shape.isReachableFromOperationInput() && isKeyConstrained(keyShape, symbolProvider)
49+
val valueConstraintViolationExists = shape.isReachableFromOperationInput() && isValueConstrained(valueShape, model, symbolProvider)
50+
if (keyConstraintViolationExists) {
4951
constraintViolationCodegenScopeMutableList.add("KeyConstraintViolationSymbol" to constraintViolationSymbolProvider.toSymbol(keyShape))
5052
}
51-
if (isValueConstrained(valueShape, model, symbolProvider)) {
53+
if (valueConstraintViolationExists) {
5254
constraintViolationCodegenScopeMutableList.add(
5355
"ValueConstraintViolationSymbol" to
5456
constraintViolationSymbolProvider.toSymbol(valueShape).letIf(
@@ -78,8 +80,8 @@ class MapConstraintViolationGenerator(
7880
##[derive(Debug, PartialEq)]
7981
pub${ if (constraintViolationVisibility == Visibility.PUBCRATE) " (crate) " else "" } enum $constraintViolationName {
8082
${if (shape.hasTrait<LengthTrait>()) "Length(usize)," else ""}
81-
${if (isKeyConstrained(keyShape, symbolProvider)) "##[doc(hidden)] Key(#{KeyConstraintViolationSymbol})," else ""}
82-
${if (isValueConstrained(valueShape, model, symbolProvider)) "##[doc(hidden)] Value(#{KeySymbol}, #{ValueConstraintViolationSymbol})," else ""}
83+
${if (keyConstraintViolationExists) "##[doc(hidden)] Key(#{KeyConstraintViolationSymbol})," else ""}
84+
${if (valueConstraintViolationExists) "##[doc(hidden)] Value(#{KeySymbol}, #{ValueConstraintViolationSymbol})," else ""}
8385
}
8486
""",
8587
*constraintViolationCodegenScope,

‎codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGeneratorTest.kt

Lines changed: 49 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,37 @@
66
package software.amazon.smithy.rust.codegen.server.smithy.generators
77

88
import org.junit.jupiter.api.Test
9-
import software.amazon.smithy.model.shapes.ListShape
10-
import software.amazon.smithy.model.shapes.StructureShape
11-
import software.amazon.smithy.rust.codegen.core.testutil.TestWorkspace
9+
import software.amazon.smithy.rust.codegen.core.rustlang.rust
1210
import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel
13-
import software.amazon.smithy.rust.codegen.core.testutil.compileAndTest
11+
import software.amazon.smithy.rust.codegen.core.testutil.testModule
1412
import software.amazon.smithy.rust.codegen.core.testutil.unitTest
15-
import software.amazon.smithy.rust.codegen.core.util.lookup
16-
import software.amazon.smithy.rust.codegen.server.smithy.ServerRustModule
17-
import software.amazon.smithy.rust.codegen.server.smithy.createTestInlineModuleCreator
18-
import software.amazon.smithy.rust.codegen.server.smithy.customizations.SmithyValidationExceptionConversionGenerator
19-
import software.amazon.smithy.rust.codegen.server.smithy.generators.protocol.ServerRestJsonProtocol
20-
import software.amazon.smithy.rust.codegen.server.smithy.renderInlineMemoryModules
21-
import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverRenderWithModelBuilder
22-
import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestCodegenContext
13+
import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverIntegrationTest
2314

2415
class UnconstrainedCollectionGeneratorTest {
2516
@Test
2617
fun `it should generate unconstrained lists`() {
2718
val model =
2819
"""
2920
namespace test
21+
22+
use aws.protocols#restJson1
23+
use smithy.framework#ValidationException
24+
25+
@restJson1
26+
service TestService {
27+
operations: ["Operation"]
28+
}
29+
30+
@http(uri: "/operation", method: "POST")
31+
operation Operation {
32+
input: OperationInputOutput
33+
output: OperationInputOutput
34+
errors: [ValidationException]
35+
}
36+
37+
structure OperationInputOutput {
38+
list: ListA
39+
}
3040
3141
list ListA {
3242
member: ListB
@@ -44,58 +54,16 @@ class UnconstrainedCollectionGeneratorTest {
4454
string: String
4555
}
4656
""".asSmithyModel()
47-
val codegenContext = serverTestCodegenContext(model)
48-
val symbolProvider = codegenContext.symbolProvider
49-
50-
val listA = model.lookup<ListShape>("test#ListA")
51-
val listB = model.lookup<ListShape>("test#ListB")
52-
53-
val project = TestWorkspace.testProject(symbolProvider)
54-
55-
project.withModule(ServerRustModule.Model) {
56-
model.lookup<StructureShape>("test#StructureC").serverRenderWithModelBuilder(
57-
project,
58-
model,
59-
symbolProvider,
60-
this,
61-
ServerRestJsonProtocol(codegenContext),
62-
)
63-
}
64-
65-
project.withModule(ServerRustModule.ConstrainedModule) {
66-
listOf(listA, listB).forEach {
67-
PubCrateConstrainedCollectionGenerator(
68-
codegenContext,
69-
this.createTestInlineModuleCreator(),
70-
it,
71-
).render()
72-
}
73-
}
74-
project.withModule(ServerRustModule.UnconstrainedModule) unconstrainedModuleWriter@{
75-
project.withModule(ServerRustModule.Model) modelsModuleWriter@{
76-
listOf(listA, listB).forEach {
77-
UnconstrainedCollectionGenerator(
78-
codegenContext,
79-
this@unconstrainedModuleWriter.createTestInlineModuleCreator(),
80-
it,
81-
).render()
82-
83-
CollectionConstraintViolationGenerator(
84-
codegenContext,
85-
this@modelsModuleWriter.createTestInlineModuleCreator(),
86-
it,
87-
CollectionTraitInfo.fromShape(it, codegenContext.constrainedShapeSymbolProvider),
88-
SmithyValidationExceptionConversionGenerator(codegenContext),
89-
).render()
90-
}
9157

92-
this@unconstrainedModuleWriter.unitTest(
93-
name = "list_a_unconstrained_fail_to_constrain_with_first_error",
94-
test = """
58+
serverIntegrationTest(model) { _, rustCrate ->
59+
rustCrate.testModule {
60+
unitTest("list_a_unconstrained_fail_to_constrain_with_first_error") {
61+
rust(
62+
"""
9563
let c_builder1 = crate::model::StructureC::builder().int(69);
9664
let c_builder2 = crate::model::StructureC::builder().string("david".to_owned());
97-
let list_b_unconstrained = list_b_unconstrained::ListBUnconstrained(vec![c_builder1, c_builder2]);
98-
let list_a_unconstrained = list_a_unconstrained::ListAUnconstrained(vec![list_b_unconstrained]);
65+
let list_b_unconstrained = crate::unconstrained::list_b_unconstrained::ListBUnconstrained(vec![c_builder1, c_builder2]);
66+
let list_a_unconstrained = crate::unconstrained::list_a_unconstrained::ListAUnconstrained(vec![list_b_unconstrained]);
9967
10068
let expected_err =
10169
crate::model::list_a::ConstraintViolation::Member(0, crate::model::list_b::ConstraintViolation::Member(
@@ -106,15 +74,16 @@ class UnconstrainedCollectionGeneratorTest {
10674
expected_err,
10775
crate::constrained::list_a_constrained::ListAConstrained::try_from(list_a_unconstrained).unwrap_err()
10876
);
109-
""",
110-
)
77+
""",
78+
)
79+
}
11180

112-
this@unconstrainedModuleWriter.unitTest(
113-
name = "list_a_unconstrained_succeed_to_constrain",
114-
test = """
81+
unitTest("list_a_unconstrained_succeed_to_constrain") {
82+
rust(
83+
"""
11584
let c_builder = crate::model::StructureC::builder().int(69).string(String::from("david"));
116-
let list_b_unconstrained = list_b_unconstrained::ListBUnconstrained(vec![c_builder]);
117-
let list_a_unconstrained = list_a_unconstrained::ListAUnconstrained(vec![list_b_unconstrained]);
85+
let list_b_unconstrained = crate::unconstrained::list_b_unconstrained::ListBUnconstrained(vec![c_builder]);
86+
let list_a_unconstrained = crate::unconstrained::list_a_unconstrained::ListAUnconstrained(vec![list_b_unconstrained]);
11887
11988
let expected: Vec<Vec<crate::model::StructureC>> = vec![vec![crate::model::StructureC {
12089
string: "david".to_owned(),
@@ -124,22 +93,22 @@ class UnconstrainedCollectionGeneratorTest {
12493
crate::constrained::list_a_constrained::ListAConstrained::try_from(list_a_unconstrained).unwrap().into();
12594
12695
assert_eq!(expected, actual);
127-
""",
128-
)
96+
""",
97+
)
98+
}
12999

130-
this@unconstrainedModuleWriter.unitTest(
131-
name = "list_a_unconstrained_converts_into_constrained",
132-
test = """
100+
unitTest("list_a_unconstrained_converts_into_constrained") {
101+
rust(
102+
"""
133103
let c_builder = crate::model::StructureC::builder();
134-
let list_b_unconstrained = list_b_unconstrained::ListBUnconstrained(vec![c_builder]);
135-
let list_a_unconstrained = list_a_unconstrained::ListAUnconstrained(vec![list_b_unconstrained]);
136-
104+
let list_b_unconstrained = crate::unconstrained::list_b_unconstrained::ListBUnconstrained(vec![c_builder]);
105+
let list_a_unconstrained = crate::unconstrained::list_a_unconstrained::ListAUnconstrained(vec![list_b_unconstrained]);
106+
137107
let _list_a: crate::constrained::MaybeConstrained<crate::constrained::list_a_constrained::ListAConstrained> = list_a_unconstrained.into();
138-
""",
139-
)
108+
""",
109+
)
110+
}
140111
}
141112
}
142-
project.renderInlineMemoryModules()
143-
project.compileAndTest()
144113
}
145114
}

0 commit comments

Comments
 (0)