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

@range implementation for integer shapes #2005

Merged
merged 78 commits into from Nov 28, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
42cddc8
Draft of @range implementation for integer shapes
LukeMathWalker Nov 18, 2022
bc62d0e
Green tests.
LukeMathWalker Nov 18, 2022
53f5c59
Merge branch 'main' into range
LukeMathWalker Nov 18, 2022
4e6db6b
Fix serialization of constrained integers.
LukeMathWalker Nov 18, 2022
1c19bc7
Fix tagger to include Integer simple shapes.
LukeMathWalker Nov 18, 2022
35148b1
Merge branch 'main' into range
LukeMathWalker Nov 18, 2022
cf72c16
Add range trait to the entry about constraint traits in our changelog.
LukeMathWalker Nov 18, 2022
2cbe2a5
Bind a range-constrained integer to various HTTP parts to test our im…
LukeMathWalker Nov 18, 2022
0ac5e66
Merge branch 'main' into range
LukeMathWalker Nov 18, 2022
f50b7b0
Rename ConstrainedIntGeneratorTest to ConstrainedIntegerGeneratorTest…
LukeMathWalker Nov 18, 2022
0b80d2e
Remove AsRef implementation.
LukeMathWalker Nov 18, 2022
2e77b8f
Fix constraints models.
LukeMathWalker Nov 18, 2022
58c156f
Fix conversion.
LukeMathWalker Nov 18, 2022
a959004
Use ReturnSymbolToParse to dry up.
LukeMathWalker Nov 18, 2022
3cc5e35
Fix builder when constrained types should not be exposed.
LukeMathWalker Nov 18, 2022
1e9d6c7
Add customisation to unwrap constrained integers.
LukeMathWalker Nov 18, 2022
22ff34f
Getting closer - collections need to be handled differently to make e…
LukeMathWalker Nov 18, 2022
395c965
Refactor `renderHeaders`
LukeMathWalker Nov 21, 2022
a65f59d
Fix constraints test.
LukeMathWalker Nov 21, 2022
250139d
Fix ebs.
LukeMathWalker Nov 21, 2022
581162b
Rename for clarity.
LukeMathWalker Nov 21, 2022
d719328
Remove unnecessary From implementation.
LukeMathWalker Nov 21, 2022
6e54c5e
Rename `Size` variant to `Range`.
LukeMathWalker Nov 21, 2022
d34a525
Remove stray comments.
LukeMathWalker Nov 21, 2022
5762d2f
Rename for clarity
LukeMathWalker Nov 21, 2022
ea9bdac
Rename for consistency
LukeMathWalker Nov 21, 2022
19297bc
Add test to guard against types for which we do not support range yet
LukeMathWalker Nov 21, 2022
35dfb82
DRY up branches and the relevant tests.
LukeMathWalker Nov 21, 2022
54e44c4
Merge branch 'main' into range
LukeMathWalker Nov 21, 2022
a94304f
Fix header name.
LukeMathWalker Nov 21, 2022
f36a617
Fix serialization bug for default values in headers.
LukeMathWalker Nov 21, 2022
c2773cd
Fix serialization issue for primitive headers.
LukeMathWalker Nov 22, 2022
f37a560
Remove SetOfRangeInteger
LukeMathWalker Nov 22, 2022
5cd6d35
Fix pubcrateconstrained unit test.
LukeMathWalker Nov 22, 2022
72cda3a
Remove duplication
LukeMathWalker Nov 22, 2022
5c5c026
Merge branch 'main' into range
LukeMathWalker Nov 22, 2022
6efc9b9
Revert "Remove SetOfRangeInteger"
LukeMathWalker Nov 22, 2022
8359468
Reintroduce `SetOfRangeInteger`, but keep them commented out.
LukeMathWalker Nov 22, 2022
d36930d
Ignore leading whitespace.
LukeMathWalker Nov 22, 2022
dc0ef4b
Merge branch 'main' into range
LukeMathWalker Nov 22, 2022
87e4766
Merge branch 'main' into range
LukeMathWalker Nov 22, 2022
7328a9e
Merge branch 'main' into range
LukeMathWalker Nov 23, 2022
d259d86
Update codegen-core/src/main/kotlin/software/amazon/smithy/rust/codeg…
LukeMathWalker Nov 23, 2022
e2c5952
Update codegen-core/src/main/kotlin/software/amazon/smithy/rust/codeg…
LukeMathWalker Nov 23, 2022
582de6d
Update codegen-server/src/main/kotlin/software/amazon/smithy/rust/cod…
LukeMathWalker Nov 23, 2022
3f1bd70
Update codegen-server/src/test/kotlin/software/amazon/smithy/rust/cod…
LukeMathWalker Nov 23, 2022
64e3312
Unify with a single rustTemplate invocation.
LukeMathWalker Nov 23, 2022
f927eaf
Rename `Type` to `NumberType`
LukeMathWalker Nov 23, 2022
922d489
Update codegen-server/src/test/kotlin/software/amazon/smithy/rust/cod…
LukeMathWalker Nov 23, 2022
563c1cc
Formatting issue.
LukeMathWalker Nov 23, 2022
0ec9f13
Move and rename test helper.
LukeMathWalker Nov 23, 2022
f39669b
Dry up the logic for default serialization.
LukeMathWalker Nov 23, 2022
bdec811
Ooops, I swapped the two branches.
LukeMathWalker Nov 23, 2022
b2c4c9b
Merge main
LukeMathWalker Nov 23, 2022
69c7b80
Add a wrapper block
LukeMathWalker Nov 23, 2022
040eef6
Fix support detection.
LukeMathWalker Nov 23, 2022
b1787bb
Fix CHANGELOG.
LukeMathWalker Nov 23, 2022
5141fa4
Add (failing) protocol tests for @range on byte/integer/long.
LukeMathWalker Nov 23, 2022
50aa3b6
Fix validation message.
LukeMathWalker Nov 23, 2022
c9a7556
Mark some tests as expected to fail.
LukeMathWalker Nov 23, 2022
3977b8c
Use ValueExpression everywhere for more granular control of the owner…
LukeMathWalker Nov 24, 2022
2b31a14
Merge branch 'main' into range
LukeMathWalker Nov 24, 2022
9ccad94
Use the new enhanced module facilities
LukeMathWalker Nov 24, 2022
6024264
Fixes.
LukeMathWalker Nov 24, 2022
1a884f6
Fix formatting
LukeMathWalker Nov 24, 2022
48f8fb9
Remove unnecessary when.
LukeMathWalker Nov 24, 2022
fc3cf47
Update codegen-core/common-test-models/constraints.smithy
LukeMathWalker Nov 25, 2022
28eae9c
Remove unused shapes.
LukeMathWalker Nov 25, 2022
5b71893
Avoid mixing concerns in our test shapes for integer constraints.
LukeMathWalker Nov 25, 2022
2211f3f
Reuse the trait info machinery
LukeMathWalker Nov 25, 2022
28357fe
Update stale comment
LukeMathWalker Nov 25, 2022
1d74bec
Fix unused attribute.
LukeMathWalker Nov 25, 2022
374f839
Update codegen-core/src/main/kotlin/software/amazon/smithy/rust/codeg…
LukeMathWalker Nov 28, 2022
2cf6141
Merge branch 'main' into range
LukeMathWalker Nov 28, 2022
04ffdb7
Avoid unnecessary bindings by manipulating the serialization context …
LukeMathWalker Nov 28, 2022
a51e18a
Avoid unnecessary bindings in header serialization by introducing and…
LukeMathWalker Nov 28, 2022
3160a92
Add code examples.
LukeMathWalker Nov 28, 2022
7d4d59f
Move `safeName` call into the if branch.
LukeMathWalker Nov 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.next.toml
Expand Up @@ -221,12 +221,13 @@ message = """

* The `length` trait on `string` shapes.
* The `length` trait on `map` shapes.
* The `range` trait on `integer` 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"]
references = ["smithy-rs#1199", "smithy-rs#1342", "smithy-rs#1401", "smithy-rs#2005"]
meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "server" }
author = "david-perez"

Expand Down
Expand Up @@ -8,13 +8,17 @@ package software.amazon.smithy.rust.codegen.core.smithy.protocols.parse
import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.model.shapes.BlobShape
import software.amazon.smithy.model.shapes.BooleanShape
import software.amazon.smithy.model.shapes.ByteShape
import software.amazon.smithy.model.shapes.CollectionShape
import software.amazon.smithy.model.shapes.DocumentShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.LongShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.NumberShape
import software.amazon.smithy.model.shapes.OperationShape
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.ShortShape
import software.amazon.smithy.model.shapes.StringShape
import software.amazon.smithy.model.shapes.StructureShape
import software.amazon.smithy.model.shapes.TimestampShape
Expand Down Expand Up @@ -50,12 +54,14 @@ import software.amazon.smithy.rust.codegen.core.smithy.isRustBoxed
import software.amazon.smithy.rust.codegen.core.smithy.protocols.HttpBindingResolver
import software.amazon.smithy.rust.codegen.core.smithy.protocols.HttpLocation
import software.amazon.smithy.rust.codegen.core.smithy.protocols.deserializeFunctionName
import software.amazon.smithy.rust.codegen.core.smithy.rustType
import software.amazon.smithy.rust.codegen.core.util.PANIC
import software.amazon.smithy.rust.codegen.core.util.dq
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.outputShape
import software.amazon.smithy.utils.StringUtils
import java.lang.IllegalStateException

/**
* Class describing a JSON parser section that can be used in a customization.
Expand Down Expand Up @@ -328,15 +334,37 @@ class JsonParserGenerator(
} else if (target.isDoubleShape) {
rustTemplate("#{expect_number_or_null}(tokens.next())?.map(|v| v.to_f64_lossy())", *codegenScope)
} else {
rustTemplate(
"""
#{expect_number_or_null}(tokens.next())?
.map(#{NumberType}::try_from)
.transpose()?
""",
"NumberType" to symbolProvider.toSymbol(target),
*codegenScope,
)
val numberSymbol = symbolProvider.toSymbol(target)
if (numberSymbol.rustType() is RustType.Opaque) {
val precision = when (target) {
is LongShape -> 64
is IntegerShape -> 32
is ShortShape -> 16
is ByteShape -> 8
else -> {
throw IllegalStateException("unreachable")
}
}
rustTemplate(
"""
#{expect_number_or_null}(tokens.next())?
.map(#{NumberPrimitive}::try_from)
.transpose()?
""",
"NumberPrimitive" to RustType.Integer(precision),
*codegenScope,
)
} else {
LukeMathWalker marked this conversation as resolved.
Show resolved Hide resolved
rustTemplate(
"""
#{expect_number_or_null}(tokens.next())?
.map(#{NumberType}::try_from)
.transpose()?
""",
"NumberType" to numberSymbol.rustType(),
*codegenScope,
)
}
}
}

Expand Down
Expand Up @@ -9,6 +9,7 @@ import software.amazon.smithy.model.shapes.BlobShape
import software.amazon.smithy.model.shapes.BooleanShape
import software.amazon.smithy.model.shapes.CollectionShape
import software.amazon.smithy.model.shapes.DocumentShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.NumberShape
Expand All @@ -29,6 +30,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
import software.amazon.smithy.rust.codegen.core.rustlang.rustBlockTemplate
import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
import software.amazon.smithy.rust.codegen.core.rustlang.withBlock
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.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
Expand Down Expand Up @@ -362,6 +364,21 @@ class JsonSerializerGenerator(
when (target) {
is StringShape -> rust("$writer.string(${value.name}.as_str());")
is BooleanShape -> rust("$writer.boolean(${value.asValue()});")
is IntegerShape -> {
val rustType = symbolProvider.toSymbol(target).rustType()
val conversion = writable {
if (rustType is RustType.Opaque) {
rust("(*${value.asRef()}.as_ref()).into()")
} else {
rust("(${value.asValue()}).into()")
}
}
rustTemplate(
"$writer.number(##[allow(clippy::useless_conversion)]#{Variant}::NegInt(#{Conversion:W}));",
"Variant" to smithyTypes.member("Number"),
"Conversion" to conversion,
)
}
LukeMathWalker marked this conversation as resolved.
Show resolved Hide resolved
is NumberShape -> {
val numberType = when (symbolProvider.toSymbol(target).rustType()) {
is RustType.Float -> "Float"
Expand Down
Expand Up @@ -9,6 +9,7 @@ import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.knowledge.NullableIndex
import software.amazon.smithy.model.shapes.CollectionShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.ServiceShape
Expand Down Expand Up @@ -103,6 +104,14 @@ class ConstrainedShapeSymbolProvider(
base.toSymbol(shape)
}
}
is IntegerShape -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this arm in ConstrainedShapeSymbolProvider.kt?

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 35dfb82

if (shape.isDirectlyConstrained(base)) {
val rustType = RustType.Opaque(shape.contextName(serviceShape).toPascalCase())
symbolBuilder(shape, rustType).locatedIn(Models).build()
} else {
base.toSymbol(shape)
}
}
else -> base.toSymbol(shape)
}
}
Expand Down
Expand Up @@ -8,6 +8,7 @@ package software.amazon.smithy.rust.codegen.server.smithy
import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.shapes.CollectionShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.ServiceShape
import software.amazon.smithy.model.shapes.Shape
Expand Down Expand Up @@ -117,6 +118,20 @@ class ConstraintViolationSymbolProvider(
.definitionFile(Models.filename)
.build()
}
is IntegerShape -> {
LukeMathWalker marked this conversation as resolved.
Show resolved Hide resolved
val namespace = "crate::${Models.namespace}::${
RustReservedWords.escapeIfNeeded(
shape.contextName(serviceShape).toSnakeCase(),
)
}"
val rustType = RustType.Opaque(constraintViolationName, namespace)
Symbol.builder()
.rustType(rustType)
.name(rustType.name)
.namespace(rustType.namespace, "::")
.definitionFile(Models.filename)
.build()
}
else -> TODO("Constraint traits on other shapes not implemented yet: $shape")
}
}
Expand Down
Expand Up @@ -9,6 +9,7 @@ import software.amazon.smithy.codegen.core.SymbolProvider
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.neighbor.Walker
import software.amazon.smithy.model.shapes.CollectionShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.Shape
Expand Down Expand Up @@ -67,6 +68,7 @@ fun Shape.isDirectlyConstrained(symbolProvider: SymbolProvider): Boolean = when
}
is MapShape -> this.hasTrait<LengthTrait>()
is StringShape -> this.hasTrait<EnumTrait>() || this.hasTrait<LengthTrait>()
is IntegerShape -> this.hasTrait<RangeTrait>()
else -> false
}

Expand Down
@@ -0,0 +1,21 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.rust.codegen.server.smithy

import software.amazon.smithy.model.traits.RangeTrait

fun RangeTrait.validationErrorMessage(): String {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably make this DRYer with LengthTraitValidationErrorMessage.kt if the protocol tests specify the messages coincide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message are currently slightly different, what do you mean by

if the protocol tests specify the messages coincide.

?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are protocol tests that assert that we reject malformed requests, and stipulate the format of the error message that we need to return in the response: https://github.com/awslabs/smithy-rs/blob/87e4766f9896663e4e00dc8d33b9cc7c49601ea0/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt#L987-L988

When we implement everything correctly, you should see those tests start passing and hence be forced to remove them from ExpectFail.

The format of the error message is here. It doesn't begin with "The integer value", but rather with "Value x at...".

My comment above was saying that we could extract the definition of ending, since it seems to be the same in both error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that Smithy only provides tests for malformed bytes and floats. We should write our own for shorts, integers, longs and doubles and contribute them upstream. Since you're only generating constrained integers, you won't be able to test that the implementation is correct unless you write tests for integers in this PR too.

val beginning = "The integer value '{}' at '{}' failed to satisfy constraint: Member must be "
val ending = if (this.min.isPresent && this.max.isPresent) {
"between ${this.min.get()} and ${this.max.get()}, inclusive"
} else if (this.min.isPresent) (
"greater than or equal to ${this.min.get()}"
) else {
check(this.max.isPresent)
"less than or equal to ${this.max.get()}"
}
return "$beginning$ending"
}
Expand Up @@ -11,6 +11,7 @@ import software.amazon.smithy.model.Model
import software.amazon.smithy.model.knowledge.NullableIndex
import software.amazon.smithy.model.neighbor.Walker
import software.amazon.smithy.model.shapes.CollectionShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.ListShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.ServiceShape
Expand Down Expand Up @@ -43,6 +44,7 @@ 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.ConstrainedIntegerGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedMapGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedStringGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedTraitForEnumGenerator
Expand Down Expand Up @@ -362,6 +364,15 @@ open class ServerCodegenVisitor(
stringShape(shape, ::serverEnumGeneratorFactory)
}

override fun integerShape(shape: IntegerShape) {
if (shape.isDirectlyConstrained(codegenContext.symbolProvider)) {
logger.info("[rust-server-codegen] Generating a constrained integer $shape")
rustCrate.withModule(ModelsModule) {
ConstrainedIntegerGenerator(codegenContext, this, shape).render()
}
}
}

protected fun stringShape(
shape: StringShape,
enumShapeGeneratorFactory: (codegenContext: ServerCodegenContext, writer: RustWriter, shape: StringShape) -> ServerEnumGenerator,
Expand Down
Expand Up @@ -141,20 +141,20 @@ fun validateOperationsWithConstrainedInputHaveValidationExceptionAttached(model:
LogMessage(
Level.SEVERE,
"""
Operation ${it.shape.id} takes in input that is constrained
(https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a validation
Operation ${it.shape.id} takes in input that is constrained
(https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a validation
exception. You must model this behavior in the operation shape in your model file.
""".trimIndent().replace("\n", "") +
"""
```smithy
use smithy.framework#ValidationException
operation ${it.shape.id.name} {
...
errors: [..., ValidationException] // <-- Add this.
}
```

```smithy
use smithy.framework#ValidationException

operation ${it.shape.id.name} {
...
errors: [..., ValidationException] // <-- Add this.
}
```
""".trimIndent(),
)
}
Expand Down Expand Up @@ -219,11 +219,12 @@ fun validateUnsupportedConstraints(model: Model, service: ServiceShape, codegenC
.map { (shape, patternTrait) -> UnsupportedPatternTraitOnStringShape(shape, patternTrait as PatternTrait) }
.toSet()

// 6. Range trait on any shape is used. It has not been implemented yet.
// 6. Range trait used on a non-integer shape. It has not been implemented yet.
// TODO(https://github.com/awslabs/smithy-rs/issues/1401)
val unsupportedRangeTraitOnShapeSet = walker
.walkShapes(service)
.asSequence()
.filter { !it.isIntegerShape }
LukeMathWalker marked this conversation as resolved.
Show resolved Hide resolved
.filterMapShapesToTraits(setOf(RangeTrait::class.java))
.map { (shape, rangeTrait) -> UnsupportedRangeTraitOnShape(shape, rangeTrait as RangeTrait) }
.toSet()
Expand Down