Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement @length on collections #2028

Merged
merged 56 commits into from
Nov 30, 2022
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
91ca287
Refactor `ConstrainedMapGenerator` slightly
jjant Nov 24, 2022
4e79202
Add `@length` list operation in `constraints.smithy`
jjant Nov 24, 2022
000175c
Add more setup required for rendering constraint `list`s
jjant Nov 24, 2022
6299e29
Add initial draft of `ConstrainedCollectionGenerator`
jjant Nov 24, 2022
617591d
Add license header to `LengthTraitCommon`
jjant Nov 24, 2022
d0a6fab
Add draft of collection constraint violation generation
jjant Nov 25, 2022
b71d1a9
Add `Visibility.toRustQualifier`
jjant Nov 25, 2022
e684ce7
Implement `CollectionConstraintViolationGenerator`
jjant Nov 25, 2022
c255474
Use `TraitInfo` on `CollectionConstraintViolationGenerator`
jjant Nov 25, 2022
c109bb5
Update docs on `ConstrainedCollectionGenerator`
jjant Nov 25, 2022
4ebc39a
Improve formatting on rust code
jjant Nov 25, 2022
eaa4e6f
Don't generate `ConstraintViolation` enum twice
jjant Nov 25, 2022
ba85018
Make symbol provider work with constrained lists
jjant Nov 25, 2022
94de917
Fix call to `ConstraintViolation::Member`
jjant Nov 25, 2022
b8b029f
Render constraint validation functions for lists
jjant Nov 25, 2022
8be886c
Fix call to `usize::to_string`
jjant Nov 25, 2022
e414189
Use map json customisation for collections as well
jjant Nov 25, 2022
273d71c
Merge branch 'main' into jjant/implement-length-trait-on-collections
jjant Nov 28, 2022
6c50000
Update to use overhauled module system
jjant Nov 28, 2022
acc3c98
Add constraint traits check for collection shapes
jjant Nov 28, 2022
6c1a2c2
Remove test checking that `@length` is not used in `list`s
jjant Nov 28, 2022
c190d36
Refactor use of `shape.isDirectlyConstrained`
jjant Nov 28, 2022
ad2d5e9
Fix failing `UnconstrainedCollectionGeneratorTest` test
jjant Nov 28, 2022
818477e
Fix test
jjant Nov 28, 2022
715f96b
Actually fix the test
jjant Nov 28, 2022
d2c59da
Update changelog
jjant Nov 28, 2022
2d22133
Fix `constrainedCollectionCheck`
jjant Nov 28, 2022
1eb8650
Add tests for `ConstrainedCollectionGenerator`
jjant Nov 28, 2022
072df40
Make tests work for when sets are implemented
jjant Nov 28, 2022
1a37a33
Merge branch 'main' into jjant/implement-length-trait-on-collections
jjant Nov 29, 2022
3f906a2
Merge branch 'main' into jjant/implement-length-trait-on-collections
jjant Nov 29, 2022
dfa3979
Remove mention of `set` in changelog
jjant Nov 29, 2022
925c225
Fix styling in `constraints.smithy`
jjant Nov 29, 2022
3b92790
Use `check` instead of `assert`
jjant Nov 29, 2022
bb24581
Grammar fix in comment about `Option<Box<_>>`
jjant Nov 29, 2022
5cebcb9
Rename unsupported length data class for blobs
jjant Nov 29, 2022
074276a
Add TODO about `uniqueItems` not being implemented yet
jjant Nov 29, 2022
28a3397
Change error message: `unconstrained` -> `not constrained`
jjant Nov 29, 2022
b0679c5
Add imports to fix docs
jjant Nov 29, 2022
ebaa3f0
Remove unused `modelsModuleWriter` parameter
jjant Nov 29, 2022
0d5a313
Use `filterIsInstance` and coalesce `filter + map`
jjant Nov 29, 2022
ac2f737
Add `check` in json customization
jjant Nov 29, 2022
57cd6f1
Use `Set` instead of `List` for supported contraints
jjant Nov 29, 2022
964d78f
Use `toMutableList` to avoid creating an extra list
jjant Nov 29, 2022
838ada5
Add `@length` list to `ConA`
jjant Nov 29, 2022
1e818e4
Add `@httpQuery`-bound `@length` list example
jjant Nov 29, 2022
111ee98
Add `@httpHeader`-bound `@length` list
jjant Nov 29, 2022
a0e561e
Fix `UnconstrainedCollectionGeneratorTest` test
jjant Nov 29, 2022
f759439
Fix rendering of constrained lists as header values
jjant Nov 29, 2022
3f97e7c
Split very long line
jjant Nov 29, 2022
9e8c22b
Add docs for `ConstraintViolation::Member` for lists
jjant Nov 29, 2022
0bea362
Merge branch 'main' into jjant/implement-length-trait-on-collections
jjant Nov 29, 2022
92ceba8
Pass `length` variable name to `LengthTrait.rustCondition`
jjant Nov 30, 2022
01cfe23
Refactor long conditional
jjant Nov 30, 2022
1ce32c8
Homogenise conditional
jjant Nov 30, 2022
3b6932f
Merge branch 'main' into jjant/implement-length-trait-on-collections
jjant Nov 30, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,16 @@ message = """

* The `length` trait on `string` shapes.
* The `length` trait on `map` shapes.
* The `length` trait on `list` shapes.
* The `length` trait on `set` shapes.
jjant marked this conversation as resolved.
Show resolved Hide resolved
* The `range` trait on `integer` shapes.
* The `pattern` trait on `string` 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", "smithy-rs#2005", "smithy-rs#1998"]
references = ["smithy-rs#1199", "smithy-rs#1342", "smithy-rs#1401", "smithy-rs#1998", "smithy-rs#2005", "smithy-rs#2028"]
meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "server" }
author = "david-perez"

Expand Down
23 changes: 23 additions & 0 deletions codegen-core/common-test-models/constraints.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ service ConstraintsService {
QueryParamsTargetingMapOfLengthStringOperation,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a list with the @length trait in ConA.

We need to add a list with the @length trait bound with @httpQuery.

We need to add a list with the @length trait bound with @httpHeader.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 964d78f...111ee98.

This uncovered a bug in http_serde.rs, I probably need to make the same changes there as for the JSON customization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed in f759439.

QueryParamsTargetingMapOfListOfLengthStringOperation,
QueryParamsTargetingMapOfSetOfLengthStringOperation,
QueryParamsTargetingMapOfLengthListOfPatternStringOperation,
QueryParamsTargetingMapOfListOfEnumStringOperation,

QueryParamsTargetingMapOfPatternStringOperation,
Expand Down Expand Up @@ -98,6 +99,13 @@ operation QueryParamsTargetingMapOfSetOfLengthStringOperation {
errors: [ValidationException]
}

@http(uri: "/query-params-targeting-map-of-length-list-of-pattern-string-operation", method: "POST")
operation QueryParamsTargetingMapOfLengthListOfPatternStringOperation {
input: QueryParamsTargetingMapOfLengthListOfPatternStringOperationInputOutput,
output: QueryParamsTargetingMapOfLengthListOfPatternStringOperationInputOutput,
errors: [ValidationException]
}

@http(uri: "/query-params-targeting-map-of-list-of-enum-string-operation", method: "POST")
operation QueryParamsTargetingMapOfListOfEnumStringOperation {
input: QueryParamsTargetingMapOfListOfEnumStringOperationInputOutput,
Expand Down Expand Up @@ -303,6 +311,11 @@ structure QueryParamsTargetingMapOfSetOfLengthStringOperationInputOutput {
mapOfSetOfLengthString: MapOfSetOfLengthString
}

structure QueryParamsTargetingMapOfLengthListOfPatternStringOperationInputOutput {
@httpQueryParams
mapOfLengthListOfPatternString: MapOfLengthListOfPatternString
}

structure QueryParamsTargetingMapOfListOfEnumStringOperationInputOutput {
@httpQueryParams
mapOfListOfEnumString: MapOfListOfEnumString
Expand Down Expand Up @@ -450,6 +463,11 @@ map MapOfSetOfLengthString {
value: ListOfLengthString
}

map MapOfLengthListOfPatternString {
key: PatternString,
value: LengthListOfPatternString
}

// 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.
// map MapOfSetOfRangeInteger {
Expand Down Expand Up @@ -564,6 +582,11 @@ list ListOfLengthPatternString {
member: LengthPatternString
}

@length(min:12, max: 39)
jjant marked this conversation as resolved.
Show resolved Hide resolved
list LengthListOfPatternString {
member: PatternString
}

structure ConB {
@required
nice: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,23 @@ fun RustType.isCopy(): Boolean = when (this) {
enum class Visibility {
PRIVATE,
PUBCRATE,
PUBLIC
PUBLIC;

companion object {
fun publicIf(condition: Boolean, ifNot: Visibility): Visibility =
jjant marked this conversation as resolved.
Show resolved Hide resolved
if (condition) {
PUBLIC
} else {
ifNot
}
}

fun toRustQualifier(): String =
when (this) {
PRIVATE -> ""
PUBCRATE -> "pub(crate)"
PUBLIC -> "pub"
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ sealed class JsonSerializerSection(name: String) : Section(name) {
JsonSerializerSection("ServerError")

/** Manipulate the serializer context for a map prior to it being serialized. **/
data class BeforeIteratingOverMap(val shape: MapShape, val context: JsonSerializerGenerator.Context<MapShape>) :
JsonSerializerSection("BeforeIteratingOverMap")
data class BeforeIteratingOverMapOrCollection(val shape: Shape, val context: JsonSerializerGenerator.Context<Shape>) :
JsonSerializerSection("BeforeIteratingOverMapOrCollection")

/** Manipulate the serializer context for a non-null member prior to it being serialized. **/
data class BeforeSerializingNonNullMember(val shape: Shape, val context: JsonSerializerGenerator.MemberContext) :
Expand All @@ -90,7 +90,7 @@ class JsonSerializerGenerator(
private val jsonName: (MemberShape) -> String,
private val customizations: List<JsonSerializerCustomization> = listOf(),
) : StructuredDataSerializerGenerator {
data class Context<T : Shape>(
data class Context<out T : Shape>(
Copy link
Contributor Author

@jjant jjant Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird syntax, but this makes Context covariant in T: it makes Context<T> : Context<U> where T : U.

In particular, it lets us use Context<CollectionShape> in a Context<Shape> argument, which I needed here.

/** Expression that retrieves a JsonValueWriter from either a JsonObjectWriter or JsonArrayWriter */
val writerExpression: String,
/** Expression representing the value to write to the JsonValueWriter */
Expand Down Expand Up @@ -455,6 +455,9 @@ class JsonSerializerGenerator(

private fun RustWriter.serializeCollection(context: Context<CollectionShape>) {
val itemName = safeName("item")
for (customization in customizations) {
customization.section(JsonSerializerSection.BeforeIteratingOverMapOrCollection(context.shape, context))(this)
}
rustBlock("for $itemName in ${context.valueExpression.asRef()}") {
serializeMember(MemberContext.collectionMember(context, itemName))
}
Expand All @@ -464,7 +467,7 @@ class JsonSerializerGenerator(
val keyName = safeName("key")
val valueName = safeName("value")
for (customization in customizations) {
customization.section(JsonSerializerSection.BeforeIteratingOverMap(context.shape, context))(
customization.section(JsonSerializerSection.BeforeIteratingOverMapOrCollection(context.shape, context))(
this,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.locatedIn
import software.amazon.smithy.rust.codegen.core.smithy.rustType
import software.amazon.smithy.rust.codegen.core.smithy.symbolBuilder
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.orNull
import software.amazon.smithy.rust.codegen.core.util.toPascalCase

/**
Expand Down Expand Up @@ -55,8 +56,8 @@ class ConstrainedShapeSymbolProvider(
) : WrappingSymbolProvider(base) {
private val nullableIndex = NullableIndex.of(model)

private fun publicConstrainedSymbolForMapShape(shape: Shape): Symbol {
check(shape is MapShape)
private fun publicConstrainedSymbolForMapOrCollectionShape(shape: Shape): Symbol {
check(shape is MapShape || shape is CollectionShape)

val rustType = RustType.Opaque(shape.contextName(serviceShape).toPascalCase())
return symbolBuilder(shape, rustType).locatedIn(ModelsModule).build()
Expand All @@ -69,13 +70,13 @@ class ConstrainedShapeSymbolProvider(
// (constraint trait precedence).
val target = model.expectShape(shape.target)
val targetSymbol = this.toSymbol(target)
// Handle boxing first so we end up with `Option<Box<_>>`, not `Box<Option<_>>`.
// Handle boxing first, so we end up with `Option<Box<_>>`, not `Box<Option<_>>`.
jjant marked this conversation as resolved.
Show resolved Hide resolved
handleOptionality(handleRustBoxing(targetSymbol, shape), shape, nullableIndex, base.config().nullabilityCheckMode)
}
is MapShape -> {
if (shape.isDirectlyConstrained(base)) {
check(shape.hasTrait<LengthTrait>()) { "Only the `length` constraint trait can be applied to maps" }
publicConstrainedSymbolForMapShape(shape)
publicConstrainedSymbolForMapOrCollectionShape(shape)
} else {
val keySymbol = this.toSymbol(shape.key)
val valueSymbol = this.toSymbol(shape.value)
Expand All @@ -86,11 +87,9 @@ class ConstrainedShapeSymbolProvider(
}
}
is CollectionShape -> {
// TODO(https://github.com/awslabs/smithy-rs/issues/1401) Both arms return the same because we haven't
// implemented any constraint trait on collection shapes yet.
if (shape.isDirectlyConstrained(base)) {
val inner = this.toSymbol(shape.member)
symbolBuilder(shape, RustType.Vec(inner.rustType())).addReference(inner).build()
assert(constrainedCollectionCheck(shape)) { "Only the `length` constraint trait can be applied to lists" }
jjant marked this conversation as resolved.
Show resolved Hide resolved
publicConstrainedSymbolForMapOrCollectionShape(shape)
} else {
val inner = this.toSymbol(shape.member)
symbolBuilder(shape, RustType.Vec(inner.rustType())).addReference(inner).build()
Expand All @@ -107,4 +106,18 @@ class ConstrainedShapeSymbolProvider(
else -> base.toSymbol(shape)
}
}

/**
* Checks that the collection:
* - Has at least 1 supported constraint applied to it, and
* - That it has no unsupported constraints applied.
*
* This check is relatively expensive, so we only run it on `assert`s.
jjant marked this conversation as resolved.
Show resolved Hide resolved
*/
private fun constrainedCollectionCheck(shape: CollectionShape): Boolean {
val supportedConstraintTraits = supportedCollectionConstraintTraits.mapNotNull { shape.getTrait(it).orNull() }.toSet()
val allConstraintTraits = allConstraintTraits.mapNotNull { shape.getTrait(it).orNull() }.toSet()

return supportedConstraintTraits.isNotEmpty() && allConstraintTraits.subtract(supportedConstraintTraits).isEmpty()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,28 @@ import software.amazon.smithy.rust.codegen.core.util.hasTrait
* we support it or not_.
*/
fun Shape.hasConstraintTrait() =
hasTrait<LengthTrait>() ||
hasTrait<EnumTrait>() ||
hasTrait<UniqueItemsTrait>() ||
hasTrait<PatternTrait>() ||
hasTrait<RangeTrait>() ||
hasTrait<RequiredTrait>()
allConstraintTraits.any(this::hasTrait)

val allConstraintTraits = setOf(
jjant marked this conversation as resolved.
Show resolved Hide resolved
LengthTrait::class.java,
PatternTrait::class.java,
RangeTrait::class.java,
UniqueItemsTrait::class.java,
EnumTrait::class.java,
RequiredTrait::class.java,
)

val supportedStringConstraintTraits: List<Class<out Trait>> = listOf(LengthTrait::class.java, PatternTrait::class.java)

/**
* Supported constraint traits for the `list` and `set` shapes.
*/
val supportedCollectionConstraintTraits: List<Class<out Trait>> = listOf(
LengthTrait::class.java,
// TODO(https://github.com/awslabs/smithy-rs/issues/1670): Not yet supported.
// UniqueItemsTrait::class.java
)

/**
* We say a shape is _directly_ constrained if:
*
Expand All @@ -72,6 +85,7 @@ fun Shape.isDirectlyConstrained(symbolProvider: SymbolProvider): Boolean = when

is MapShape -> this.hasTrait<LengthTrait>()
is StringShape -> this.hasTrait<EnumTrait>() || supportedStringConstraintTraits.any { this.hasTrait(it) }
is CollectionShape -> supportedCollectionConstraintTraits.any { this.hasTrait(it) }
is IntegerShape -> this.hasTrait<RangeTrait>()
else -> false
}
Expand All @@ -96,6 +110,7 @@ fun MemberShape.targetCanReachConstrainedShape(model: Model, symbolProvider: Sym
model.expectShape(this.target).canReachConstrainedShape(model, symbolProvider)

fun Shape.hasPublicConstrainedWrapperTupleType(model: Model, publicConstrainedTypes: Boolean): Boolean = when (this) {
is CollectionShape -> publicConstrainedTypes && supportedCollectionConstraintTraits.any(this::hasTrait)
is MapShape -> publicConstrainedTypes && this.hasTrait<LengthTrait>()
is StringShape -> !this.hasTrait<EnumTrait>() && (publicConstrainedTypes && supportedStringConstraintTraits.any(this::hasTrait))
is IntegerShape -> publicConstrainedTypes && this.hasTrait<RangeTrait>()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ import software.amazon.smithy.rust.codegen.core.smithy.transformers.RecursiveSha
import software.amazon.smithy.rust.codegen.core.util.CommandFailed
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.runCommand
import software.amazon.smithy.rust.codegen.server.smithy.generators.CollectionConstraintViolationGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.CollectionTraitInfo
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedCollectionGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedIntegerGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedMapGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedStringGenerator
Expand Down Expand Up @@ -278,11 +281,14 @@ open class ServerCodegenVisitor(
override fun setShape(shape: SetShape) = collectionShape(shape)

private fun collectionShape(shape: CollectionShape) {
if (shape.isReachableFromOperationInput() && shape.canReachConstrainedShape(
val renderUnconstrainedList =
shape.isReachableFromOperationInput() && shape.canReachConstrainedShape(
model,
codegenContext.symbolProvider,
)
) {
val isDirectlyConstrained = shape.isDirectlyConstrained(codegenContext.symbolProvider)

if (renderUnconstrainedList) {
jjant marked this conversation as resolved.
Show resolved Hide resolved
logger.info("[rust-server-codegen] Generating an unconstrained type for collection shape $shape")
rustCrate.withModule(UnconstrainedModule) unconstrainedModuleWriter@{
rustCrate.withModule(ModelsModule) modelsModuleWriter@{
Expand All @@ -295,9 +301,30 @@ open class ServerCodegenVisitor(
}
}

logger.info("[rust-server-codegen] Generating a constrained type for collection shape $shape")
rustCrate.withModule(ConstrainedModule) {
PubCrateConstrainedCollectionGenerator(codegenContext, this, shape).render()
if (!isDirectlyConstrained) {
logger.info("[rust-server-codegen] Generating a constrained type for collection shape $shape")
rustCrate.withModule(ConstrainedModule) {
PubCrateConstrainedCollectionGenerator(codegenContext, this, shape).render()
}
}
}

val constraintsInfo = CollectionTraitInfo.fromShape(shape)
if (isDirectlyConstrained) {
rustCrate.withModule(ModelsModule) {
ConstrainedCollectionGenerator(
codegenContext,
this,
shape,
constraintsInfo,
if (renderUnconstrainedList) codegenContext.unconstrainedShapeSymbolProvider.toSymbol(shape) else null,
).render()
}
}

if (isDirectlyConstrained || renderUnconstrainedList) {
rustCrate.withModule(ModelsModule) {
CollectionConstraintViolationGenerator(codegenContext, this, shape, constraintsInfo).render()
}
}
}
Expand All @@ -308,21 +335,22 @@ open class ServerCodegenVisitor(
model,
codegenContext.symbolProvider,
)
val isDirectlyConstrained = shape.isDirectlyConstrained(codegenContext.symbolProvider)

if (renderUnconstrainedMap) {
logger.info("[rust-server-codegen] Generating an unconstrained type for map $shape")
rustCrate.withModule(UnconstrainedModule) {
UnconstrainedMapGenerator(codegenContext, this, shape).render()
}

if (!shape.isDirectlyConstrained(codegenContext.symbolProvider)) {
if (!isDirectlyConstrained) {
logger.info("[rust-server-codegen] Generating a constrained type for map $shape")
rustCrate.withModule(ConstrainedModule) {
PubCrateConstrainedMapGenerator(codegenContext, this, shape).render()
}
}
}

val isDirectlyConstrained = shape.isDirectlyConstrained(codegenContext.symbolProvider)
if (isDirectlyConstrained) {
rustCrate.withModule(ModelsModule) {
ConstrainedMapGenerator(
Expand Down
Loading