From b270e8b11adbb3d21472cfa12fbb0f6b5592729b Mon Sep 17 00:00:00 2001 From: david-perez Date: Fri, 9 Dec 2022 16:53:36 +0100 Subject: [PATCH] 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. --- .../common-test-models/constraints.smithy | 18 +++--- .../ConstrainedCollectionGenerator.kt | 10 +++- .../generators/ConstrainedMapGenerator.kt | 9 ++- .../UnconstrainedCollectionGenerator.kt | 60 +++++++++++++------ .../protocol/ServerProtocolTestGenerator.kt | 4 -- 5 files changed, 69 insertions(+), 32 deletions(-) diff --git a/codegen-core/common-test-models/constraints.smithy b/codegen-core/common-test-models/constraints.smithy index 3e6ea4bf2e..3963f6f3ff 100644 --- a/codegen-core/common-test-models/constraints.smithy +++ b/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 diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ConstrainedCollectionGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ConstrainedCollectionGenerator.kt index 857b6aa829..125293548e 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ConstrainedCollectionGenerator.kt +++ b/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} { diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ConstrainedMapGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ConstrainedMapGenerator.kt index 69cfafc698..af59163487 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ConstrainedMapGenerator.kt +++ b/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} { diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt index b337c6f629..a2e3e94265 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedCollectionGenerator.kt @@ -8,6 +8,8 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators import software.amazon.smithy.model.shapes.CollectionShape 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 +40,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 +51,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 +77,49 @@ 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 { - let res: Result<_, (usize, #{InnerConstraintViolationSymbol})> = value + rustBlock("fn try_from(value: $name) -> Result") { + if (innerShape.canReachConstrainedShape(model, symbolProvider)) { + val innerConstrainedSymbol = constrainedShapeSymbolProvider.toSymbol(innerShape) + + rustTemplate( + """ + let res: Result, (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))") + } + } } } } diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt index e73c3902fb..b441387f67 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt @@ -947,10 +947,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),