Skip to content

Commit

Permalink
Builders of builders (#1342)
Browse files Browse the repository at this point in the history
This patchset, affectionately called "Builders of builders", lays the
groundwork for fully implementing [Constraint traits] in the server SDK
generator. [The RFC] illustrates what the end goal looks like, and is
recommended prerrequisite reading to understanding this cover letter.

This commit makes the sever deserializers work with _unconstrained_ types
during request parsing, and only after the entire request is parsed are
constraints enforced. Values for a constrained shape are stored in the
correspondingly unconstrained shape, and right before the operation input is
built, the values are constrained via a `TryFrom<UnconstrainedShape> for
ConstrainedShape` implementation that all unconstrained types enjoy. The
service owner only interacts with constrained types, the unconstrained ones are
`pub(crate)` and for use by the framework only.

In the case of structure shapes, the corresponding unconstrained shape is their
builders. This is what gives this commit its title: during request
deserialization, arbitrarily nested structures are parsed into _builders that
hold builders_. Builders keep track of whether their members are constrained or
not by storing its members in a `MaybeConstrained`
[Cow](https://doc.rust-lang.org/std/borrow/enum.Cow.html)-like `enum` type:

```rust
pub(crate) trait Constrained {
    type Unconstrained;
}

#[derive(Debug, Clone)]
pub(crate) enum MaybeConstrained<T: Constrained> {
    Constrained(T),
    Unconstrained(T::Unconstrained),
}
```

Consult the documentation for the generator in `ServerBuilderGenerator.kt` for
more implementation details and for the differences with the builder types the
server has been using, generated by `BuilderGenerator.kt`, which after this
commit are exclusively used by clients.

Other shape types, when they are constrained, get generated with their
correspondingly unconstrained counterparts. Their Rust types are essentially
wrapper newtypes, and similarly enjoy `TryFrom` converters to constrain them.
See the documentation in `UnconstrainedShapeSymbolProvider.kt` for details and
an example.

When constraints are not met, the converters raise _constraint violations_.
These are currently `enum`s holding the _first_ encountered violation.

When a shape is _transitively but not directly_ constrained, newtype wrappers
are also generated to hold the nested constrained values. To illustrate their
need, consider for example a list of `@length` strings. Upon request parsing,
the server deserializers need a way to hold a vector of unconstrained regular
`String`s, and a vector of the constrained newtyped `LengthString`s. The former
requirement is already satisfied by the generated unconstrained types, but for
the latter we need to generate an intermediate constrained
`ListUnconstrained(Vec<LengthString>)` newtype that will eventually be
unwrapped into the `Vec<LengthString>` the user is handed. This is the purpose
of the `PubCrate*` generators: consult the documentation in
`PubCrateConstrainedShapeSymbolProvider.kt`,
`PubCrateConstrainedCollectionGenerator.kt`, and
`PubCrateConstrainedMapGenerator.kt` for more details. As their name implies,
all of these types are `pub(crate)`, and the user never interacts with them.

For users that would not like their application code to make use of constrained
newtypes for their modeled constrained shapes, a `codegenConfig` setting
`publicConstrainedTypes` has been added. They opt out of these by setting it to
`false`, and use the inner types directly: the framework will still enforce
constraints upon request deserialization, but once execution enters an
application handler, the user is on their own to honor (or not) the modeled
constraints. No user interest has been expressed for this feature, but I expect
we will see demand for it. Moreover, it's a good stepping stone for users that
want their services to honor constraints, but are not ready to migrate their
application code to constrained newtypes. As for how it's implemented, several
parts of the codebase inspect the setting and toggle or tweak generators based
on its value. Perhaps the only detail worth mentioning in this commit message
is that the structure shape builder types are generated by a much simpler and
entirely different generator, in
`ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt`. Note that this
builder _does not_ enforce constraints, except for `required` and `enum`, which
are always (and already) baked into the type system. When
`publicConstrainedTypes` is disabled, this is the builder that end users
interact with, while the one that enforces all constraints,
`ServerBuilderGenerator`, is now generated as `pub(crate)` and left for
exclusive use by the deserializers. See the relevant documentation for the
details and differences among the builder types.

As proof that these foundations are sound, this commit also implements the
`length` constraint trait on Smithy map and string shapes. Likewise, the
`required` and `enum` traits, which were already baked in the generated types
as non-`Option`al and `enum` Rust types, respectively, are now also treated
like the rest of constraint traits upon request deserialization. See the
documentation in `ConstrainedMapGenerator.kt` and
`ConstrainedStringGenerator.kt` for details.

The rest of the constraint traits and target shapes are left as an exercise to
the reader, but hopefully the reader has been convinced that all of them can be
enforced within this framework, paving the way for straightforward
implementations. The diff is already large as it is. Any reamining work is
being tracked in #1401; this and other issues are referenced in the code as
TODOs.

So as to not give users the impression that the server SDK plugin _fully_
honors constraints as per the Smithy specification, a validator in
`ValidateUnsupportedConstraintsAreNotUsed.kt` has been added. This traverses
the model and detects yet-unsupported parts of the spec, aborting code
generation and printing informative warnings referencing the relevant tracking
issues. This is a regression in that models that used constraint traits
previously built fine (even though the constraint traits were silently not
being honored), and now they will break. To unblock generation of these models,
this commit adds another `codegenConfig` setting,
`ignoreUnsupportedConstraints`, that users can opt into.

Closes #1714.

Testing
-------

Several Kotlin unit test classes exercising the finer details of the added
generators and symbol providers have been added. However, the best way to test
is to generate server SDKs from models making use of constraint traits. The
biggest assurances come from the newly added `constraints.smithy` model, an
"academic" service that _heavily_ exercises constraint traits. It's a
`restJson1` service that also tests binding of constrained shapes to different
parts of the HTTP message. Deeply nested hierarchies and recursive shapes are
also featured.

```sh
./gradlew -P modules='constraints' codegen-server-test:build
```

This model is _additionally_ generated in CI with the `publicConstrainedTypes`
setting disabled:

```sh
./gradlew -P modules='constraints_without_public_constrained_types' codegen-server-test:build
``````

Similarly, models using currently unsupported constraints are now being
generated with the `ignoreUnsupportedConstraints` setting enabled.

See `codegen-server-test/build.gradle.kts` for more details.

[Constraint traits]: https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html
[The RFC]: #1199
  • Loading branch information
david-perez authored Nov 15, 2022
1 parent b2528a1 commit b43905e
Show file tree
Hide file tree
Showing 113 changed files with 7,438 additions and 634 deletions.
125 changes: 125 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,128 @@ message = "Several breaking changes have been made to errors. See [the upgrade g
references = ["smithy-rs#1926", "smithy-rs#1819"]
meta = { "breaking" = true, "tada" = false, "bug" = false }
author = "jdisanti"

[[smithy-rs]]
message = """
[Constraint traits](https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html) in server SDKs are beginning to be supported. The following are now supported:
* The `length` trait on `string` shapes.
* The `length` trait on `map` shapes.
Upon receiving a request that violates the modeled constraints, the server SDK will reject it with a message indicating why.
Unsupported (constraint trait, target shape) combinations will now fail at code generation time, whereas previously they were just ignored. This is a breaking change to raise awareness in service owners of their server SDKs behaving differently than what was modeled. To continue generating a server SDK with unsupported constraint traits, set `codegenConfig.ignoreUnsupportedConstraints` to `true` in your `smithy-build.json`.
"""
references = ["smithy-rs#1199", "smithy-rs#1342", "smithy-rs#1401"]
meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "server" }
author = "david-perez"

[[smithy-rs]]
message = """
Server SDKs now generate "constrained types" for constrained shapes. Constrained types are [newtypes](https://rust-unofficial.github.io/patterns/patterns/behavioural/newtype.html) that encapsulate the modeled constraints. They constitute a [widespread pattern to guarantee domain invariants](https://www.lpalmieri.com/posts/2020-12-11-zero-to-production-6-domain-modelling/) and promote correctness in your business logic. So, for example, the model:
```smithy
@length(min: 1, max: 69)
string NiceString
```
will now render a `struct NiceString(String)`. Instantiating a `NiceString` is a fallible operation:
```rust
let data: String = ... ;
let nice_string = NiceString::try_from(data).expect("data is not nice");
```
A failed attempt to instantiate a constrained type will yield a `ConstraintViolation` error type you may want to handle. This type's API is subject to change.
Constrained types _guarantee_, by virtue of the type system, that your service's operation outputs adhere to the modeled constraints. To learn more about the motivation for constrained types and how they work, see [the RFC](https://github.com/awslabs/smithy-rs/pull/1199).
If you'd like to opt-out of generating constrained types, you can set `codegenConfig.publicConstrainedTypes` to `false`. Note that if you do, the generated server SDK will still honor your operation input's modeled constraints upon receiving a request, but will not help you in writing business logic code that adheres to the constraints, and _will not prevent you from returning responses containing operation outputs that violate said constraints_.
"""
references = ["smithy-rs#1342", "smithy-rs#1119"]
meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "server" }
author = "david-perez"

[[smithy-rs]]
message = """
Structure builders in server SDKs have undergone significant changes.
The API surface has been reduced. It is now simpler and closely follows what you would get when using the [`derive_builder`](https://docs.rs/derive_builder/latest/derive_builder/) crate:
1. Builders no longer have `set_*` methods taking in `Option<T>`. You must use the unprefixed method, named exactly after the structure's field name, and taking in a value _whose type matches exactly that of the structure's field_.
2. Builders no longer have convenience methods to pass in an element for a field whose type is a vector or a map. You must pass in the entire contents of the collection up front.
3. Builders no longer implement [`PartialEq`](https://doc.rust-lang.org/std/cmp/trait.PartialEq.html).
Bug fixes:
4. Builders now always fail to build if a value for a `required` member is not provided. Previously, builders were falling back to a default value (e.g. `""` for `String`s) for some shapes. This was a bug.
Additions:
5. A structure `Structure` with builder `Builder` now implements `TryFrom<Builder> for Structure` or `From<Builder> for Structure`, depending on whether the structure [is constrained](https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html) or not, respectively.
To illustrate how to migrate to the new API, consider the example model below.
```smithy
structure Pokemon {
@required
name: String,
@required
description: String,
@required
evolvesTo: PokemonList
}
list PokemonList {
member: Pokemon
}
```
In the Rust code below, note the references calling out the changes described in the numbered list above.
Before:
```rust
let eevee_builder = Pokemon::builder()
// (1) `set_description` takes in `Some<String>`.
.set_description(Some("Su código genético es muy inestable. Puede evolucionar en diversas razas de Pokémon.".to_owned()))
// (2) Convenience method to add one element to the `evolvesTo` list.
.evolves_to(vaporeon)
.evolves_to(jolteon)
.evolves_to(flareon);
// (3) Builder types can be compared.
assert_ne!(eevee_builder, Pokemon::builder());
// (4) Builds fine even though we didn't provide a value for `name`, which is `required`!
let _eevee = eevee_builder.build();
```
After:
```rust
let eevee_builder = Pokemon::builder()
// (1) `set_description` no longer exists. Use `description`, which directly takes in `String`.
.description("Su código genético es muy inestable. Puede evolucionar en diversas razas de Pokémon.".to_owned())
// (2) Convenience methods removed; provide the entire collection up front.
.evolves_to(vec![vaporeon, jolteon, flareon]);
// (3) Binary operation `==` cannot be applied to `pokemon::Builder`.
// assert_ne!(eevee_builder, Pokemon::builder());
// (4) `required` member `name` was not set.
// (5) Builder type can be fallibly converted to the structure using `TryFrom` or `TryInto`.
let _error = Pokemon::try_from(eevee_builder).expect_err("name was not provided");
```
"""
references = ["smithy-rs#1714", "smithy-rs#1342"]
meta = { "breaking" = true, "tada" = true, "bug" = true, "target" = "server" }
author = "david-perez"

[[smithy-rs]]
message = """
Server SDKs now correctly reject operation inputs that don't set values for `required` structure members. Previously, in some scenarios, server SDKs would accept the request and set a default value for the member (e.g. `""` for a `String`), even when the member shape did not have [Smithy IDL v2's `default` trait](https://awslabs.github.io/smithy/2.0/spec/type-refinement-traits.html#smithy-api-default-trait) attached. The `default` trait is [still unsupported](https://github.com/awslabs/smithy-rs/issues/1860).
"""
references = ["smithy-rs#1714", "smithy-rs#1342", "smithy-rs#1860"]
meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "server" }
author = "david-perez"
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,34 @@
package software.amazon.smithy.rust.codegen.client.smithy.generators

import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.StructureShape
import software.amazon.smithy.rust.codegen.core.rustlang.Writable
import software.amazon.smithy.rust.codegen.core.rustlang.rust
import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.CodegenContext
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.Instantiator
import software.amazon.smithy.rust.codegen.core.smithy.generators.setterName

private fun enumFromStringFn(enumSymbol: Symbol, data: String): Writable = writable {
rust("#T::from($data)", enumSymbol)
}

class ClientBuilderKindBehavior(val codegenContext: CodegenContext) : Instantiator.BuilderKindBehavior {
override fun hasFallibleBuilder(shape: StructureShape): Boolean =
BuilderGenerator.hasFallibleBuilder(shape, codegenContext.symbolProvider)

override fun setterName(memberShape: MemberShape): String = memberShape.setterName()

override fun doesSetterTakeInOption(memberShape: MemberShape): Boolean = true
}

fun clientInstantiator(codegenContext: CodegenContext) =
Instantiator(
codegenContext.symbolProvider,
codegenContext.model,
codegenContext.runtimeConfig,
ClientBuilderKindBehavior(codegenContext),
::enumFromStringFn,
)
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationCustomization
import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationSection
import software.amazon.smithy.rust.codegen.core.smithy.customize.writeCustomizations
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.builderSymbol
import software.amazon.smithy.rust.codegen.core.smithy.generators.error.errorSymbol
import software.amazon.smithy.rust.codegen.core.smithy.generators.http.ResponseBindingGenerator
Expand Down Expand Up @@ -332,7 +332,7 @@ class HttpBoundProtocolTraitImplGenerator(
}
}

val err = if (StructureGenerator.hasFallibleBuilder(outputShape, symbolProvider)) {
val err = if (BuilderGenerator.hasFallibleBuilder(outputShape, symbolProvider)) {
".map_err(${format(errorSymbol)}::unhandled)?"
} else ""

Expand Down
Loading

0 comments on commit b43905e

Please sign in to comment.