Skip to content

Commit

Permalink
Operations with an event stream member shape must have `ValidationExc…
Browse files Browse the repository at this point in the history
…eption` (#3814)

This PR addresses an issue where, if an operation's input includes an
event streaming member, the builder for the operation input or output
may raise a `ConstraintViolation` when `build_enforcing_all_constraints`
is called and the event streaming member field is not set. This occurs
because the member shape is required.

The standard error message that is shown when `ValidationException` is
not attached to an operation is also displayed in this case:

*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.
}
```
Closes: [3813](#3813)

---------

Co-authored-by: Fahad Zubair <[email protected]>
  • Loading branch information
drganjoo and Fahad Zubair authored Sep 4, 2024
1 parent ce46875 commit 2c3a4c1
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 2 deletions.
9 changes: 9 additions & 0 deletions .changelog/7869753.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
applies_to: ["server"]
authors: ["drganjoo"]
references: ["smithy-rs#3813"]
breaking: true
new_feature: false
bug_fix: true
---
Operations with event stream member shapes must include `ValidationException` in the errors list. This is necessary because the member shape is a required field, and the builder for the operation input or output returns a `std::result::Result` with the error set to `crate::model::ValidationExceptionField`.
1 change: 1 addition & 0 deletions codegen-core/common-test-models/constraints.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ operation StreamingBlobOperation {
operation EventStreamsOperation {
input: EventStreamsOperationInputOutput,
output: EventStreamsOperationInputOutput,
errors: [ValidationException]
}

structure ConstrainedShapesOperationInputOutput {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import software.amazon.smithy.model.traits.UniqueItemsTrait
import software.amazon.smithy.rust.codegen.core.smithy.DirectedWalker
import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticEventStreamUnionTrait
import software.amazon.smithy.rust.codegen.core.util.expectTrait
import software.amazon.smithy.rust.codegen.core.util.hasEventStreamMember
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.inputShape
import software.amazon.smithy.rust.codegen.core.util.orNull
Expand Down Expand Up @@ -190,7 +191,7 @@ fun operationShapesThatMustHaveValidationException(
.filter { operationShape ->
// Walk the shapes reachable via this operation input.
walker.walkShapes(operationShape.inputShape(model))
.any { it is SetShape || it is EnumShape || it.hasConstraintTrait() }
.any { it is SetShape || it is EnumShape || it.hasConstraintTrait() || it.hasEventStreamMember(model) }
}
.toSet()
}
Expand All @@ -207,7 +208,6 @@ fun validateOperationsWithConstrainedInputHaveValidationExceptionAttached(
// `ValidationException` attached in `errors`. https://github.com/smithy-lang/smithy-rs/pull/1199#discussion_r809424783
// TODO(https://github.com/smithy-lang/smithy-rs/issues/1401): This check will go away once we add support for
// `disableDefaultValidation` set to `true`, allowing service owners to map from constraint violations to operation errors.
val walker = DirectedWalker(model)
val operationsWithConstrainedInputWithoutValidationExceptionSet =
operationShapesThatMustHaveValidationException(model, service)
.filter { !it.errors.contains(validationExceptionShapeId) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,51 @@ internal class ValidateUnsupportedConstraintsAreNotUsedTest {
""".trimIndent()
}

@Test
fun `should detect when event streams are used, even without constraints, as the event member is required`() {
val model =
"""
$baseModel
structure TestInputOutput {
eventStream: EventStream
}
@streaming
union EventStream {
message: Message,
error: Error
}
structure Message {
lengthString: String
}
structure Error {
message: String
}
""".asSmithyModel()
val service = model.lookup<ServiceShape>("test#TestService")
val validationResult =
validateOperationsWithConstrainedInputHaveValidationExceptionAttached(
model,
service,
SmithyValidationExceptionConversionGenerator.SHAPE_ID,
)

validationResult.messages shouldHaveSize 1

// 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.
}
```
""".trimIndent()
}

private val constraintTraitOnStreamingBlobShapeModel =
"""
$baseModel
Expand Down

0 comments on commit 2c3a4c1

Please sign in to comment.