From 306eccd3bd607ab2e6f655baa7bd1309b9e350fc Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 17 Feb 2022 13:50:39 +0100 Subject: [PATCH 1/5] Add server SDK constraint traits RFC --- design/src/rfcs/overview.md | 1 + design/src/rfcs/rfc0012_constraint_traits.md | 424 +++++++++++++++++++ 2 files changed, 425 insertions(+) create mode 100644 design/src/rfcs/rfc0012_constraint_traits.md diff --git a/design/src/rfcs/overview.md b/design/src/rfcs/overview.md index dc2c4a8fe2..59d8eae2cd 100644 --- a/design/src/rfcs/overview.md +++ b/design/src/rfcs/overview.md @@ -11,3 +11,4 @@ - [RFC-0009: Example Consolidation](./rfc0009_example_consolidation.md) - [RFC-0010: Waiters](./rfc0010_waiters.md) - [RFC-0011: Publishing Alpha to Crates.io](./rfc0011_crates_io_alpha_publishing.md) +- [RFC-0012: Constraint traits](./rfc0011_constraint_traits.md) diff --git a/design/src/rfcs/rfc0012_constraint_traits.md b/design/src/rfcs/rfc0012_constraint_traits.md new file mode 100644 index 0000000000..e8a7d2b340 --- /dev/null +++ b/design/src/rfcs/rfc0012_constraint_traits.md @@ -0,0 +1,424 @@ +RFC: Constraint traits +====================== + +> Status: Not Accepted + +[Constraint traits] are used to constrain the values that can be provided for a +shape. + +For example, given the following Smithy model, + +```smithy +@length(min: 18) +Integer Age +``` + +the integer `Age` must take values greater than or equal to 18. + +Constraint traits are most useful when enforced as part of _input_ model +validation to a service. When a server receives a request whose contents +deserialize to input data that violates the modeled constraints, the operation +execution's preconditions are not met, and as such rejecting the request +without executing the operation is expected behavior. + +Constraint traits can also be applied to operation output member shapes, but +the expectation is that service implementations _not fail_ to render a response +when an output value does not meet the specified constraints. From +[awslabs/smithy#1039]: + +> This might seem counterintuitive, but our philosophy is that a change in +> server-side state should not be hidden from the caller unless absolutely +> necessary. Refusing to service an invalid request should always prevent +> server-side state changes, but refusing to send a response will not, as +> there's generally no reasonable route for a server implementation to unwind +> state changes due to a response serialization failure. + +_In general_, clients should not enforce constraint traits in generated code. +Clients must also never enforce constraint traits when sending requests. This +is because: + +* addition and removal of constraint traits are backwards-compatible from a + client's perspective (although this is _not_ documented anywhere in the + Smithy specification), +* the client may have been generated with an older version of the model; and +* the most recent model version might have lifted some constraints. + +On the other hand, server SDKs constitute _the_ source of truth for the +service's behavior, so they interpret the model in all its strictness. + +The Smithy spec defines 8 constraint traits: + +* [`enum`], +* [`idRef`], +* [`length`], +* [`pattern`], +* [`private`], +* [`range`], +* [`required`]; and +* [`uniqueItems`]. + +The `idRef` and `private` traits are enforced at SDK generation time by the +[`awslabs/smithy`] libraries and bear no relation to generated Rust code. + +The only constraint trait enforcement that is generated by smithy-rs clients +should be and is the `enum` trait, which renders Rust `enum`s. + +The `required` trait is already and only enforced by smithy-rs servers since +[#1148](https://github.com/awslabs/smithy-rs/pull/1148). + +That leaves 4 traits: `length`, `pattern`, `range`, and `uniqueItems`. + +[Constraint traits]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html +[awslabs/smithy#1039]: https://github.com/awslabs/smithy/issues/1039 +[`enum`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#enum-trait +[`idRef`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#idref-trait +[`length`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#length-trait +[`pattern`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#pattern-trait +[`private`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#private-trait +[`range`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#range-trait +[`required`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#required-trait +[`uniqueItems`]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#uniqueItems-trait +[`awslabs/smithy`]: https://github.com/awslabs/smithy + +Implementation +-------------- + +This section addresses how to implement and enforce the `length`, `pattern`, +`range`, and `uniqueItems` traits. We will use the `length` trait applied to a +`string` shape as a running example. The implementation of this trait mostly +carries over to the other three. + +### Example implementation for the `length` trait + +Consider the following Smithy model: + +```smithy +@length(min: 1, max: 69) +string NiceString +``` + +The central idea to the implementation of constraint traits is: [parse, don't +validate]. Instead of code-generating a Rust `String` to represent `NiceString` +values and perform the _validation_ at request deserialization, we can +[leverage Rust's type system to guarantee domain invariants]. We can generate a +wrapper [tuple struct] that _parses_ the string's value and is "tight" in the +set of values it can accept: + +```rust +pub struct NiceString(String); + +impl NiceString { + pub fn parse(inner: String) -> Result { + Self::try_from(inner) + } +} + +impl TryFrom for NiceString { + type Error = NiceStringValidationError; + + fn try_from(value: String) -> Result { + let num_code_points = value.chars().count(); + if 1 <= num_code_points && num_code_points <= 69 { + Ok(Self(value)) + } else { + Err(NiceStringValidationError::LengthViolation(num_code_points)) + } + } +} +``` + +(Note that we're using the _linear_ time check `chars().count()` instead of +`len()` on the input value, since the Smithy specification says the `length` +trait counts [the number of _Unicode code points_ when applied to string +shapes].) + +The goal is to enforce, at the type-system level, that these constrained +structs always hold valid data. It should be impossible for the service +implementer, without resorting to `unsafe` Rust, to construct a `NiceString` +that violates the model. The actual check is performed in the implementation of +[`TryFrom`]`` for the generated struct, which makes it convenient to use +the [`?` operator for error propagation]. Each constrained struct will have a +related `std::error::Error` enum type to signal the _first_ parsing failure, +with one enum variant per applied constraint trait: + +```rust +pub enum NiceStringValidationError { + /// Validation error holding the number of Unicode code points found, when a value between `1` and + /// `69` (inclusive) was expected. + LengthViolation(usize), +} + +impl std::error::Error for NiceStringValidationError {} +``` + +[`std::error::Error`] requires [`Display`] and [`Debug`]. We will +`#[derive(Debug)]`, unless the shape also has the [`sensitive` trait], in which +case we will just print the name of the struct: + +```rust +impl std::fmt::Debug for NiceStringValidationError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut formatter = f.debug_struct("NiceStringValidationError"); + formatter.finish() + } +} +``` + +`Display` is used to produce human-friendlier representations. Its +implementation will be called when formatting a [400 HTTP response message] in +certain protocols, for example. The format of the error messages will _not_ be +configurable by service implementers and will be + +``` +`` validation error: violation: +``` + +where: + +* is the name of the generated struct, +* is the name of the violated Smithy constraint; and +* is a templated message specific to the particular constraint + instantiation (so e.g. `@length(min: 1)` may have a different template than + `@length(min: 1, max: 69)`. + +If the shape is marked with `@sensitive`, the format of the message will be: + +``` +`) -> std::fmt::Result { + match self { + NiceStringValidationError::LengthViolation(found) => write!( + f, + "MyStruct validation error: length violation: expected value between 1 and 69, but found {}", + found + ), + } + } +} +``` + +[parse, don't validate]: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/ +[leverage Rust's type system to guarantee domain invariants]: https://www.lpalmieri.com/posts/2020-12-11-zero-to-production-6-domain-modelling/ +[tuple struct]: https://doc.rust-lang.org/1.9.0/book/structs.html#tuple-structs +[the number of _Unicode code points_ when applied to string shapes]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#length-trait +[`?` operator for error propagation]: https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html#a-shortcut-for-propagating-errors-the--operator +[`std::error::Error`]: https://doc.rust-lang.org/nightly/std/error/trait.Error.html +[`sensitive` trait]: https://awslabs.github.io/smithy/1.0/spec/core/documentation-traits.html#sensitive-trait +[400 HTTP response message]: https://developer.mozilla.org/en-US/docs/web/http/status/400 +[`TryFrom`]: https://doc.rust-lang.org/std/convert/trait.TryFrom.html +[`Display`]: https://doc.rust-lang.org/std/fmt/trait.Display.html +[`Debug`]: https://doc.rust-lang.org/std/fmt/trait.Debug.html + +### Request deserialization + +We will continue to deserialize the different parts of the HTTP message into +the regular Rust standard library types. However, just before the +deserialization function returns, we will convert the type into the wrapper +tuple struct that we will eventually be handed over to the operation handler. +This is what we're _already_ doing when deserializing strings into `enums`. For +example, given the Smithy model: + +```smithy +@enum([ + { name: "Spanish", value: "es" }, + { name: "English", value: "en" }, + { name: "Japanese", value: "jp" }, +]) +string Language +``` + +the code the client generates when deserializing a string from a JSON document +into the `Language` enum is (excerpt): + +```rust +... +match key.to_unescaped()?.as_ref() { + "language" => { + builder = builder.set_language( + aws_smithy_json::deserialize::token::expect_string_or_null( + tokens.next(), + )? + .map(|s| { + s.to_unescaped() + .map(|u| crate::model::Language::from(u.as_ref())) + }) + .transpose()?, + ); + } + _ => aws_smithy_json::deserialize::token::skip_value(tokens)?, +} +... +``` + +Note how the `String` gets converted to the enum via `Language::from()`. + +```rust +impl std::convert::From<&str> for Language { + fn from(s: &str) -> Self { + match s { + "es" => Language::Spanish, + "en" => Language::English, + "jp" => Language::Japanese, + other => Language::Unknown(other.to_owned()), + } + } +} +``` + +For constrained shapes we would do the same to parse the inner deserialized +value into the wrapper tuple struct, except for these differences: + +1. For enums, the client generates an `Unknown` variant that "contains new + variants that have been added since this code was generated". The server + does not need such a variant ([#1187]). +2. Conversions into the tuple struct are fallible (`try_from()` instead of + `from()`). These errors will result in a `MyStructValidationError`, wrapped + in the runtime type error + `aws_smithy_http_server::rejection::ConstraintViolation` rejection. The + deseralizer function will wrap this rejection in an + [`aws_smithy_http_server::rejection::Deserialize`] rejection, to indicate + that deserialization failed as a result of a constraint violation. + Eventually this rejection will be wrapped in an + [`aws_smithy_http_server::rejection::SmithyRejection`], our parent rejection + enum type for all errors that can occur in the smithy-rs service framework. + When serializing this error into an HTTP response, the error message that + naturally arises by calling `std::fmt::Display::fmt()` will trace what + exactly went wrong for the caller client. + +`length` trait +-------------- + +We will enforce the length constraint by calling `len()` on Rust's `Vec` +(`list` and `set` shapes), `HashMap` (`map` shapes), `String` (`string` +shapes), and our [`aws_smithy_types::Blob`] (`bytes` shapes). + +`pattern` trait +--------------- + +The [`pattern` trait] + +> restricts string shape values to a specified regular expression. + +We will implement this by using the `regex`'s crate [`is_match`]. We will use +[`lazy_static!`] to compile the regex only the first time it is required. + +`uniqueItems` trait +------------------- + +The [`uniqueItems` trait] + +> indicates that the items in a [`List`] MUST be unique. + +If the list shape is `sparse`, more than one `null` value violates this +constraint. + +We will enforce this by copying _references_ to the `Vec`'s elements into a +`HashSet` and checking that the sizes of both containers coincide. Smithy +allows you to apply `uniqueItems` to _any_ list, [even if its members are not +equipped with an equivalence relation], so we would have to manually check that +the list's members are all [`Eq`]. + +[`Eq`]: https://doc.rust-lang.org/std/cmp/trait.Eq.html +[even if its members are not equipped with an equivalence relation]: https://github.com/awslabs/smithy/issues/1093 +[`List`]: https://awslabs.github.io/smithy/1.0/spec/core/model.html#list +[`uniqueItems` trait]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#uniqueitems-trait +[`is_match`]: https://docs.rs/regex/latest/regex/struct.Regex.html#method.is_match +[`pattern` trait]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html#pattern-trait +[`aws_smithy_types::Blob`]: https://docs.rs/aws-smithy-types/latest/aws_smithy_types/struct.Blob.html +[`aws_smithy_http_server::rejection::SmithyRejection`]: https://docs.rs/aws-smithy-http-server/latest/aws_smithy_http_server/rejection/enum.SmithyRejection.html +[`aws_smithy_http_server::rejection::Deserialize`]: https://docs.rs/aws-smithy-http-server/latest/aws_smithy_http_server/rejection/struct.Deserialize.html +[#1187]: https://github.com/awslabs/smithy-rs/issues/1187 +[`lazy_static!`](https://docs.rs/lazy_static/1.4.0/lazy_static/) + +Trait precedence and naming of the tuple struct +----------------------------------------------- + +From [the spec]: + +> Some constraints can be applied to shapes as well as structure members. If a +> constraint of the same type is applied to a structure member and the shape +> that the member targets, the trait applied to the member takes precedence. + +```smithy +structure ShoppingCart { + @range(min: 7, max:12) + numberOfItems: PositiveInteger +} + +@range(min: 1) +integer PositiveInteger +``` + +In the above example, + +> the range trait applied to numberOfItems takes precedence over the one +> applied to PositiveInteger. The resolved minimum will be 7, and the maximum +> 12. + +When the constraint trait is applied to a member shape, the tuple struct's name +will be the PascalCased name of the member shape, `NumberOfItems`. + +[the spec]: https://awslabs.github.io/smithy/1.0/spec/core/constraint-traits.html?highlight=enum#trait-precedence + +Unresolved questions +-------------------- + +1. Should we code-generate unsigned integer types (`u16`, `u32`, `u64`) when + the `range` trait is applied with `min` set to a value greater than or equal + to 0? + - A user has even suggested to use the `std::num::NonZeroUX` types (e.g. + [`NonZeroU64`]) when `range` is applied with `min` set to a value greater + than 0. +2. In request deserialization, should we fail with the first violation and + immediately render a response, or attempt to parse the entire request and + provide a complete and structured report? +3. Should we provide a mechanism for the service implementer to construct a + Rust type violating the modeled constraints in their business logic e.g. a + `T::new_unchecked()` constructor? This could be useful (1) when the user + _knows_ the provided inner value does not violate the constraints and + doesn't want to incur the performance penalty of the check; (2) when the + struct is in a transient invalid state. However: + - (2) is arguably a modelling mistake and a separate struct to represent + the transient state would be a better approach, + - the user can always use `unsafe` Rust to bypass the validation; and + - adding this constructor is a backwards-compatible change, so it can + always be added later if this feature is requested. + +[`NonZeroU64`]: https://doc.rust-lang.org/std/num/struct.NonZeroU64.html + +Alternative design +------------------ + +An alternative design with less public API surface would be to perform +constraint validation at request deserialization, but hand over a regular +"loose" type (e.g. `String` instead of `NiceString`) that allows for values +violating the constraints. If we were to implement this approach, we can +implement it by wrapping the incoming value in the aforementioned tuple struct +to perform the validation, and immediately unwrap it. + +Comparative advantages: + +* Validation remains an internal detail of the framework. If the semantics of a + constraint trait change, the behavior of the service is still + backwards-incompatibly affected, but user code is not. +* Less "invasive". Baking validation in the generated type might be deemed as + the service framework overreaching responsibilities. + +Comparative disadvantages: + +* It becomes _possible_ to send responses with invalid operation outputs. All + the service framework could do is log the validation errors. +* Baking validation at the type-system level gets rid of an entire class of + logic errors. +* Less idiomatic (this is subjective). The pattern of wrapping a more primitive + type to guarantee domain invariants is widespread in the Rust ecosystem. The + standard library makes use of it extensively. + +Note that _both_ designs are backwards incompatible in the sense that you can't +migrate from one to the other without breaking user code. From 4963f069a90c0351b81925484da9d9049830d707 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 17 Feb 2022 13:50:54 +0100 Subject: [PATCH 2/5] Add POC code for constraint traits RFC --- .../aws-smithy-http-server/src/main.rs | 86 +++++++++++++++++++ .../aws-smithy-http-server/src/rejection.rs | 6 ++ 2 files changed, 92 insertions(+) create mode 100644 rust-runtime/aws-smithy-http-server/src/main.rs diff --git a/rust-runtime/aws-smithy-http-server/src/main.rs b/rust-runtime/aws-smithy-http-server/src/main.rs new file mode 100644 index 0000000000..05b32e4f93 --- /dev/null +++ b/rust-runtime/aws-smithy-http-server/src/main.rs @@ -0,0 +1,86 @@ +use std::convert::TryFrom; + +pub enum NiceStringValidationError { + /// Validation error holding the number of Unicode code points found, when a value between `1` and + /// `69` (inclusive) was expected. + LengthViolation(usize), +} + +pub struct NiceString(String); + +impl NiceString { + pub fn parse(inner: String) -> Result { + Self::try_from(inner) + } +} + +impl TryFrom for NiceString { + type Error = NiceStringValidationError; + + fn try_from(value: String) -> Result { + let num_code_points = value.chars().count(); + if 1 <= num_code_points && num_code_points <= 69 { + Ok(Self(value)) + } else { + Err(NiceStringValidationError::LengthViolation(num_code_points)) + } + } +} + +// This implementation is used when formatting the error in the 400 HTTP response body, among other +// places. +// The format of the error messages will not be configurable by service implementers and its format +// will be: +// +// `` validation error: violation: +// +// where: +// * is the name of the generated struct, +// * is the name of the violated Smithy constraint; and +// * is a templated message specific to the particular constraint instantiation (so +// e.g. `@length(min: 1)` may have a different template than `@length(min: 1, max: 69)`. +// +// If the shape is marked with `@sensitive`, the format of the message will be: +// +// `) -> std::fmt::Result { + match self { + NiceStringValidationError::LengthViolation(found) => write!( + f, + "MyStruct validation error: length violation: expected value between 1 and 69, but found {}", + found + ), + } + } +} + +// This is the `Debug` implementation if the shape is marked with `@sensitive`. +// If the shape is not marked with `@sensitive`, we will just `#[derive(Debug)]`. +impl std::fmt::Debug for NiceStringValidationError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut formatter = f.debug_struct("NiceStringValidationError"); + formatter.finish() + } +} + +impl std::error::Error for NiceStringValidationError {} + +// pub(crate) struct MyStructValidationErrorWrapper(MyStructValidationError); + +// impl axum_core::response::IntoResponse for MyStructValidationErrorWrapper { +// fn into_response(self) -> axum_core::response::Response { +// let err = MyStructValidationError::LengthViolation(70); +// let rejection = aws_smithy_http_server::rejection::SmithyRejection::ConstraintViolation( +// aws_smithy_http_server::rejection::ConstraintViolation::from_err(err), +// ); +// rejection.into_response() +// } +// } + +pub fn main() { + let err = NiceStringValidationError::LengthViolation(70); + let constraint_rejection = aws_smithy_http_server::rejection::ConstraintViolation::from_err(err); + let deserialize_rejection = aws_smithy_http_server::rejection::Deserialize::from_err(constraint_rejection); + let smithy_rejection = aws_smithy_http_server::rejection::SmithyRejection::Deserialize(deserialize_rejection); +} diff --git a/rust-runtime/aws-smithy-http-server/src/rejection.rs b/rust-runtime/aws-smithy-http-server/src/rejection.rs index 2787bcab2c..79ed40076d 100644 --- a/rust-runtime/aws-smithy-http-server/src/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/rejection.rs @@ -94,6 +94,12 @@ define_rejection! { pub struct MissingExtension(Error); } +define_rejection! { + #[status = BAD_REQUEST] + #[body = "Constraint violation"] + pub struct ConstraintViolation(Error); +} + composite_rejection! { /// Rejection used for `Content-Type` errors such as missing `Content-Type` /// header, MIME parse issues, etc. From 1f5853d619ffaa22762f392944e9257a66829c83 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 17 Feb 2022 13:51:01 +0100 Subject: [PATCH 3/5] Revert "Add POC code for constraint traits RFC" This reverts commit 4963f069a90c0351b81925484da9d9049830d707. --- .../aws-smithy-http-server/src/main.rs | 86 ------------------- .../aws-smithy-http-server/src/rejection.rs | 6 -- 2 files changed, 92 deletions(-) delete mode 100644 rust-runtime/aws-smithy-http-server/src/main.rs diff --git a/rust-runtime/aws-smithy-http-server/src/main.rs b/rust-runtime/aws-smithy-http-server/src/main.rs deleted file mode 100644 index 05b32e4f93..0000000000 --- a/rust-runtime/aws-smithy-http-server/src/main.rs +++ /dev/null @@ -1,86 +0,0 @@ -use std::convert::TryFrom; - -pub enum NiceStringValidationError { - /// Validation error holding the number of Unicode code points found, when a value between `1` and - /// `69` (inclusive) was expected. - LengthViolation(usize), -} - -pub struct NiceString(String); - -impl NiceString { - pub fn parse(inner: String) -> Result { - Self::try_from(inner) - } -} - -impl TryFrom for NiceString { - type Error = NiceStringValidationError; - - fn try_from(value: String) -> Result { - let num_code_points = value.chars().count(); - if 1 <= num_code_points && num_code_points <= 69 { - Ok(Self(value)) - } else { - Err(NiceStringValidationError::LengthViolation(num_code_points)) - } - } -} - -// This implementation is used when formatting the error in the 400 HTTP response body, among other -// places. -// The format of the error messages will not be configurable by service implementers and its format -// will be: -// -// `` validation error: violation: -// -// where: -// * is the name of the generated struct, -// * is the name of the violated Smithy constraint; and -// * is a templated message specific to the particular constraint instantiation (so -// e.g. `@length(min: 1)` may have a different template than `@length(min: 1, max: 69)`. -// -// If the shape is marked with `@sensitive`, the format of the message will be: -// -// `) -> std::fmt::Result { - match self { - NiceStringValidationError::LengthViolation(found) => write!( - f, - "MyStruct validation error: length violation: expected value between 1 and 69, but found {}", - found - ), - } - } -} - -// This is the `Debug` implementation if the shape is marked with `@sensitive`. -// If the shape is not marked with `@sensitive`, we will just `#[derive(Debug)]`. -impl std::fmt::Debug for NiceStringValidationError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut formatter = f.debug_struct("NiceStringValidationError"); - formatter.finish() - } -} - -impl std::error::Error for NiceStringValidationError {} - -// pub(crate) struct MyStructValidationErrorWrapper(MyStructValidationError); - -// impl axum_core::response::IntoResponse for MyStructValidationErrorWrapper { -// fn into_response(self) -> axum_core::response::Response { -// let err = MyStructValidationError::LengthViolation(70); -// let rejection = aws_smithy_http_server::rejection::SmithyRejection::ConstraintViolation( -// aws_smithy_http_server::rejection::ConstraintViolation::from_err(err), -// ); -// rejection.into_response() -// } -// } - -pub fn main() { - let err = NiceStringValidationError::LengthViolation(70); - let constraint_rejection = aws_smithy_http_server::rejection::ConstraintViolation::from_err(err); - let deserialize_rejection = aws_smithy_http_server::rejection::Deserialize::from_err(constraint_rejection); - let smithy_rejection = aws_smithy_http_server::rejection::SmithyRejection::Deserialize(deserialize_rejection); -} diff --git a/rust-runtime/aws-smithy-http-server/src/rejection.rs b/rust-runtime/aws-smithy-http-server/src/rejection.rs index 79ed40076d..2787bcab2c 100644 --- a/rust-runtime/aws-smithy-http-server/src/rejection.rs +++ b/rust-runtime/aws-smithy-http-server/src/rejection.rs @@ -94,12 +94,6 @@ define_rejection! { pub struct MissingExtension(Error); } -define_rejection! { - #[status = BAD_REQUEST] - #[body = "Constraint violation"] - pub struct ConstraintViolation(Error); -} - composite_rejection! { /// Rejection used for `Content-Type` errors such as missing `Content-Type` /// header, MIME parse issues, etc. From 14f6b1a2fe99a7f813755e29227e55ad34058156 Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 30 Nov 2022 16:49:06 +0100 Subject: [PATCH 4/5] Remove RFC --- ...fc0025_custom_validation_error_messages.md | 493 ------------------ 1 file changed, 493 deletions(-) delete mode 100644 design/src/rfcs/rfc0025_custom_validation_error_messages.md diff --git a/design/src/rfcs/rfc0025_custom_validation_error_messages.md b/design/src/rfcs/rfc0025_custom_validation_error_messages.md deleted file mode 100644 index 76a442864a..0000000000 --- a/design/src/rfcs/rfc0025_custom_validation_error_messages.md +++ /dev/null @@ -1,493 +0,0 @@ -RFC: Better Constraint Violations -================================= - -> Status: RFC -> -> Applies to: server - -For a summarized list of proposed changes, see the [Changes Checklist](#changes-checklist) section. - - -This RFC defines how... - -Terminology ------------ - -- **Shape closure**: the set of shapes a shape can "reach", including itself. -- **Transitively constrained shape**: a shape whose closure includes: - 1. a shape with a [constraint trait] attached, - 2. a (member) shape with a [`required` trait] attached; or - 3. an [`enum` shape]. -- A **directly constrained shape** is a any of these: - 1. a shape with a [constraint trait] attached, - 2. a (member) shape with a [`required` trait] attached, - 3. an [`enum` shape]; or - 4. a [`structure` shape] with at least one `required` member shape. -- **Constrained type**: the Rust type a constrained shape gets rendered as. For shapes that are not `structure`, `union`, or `enum` shapes, these are wrapper [newtype]s. -- **Constrained operation**: an operation whose input shape is transitively constrained. - -In the absence of a qualifier, "constrained shape" should be interpreted as "transitively constrained shape". - -Impossible constraint violations --------------------------------- - -### Background - -A constrained type has a fallible constructor by virtue of it implementing the [`TryFrom`] trait. The error type this constructor may yield is known as a constraint violation: - -```rust -impl TryFrom for ConstrainedType { - type Error = ConstraintViolation; - - fn try_from(value: UnconstrainedType) -> Result { - ... - } -} -``` - -The `ConstraintViolation` type is currently a Rust `enum` with one variant per way "constraining" the input value may fail. So, for example, the following Smithy model: - - -Yields: - - -```rust - -``` - -Constraint violations are always Rust `enum`s, even if they only have one variant. - -### Problem - -Currently, the constraint violation types we generate are used by _both_: - -1. the server framework upon request deserialization; and -2. by application code. - -However, the kinds of constraint violations that can occur in application code can sometimes be a strict subset of those that can occur during request deserialization. - -Consider the following model: - -``` -@length(min: 1, max: 69) -map LengthMap { - key: String, - value: LengthString -} - -@length(min: 2, max: 69) -string LengthString -``` - -This produces: - -```rust -pub struct LengthMap( - pub(crate) std::collections::HashMap, -); - -impl - std::convert::TryFrom< - std::collections::HashMap, - > for LengthMap -{ - type Error = crate::model::length_map::ConstraintViolation; - - /// Constructs a `LengthMap` from an - /// [`std::collections::HashMap`], failing when the provided value does not - /// satisfy the modeled constraints. - fn try_from( - value: std::collections::HashMap, - ) -> Result { - let length = value.len(); - if (1..=69).contains(&length) { - Ok(Self(value)) - } else { - Err(crate::model::length_map::ConstraintViolation::Length(length)) - } - } -} - -pub mod length_map { - pub enum ConstraintViolation { - Length(usize), - Value( - std::string::String, - crate::model::length_string::ConstraintViolation, - ), - } - ... -} -``` - -Observe how the `ConstraintViolation::Value` variant is never constructed. Indeed, this variant is impossible to be constructed _in application code_: a user has to provide a map whose values are already constrained `LengthString`s to the `try_from` constructor, which only enforces the map's `@length` trait. - -The reason why these seemingly "impossible violations" are being generated is because they can arise during request deserialization. Indeed, the server framework deserializes requests into _fully unconstrained types_. These are types holding unconstrained types all the way through their closures. For instance, in the case of structure shapes, builder types (the unconstrained type for the structure shape) [hold builders] all the way down. - -In the case of the above model, below is the alternate constructor the server framework uses upon deserialization. Observe how `LengthMapOfLengthStringsUnconstrained` is _fully unconstrained_ and how the `TryFrom` constructor can yield `ConstraintViolation::Value`. - -```rust -pub(crate) mod length_map_of_length_strings_unconstrained { - #[derive(Debug, Clone)] - pub(crate) struct LengthMapOfLengthStringsUnconstrained( - pub(crate) std::collections::HashMap, - ); - - impl From - for crate::constrained::MaybeConstrained - { - fn from(value: LengthMapOfLengthStringsUnconstrained) -> Self { - Self::Unconstrained(value) - } - } - impl std::convert::TryFrom - for crate::model::LengthMapOfLengthStrings - { - type Error = crate::model::length_map_of_length_strings::ConstraintViolation; - fn try_from(value: LengthMapOfLengthStringsUnconstrained) -> Result { - let res: Result< - std::collections::HashMap, - Self::Error, - > = value - .0 - .into_iter() - .map(|(k, v)| { - let k: crate::model::LengthString = k.try_into().map_err(Self::Error::Key)?; - - Ok((k, v)) - }) - .collect(); - let hm = res?; - Self::try_from(hm) - } - } -} -``` - -In conclusion, the user is currently exposed to an internal detail of how the framework operates that has no bearing on their application code. They shouldn't be exposed to impossible constraint violation variants in their Rust docs, nor have to `match` on these variants when handling errors. - -Note: [this comment] alludes to the problem described above. - -[hold builders]: -[this comment]: - -### Solution proposal - -The problem can be mitigated by adding `#[doc(hidden)]` to the internal variants and `#[non_exhaustive]` to the enum. We're already doing this in some constraint violation types. - -However, a "less leaky" solution is achieved by _splitting_ the constraint violation type into two types: - -1. one for use by the framework, with `pub (crate)` visibility, named `ConstraintViolationException`; and -2. one for use by user application code, with `pub` visibility, named `ConstraintViolation`. - -```rust -pub mod length_map { - pub enum ConstraintViolation { - Length(usize), - } - pub (crate) enum ConstraintViolationException { - Length(usize), - Value( - std::string::String, - crate::model::length_string::ConstraintViolation, - ), - } -} -``` - -Note that, to some extent, this solution is [already currently present](https://github.com/awslabs/smithy-rs/blob/9a4c1f304f6f5237d480cfb56dad2951d927d424/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt#L78-L81) in the case of builder types when `publicConstrainedTypes` is set to `false`: - -1. `ServerBuilderGenerator.kt` renders the usual builder type that enforces constraint traits, setting its visibility to `pub (crate)`, for exclusive use by the framework. -2. `ServerBuilderGeneratorWithoutPublicConstrainedTypes` renders the builder type the user is exposed to: this builder does not take in constrained types and does not enforce all modeled constraints. - -Collecting constraint violations --------------------------------- - -### Background - -It was mentioned in the [constraint traits RFC](https://github.com/awslabs/smithy-rs/pull/1199#discussion_r809300673), and implicit in the definition of Smithy's [`smithy.framework.ValidationException`](https://github.com/awslabs/smithy/blob/main/smithy-validation-model/model/smithy.framework.validation.smithy) shape, that server frameworks should respond with a _full_ collection of errors encountered during constraint trait enforcement to the client. - -### Problem - -As of writing, the `TryFrom` constructor of constrained types whose shapes have more than one constraint trait attached can only yield a single error. For example, the following shape: - -```smithy -@pattern("[a-f0-5]*") -@length(min: 5, max: 10) -string LengthPatternString -``` - -Is rendered as: - -```rust -impl LengthPatternString { - fn check_length( - string: &str, - ) -> Result<(), crate::model::length_pattern_string::ConstraintViolation> { - let length = string.chars().count(); - - if (5..=10).contains(&length) { - Ok(()) - } else { - Err(crate::model::length_pattern_string::ConstraintViolation::Length(length)) - } - } - - fn check_pattern( - string: String, - ) -> Result { - let regex = Self::compile_regex(); - - if regex.is_match(&string) { - Ok(string) - } else { - Err(crate::model::length_pattern_string::ConstraintViolation::Pattern(string)) - } - } - - pub fn compile_regex() -> &'static regex::Regex { - static REGEX: once_cell::sync::Lazy = once_cell::sync::Lazy::new(|| { - regex::Regex::new(r#"[a-f0-5]*"#).expect(r#"The regular expression [a-f0-5]* is not supported by the `regex` crate; feel free to file an issue under https://github.com/awslabs/smithy-rs/issues for support"#) - }); - - ®EX - } -} -impl std::convert::TryFrom for LengthPatternString { - type Error = crate::model::length_pattern_string::ConstraintViolation; - - /// Constructs a `LengthPatternString` from an [`std::string::String`], failing when the provided value does not satisfy the modeled constraints. - fn try_from(value: std::string::String) -> Result { - Self::check_length(&value)?; - - let value = Self::check_pattern(value)?; - - Ok(Self(value)) - } -} -``` - -Observe how a failure to adhere to the `@length` trait will short-circuit the evaluation of the constructor, when the value could technically also not adhere with the `@pattern` trait. - -Similarly, - -* collections whose members are constrained fail upon encountering the first member that violates the constraint, -* maps whose keys and/or values are constrained fail upon encountering the first violation; and -* constrained structures fail upon encountering the first member that violates a constraint. - -In summary, any shape that is transitively constrained yields types whose constructors currently fail upon encountering the first violation. - -### Solution proposal - -The deserializing architecture lends itself to be easily refactored so that we can collect constraint violations before returning them. Indeed, note that deserializers enforce constraint traits in a two-step phase: first, the _entirety_ of the unconstrained value is deserialized, _then_ constraint traits are enforced by feeding the entire value to the `TryFrom` constructor. - -We will introduce a `ConstraintViolations` type (note the plural) that represents a collection of constraint violations. Roughly: - -```rust -pub ConstraintViolations(pub(crate) Vec); - -impl IntoIterator for ConstraintViolations { ... } - -impl std::convert::TryFrom for LengthPatternString { - type Error = ConstraintViolations; - - fn try_from(value: std::string::String) -> Result { - // Check constraints and collect violations. - ... - } -} -``` - -* The main reason for wrapping a vector in `ConstraintViolations` as opposed to directly returning the vector is forwards-compatibility: we may want to expand `ConstraintViolations` with conveniences (see below). -* If the constrained type can only ever yield a single violation, we will dispense with `ConstraintViolations` and keep directly returning the `crate::model::shape_name::ConstraintViolation` type. - -### Unresolved questions - -#### "Tightness" of constraint violations - -`ConstraintViolations` is - -#### Collecting constraint violations may constitute a DOS attack vector - -This is a problem that _already_ exists as of writing, but that collecting constraint violations highlights, so it is a good opportunity, from a pedagogical perspective, to explain it here. Consider the following model: - -```smithy -@length(max: 3) -list ListOfPatternStrings { - member: PatternString -} - -@pattern("expensive regex to evaluate") -string PatternString -``` - -Our implementation currently enforces constraints _from the leaf to the root_: when enforcing the `@length` constraint, the `TryFrom` constructor the server framework uses gets a `Vec` and _first_ checks the members adhere to the `@pattern` trait, and only _after_ is the `@length` trait checked at the end. This means that if a client sends a request `n >>> 3` times, the expensive check runs `n` times, when a constant-time check inspecting the length of the input vector would have sufficed to reject the request. - -* A possibility to circumvent this is making the `@length` validator special, having it bound the other validators via effectively permuting the order of the checks and thus short-circuiting. In general, it's unclear what constraint traits should cause short-circuiting. A probably reasonable rule of thumb is to include traits that can be attached directly to aggregate shapes: as of writing, that would be `@uniqueItems` on list shapes and `@length` on list shapes. -* Another possiblity is to _do nothing_ and value _complete_ validation exception response messages over trying to mitigate this with special handling. One could argue that these kind of DOS attack vectors should be taken care of with a separate solution e.g. a layer that bounds a request body's size to a reasonable default (see [how Axum added this](https://github.com/tokio-rs/axum/pull/1420)). - -TODO: Mention stack overflow DOS attack vector in comment. - -Custom error response messages ------------------------------- - -### Background - -Constrained operations are currently required to have `smithy.framework#ValidationException` as a member in their [`errors` property](https://smithy.io/2.0/spec/service-types.html#operation). This is the shape that is rendered in responses when a request contains data that violates the modeled constraints. - -The shape is defined in the [`smithy-validation-model` Maven package](https://search.maven.org/artifact/software.amazon.smithy/smithy-validation-model), [as follows](https://github.com/awslabs/smithy/blob/main/smithy-validation-model/model/smithy.framework.validation.smithy): - -```smithy -$version: "2.0" - -namespace smithy.framework - -/// A standard error for input validation failures. -/// This should be thrown by services when a member of the input structure -/// falls outside of the modeled or documented constraints. -@error("client") -structure ValidationException { - - /// A summary of the validation failure. - @required - message: String, - - /// A list of specific failures encountered while validating the input. - /// A member can appear in this list more than once if it failed to satisfy multiple constraints. - fieldList: ValidationExceptionFieldList -} - -/// Describes one specific validation failure for an input member. -structure ValidationExceptionField { - /// A JSONPointer expression to the structure member whose value failed to satisfy the modeled constraints. - @required - path: String, - - /// A detailed description of the validation failure. - @required - message: String -} - -list ValidationExceptionFieldList { - member: ValidationExceptionField -} -``` - -Smithy's protocol tests provide a normative reference for what the error message contents' should be. For example, the [RestJsonMalformedPatternMapValue](https://github.com/awslabs/smithy/blob/31ddf685d7e3fe287eac51442621975d585972fd/smithy-aws-protocol-tests/model/restJson1/validation/malformed-pattern.smithy#L154-L154) test stipulates that upon receiving the JSON value: - -```json -{ "map" : { "abc": "ABC" } } -``` - -for the modeled map: - -```smithy -map PatternMap { - key: PatternString, - value: PatternString -} - -@pattern("^[a-m]+$") -string PatternString -``` - -the JSON response should be: - -```json -{ - { - "message" : "1 validation error detected. Value abc at '/map/abc' failed to satisfy constraint: Member must satisfy regular expression pattern: ^[a-m]+$$", - "fieldList" : [ - { - "message": "Value ABC at '/map/abc' failed to satisfy constraint: Member must satisfy regular expression pattern: ^[a-m]+$$", - "path": "/map/abc" - } - ] - } -} -``` - -Currently, the framework is mapping from the operation input shape's `ConstraintViolation` to a `ValidationException`. This mapping is hardcoded so as to make the protocol tests pass. - -### Problem - -Some users have expressed interest in being able to customize the error responses. So far their use cases mainly revolve around using smithy-rs server SDKs in frontend services that are invoked by user interface code: for example, said service might want to internationalize or enhance error messages that are displayed directly to users in a web form. - -Currently, it is a _strict requirement_ that constrained operations's errors contain `smithy.framework#ValidationException`, and its error messages are not configurable. - -### Solution proposal - -It is worth noting that the _vast_ majority of smithy-rs server SDK users are operating within a backend service, and that the default behavior prescribed by Smithy's protocol tests is satisfactory for their use cases. - -Therefore, it is reasonable to expect that a solution to lift the aforementioned restriction be _opt-in_. We will introduce a `codegenConfig.disableDefaultValidation` flag in `smithy-build.json` with the following semantics: - -* If `disableDefaultValidation` is set to `false`, we will continue operating as we currently do: all constrained operations _must_ have `smithy.framework#ValidationException` attached. -* If `disableDefaultValidation` is set to `true`, any constrained operation that does not have `smithy.framework#ValidationException` attached will make it so that the generated SDK forces the user to provide a [`core::ops::Fn`](https://doc.rust-lang.org/nightly/core/ops/trait.Fn.html) that maps from the constrained operation's `ConstraintViolations` container newtype to any modeled error shape attached to the operation. - -For example, an operation like: - -```smithy -operation Operation { - input: OperationInput, - output: OperationOutput, - errors: [ValidationCustomError, OperationError1, ... OperationErrorN] -} - -@error("client") -structure ValidationCustomError { - validationErrorMessage: String -} -``` - -in a model generated with `codegenConfig.disableDefaultValidation` set to `true` will be implemented in user code as: - -```rust -use my_server_sdk::{input, output, error} - -fn map_operation_validation_failure_to_error(errors: operation::operation_input::ConstraintViolations) -> error::OperationError { - let mut validation_error_message: String::new(); - - for e in errors.into_iter() { - match e { - // ... - } - } - - error::OperationError { - validation_error_message - } -} - -fn operation_handler(input: input::OperationInput) -> Result { - // ... -} -``` - -Since the operation does not have `smithy.framework#ValidationException` attached, when registering the handler in the service builder, the user must provide the mapping to an `error::OperationError` variant when constraint trait enforcement fails. - -```rust -let my_service = MyService::builder_without_plugins() - .my_operation(operation_handler) - .on_my_operation_validation_failure(map_operation_validation_failure_to_error) - .build(); -``` - -Alternative: register a tuple in the service builder. - -This solution is ~shamelessly pilfered~ inspired by what the smithy-typescript server SDK implements, which is described [in this message]. - -Note: [this comment] in `ValidateUnsupportedConstraints.kt` alludes to the solution proposed above. - -[in this message]: https://github.com/awslabs/smithy-rs/pull/1199#discussion_r809424783 - -[this comment]: https://github.com/awslabs/smithy-rs/blob/0adad6a72dd872044765e9fbbb220264c849ce39/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraints.kt#L150C6-L153 - -### Unresolved questions - -How much freedom - -Changes checklist ------------------ - -- [x] Create new struct `NewFeature` From 118ba860a3f989e65362179bb815deb641b1a299 Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 30 Nov 2022 17:01:06 +0100 Subject: [PATCH 5/5] Accuracy updates --- design/src/rfcs/rfc0025_constraint_traits.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/design/src/rfcs/rfc0025_constraint_traits.md b/design/src/rfcs/rfc0025_constraint_traits.md index 06651306ab..a60b24e574 100644 --- a/design/src/rfcs/rfc0025_constraint_traits.md +++ b/design/src/rfcs/rfc0025_constraint_traits.md @@ -255,8 +255,11 @@ value into the wrapper tuple struct, except for these differences: -------------- We will enforce the length constraint by calling `len()` on Rust's `Vec` -(`list` and `set` shapes), `HashMap` (`map` shapes), `String` (`string` -shapes), and our [`aws_smithy_types::Blob`] (`bytes` shapes). +(`list` and `set` shapes), `HashMap` (`map` shapes) and our +[`aws_smithy_types::Blob`] (`bytes` shapes). + +We will enforce the length constraint trait on `String` (`string` shapes) by +calling `.chars().count()`. `pattern` trait --------------- @@ -279,10 +282,7 @@ If the list shape is `sparse`, more than one `null` value violates this constraint. We will enforce this by copying _references_ to the `Vec`'s elements into a -`HashSet` and checking that the sizes of both containers coincide. Smithy -allows you to apply `uniqueItems` to _any_ list, [even if its members are not -equipped with an equivalence relation], so we would have to manually check that -the list's members are all [`Eq`]. +`HashSet` and checking that the sizes of both containers coincide. [`Eq`]: https://doc.rust-lang.org/std/cmp/trait.Eq.html [even if its members are not equipped with an equivalence relation]: https://github.com/awslabs/smithy/issues/1093