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 long and byte shapes #2036

Merged
merged 8 commits into from Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
6 changes: 4 additions & 2 deletions CHANGELOG.next.toml
Expand Up @@ -242,15 +242,17 @@ message = """

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

Expand Down
132 changes: 131 additions & 1 deletion codegen-core/common-test-models/constraints.smithy
Expand Up @@ -50,7 +50,7 @@ operation ConstrainedShapesOperation {
}

@http(
uri: "/constrained-http-bound-shapes-operation/{rangeIntegerLabel}/{rangeShortLabel}/{lengthStringLabel}/{enumStringLabel}",
uri: "/constrained-http-bound-shapes-operation/{rangeIntegerLabel}/{rangeShortLabel}/{rangeLongLabel}/{rangeByteLabel}/{lengthStringLabel}/{enumStringLabel}",
method: "POST"
)
operation ConstrainedHttpBoundShapesOperation {
Expand Down Expand Up @@ -186,6 +186,14 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpLabel
rangeShortLabel: RangeShort,

@required
@httpLabel
rangeLongLabel: RangeLong,

@required
@httpLabel
rangeByteLabel: RangeByte,

@required
@httpLabel
enumStringLabel: EnumString,
Expand All @@ -203,6 +211,12 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpHeader("X-Range-Short")
rangeShortHeader: RangeShort,

@httpHeader("X-Range-Long")
rangeLongHeader: RangeLong,

@httpHeader("X-Range-Byte")
rangeByteHeader: RangeByte,

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

Expand All @@ -220,13 +234,23 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
// rangeIntegerSetHeader: SetOfRangeInteger,
// @httpHeader("X-Range-Short-Set")
// rangeShortSetHeader: SetOfShortInteger,
// @httpHeader("X-Range-Long-Set")
// rangeLongSetHeader: SetOfRangeLong,
// @httpHeader("X-Range-Byte-Set")
// rangeByteSetHeader: SetOfByteInteger,

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

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

@httpHeader("X-Range-Long-List")
rangeLongListHeader: ListOfRangeLong,

@httpHeader("X-Range-Byte-List")
rangeByteListHeader: ListOfRangeByte,

// TODO(https://github.com/awslabs/smithy-rs/issues/1431)
// @httpHeader("X-Enum")
//enumStringHeader: EnumString,
Expand All @@ -243,6 +267,12 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpQuery("rangeShort")
rangeShortQuery: RangeShort,

@httpQuery("rangeLong")
rangeLongQuery: RangeLong,

@httpQuery("rangeByte")
rangeByteQuery: RangeByte,

@httpQuery("enumString")
enumStringQuery: EnumString,

Expand All @@ -260,12 +290,22 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
@httpQuery("rangeShortList")
rangeShortListQuery: ListOfRangeShort,

@httpQuery("rangeLongList")
rangeLongListQuery: ListOfRangeLong,

@httpQuery("rangeByteList")
rangeByteListQuery: ListOfRangeByte,

// 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("rangeLongSet")
// rangeLongSetQuery: SetOfRangeLong,
// @httpQuery("rangeByteSet")
// rangeByteSetQuery: SetOfRangeByte,

@httpQuery("enumStringList")
enumStringListQuery: ListOfEnumString,
Expand Down Expand Up @@ -392,6 +432,16 @@ structure ConA {
maxRangeShort: MaxRangeShort,
fixedValueShort: FixedValueShort,

rangeLong: RangeLong,
minRangeLong: MinRangeLong,
maxRangeLong: MaxRangeLong,
fixedValueLong: FixedValueLong,

rangeByte: RangeByte,
minRangeByte: MinRangeByte,
maxRangeByte: MaxRangeByte,
fixedValueByte: FixedValueByte,

conBList: ConBList,
conBList2: ConBList2,

Expand Down Expand Up @@ -424,6 +474,18 @@ structure ConA {
// setOfRangeShort: SetOfRangeShort,
mapOfRangeShort: MapOfRangeShort,

listOfRangeLong: ListOfRangeLong,
// 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.
// setOfRangeLong: SetOfRangeLong,
mapOfRangeLong: MapOfRangeLong,

listOfRangeByte: ListOfRangeByte,
// 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.
// setOfRangeByte: SetOfRangeByte,
mapOfRangeByte: MapOfRangeByte,

nonStreamingBlob: NonStreamingBlob

patternString: PatternString,
Expand Down Expand Up @@ -456,6 +518,16 @@ map MapOfRangeShort {
value: RangeShort,
}

map MapOfRangeLong {
key: String,
value: RangeLong,
}

map MapOfRangeByte {
key: String,
value: RangeByte,
}

map MapOfEnumString {
key: EnumString,
value: EnumString,
Expand Down Expand Up @@ -503,6 +575,20 @@ map MapOfSetOfLengthString {
// value: SetOfRangeShort,
// }

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

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

@length(min: 2, max: 8)
list LengthListOfLengthString {
member: LengthString
Expand Down Expand Up @@ -555,6 +641,30 @@ short MaxRangeShort
@range(min: 10, max: 10)
short FixedValueShort

@range(min: -0, max: 10)
long RangeLong

@range(min: -10)
long MinRangeLong

@range(max: 11)
long MaxRangeLong

@range(min: 10, max: 10)
long FixedValueLong

@range(min: -0, max: 10)
byte RangeByte

@range(min: -10)
byte MinRangeByte

@range(max: 11)
byte MaxRangeByte

@range(min: 10, max: 10)
byte FixedValueByte

/// A union with constrained members.
union ConstrainedUnion {
enumString: EnumString,
Expand Down Expand Up @@ -620,6 +730,26 @@ list ListOfRangeShort {
member: RangeShort
}

// 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 SetOfRangeLong {
// member: RangeLong
// }

list ListOfRangeLong {
member: RangeLong
}

// 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 SetOfRangeByte {
// member: RangeByte
// }

list ListOfRangeByte {
member: RangeByte
}

list ListOfEnumString {
member: EnumString
}
Expand Down
Expand Up @@ -8,8 +8,10 @@ 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.knowledge.NullableIndex
import software.amazon.smithy.model.shapes.ByteShape
import software.amazon.smithy.model.shapes.CollectionShape
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.ServiceShape
Expand Down Expand Up @@ -98,10 +100,11 @@ class ConstrainedShapeSymbolProvider(
}
}

is StringShape, is IntegerShape, is ShortShape -> {
is StringShape, is IntegerShape, is ShortShape, is LongShape, is ByteShape -> {
if (shape.isDirectlyConstrained(base)) {
val rustType = RustType.Opaque(shape.contextName(serviceShape).toPascalCase())
symbolBuilder(shape, rustType).locatedIn(ModelsModule).build()
symbolBuilder(shape, rustType).locatedIn(ModelsModule).build()
LukeMathWalker marked this conversation as resolved.
Show resolved Hide resolved
} else {
base.toSymbol(shape)
}
Expand Down
Expand Up @@ -7,8 +7,10 @@ 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.ByteShape
import software.amazon.smithy.model.shapes.CollectionShape
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.ServiceShape
import software.amazon.smithy.model.shapes.Shape
Expand Down Expand Up @@ -115,7 +117,7 @@ class ConstraintViolationSymbolProvider(
.build()
}

is StringShape, is IntegerShape, is ShortShape -> {
is StringShape, is IntegerShape, is ShortShape, is LongShape, is ByteShape -> {
val module = shape.shapeModule()
val rustType = RustType.Opaque(constraintViolationName, module.fullyQualifiedPath())
Symbol.builder()
Expand Down
Expand Up @@ -8,8 +8,10 @@ package software.amazon.smithy.rust.codegen.server.smithy
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.ByteShape
import software.amazon.smithy.model.shapes.CollectionShape
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.Shape
Expand Down Expand Up @@ -73,7 +75,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, is ShortShape -> this.hasTrait<RangeTrait>()
is IntegerShape, is ShortShape, is LongShape, is ByteShape -> this.hasTrait<RangeTrait>()
else -> false
}

Expand All @@ -99,7 +101,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, is ShortShape -> publicConstrainedTypes && this.hasTrait<RangeTrait>()
is IntegerShape, is ShortShape, is LongShape, is ByteShape -> publicConstrainedTypes && this.hasTrait<RangeTrait>()
is MemberShape -> model.expectShape(this.target).hasPublicConstrainedWrapperTupleType(model, publicConstrainedTypes)
else -> false
}
Expand Down
Expand Up @@ -10,9 +10,11 @@ import software.amazon.smithy.codegen.core.CodegenException
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.ByteShape
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.LongShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.ServiceShape
import software.amazon.smithy.model.shapes.SetShape
Expand Down Expand Up @@ -44,7 +46,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.ConstrainedByteGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedIntegerGenerator
import software.amazon.smithy.rust.codegen.server.smithy.generators.ConstrainedLongGenerator
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
Expand Down Expand Up @@ -372,6 +376,24 @@ open class ServerCodegenVisitor(
}
}

override fun longShape(shape: LongShape) {
if (shape.isDirectlyConstrained(codegenContext.symbolProvider)) {
logger.info("[rust-server-codegen] Generating a constrained long $shape")
rustCrate.withModule(ModelsModule) {
ConstrainedLongGenerator(codegenContext, this, shape).render()
}
}
}
jjant marked this conversation as resolved.
Show resolved Hide resolved

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

protected fun stringShape(
shape: StringShape,
enumShapeGeneratorFactory: (codegenContext: ServerCodegenContext, writer: RustWriter, shape: StringShape) -> ServerEnumGenerator,
Expand Down
Expand Up @@ -8,9 +8,11 @@ package software.amazon.smithy.rust.codegen.server.smithy
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.neighbor.Walker
import software.amazon.smithy.model.shapes.BlobShape
import software.amazon.smithy.model.shapes.ByteShape
import software.amazon.smithy.model.shapes.CollectionShape
import software.amazon.smithy.model.shapes.EnumShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.LongShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.OperationShape
import software.amazon.smithy.model.shapes.ServiceShape
Expand Down Expand Up @@ -241,12 +243,12 @@ fun validateUnsupportedConstraints(
.map { UnsupportedLengthTraitOnCollectionOrOnBlobShape(it, it.expectTrait()) }
.toSet()

// 5. Range trait used on a non-integer shape. It has not been implemented yet.
// 5. Range trait used on unsupported shapes.
// TODO(https://github.com/awslabs/smithy-rs/issues/1401)
val unsupportedRangeTraitOnShapeSet = walker
.walkShapes(service)
.asSequence()
.filterNot { it is IntegerShape || it is ShortShape }
.filterNot { it is IntegerShape || it is ShortShape || it is LongShape || it is ByteShape }
.filterMapShapesToTraits(setOf(RangeTrait::class.java))
.map { (shape, rangeTrait) -> UnsupportedRangeTraitOnShape(shape, rangeTrait as RangeTrait) }
.toSet()
Comment on lines +246 to 254
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this? I think @range on other shapes is rejected by smithy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@range is allowed on floats and doubles, according to the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right (and probably bigDecimal and bigInteger).
I'd suggest using filter { it is FloatShape || it is DoubleShape || .. } instead of the negative as it makes it easier to see which are missing (but this is just a nit).

Expand Down