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 @range on short #2034

Merged
merged 8 commits into from
Nov 29, 2022
3 changes: 2 additions & 1 deletion CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,14 @@ message = """
* The `length` trait on `string` shapes.
* The `length` trait on `map` shapes.
* The `range` trait on `integer` shapes.
* The `range` trait on `short` 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#2005", "smithy-rs#1998", "smithy-rs#2034"]
meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "server" }
author = "david-perez"

Expand Down
72 changes: 70 additions & 2 deletions codegen-core/common-test-models/constraints.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ operation ConstrainedShapesOperation {
errors: [ValidationException]
}

@http(uri: "/constrained-http-bound-shapes-operation/{rangeIntegerLabel}/{lengthStringLabel}/{enumStringLabel}", method: "POST")
@http(
uri: "/constrained-http-bound-shapes-operation/{rangeIntegerLabel}/{rangeShortLabel}/{lengthStringLabel}/{enumStringLabel}",
method: "POST"
)
operation ConstrainedHttpBoundShapesOperation {
input: ConstrainedHttpBoundShapesOperationInputOutput,
output: ConstrainedHttpBoundShapesOperationInputOutput,
Expand Down Expand Up @@ -179,6 +182,10 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpLabel
rangeIntegerLabel: RangeInteger,

@required
@httpLabel
rangeShortLabel: RangeShort,

@required
@httpLabel
enumStringLabel: EnumString,
Expand All @@ -193,6 +200,9 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpHeader("X-Range-Integer")
rangeIntegerHeader: RangeInteger,

@httpHeader("X-Range-Short")
rangeShortHeader: RangeShort,
hlbarber marked this conversation as resolved.
Show resolved Hide resolved

// @httpHeader("X-Length-MediaType")
// lengthStringHeaderWithMediaType: MediaTypeLengthString,

Expand All @@ -208,10 +218,15 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
// just a `list` shape with `uniqueItems`, which hasn't been implemented yet.
// @httpHeader("X-Range-Integer-Set")
// rangeIntegerSetHeader: SetOfRangeInteger,
// @httpHeader("X-Range-Short-Set")
// rangeShortSetHeader: SetOfShortInteger,

@httpHeader("X-Range-Integer-List")
rangeIntegerListHeader: ListOfRangeInteger,

@httpHeader("X-Range-Short-List")
rangeShortListHeader: ListOfRangeShort,

// TODO(https://github.com/awslabs/smithy-rs/issues/1431)
// @httpHeader("X-Enum")
//enumStringHeader: EnumString,
Expand All @@ -225,6 +240,9 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpQuery("rangeInteger")
rangeIntegerQuery: RangeInteger,

@httpQuery("rangeShort")
rangeShortQuery: RangeShort,

@httpQuery("enumString")
enumStringQuery: EnumString,

Expand All @@ -239,10 +257,15 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpQuery("rangeIntegerList")
rangeIntegerListQuery: ListOfRangeInteger,

@httpQuery("rangeShortList")
rangeShortListQuery: ListOfRangeShort,

// 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.
// @httpQuery("rangeIntegerSet")
// rangeIntegerSetQuery: SetOfRangeInteger,
// @httpQuery("rangeShortSet")
// rangeShortSetQuery: SetOfRangeShort,

@httpQuery("enumStringList")
enumStringListQuery: ListOfEnumString,
Expand Down Expand Up @@ -364,6 +387,11 @@ structure ConA {
maxRangeInteger: MaxRangeInteger,
fixedValueInteger: FixedValueInteger,

rangeShort: RangeShort,
minRangeShort: MinRangeShort,
maxRangeShort: MaxRangeShort,
fixedValueShort: FixedValueShort,

conBList: ConBList,
conBList2: ConBList2,

Expand All @@ -390,6 +418,12 @@ structure ConA {
// setOfRangeInteger: SetOfRangeInteger,
mapOfRangeInteger: MapOfRangeInteger,

listOfRangeShort: ListOfRangeShort,
// 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.
// setOfRangeShort: SetOfRangeShort,
mapOfRangeShort: MapOfRangeShort,

nonStreamingBlob: NonStreamingBlob

patternString: PatternString,
Expand Down Expand Up @@ -417,6 +451,11 @@ map MapOfRangeInteger {
value: RangeInteger,
}

map MapOfRangeShort {
key: String,
value: RangeShort,
}

map MapOfEnumString {
key: EnumString,
value: EnumString,
Expand Down Expand Up @@ -453,10 +492,17 @@ map MapOfSetOfLengthString {
// 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 {
// key: LengthString,
// key: String,
// value: SetOfRangeInteger,
// }

// 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 MapOfSetOfRangeShort {
// key: String,
// value: SetOfRangeShort,
// }

@length(min: 2, max: 8)
list LengthListOfLengthString {
member: LengthString
Expand Down Expand Up @@ -497,6 +543,18 @@ integer MaxRangeInteger
@range(min: 69, max: 69)
integer FixedValueInteger

@range(min: -0, max: 10)
integer RangeShort
hlbarber marked this conversation as resolved.
Show resolved Hide resolved

@range(min: -10)
integer MinRangeShort

@range(max: 11)
integer MaxRangeShort

@range(min: 10, max: 10)
integer FixedValueShort

/// A union with constrained members.
union ConstrainedUnion {
enumString: EnumString,
Expand Down Expand Up @@ -552,6 +610,16 @@ list ListOfRangeInteger {
member: RangeInteger
}

// 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.
// set SetOfRangeShort {
// member: RangeShort
// }

list ListOfRangeShort {
member: RangeShort
}

list ListOfEnumString {
member: EnumString
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.ServiceShape
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.traits.LengthTrait
import software.amazon.smithy.rust.codegen.core.rustlang.RustType
Expand Down Expand Up @@ -96,14 +97,16 @@ class ConstrainedShapeSymbolProvider(
symbolBuilder(shape, RustType.Vec(inner.rustType())).addReference(inner).build()
}
}
is StringShape, is IntegerShape -> {

is StringShape, is IntegerShape, is ShortShape -> {
if (shape.isDirectlyConstrained(base)) {
val rustType = RustType.Opaque(shape.contextName(serviceShape).toPascalCase())
symbolBuilder(shape, rustType).locatedIn(ModelsModule).build()
} else {
base.toSymbol(shape)
}
}

else -> base.toSymbol(shape)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ 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
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.UnionShape
Expand Down Expand Up @@ -113,7 +114,8 @@ class ConstraintViolationSymbolProvider(
.locatedIn(module)
.build()
}
is StringShape, is IntegerShape -> {

is StringShape, is IntegerShape, is ShortShape -> {
val module = shape.shapeModule()
val rustType = RustType.Opaque(constraintViolationName, module.fullyQualifiedPath())
Symbol.builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ 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
import software.amazon.smithy.model.shapes.ShortShape
import software.amazon.smithy.model.shapes.SimpleShape
import software.amazon.smithy.model.shapes.StringShape
import software.amazon.smithy.model.shapes.StructureShape
Expand Down Expand Up @@ -72,7 +73,7 @@ fun Shape.isDirectlyConstrained(symbolProvider: SymbolProvider): Boolean = when

is MapShape -> this.hasTrait<LengthTrait>()
is StringShape -> this.hasTrait<EnumTrait>() || supportedStringConstraintTraits.any { this.hasTrait(it) }
is IntegerShape -> this.hasTrait<RangeTrait>()
is IntegerShape, is ShortShape -> this.hasTrait<RangeTrait>()
else -> false
}

Expand All @@ -98,7 +99,7 @@ fun MemberShape.targetCanReachConstrainedShape(model: Model, symbolProvider: Sym
fun Shape.hasPublicConstrainedWrapperTupleType(model: Model, publicConstrainedTypes: Boolean): Boolean = when (this) {
is MapShape -> publicConstrainedTypes && this.hasTrait<LengthTrait>()
is StringShape -> !this.hasTrait<EnumTrait>() && (publicConstrainedTypes && supportedStringConstraintTraits.any(this::hasTrait))
is IntegerShape -> publicConstrainedTypes && this.hasTrait<RangeTrait>()
is IntegerShape, is ShortShape -> publicConstrainedTypes && this.hasTrait<RangeTrait>()
is MemberShape -> model.expectShape(this.target).hasPublicConstrainedWrapperTupleType(model, publicConstrainedTypes)
else -> false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import software.amazon.smithy.model.shapes.ServiceShape
import software.amazon.smithy.model.shapes.SetShape
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.ShapeVisitor
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.UnionShape
Expand Down Expand Up @@ -45,6 +46,7 @@ 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.ConstrainedShortGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedStringGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedTraitForEnumGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.MapConstraintViolationGenerator
Expand Down Expand Up @@ -361,6 +363,15 @@ open class ServerCodegenVisitor(
}
}

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

protected fun stringShape(
shape: StringShape,
enumShapeGeneratorFactory: (codegenContext: ServerCodegenContext, writer: RustWriter, shape: StringShape) -> ServerEnumGenerator,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import software.amazon.smithy.model.shapes.ServiceShape
import software.amazon.smithy.model.shapes.SetShape
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.ShapeId
import software.amazon.smithy.model.shapes.ShortShape
import software.amazon.smithy.model.shapes.UnionShape
import software.amazon.smithy.model.traits.EnumTrait
import software.amazon.smithy.model.traits.LengthTrait
Expand Down Expand Up @@ -245,7 +246,7 @@ fun validateUnsupportedConstraints(
val unsupportedRangeTraitOnShapeSet = walker
.walkShapes(service)
.asSequence()
.filterNot { it is IntegerShape }
.filterNot { it is IntegerShape || it is ShortShape }
.filterMapShapesToTraits(setOf(RangeTrait::class.java))
.map { (shape, rangeTrait) -> UnsupportedRangeTraitOnShape(shape, rangeTrait as RangeTrait) }
.toSet()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package software.amazon.smithy.rust.codegen.server.smithy.customizations

import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.ShortShape
import software.amazon.smithy.rust.codegen.core.rustlang.Writable
import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.protocols.serialize.JsonSerializerCustomization
Expand All @@ -27,7 +28,7 @@ class BeforeSerializingMemberJsonCustomization(private val codegenContext: Serve
codegenContext.settings.codegenConfig.publicConstrainedTypes,
)
) {
if (section.shape is IntegerShape) {
if (section.shape is IntegerShape || section.shape is ShortShape) {
section.context.valueExpression =
ValueExpression.Reference("&${section.context.valueExpression.name}.0")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators

import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.NumberShape
import software.amazon.smithy.model.shapes.ShortShape
import software.amazon.smithy.model.traits.RangeTrait
import software.amazon.smithy.rust.codegen.core.rustlang.Attribute
import software.amazon.smithy.rust.codegen.core.rustlang.RustMetadata
Expand All @@ -31,15 +33,40 @@ import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext
import software.amazon.smithy.rust.codegen.server.smithy.traits.isReachableFromOperationInput
import software.amazon.smithy.rust.codegen.server.smithy.validationErrorMessage

class ConstrainedIntegerGenerator(
val codegenContext: ServerCodegenContext,
val writer: RustWriter,
val shape: IntegerShape,
) {
val inner = ConstrainedNumberGenerator(codegenContext, writer, shape, RustType.Integer(32))

fun render() {
inner.render()
}
}

class ConstrainedShortGenerator(
val codegenContext: ServerCodegenContext,
val writer: RustWriter,
val shape: ShortShape,
) {
val inner = ConstrainedNumberGenerator(codegenContext, writer, shape, RustType.Integer(16))

fun render() {
inner.render()
}
}

/**
* [ConstrainedIntegerGenerator] generates a wrapper newtype holding a constrained `i32`.
* [ConstrainedNumberGenerator] generates a wrapper newtype holding a constrained number primitive.
* This type can be built from unconstrained values, yielding a `ConstraintViolation` when the input does not satisfy
* the constraints.
*/
class ConstrainedIntegerGenerator(
class ConstrainedNumberGenerator(
val codegenContext: ServerCodegenContext,
val writer: RustWriter,
val shape: IntegerShape,
val shape: NumberShape,
private val unconstrainedType: RustType,
) {
val model = codegenContext.model
val constrainedShapeSymbolProvider = codegenContext.constrainedShapeSymbolProvider
Expand All @@ -58,7 +85,7 @@ class ConstrainedIntegerGenerator(

val symbol = constrainedShapeSymbolProvider.toSymbol(shape)
val constrainedTypeName = symbol.name
val unconstrainedTypeName = RustType.Integer(32).render()
val unconstrainedTypeName = unconstrainedType.render()
val constraintViolation = constraintViolationSymbolProvider.toSymbol(shape)
val constraintsInfo = listOf(Range(rangeTrait).toTraitInfo(unconstrainedTypeName))

Expand Down Expand Up @@ -171,7 +198,7 @@ private data class Range(val rangeTrait: RangeTrait) {
fun toTraitInfo(unconstrainedTypeName: String): TraitInfo = TraitInfo(
{ rust("Self::check_range(value)?;") },
{
docs("Error when an integer doesn't satisfy its `@range` requirements.")
docs("Error when a number doesn't satisfy its `@range` requirements.")
rust("Range($unconstrainedTypeName)")
},
{
Expand All @@ -188,7 +215,7 @@ private data class Range(val rangeTrait: RangeTrait) {
)

/**
* Renders a `check_range` function to validate the integer matches the
* Renders a `check_range` function to validate that the value matches the
* required range indicated by the `@range` trait.
*/
private fun renderValidationFunction(constraintViolation: Symbol, unconstrainedTypeName: String): Writable = {
Expand Down
Loading