Fix 3 bugs in @length-constrained collection and map shapes (#2085)

* Fix 3 bugs in `@length`-constrained collection and map shapes

The 3 bugs are related, hence why a single commit to address them all.

1. #2028 added support for the `@length` constraint trait on collection
   shapes, but the code enforcing the trait was not being exercised upon
   receiving a request, specifically when converting the input
   unconstrained list into the constrained one. Note that #2028 did not
   result in the removal of the relevant protocol tests from
   `ServerProtocolTestGenerator`'s list of known failing tests.
2. Fixes code generation of `@length`-constrained list shapes whose
   members are not constrained: the converter being generated only
   worked for the case where the member was (transitively) constrained.
   The `constraints.smithy` model has been expanded to cover this case.
3. Fixes bug in code generation, when the
   `codegenConfig.publicConstrainedTypes` setting is set to `false`, of
   `@length`-constrained map shapes and collection shapes whose values
   or members (respectively) are constrained, but result in Rust types
   that would have been public regardless of the setting's value. This
   is the case only when they are modeled as structure shapes or union
   shapes. In these cases, two converters from the constrained type to
   the inner type were being generated, resulting in two `From` trait
   implementations. The `constraints.smithy` model has been expanded to
   cover this case.

* Use correct symbol provider

* ./gradlew ktlintFormat

Co-authored-by: Harry Barber <[email protected]>

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

@@ -467,13 +467,14 @@ structure ConA {
     fixedValueByte: FixedValueByte,
 
     conBList: ConBList,
-    conBList2: ConBList2,
+    lengthList: LengthList,
 
     // TODO(https://github.com/awslabs/smithy-rs/issues/1401): a `set` shape is
     //  just a `list` shape with `uniqueItems`, which hasn't been implemented yet.
     // conBSet: ConBSet,
 
     conBMap: ConBMap,
+    lengthMap: LengthMap,
 
     mapOfMapOfListOfListOfConB: MapOfMapOfListOfListOfConB,
 
@@ -837,14 +838,11 @@ list RecursiveList {
 }
 
 list ConBList {
-    member: NestedList
+    member: LengthList
 }
 
-list ConBList2 {
-    member: ConB
-}
-
-list NestedList {
+@length(max: 69)
+list LengthList {
     member: ConB
 }
 
@@ -874,6 +872,12 @@ map ConBMap {
     value: LengthString
 }
 
+@length(min: 1, max: 69)
+map LengthMap {
+    key: String,
+    value: String
+}
+
 @error("client")
 structure ErrorWithLengthStringMessage {
     // TODO Doesn't work yet because constrained string types don't implement

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

@@ -7,6 +7,8 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators
 
 import software.amazon.smithy.codegen.core.Symbol
 import software.amazon.smithy.model.shapes.CollectionShape
+import software.amazon.smithy.model.shapes.StructureShape
+import software.amazon.smithy.model.shapes.UnionShape
 import software.amazon.smithy.model.traits.LengthTrait
 import software.amazon.smithy.model.traits.Trait
 import software.amazon.smithy.rust.codegen.core.rustlang.Attribute
@@ -23,6 +25,7 @@ import software.amazon.smithy.rust.codegen.core.util.PANIC
 import software.amazon.smithy.rust.codegen.core.util.orNull
 import software.amazon.smithy.rust.codegen.server.smithy.PubCrateConstraintViolationSymbolProvider
 import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext
+import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape
 import software.amazon.smithy.rust.codegen.server.smithy.supportedCollectionConstraintTraits
 import software.amazon.smithy.rust.codegen.server.smithy.validationErrorMessage
 
@@ -132,7 +135,12 @@ class ConstrainedCollectionGenerator(
             "ValidationFunctions" to constraintsInfo.map { it.validationFunctionDefinition(constraintViolation, inner) }.join("\n"),
         )
 
-        if (!publicConstrainedTypes && isValueConstrained(shape, model, symbolProvider)) {
+        val innerShape = model.expectShape(shape.member.target)
+        if (!publicConstrainedTypes &&
+            innerShape.canReachConstrainedShape(model, symbolProvider) &&
+            innerShape !is StructureShape &&
+            innerShape !is UnionShape
+        ) {
             writer.rustTemplate(
                 """
                 impl #{From}<$name> for #{FullyUnconstrainedSymbol} {

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

@@ -7,6 +7,8 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators
 
 import software.amazon.smithy.codegen.core.Symbol
 import software.amazon.smithy.model.shapes.MapShape
+import software.amazon.smithy.model.shapes.StructureShape
+import software.amazon.smithy.model.shapes.UnionShape
 import software.amazon.smithy.model.traits.LengthTrait
 import software.amazon.smithy.rust.codegen.core.rustlang.Attribute
 import software.amazon.smithy.rust.codegen.core.rustlang.RustMetadata
@@ -115,7 +117,12 @@ class ConstrainedMapGenerator(
             *codegenScope,
         )
 
-        if (!publicConstrainedTypes && isValueConstrained(shape, model, symbolProvider)) {
+        val valueShape = model.expectShape(shape.value.target)
+        if (!publicConstrainedTypes &&
+            isValueConstrained(valueShape, model, symbolProvider) &&
+            valueShape !is StructureShape &&
+            valueShape !is UnionShape
+        ) {
             writer.rustTemplate(
                 """
                 impl #{From}<$name> for #{FullyUnconstrainedSymbol} {

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

@@ -6,8 +6,12 @@
 package software.amazon.smithy.rust.codegen.server.smithy.generators
 
 import software.amazon.smithy.model.shapes.CollectionShape
+import software.amazon.smithy.model.shapes.StructureShape
+import software.amazon.smithy.model.shapes.UnionShape
 import software.amazon.smithy.rust.codegen.core.rustlang.RustType
 import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
+import software.amazon.smithy.rust.codegen.core.rustlang.rust
+import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
 import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
 import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
 import software.amazon.smithy.rust.codegen.core.smithy.makeMaybeConstrained
@@ -38,6 +42,8 @@ class UnconstrainedCollectionGenerator(
     private val symbolProvider = codegenContext.symbolProvider
     private val unconstrainedShapeSymbolProvider = codegenContext.unconstrainedShapeSymbolProvider
     private val pubCrateConstrainedShapeSymbolProvider = codegenContext.pubCrateConstrainedShapeSymbolProvider
+    private val symbol = unconstrainedShapeSymbolProvider.toSymbol(shape)
+    private val name = symbol.name
     private val publicConstrainedTypes = codegenContext.settings.codegenConfig.publicConstrainedTypes
     private val constraintViolationSymbolProvider =
         with(codegenContext.constraintViolationSymbolProvider) {
@@ -47,22 +53,20 @@ class UnconstrainedCollectionGenerator(
                 PubCrateConstraintViolationSymbolProvider(this)
             }
         }
+    private val constraintViolationSymbol = constraintViolationSymbolProvider.toSymbol(shape)
     private val constrainedShapeSymbolProvider = codegenContext.constrainedShapeSymbolProvider
     private val constrainedSymbol = if (shape.isDirectlyConstrained(symbolProvider)) {
         constrainedShapeSymbolProvider.toSymbol(shape)
     } else {
         pubCrateConstrainedShapeSymbolProvider.toSymbol(shape)
     }
+    private val innerShape = model.expectShape(shape.member.target)
 
     fun render() {
         check(shape.canReachConstrainedShape(model, symbolProvider))
 
-        val symbol = unconstrainedShapeSymbolProvider.toSymbol(shape)
-        val name = symbol.name
         val innerShape = model.expectShape(shape.member.target)
         val innerUnconstrainedSymbol = unconstrainedShapeSymbolProvider.toSymbol(innerShape)
-        val constraintViolationSymbol = constraintViolationSymbolProvider.toSymbol(shape)
-        val innerConstraintViolationSymbol = constraintViolationSymbolProvider.toSymbol(innerShape)
 
         unconstrainedModuleWriter.withInlineModule(symbol.module()) {
             rustTemplate(
@@ -75,29 +79,58 @@ class UnconstrainedCollectionGenerator(
                         Self::Unconstrained(value)
                     }
                 }
+                """,
+                "InnerUnconstrainedSymbol" to innerUnconstrainedSymbol,
+                "MaybeConstrained" to constrainedSymbol.makeMaybeConstrained(),
+            )
+
+            renderTryFromUnconstrainedForConstrained(this)
+        }
+    }
+
+    private fun renderTryFromUnconstrainedForConstrained(writer: RustWriter) {
+        val innerConstraintViolationSymbol = constraintViolationSymbolProvider.toSymbol(innerShape)
 
-                impl #{TryFrom}<$name> for #{ConstrainedSymbol} {
-                    type Error = #{ConstraintViolationSymbol};
+        writer.rustBlock("impl std::convert::TryFrom<$name> for #{T}", constrainedSymbol) {
+            rust("type Error = #T;", constraintViolationSymbol)
 
-                    fn try_from(value: $name) -> Result<Self, Self::Error> {
-                        let res: Result<_, (usize, #{InnerConstraintViolationSymbol})> = value
+            rustBlock("fn try_from(value: $name) -> Result<Self, Self::Error>") {
+                if (innerShape.canReachConstrainedShape(model, symbolProvider)) {
+                    val resolvesToNonPublicConstrainedValueType =
+                        innerShape.canReachConstrainedShape(model, symbolProvider) &&
+                            !innerShape.isDirectlyConstrained(symbolProvider) &&
+                            innerShape !is StructureShape &&
+                            innerShape !is UnionShape
+                    val innerConstrainedSymbol = if (resolvesToNonPublicConstrainedValueType) {
+                        pubCrateConstrainedShapeSymbolProvider.toSymbol(innerShape)
+                    } else {
+                        constrainedShapeSymbolProvider.toSymbol(innerShape)
+                    }
+
+                    rustTemplate(
+                        """
+                        let res: Result<std::vec::Vec<#{InnerConstrainedSymbol}>, (usize, #{InnerConstraintViolationSymbol})> = value
                             .0
                             .into_iter()
                             .enumerate()
                             .map(|(idx, inner)| inner.try_into().map_err(|inner_violation| (idx, inner_violation)))
                             .collect();
-                        res.map(Self)
-                           .map_err(|(idx, inner_violation)| #{ConstraintViolationSymbol}::Member(idx, inner_violation))
-                    }
+                        let inner = res.map_err(|(idx, inner_violation)| Self::Error::Member(idx, inner_violation))?;
+                        """,
+                        "InnerConstrainedSymbol" to innerConstrainedSymbol,
+                        "InnerConstraintViolationSymbol" to innerConstraintViolationSymbol,
+                        "TryFrom" to RuntimeType.TryFrom,
+                    )
+                } else {
+                    rust("let inner = value.0;")
                 }
-                """,
-                "InnerUnconstrainedSymbol" to innerUnconstrainedSymbol,
-                "InnerConstraintViolationSymbol" to innerConstraintViolationSymbol,
-                "ConstrainedSymbol" to constrainedSymbol,
-                "ConstraintViolationSymbol" to constraintViolationSymbol,
-                "MaybeConstrained" to constrainedSymbol.makeMaybeConstrained(),
-                "TryFrom" to RuntimeType.TryFrom,
-            )
+
+                if (shape.isDirectlyConstrained(symbolProvider)) {
+                    rust("Self::try_from(inner)")
+                } else {
+                    rust("Ok(Self(inner))")
+                }
+            }
         }
     }
 }

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

@@ -7,6 +7,8 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators
 
 import software.amazon.smithy.model.shapes.MapShape
 import software.amazon.smithy.model.shapes.StringShape
+import software.amazon.smithy.model.shapes.StructureShape
+import software.amazon.smithy.model.shapes.UnionShape
 import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
 import software.amazon.smithy.rust.codegen.core.rustlang.join
 import software.amazon.smithy.rust.codegen.core.rustlang.rust
@@ -95,11 +97,12 @@ class UnconstrainedMapGenerator(
 
             rustBlock("fn try_from(value: $name) -> Result<Self, Self::Error>") {
                 if (isKeyConstrained(keyShape, symbolProvider) || isValueConstrained(valueShape, model, symbolProvider)) {
-                    val resolveToNonPublicConstrainedValueType =
+                    val resolvesToNonPublicConstrainedValueType =
                         isValueConstrained(valueShape, model, symbolProvider) &&
                             !valueShape.isDirectlyConstrained(symbolProvider) &&
-                            !valueShape.isStructureShape
-                    val constrainedValueSymbol = if (resolveToNonPublicConstrainedValueType) {
+                            valueShape !is StructureShape &&
+                            valueShape !is UnionShape
+                    val constrainedValueSymbol = if (resolvesToNonPublicConstrainedValueType) {
                         pubCrateConstrainedShapeSymbolProvider.toSymbol(valueShape)
                     } else {
                         constrainedShapeSymbolProvider.toSymbol(valueShape)
@@ -154,7 +157,7 @@ class UnconstrainedMapGenerator(
                     )
 
                     val constrainedValueTypeIsNotFinalType =
-                        resolveToNonPublicConstrainedValueType && shape.isDirectlyConstrained(symbolProvider)
+                        resolvesToNonPublicConstrainedValueType && shape.isDirectlyConstrained(symbolProvider)
                     if (constrainedValueTypeIsNotFinalType) {
                         // The map is constrained. Its value shape reaches a constrained shape, but the value shape itself
                         // is not directly constrained. The value shape must be an aggregate shape. But it is not a

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt

@@ -946,10 +946,6 @@ class ServerProtocolTestGenerator(
             // See https://github.com/awslabs/smithy-rs/issues/1401.
             FailingTest(RestJsonValidation, "RestJsonMalformedLengthBlob_case0", TestType.MalformedRequest),
             FailingTest(RestJsonValidation, "RestJsonMalformedLengthBlob_case1", TestType.MalformedRequest),
-            FailingTest(RestJsonValidation, "RestJsonMalformedLengthList_case0", TestType.MalformedRequest),
-            FailingTest(RestJsonValidation, "RestJsonMalformedLengthList_case1", TestType.MalformedRequest),
-            FailingTest(RestJsonValidation, "RestJsonMalformedLengthMapValue_case0", TestType.MalformedRequest),
-            FailingTest(RestJsonValidation, "RestJsonMalformedLengthMapValue_case1", TestType.MalformedRequest),
             FailingTest(RestJsonValidation, "RestJsonMalformedRangeFloat_case0", TestType.MalformedRequest),
             FailingTest(RestJsonValidation, "RestJsonMalformedRangeFloat_case1", TestType.MalformedRequest),
             FailingTest(RestJsonValidation, "RestJsonMalformedRangeMaxFloat", TestType.MalformedRequest),