From 3e6adcd4150107ab546a985c283c15a4b6318c0d Mon Sep 17 00:00:00 2001 From: david-perez Date: Tue, 20 Dec 2022 14:55:21 +0100 Subject: [PATCH] Disallow ignoring a constrained shape in an event stream's closure (#2113) The server SDK will otherwise produce Rust code that cannot be compiled, so it doesn't make sense to prompt users to disregard the warning and opt into `ignoreUnsupportedConstraints`. To continue synthesizing the model, the user must remove any constrained traits from the event stream's closure. This commit also tweaks formatting of errors yielded by `ValidateUnsupportedConstraints`. Prior to this commit, they were a bit off due to carelessness when interpolation occurs. For example: ``` [SEVERE] Operation com.amazonaws.constraints#ConstrainedShapesOperation takes in input that is constrained(https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a validationexception. You must model this behavior in the operation shape in your model file. ``` --- .../smithy/ValidateUnsupportedConstraints.kt | 34 +++++--- ...ateUnsupportedConstraintsAreNotUsedTest.kt | 80 +++++++++++++------ 2 files changed, 75 insertions(+), 39 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraints.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraints.kt index 0f2f153f7..3b10112f3 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraints.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraints.kt @@ -37,15 +37,21 @@ private sealed class UnsupportedConstraintMessageKind { private val constraintTraitsUberIssue = "https://github.com/awslabs/smithy-rs/issues/1401" fun intoLogMessage(ignoreUnsupportedConstraints: Boolean): LogMessage { - fun buildMessage(intro: String, willSupport: Boolean, trackingIssue: String) = - """ - $intro - This is not supported in the smithy-rs server SDK. - ${if (willSupport) "It will be supported in the future." else ""} - See the tracking issue ($trackingIssue). - If you want to go ahead and generate the server SDK ignoring unsupported constraint traits, set the key `ignoreUnsupportedConstraints` - inside the `runtimeConfig.codegenConfig` JSON object in your `smithy-build.json` to `true`. - """.trimIndent().replace("\n", " ") + fun buildMessage(intro: String, willSupport: Boolean, trackingIssue: String, canBeIgnored: Boolean = true): String { + var msg = """ + $intro + This is not supported in the smithy-rs server SDK.""" + if (willSupport) { + msg += """ + It will be supported in the future. See the tracking issue ($trackingIssue).""" + } + if (canBeIgnored) { + msg += """ + If you want to go ahead and generate the server SDK ignoring unsupported constraint traits, set the key `ignoreUnsupportedConstraints` + inside the `runtimeConfig.codegenConfig` JSON object in your `smithy-build.json` to `true`.""" + } + return msg.trimIndent().replace("\n", " ") + } fun buildMessageShapeHasUnsupportedConstraintTrait( shape: Shape, @@ -67,14 +73,16 @@ private sealed class UnsupportedConstraintMessageKind { ) is UnsupportedConstraintOnShapeReachableViaAnEventStream -> LogMessage( - level, + Level.SEVERE, buildMessage( """ The ${shape.type} shape `${shape.id}` has the constraint trait `${constraintTrait.toShapeId()}` attached. This shape is also part of an event stream; it is unclear what the semantics for constrained shapes in event streams are. + Please remove the trait from the shape to synthesize your model. """.trimIndent().replace("\n", " "), willSupport = false, "https://github.com/awslabs/smithy/issues/1388", + canBeIgnored = false, ), ) @@ -161,9 +169,9 @@ fun validateOperationsWithConstrainedInputHaveValidationExceptionAttached( LogMessage( Level.SEVERE, """ - Operation ${it.shape.id} takes in input that is constrained - (https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a validation - exception. You must model this behavior in the operation shape in your model file. + Operation ${it.shape.id} takes in input that is constrained + (https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a + validation exception. You must model this behavior in the operation shape in your model file. """.trimIndent().replace("\n", "") + """ diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraintsAreNotUsedTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraintsAreNotUsedTest.kt index a6dce7826..c1e016177 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraintsAreNotUsedTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraintsAreNotUsedTest.kt @@ -12,6 +12,7 @@ import io.kotest.matchers.collections.shouldHaveAtLeastSize import io.kotest.matchers.collections.shouldHaveSize import io.kotest.matchers.shouldBe import io.kotest.matchers.string.shouldContain +import io.kotest.matchers.string.shouldNotContain import org.junit.jupiter.api.Test import software.amazon.smithy.model.Model import software.amazon.smithy.model.shapes.ServiceShape @@ -56,7 +57,17 @@ internal class ValidateUnsupportedConstraintsAreNotUsedTest { val validationResult = validateOperationsWithConstrainedInputHaveValidationExceptionAttached(model, service) validationResult.messages shouldHaveSize 1 - validationResult.messages[0].message shouldContain "Operation test#TestOperation takes in input that is constrained" + + // Asserts the exact message, to ensure the formatting is appropriate. + validationResult.messages[0].message shouldBe """Operation test#TestOperation takes in input that is constrained (https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a validation exception. You must model this behavior in the operation shape in your model file. +```smithy +use smithy.framework#ValidationException + +operation TestOperation { + ... + errors: [..., ValidationException] // <-- Add this. +} +```""" } @Test @@ -120,35 +131,36 @@ internal class ValidateUnsupportedConstraintsAreNotUsedTest { } } - @Test - fun `it should detect when constraint traits in event streams are used`() { - val model = - """ - $baseModel + val constrainedShapesInEventStreamModel = + """ + $baseModel - structure TestInputOutput { - eventStream: EventStream - } + structure TestInputOutput { + eventStream: EventStream + } - @streaming - union EventStream { - message: Message, - error: Error - } + @streaming + union EventStream { + message: Message, + error: Error + } - structure Message { - lengthString: LengthString - } + structure Message { + lengthString: LengthString + } - structure Error { - @required - message: String - } + structure Error { + @required + message: String + } - @length(min: 1) - string LengthString - """.asSmithyModel() - val validationResult = validateModel(EventStreamNormalizer.transform(model)) + @length(min: 1) + string LengthString + """.asSmithyModel() + + @Test + fun `it should detect when constraint traits in event streams are used`() { + val validationResult = validateModel(EventStreamNormalizer.transform(constrainedShapesInEventStreamModel)) validationResult.messages shouldHaveSize 2 validationResult.messages.forOne { @@ -156,17 +168,31 @@ internal class ValidateUnsupportedConstraintsAreNotUsedTest { """ The string shape `test#LengthString` has the constraint trait `smithy.api#length` attached. This shape is also part of an event stream; it is unclear what the semantics for constrained shapes in event streams are. + Please remove the trait from the shape to synthesize your model. """.trimIndent().replace("\n", " ") + it.message shouldNotContain "If you want to go ahead and generate the server SDK ignoring unsupported constraint traits" } validationResult.messages.forOne { it.message shouldContain """ The member shape `test#Error${"$"}message` has the constraint trait `smithy.api#required` attached. This shape is also part of an event stream; it is unclear what the semantics for constrained shapes in event streams are. + Please remove the trait from the shape to synthesize your model. """.trimIndent().replace("\n", " ") + it.message shouldNotContain "If you want to go ahead and generate the server SDK ignoring unsupported constraint traits" } } + @Test + fun `it should abort when constraint traits in event streams are used, despite opting into ignoreUnsupportedConstraintTraits`() { + val validationResult = validateModel( + EventStreamNormalizer.transform(constrainedShapesInEventStreamModel), + ServerCodegenConfig().copy(ignoreUnsupportedConstraints = true), + ) + + validationResult.shouldAbort shouldBe true + } + @Test fun `it should detect when the length trait on blob shapes is used`() { val model = @@ -183,7 +209,9 @@ internal class ValidateUnsupportedConstraintsAreNotUsedTest { val validationResult = validateModel(model) validationResult.messages shouldHaveSize 1 - validationResult.messages[0].message shouldContain "The blob shape `test#LengthBlob` has the constraint trait `smithy.api#length` attached" + + // This test additionally asserts the exact message, to ensure the formatting is appropriate. + validationResult.messages[0].message shouldBe "The blob shape `test#LengthBlob` has the constraint trait `smithy.api#length` attached. This is not supported in the smithy-rs server SDK. It will be supported in the future. See the tracking issue (https://github.com/awslabs/smithy-rs/issues/1401). If you want to go ahead and generate the server SDK ignoring unsupported constraint traits, set the key `ignoreUnsupportedConstraints` inside the `runtimeConfig.codegenConfig` JSON object in your `smithy-build.json` to `true`." } @Test -- GitLab