From 77d5a51978244096c9ecc9028dd3552826c6830a Mon Sep 17 00:00:00 2001 From: 82marbag <69267416+82marbag@users.noreply.github.com> Date: Tue, 20 Dec 2022 10:04:49 +0100 Subject: [PATCH 01/11] Default trait in server Signed-off-by: Daniele Ahmed --- .../common-test-models/constraints.smithy | 2 +- codegen-core/common-test-models/simple.smithy | 126 +------ .../rust/codegen/server/smithy/Constraints.kt | 2 +- .../server/smithy/ServerCargoDependency.kt | 1 + .../ServerBuilderConstraintViolations.kt | 7 +- .../generators/ServerBuilderGenerator.kt | 252 ++++++++++++-- ...rGeneratorWithoutPublicConstrainedTypes.kt | 46 ++- .../ServerBuilderDefaultValuesTest.kt | 326 ++++++++++++++++++ .../src/types.rs | 6 + 9 files changed, 608 insertions(+), 160 deletions(-) create mode 100644 codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt diff --git a/codegen-core/common-test-models/constraints.smithy b/codegen-core/common-test-models/constraints.smithy index 138c9e632f..e1f8215877 100644 --- a/codegen-core/common-test-models/constraints.smithy +++ b/codegen-core/common-test-models/constraints.smithy @@ -651,7 +651,7 @@ string LengthPatternString @length(min: 1, max: 69) string MediaTypeLengthString -@range(min: -0, max: 69) +@range(min: -7, max: 69) integer RangeInteger @range(min: -10) diff --git a/codegen-core/common-test-models/simple.smithy b/codegen-core/common-test-models/simple.smithy index 43c4bc6aca..c21aec67b7 100644 --- a/codegen-core/common-test-models/simple.smithy +++ b/codegen-core/common-test-models/simple.smithy @@ -1,4 +1,4 @@ -$version: "1.0" +$version: "2.0" namespace com.amazonaws.simple @@ -12,125 +12,23 @@ use smithy.framework#ValidationException @documentation("A simple service example, with a Service resource that can be registered and a readonly healthcheck") service SimpleService { version: "2022-01-01", - resources: [ - Service, - ], operations: [ - Healthcheck, - StoreServiceBlob, + HealthCheck, ], } -@documentation("Id of the service that will be registered") -string ServiceId - -@documentation("Name of the service that will be registered") -string ServiceName - -@error("client") -@documentation( - """ - Returned when a new resource cannot be created because one already exists. - """ -) -structure ResourceAlreadyExists { - @required - message: String -} - -@documentation("A resource that can register services") -resource Service { - identifiers: { id: ServiceId }, - put: RegisterService, -} - -@idempotent -@http(method: "PUT", uri: "/service/{id}") -@documentation("Service register operation") -@httpRequestTests([ - { - id: "RegisterServiceRequestTest", - protocol: "aws.protocols#restJson1", - uri: "/service/1", - headers: { - "Content-Type": "application/json", - }, - params: { id: "1", name: "TestService" }, - body: "{\"name\":\"TestService\"}", - method: "PUT", - } -]) -@httpResponseTests([ - { - id: "RegisterServiceResponseTest", - protocol: "aws.protocols#restJson1", - params: { id: "1", name: "TestService" }, - body: "{\"id\":\"1\",\"name\":\"TestService\"}", - code: 200, - headers: { - "Content-Length": "31" - } - } -]) -operation RegisterService { - input: RegisterServiceInputRequest, - output: RegisterServiceOutputResponse, - errors: [ResourceAlreadyExists, ValidationException] -} - -@documentation("Service register input structure") -structure RegisterServiceInputRequest { - @required - @httpLabel - id: ServiceId, - name: ServiceName, -} - -@documentation("Service register output structure") -structure RegisterServiceOutputResponse { - @required - id: ServiceId, - name: ServiceName, -} - -@readonly -@http(uri: "/healthcheck", method: "GET") -@documentation("Read-only healthcheck operation") -operation Healthcheck { - input: HealthcheckInputRequest, - output: HealthcheckOutputResponse -} - -@documentation("Service healthcheck output structure") -structure HealthcheckInputRequest { - -} - -@documentation("Service healthcheck input structure") -structure HealthcheckOutputResponse { - -} - -@readonly -@http(method: "POST", uri: "/service/{id}/blob") -@documentation("Stores a blob for a service id") -operation StoreServiceBlob { - input: StoreServiceBlobInput, - output: StoreServiceBlobOutput, +@http(uri: "/", method: "POST") +operation HealthCheck { + input: Input, errors: [ValidationException] } -@documentation("Store a blob for a service id input structure") -structure StoreServiceBlobInput { - @required - @httpLabel - id: ServiceId, - @required - @httpPayload - content: Blob, +@input +structure Input { + @default(15) + int:ConstrainedInteger } -@documentation("Store a blob for a service id output structure") -structure StoreServiceBlobOutput { - -} +@default(15) +@range(min: 10, max: 29) +integer ConstrainedInteger diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/Constraints.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/Constraints.kt index 8544fc1eb1..a7af29426e 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/Constraints.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/Constraints.kt @@ -94,7 +94,7 @@ fun Shape.isDirectlyConstrained(symbolProvider: SymbolProvider): Boolean = when } fun MemberShape.hasConstraintTraitOrTargetHasConstraintTrait(model: Model, symbolProvider: SymbolProvider): Boolean = - this.isDirectlyConstrained(symbolProvider) || (model.expectShape(this.target).isDirectlyConstrained(symbolProvider)) + this.isDirectlyConstrained(symbolProvider) || model.expectShape(this.target).isDirectlyConstrained(symbolProvider) fun Shape.isTransitivelyButNotDirectlyConstrained(model: Model, symbolProvider: SymbolProvider): Boolean = !this.isDirectlyConstrained(symbolProvider) && this.canReachConstrainedShape(model, symbolProvider) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt index 5bfb0f98f2..6b4df3204e 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt @@ -29,6 +29,7 @@ object ServerCargoDependency { val Regex: CargoDependency = CargoDependency("regex", CratesIo("1.5.5")) fun smithyHttpServer(runtimeConfig: RuntimeConfig) = runtimeConfig.smithyRuntimeCrate("smithy-http-server") + fun smithyTypes(runtimeConfig: RuntimeConfig) = runtimeConfig.smithyRuntimeCrate("smithy-types") } /** diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt index f2c572eedc..b416af4db7 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt @@ -62,13 +62,16 @@ class ServerBuilderConstraintViolations( nonExhaustive: Boolean, shouldRenderAsValidationExceptionFieldList: Boolean, ) { + check(all.isNotEmpty()) + Attribute.Derives(setOf(RuntimeType.Debug, RuntimeType.PartialEq)).render(writer) writer.docs("Holds one variant for each of the ways the builder can fail.") if (nonExhaustive) Attribute.NonExhaustive.render(writer) val constraintViolationSymbolName = constraintViolationSymbolProvider.toSymbol(shape).name - writer.rustBlock("pub${ if (visibility == Visibility.PUBCRATE) " (crate) " else "" } enum $constraintViolationSymbolName") { + writer.rustBlock("pub${if (visibility == Visibility.PUBCRATE) " (crate) " else ""} enum $constraintViolationSymbolName") { renderConstraintViolations(writer) } + renderImplDisplayConstraintViolation(writer) writer.rust("impl #T for ConstraintViolation { }", RuntimeType.StdError) @@ -93,7 +96,7 @@ class ServerBuilderConstraintViolations( fun forMember(member: MemberShape): ConstraintViolation? { check(members.contains(member)) // TODO(https://github.com/awslabs/smithy-rs/issues/1302, https://github.com/awslabs/smithy/issues/1179): See above. - return if (symbolProvider.toSymbol(member).isOptional()) { + return if (symbolProvider.toSymbol(member).isOptional() || member.hasNonNullDefault()) { null } else { ConstraintViolation(member, ConstraintViolationKind.MISSING_MEMBER) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index facb2c5b0a..59133e1e39 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -5,16 +5,41 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators +import software.amazon.smithy.codegen.core.CodegenException import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolProvider import software.amazon.smithy.model.Model +import software.amazon.smithy.model.node.ArrayNode +import software.amazon.smithy.model.node.BooleanNode +import software.amazon.smithy.model.node.NullNode +import software.amazon.smithy.model.node.NumberNode +import software.amazon.smithy.model.node.ObjectNode +import software.amazon.smithy.model.node.StringNode +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.DocumentShape +import software.amazon.smithy.model.shapes.DoubleShape +import software.amazon.smithy.model.shapes.EnumShape +import software.amazon.smithy.model.shapes.FloatShape +import software.amazon.smithy.model.shapes.IntEnumShape +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.MemberShape +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 import software.amazon.smithy.model.shapes.UnionShape +import software.amazon.smithy.model.traits.DefaultTrait +import software.amazon.smithy.model.traits.EnumDefinition import software.amazon.smithy.rust.codegen.core.rustlang.Attribute import software.amazon.smithy.rust.codegen.core.rustlang.RustType import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter import software.amazon.smithy.rust.codegen.core.rustlang.Visibility +import software.amazon.smithy.rust.codegen.core.rustlang.Writable import software.amazon.smithy.rust.codegen.core.rustlang.conditionalBlock import software.amazon.smithy.rust.codegen.core.rustlang.deprecatedShape import software.amazon.smithy.rust.codegen.core.rustlang.docs @@ -28,7 +53,9 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate import software.amazon.smithy.rust.codegen.core.rustlang.stripOuter 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.RuntimeConfig import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType +import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata import software.amazon.smithy.rust.codegen.core.smithy.isOptional import software.amazon.smithy.rust.codegen.core.smithy.isRustBoxed @@ -39,11 +66,15 @@ import software.amazon.smithy.rust.codegen.core.smithy.mapRustType import software.amazon.smithy.rust.codegen.core.smithy.module import software.amazon.smithy.rust.codegen.core.smithy.rustType import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait +import software.amazon.smithy.rust.codegen.core.util.UNREACHABLE import software.amazon.smithy.rust.codegen.core.util.dq +import software.amazon.smithy.rust.codegen.core.util.expectTrait import software.amazon.smithy.rust.codegen.core.util.hasTrait +import software.amazon.smithy.rust.codegen.core.util.isStreaming import software.amazon.smithy.rust.codegen.core.util.letIf import software.amazon.smithy.rust.codegen.core.util.redactIfNecessary import software.amazon.smithy.rust.codegen.core.util.toSnakeCase +import software.amazon.smithy.rust.codegen.server.smithy.ServerCargoDependency import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape @@ -99,15 +130,18 @@ class ServerBuilderGenerator( model: Model, symbolProvider: SymbolProvider, takeInUnconstrainedTypes: Boolean, - ): Boolean = - if (takeInUnconstrainedTypes) { - structureShape.canReachConstrainedShape(model, symbolProvider) + ): Boolean { + val members = structureShape.members() + val allOptional = members.all { symbolProvider.toSymbol(it).isOptional() } + val allUnconstrainedDefault = members.all { it.hasNonNullDefault() && !it.canReachConstrainedShape(model, symbolProvider) } + val notFallible = allOptional || allUnconstrainedDefault + + return if (takeInUnconstrainedTypes) { + !notFallible && structureShape.canReachConstrainedShape(model, symbolProvider) } else { - structureShape - .members() - .map { symbolProvider.toSymbol(it) } - .any { !it.isOptional() } + !notFallible } + } } private val takeInUnconstrainedTypes = shape.isReachableFromOperationInput() @@ -142,24 +176,26 @@ class ServerBuilderGenerator( private fun renderBuilder(writer: RustWriter) { if (isBuilderFallible) { - serverBuilderConstraintViolations.render( - writer, - visibility, - nonExhaustive = true, - shouldRenderAsValidationExceptionFieldList = shape.isReachableFromOperationInput(), - ) + if (!members.all { it.hasNonNullDefault() && !it.hasConstraintTraitOrTargetHasConstraintTrait(model, symbolProvider) }) { + serverBuilderConstraintViolations.render( + writer, + visibility, + nonExhaustive = true, + shouldRenderAsValidationExceptionFieldList = shape.isReachableFromOperationInput(), + ) + + // Only generate converter from `ConstraintViolation` into `RequestRejection` if the structure shape is + // an operation input shape. + if (shape.hasTrait()) { + renderImplFromConstraintViolationForRequestRejection(writer) + } - // Only generate converter from `ConstraintViolation` into `RequestRejection` if the structure shape is - // an operation input shape. - if (shape.hasTrait()) { - renderImplFromConstraintViolationForRequestRejection(writer) - } + if (takeInUnconstrainedTypes) { + renderImplFromBuilderForMaybeConstrained(writer) + } - if (takeInUnconstrainedTypes) { - renderImplFromBuilderForMaybeConstrained(writer) + renderTryFromBuilderImpl(writer) } - - renderTryFromBuilderImpl(writer) } else { renderFromBuilderImpl(writer) } @@ -396,10 +432,11 @@ class ServerBuilderGenerator( } private fun renderTryFromBuilderImpl(writer: RustWriter) { + val errorType = if (!isBuilderFallible) "std::convert::Infallible" else "ConstraintViolation" writer.rustTemplate( """ impl #{TryFrom} for #{Structure} { - type Error = ConstraintViolation; + type Error = $errorType; fn try_from(builder: Builder) -> Result { builder.build() @@ -496,6 +533,16 @@ class ServerBuilderGenerator( val memberName = symbolProvider.toMemberName(member) withBlock("$memberName: self.$memberName", ",") { + val wrapDefault: (String) -> String = { + if (member.isStreaming(model)) { + // We set ByteStream to Default::default() until it is easier to use the full namespace for python. + // Use `unwrap_or_default` to make clippy happy. + ".unwrap_or_default()" + } else { + """.unwrap_or_else(|| $it.try_into().expect("This check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues"))""" + } + } + // Write the modifier(s). serverBuilderConstraintViolations.builderConstraintViolationForMember(member)?.also { constraintViolation -> val hasBox = builderMemberSymbol(member) @@ -523,14 +570,21 @@ class ServerBuilderGenerator( #{MaybeConstrained}::Constrained(x) => Ok(x), #{MaybeConstrained}::Unconstrained(x) => x.try_into(), }) - .map(|res| - res${if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())"} - .map_err(ConstraintViolation::${constraintViolation.name()}) - ) - .transpose()? """, *codegenScope, ) + if (isBuilderFallible) { + rustTemplate( + """ + .map(|res| + res${if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())"} + .map_err(ConstraintViolation::${constraintViolation.name()}) + ) + .transpose()? + """, + *codegenScope, + ) + } // Constrained types are not public and this is a member shape that would have generated a // public constrained type, were the setting to be enabled. @@ -543,8 +597,32 @@ class ServerBuilderGenerator( constrainedShapeSymbolProvider.toSymbol(model.expectShape(member.target)), ) } + if (member.hasNonNullDefault()) { + rustTemplate( + """#{Default:W}""", + "Default" to renderDefaultBuilder( + model, + runtimeConfig, + symbolProvider, + member, + wrapDefault, + ), + ) + if (!isBuilderFallible) { + // unwrap the Option + rust(".unwrap()") + } + } + } + } ?: run { + if (member.hasNonNullDefault()) { + rustTemplate( + "#{Default:W}", + "Default" to renderDefaultBuilder(model, runtimeConfig, symbolProvider, member, wrapDefault), + ) } } + // This won't run if there is a default value serverBuilderConstraintViolations.forMember(member)?.also { rust(".ok_or(ConstraintViolation::${it.name()})?") } @@ -561,3 +639,121 @@ fun buildFnReturnType(isBuilderFallible: Boolean, structureSymbol: Symbol) = wri rust("#T", structureSymbol) } } + +fun renderDefaultBuilder(model: Model, runtimeConfig: RuntimeConfig, symbolProvider: RustSymbolProvider, member: MemberShape, wrap: (s: String) -> String = { it }): Writable { + return writable { + val node = member.expectTrait().toNode()!! + val name = member.memberName + val types = ServerCargoDependency.smithyTypes(runtimeConfig).toType() + when (val target = model.expectShape(member.target)) { + is EnumShape, is IntEnumShape -> { + val value = when (target) { + is IntEnumShape -> node.expectNumberNode().value + is EnumShape -> node.expectStringNode().value + else -> throw CodegenException("Default value for $name must be of EnumShape or IntEnumShape") + } + val enumValues = when (target) { + is IntEnumShape -> target.enumValues + is EnumShape -> target.enumValues + else -> UNREACHABLE("It must be an [Int]EnumShape, otherwise it'd have failed above") + } + val variant = enumValues + .entries + .filter { entry -> entry.value == value } + .map { entry -> + symbolProvider.toEnumVariantName( + EnumDefinition.builder().name(entry.key).value(entry.value.toString()).build(), + )!! + } + .first() + val symbol = symbolProvider.toSymbol(target) + val result = "$symbol::${variant.name}" + rust(wrap(result)) + } + + is ByteShape -> rust(wrap(node.expectNumberNode().value.toString() + "i8")) + is ShortShape -> rust(wrap(node.expectNumberNode().value.toString() + "i16")) + is IntegerShape -> rust(wrap(node.expectNumberNode().value.toString() + "i32")) + is LongShape -> rust(wrap(node.expectNumberNode().value.toString() + "i64")) + is FloatShape -> rust(wrap(node.expectNumberNode().value.toFloat().toString() + "f32")) + is DoubleShape -> rust(wrap(node.expectNumberNode().value.toDouble().toString() + "f64")) + is BooleanShape -> rust(wrap(node.expectBooleanNode().value.toString())) + is StringShape -> rust(wrap("String::from(${node.expectStringNode().value.dq()})")) + is TimestampShape -> when (node) { + is NumberNode -> rust(wrap(node.expectNumberNode().value.toString())) + is StringNode -> { + val value = node.expectStringNode().value + rustTemplate( + wrap( + """ + #{SmithyTypes}::DateTime::from_str("$value", #{SmithyTypes}::date_time::Format::DateTime) + .expect("default value `$value` cannot be parsed into a valid date time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""", + ), + "SmithyTypes" to types, + ) + } + + else -> throw CodegenException("Default value for $name is unsupported") + } + + is ListShape -> { + check(node is ArrayNode && node.isEmpty) + rust(wrap("Vec::new()")) + } + + is MapShape -> { + check(node is ObjectNode && node.isEmpty) + rust(wrap("std::collections::HashMap::new()")) + } + + is DocumentShape -> { + when (node) { + is NullNode -> rustTemplate( + "#{SmithyTypes}::Document::Null", + "SmithyTypes" to types, + ) + + is BooleanNode -> rust(wrap(node.value.toString())) + is StringNode -> rust(wrap("String::from(${node.value.dq()})")) + is NumberNode -> { + val value = node.value.toString() + val variant = when (node.value) { + is Float, is Double -> "Float" + else -> if (node.value.toLong() >= 0) "PosInt" else "NegInt" + } + rustTemplate( + wrap( + "#{SmithyTypes}::Document::Number(#{SmithyTypes}::Number::$variant($value))", + ), + "SmithyTypes" to types, + ) + } + + is ArrayNode -> { + check(node.isEmpty) + rust(wrap("Vec::new()")) + } + + is ObjectNode -> { + check(node.isEmpty) + rust(wrap("std::collections::HashMap::new()")) + } + + else -> throw CodegenException("Default value $node for member shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") + } + } + + is BlobShape -> { + val value = if (member.isStreaming(model)) { + /* ByteStream to work in Python and Rust without explicit dependency */ + "Default::default()" + } else { + "Vec::new()" + } + rust(wrap(value)) + } + + else -> throw CodegenException("Default value for $name is unsupported or cannot exist") + } + } +} diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt index d83446dc83..58e83735ab 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt @@ -7,6 +7,7 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolProvider +import software.amazon.smithy.model.Model import software.amazon.smithy.model.shapes.MemberShape import software.amazon.smithy.model.shapes.StructureShape import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter @@ -25,8 +26,11 @@ import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata import software.amazon.smithy.rust.codegen.core.smithy.isOptional import software.amazon.smithy.rust.codegen.core.smithy.makeOptional import software.amazon.smithy.rust.codegen.core.smithy.module +import software.amazon.smithy.rust.codegen.core.util.isStreaming import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType +import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape +import software.amazon.smithy.rust.codegen.server.smithy.hasConstraintTraitOrTargetHasConstraintTrait /** * Generates a builder for the Rust type associated with the [StructureShape]. @@ -53,23 +57,28 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( * This builder only enforces the `required` trait. */ fun hasFallibleBuilder( + model: Model, structureShape: StructureShape, symbolProvider: SymbolProvider, - ): Boolean = - structureShape - .members() - .map { symbolProvider.toSymbol(it) } - .any { !it.isOptional() } + ): Boolean { + val members = structureShape.members() + val allOptional = members.all { symbolProvider.toSymbol(it).isOptional() } + val allUnconstrainedDefault = members.all { it.hasNonNullDefault() && !it.canReachConstrainedShape(model, symbolProvider) } + val notFallible = allOptional || allUnconstrainedDefault + + return !notFallible + } } private val model = codegenContext.model private val symbolProvider = codegenContext.symbolProvider private val members: List = shape.allMembers.values.toList() + private val runtimeConfig = codegenContext.runtimeConfig private val structureSymbol = symbolProvider.toSymbol(shape) private val builderSymbol = shape.serverBuilderSymbol(symbolProvider, false) private val moduleName = builderSymbol.namespace.split("::").last() - private val isBuilderFallible = hasFallibleBuilder(shape, symbolProvider) + private val isBuilderFallible = hasFallibleBuilder(model, shape, symbolProvider) private val serverBuilderConstraintViolations = ServerBuilderConstraintViolations(codegenContext, shape, builderTakesInUnconstrainedTypes = false) @@ -90,14 +99,16 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( private fun renderBuilder(writer: RustWriter) { if (isBuilderFallible) { - serverBuilderConstraintViolations.render( - writer, - Visibility.PUBLIC, - nonExhaustive = false, - shouldRenderAsValidationExceptionFieldList = false, - ) + if (!members.all { it.hasNonNullDefault() && !it.hasConstraintTraitOrTargetHasConstraintTrait(model, symbolProvider) }) { + serverBuilderConstraintViolations.render( + writer, + Visibility.PUBLIC, + nonExhaustive = false, + shouldRenderAsValidationExceptionFieldList = false, + ) - renderTryFromBuilderImpl(writer) + renderTryFromBuilderImpl(writer) + } } else { renderFromBuilderImpl(writer) } @@ -158,6 +169,12 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( val memberName = symbolProvider.toMemberName(member) withBlock("$memberName: self.$memberName", ",") { + if (member.hasNonNullDefault()) { + val into = if (member.isStreaming(model)) { + "" + } else { ".into()" } + rustTemplate("""#{default:W}""", "default" to renderDefaultBuilder(model, runtimeConfig, symbolProvider, member) { ".unwrap_or_else(|| $it$into)" }) + } serverBuilderConstraintViolations.forMember(member)?.also { rust(".ok_or(ConstraintViolation::${it.name()})?") } @@ -204,10 +221,11 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( } private fun renderTryFromBuilderImpl(writer: RustWriter) { + val errorType = if (!isBuilderFallible) "std::convert::Infallible" else "ConstraintViolation" writer.rustTemplate( """ impl #{TryFrom} for #{Structure} { - type Error = ConstraintViolation; + type Error = $errorType; fn try_from(builder: Builder) -> Result { builder.build() diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt new file mode 100644 index 0000000000..f4df8daf8e --- /dev/null +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt @@ -0,0 +1,326 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.rust.codegen.server.smithy.generators + +import org.junit.jupiter.api.TestInstance +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.Arguments +import org.junit.jupiter.params.provider.MethodSource +import software.amazon.smithy.model.Model +import software.amazon.smithy.model.shapes.EnumShape +import software.amazon.smithy.model.shapes.StructureShape +import software.amazon.smithy.rust.codegen.core.rustlang.RustModule +import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter +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.rustTemplate +import software.amazon.smithy.rust.codegen.core.rustlang.writable +import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider +import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureGenerator +import software.amazon.smithy.rust.codegen.core.smithy.generators.implBlock +import software.amazon.smithy.rust.codegen.core.testutil.TestWorkspace +import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel +import software.amazon.smithy.rust.codegen.core.testutil.compileAndTest +import software.amazon.smithy.rust.codegen.core.testutil.unitTest +import software.amazon.smithy.rust.codegen.core.util.dq +import software.amazon.smithy.rust.codegen.core.util.lookup +import software.amazon.smithy.rust.codegen.core.util.toPascalCase +import software.amazon.smithy.rust.codegen.core.util.toSnakeCase +import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestCodegenContext +import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestSymbolProvider +import java.util.stream.Stream + +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +class ServerBuilderDefaultValuesTest { + // When defaults are used, the model will be generated with these default values + private val defaultValues = mapOf( + "Boolean" to "true", + "String" to "foo".dq(), + "Byte" to "5", + "Short" to "55", + "Integer" to "555", + "Long" to "5555", + "Float" to "0.5", + "Double" to "0.55", + "Timestamp" to "1985-04-12T23:20:50.52Z".dq(), + // "BigInteger" to "55555", "BigDecimal" to "0.555", // TODO(https://github.com/awslabs/smithy-rs/issues/312) + "StringList" to "[]", + "IntegerMap" to "{}", + "Language" to "en".dq(), + "DocumentBoolean" to "true", + "DocumentString" to "foo".dq(), + "DocumentNumberPosInt" to "100", + "DocumentNumberNegInt" to "-100", + "DocumentNumberFloat" to "0.1", + "DocumentList" to "[]", + "DocumentMap" to "{}", + ) + + // When the test applies values to validate we honor custom values, use these values + private val customValues = + mapOf( + "Boolean" to "false", + "String" to "bar".dq(), + "Byte" to "6", + "Short" to "66", + "Integer" to "666", + "Long" to "6666", + "Float" to "0.6", + "Double" to "0.66", + "Timestamp" to "2022-11-25T17:30:50.00Z".dq(), + // "BigInteger" to "55555", "BigDecimal" to "0.555", // TODO(https://github.com/awslabs/smithy-rs/issues/312) + "StringList" to "[]", + "IntegerMap" to "{}", + "Language" to "fr".dq(), + "DocumentBoolean" to "false", + "DocumentString" to "bar".dq(), + "DocumentNumberPosInt" to "1000", + "DocumentNumberNegInt" to "-1000", + "DocumentNumberFloat" to "0.01", + "DocumentList" to "[]", + "DocumentMap" to "{}", + ) + + @ParameterizedTest(name = "(#{index}) Generate default value. Required Trait: {1}, nulls: {2}, optionals: {3}") + @MethodSource("localParameters") + fun `default values are generated and builders respect default and overrides`(testConfig: TestConfig, setupFiles: (w: RustWriter, m: Model, s: RustSymbolProvider) -> Unit) { + val initialSetValues = defaultValues.mapValues { if (testConfig.nullDefault) null else it.value } + val model = generateModel(testConfig, initialSetValues) + val symbolProvider = serverTestSymbolProvider(model) + val project = TestWorkspace.testProject(symbolProvider) + project.withModule(RustModule.public("model")) { + setupFiles(this, model, symbolProvider) + + val rustValues = setupRustValuesForTest(testConfig.assertValues) + val applySetters = testConfig.applyDefaultValues + val setters = if (applySetters) structSetters(rustValues, testConfig.nullDefault && !testConfig.requiredTrait) else writable { } + // unwrap when the builder is fallible + // enums are constrained + val unwrapBuilder = if ((testConfig.nullDefault && testConfig.requiredTrait) || (testConfig.applyDefaultValues && !testConfig.nullDefault)) ".unwrap()" else "" + unitTest( + name = "generates_default_required_values", + block = writable { + rustTemplate( + """ + let my_struct = MyStruct::builder() + #{Setters:W} + .build()$unwrapBuilder; + + #{Assertions:W} + """, + "Assertions" to assertions(rustValues, applySetters, testConfig.nullDefault, testConfig.requiredTrait, testConfig.applyDefaultValues), + "Setters" to setters, + ) + }, + ) + } + project.compileAndTest() + } + + private fun setupRustValuesForTest(valuesMap: Map): Map { + return valuesMap + mapOf( + "Byte" to "${valuesMap["Byte"]}i8", + "Short" to "${valuesMap["Short"]}i16", + "Integer" to "${valuesMap["Integer"]}i32", + "Long" to "${valuesMap["Long"]}i64", + "Float" to "${valuesMap["Float"]}f32", + "Double" to "${valuesMap["Double"]}f64", + "Language" to "crate::model::Language::${valuesMap["Language"]!!.replace(""""""", "").toPascalCase()}", + "Timestamp" to """aws_smithy_types::DateTime::from_str(${valuesMap["Timestamp"]}, aws_smithy_types::date_time::Format::DateTime).unwrap()""", + // These must be empty + "StringList" to "Vec::::new()", + "IntegerMap" to "std::collections::HashMap::::new()", + "DocumentList" to "Vec::::new()", + "DocumentMap" to "std::collections::HashMap::::new()", + ) + } + + private fun writeServerBuilderGeneratorWithoutPublicConstrainedTypes(writer: RustWriter, model: Model, symbolProvider: RustSymbolProvider) { + writer.rust("##![allow(deprecated)]") + val struct = model.lookup("com.test#MyStruct") + val codegenContext = serverTestCodegenContext(model) + val builderGenerator = ServerBuilderGeneratorWithoutPublicConstrainedTypes(codegenContext, struct) + + writer.implBlock(struct, symbolProvider) { + builderGenerator.renderConvenienceMethod(writer) + } + builderGenerator.render(writer) + + ServerEnumGenerator(codegenContext, writer, model.lookup("com.test#Language")).render() + StructureGenerator(model, symbolProvider, writer, struct).render() + } + + private fun writeServerBuilderGenerator(writer: RustWriter, model: Model, symbolProvider: RustSymbolProvider) { + writer.rust("##![allow(deprecated)]") + val struct = model.lookup("com.test#MyStruct") + val codegenContext = serverTestCodegenContext(model) + val builderGenerator = ServerBuilderGenerator(codegenContext, struct) + + writer.implBlock(struct, symbolProvider) { + builderGenerator.renderConvenienceMethod(writer) + } + builderGenerator.render(writer) + + ServerEnumGenerator(codegenContext, writer, model.lookup("com.test#Language")).render() + StructureGenerator(model, symbolProvider, writer, struct).render() + } + + private fun structSetters(values: Map, optional: Boolean): Writable { + return writable { + values.entries.forEach { + rust(".${it.key.toSnakeCase()}(") + if (optional) { + rust("Some(") + } + when (it.key) { + "String" -> { + rust("${it.value}.into()") + } + + "DocumentNull" -> + rust("aws_smithy_types::Document::Null") + + "DocumentString" -> { + rust("aws_smithy_types::Document::String(String::from(${it.value}))") + } + + else -> { + if (it.key.startsWith("DocumentNumber")) { + val type = it.key.replace("DocumentNumber", "") + rust("aws_smithy_types::Document::Number(aws_smithy_types::Number::$type(${it.value}))") + } else { + rust("${it.value}.into()") + } + } + } + if (optional) { + rust(")") + } + rust(")") + } + } + } + + private fun assertions(values: Map, hasSetValues: Boolean, hasNullValues: Boolean, requiredTrait: Boolean, hasDefaults: Boolean): Writable { + return writable { + for (it in values.entries) { + rust("assert_eq!(my_struct.${it.key.toSnakeCase()} ") + if (!hasSetValues) { + rust(".is_none(), true);") + continue + } + + val expected = if (it.key == "DocumentNull") { + "aws_smithy_types::Document::Null" + } else if (it.key == "DocumentString") { + "String::from(${it.value}).into()" + } else if (it.key.startsWith("DocumentNumber")) { + val type = it.key.replace("DocumentNumber", "") + "aws_smithy_types::Document::Number(aws_smithy_types::Number::$type(${it.value}))" + } else if (it.key.startsWith("Document")) { + "${it.value}.into()" + } else { + "${it.value}" + } + + if (!requiredTrait && !(hasDefaults && !hasNullValues)) { + rust(".unwrap()") + } + + rust(", $expected);") + } + } + } + + private fun generateModel(testConfig: TestConfig, values: Map): Model { + val requiredTrait = if (testConfig.requiredTrait) "@required" else "" + + val members = values.entries.joinToString(", ") { + val value = if (testConfig.applyDefaultValues) { + "= ${it.value}" + } else if (testConfig.nullDefault) { + "= null" + } else { "" } + """ + $requiredTrait + ${it.key.toPascalCase()}: ${it.key} $value + """ + } + val model = + """ + namespace com.test + use smithy.framework#ValidationException + + structure MyStruct { + $members + } + + enum Language { + EN = "en", + FR = "fr", + } + + list StringList { + member: String + } + + map IntegerMap { + key: String + value: Integer + } + + document DocumentNull + document DocumentBoolean + document DocumentString + document DocumentDecimal + document DocumentNumberNegInt + document DocumentNumberPosInt + document DocumentNumberFloat + document DocumentList + document DocumentMap + """ + return model.asSmithyModel(smithyVersion = "2") + } + + private fun localParameters(): Stream { + val builderWriters = listOf( + ::writeServerBuilderGenerator, + ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes, + ) + return Stream.of( + TestConfig(defaultValues, requiredTrait = false, nullDefault = true, applyDefaultValues = true), + TestConfig(defaultValues, requiredTrait = false, nullDefault = true, applyDefaultValues = false), + + TestConfig(customValues, requiredTrait = false, nullDefault = true, applyDefaultValues = true), + TestConfig(customValues, requiredTrait = false, nullDefault = true, applyDefaultValues = false), + + TestConfig(defaultValues, requiredTrait = true, nullDefault = true, applyDefaultValues = true), + TestConfig(customValues, requiredTrait = true, nullDefault = true, applyDefaultValues = true), + + TestConfig(defaultValues, requiredTrait = false, nullDefault = false, applyDefaultValues = true), + TestConfig(defaultValues, requiredTrait = false, nullDefault = false, applyDefaultValues = false), + + TestConfig(customValues, requiredTrait = false, nullDefault = false, applyDefaultValues = true), + TestConfig(customValues, requiredTrait = false, nullDefault = false, applyDefaultValues = false), + + TestConfig(defaultValues, requiredTrait = true, nullDefault = false, applyDefaultValues = true), + + TestConfig(customValues, requiredTrait = true, nullDefault = false, applyDefaultValues = true), + + ).flatMap { builderWriters.stream().map { builderWriter -> Arguments.of(it, builderWriter) } } + } + + data class TestConfig( + // The values in the setters and assert!() calls + val assertValues: Map, + // Whether to apply @required to all members + val requiredTrait: Boolean, + // Whether to set all members to `null` and force them to be optional + val nullDefault: Boolean, + // Whether to set `assertValues` in the builder + val applyDefaultValues: Boolean, + ) +} diff --git a/rust-runtime/aws-smithy-http-server-python/src/types.rs b/rust-runtime/aws-smithy-http-server-python/src/types.rs index db3aa183da..1ce7779482 100644 --- a/rust-runtime/aws-smithy-http-server-python/src/types.rs +++ b/rust-runtime/aws-smithy-http-server-python/src/types.rs @@ -322,6 +322,12 @@ impl ByteStream { } } +impl Default for ByteStream { + fn default() -> Self { + Self::new(aws_smithy_http::body::SdkBody::from("")) + } +} + /// ByteStream Abstractions. #[pymethods] impl ByteStream { From 6c6e863ff65ced48e78685bd085722f287d20001 Mon Sep 17 00:00:00 2001 From: 82marbag <69267416+82marbag@users.noreply.github.com> Date: Tue, 20 Dec 2022 17:48:58 +0100 Subject: [PATCH 02/11] Revert .smithy changes Signed-off-by: Daniele Ahmed --- .../common-test-models/constraints.smithy | 2 +- codegen-core/common-test-models/simple.smithy | 126 ++++++++++++++++-- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/codegen-core/common-test-models/constraints.smithy b/codegen-core/common-test-models/constraints.smithy index e1f8215877..138c9e632f 100644 --- a/codegen-core/common-test-models/constraints.smithy +++ b/codegen-core/common-test-models/constraints.smithy @@ -651,7 +651,7 @@ string LengthPatternString @length(min: 1, max: 69) string MediaTypeLengthString -@range(min: -7, max: 69) +@range(min: -0, max: 69) integer RangeInteger @range(min: -10) diff --git a/codegen-core/common-test-models/simple.smithy b/codegen-core/common-test-models/simple.smithy index c21aec67b7..43c4bc6aca 100644 --- a/codegen-core/common-test-models/simple.smithy +++ b/codegen-core/common-test-models/simple.smithy @@ -1,4 +1,4 @@ -$version: "2.0" +$version: "1.0" namespace com.amazonaws.simple @@ -12,23 +12,125 @@ use smithy.framework#ValidationException @documentation("A simple service example, with a Service resource that can be registered and a readonly healthcheck") service SimpleService { version: "2022-01-01", + resources: [ + Service, + ], operations: [ - HealthCheck, + Healthcheck, + StoreServiceBlob, ], } -@http(uri: "/", method: "POST") -operation HealthCheck { - input: Input, +@documentation("Id of the service that will be registered") +string ServiceId + +@documentation("Name of the service that will be registered") +string ServiceName + +@error("client") +@documentation( + """ + Returned when a new resource cannot be created because one already exists. + """ +) +structure ResourceAlreadyExists { + @required + message: String +} + +@documentation("A resource that can register services") +resource Service { + identifiers: { id: ServiceId }, + put: RegisterService, +} + +@idempotent +@http(method: "PUT", uri: "/service/{id}") +@documentation("Service register operation") +@httpRequestTests([ + { + id: "RegisterServiceRequestTest", + protocol: "aws.protocols#restJson1", + uri: "/service/1", + headers: { + "Content-Type": "application/json", + }, + params: { id: "1", name: "TestService" }, + body: "{\"name\":\"TestService\"}", + method: "PUT", + } +]) +@httpResponseTests([ + { + id: "RegisterServiceResponseTest", + protocol: "aws.protocols#restJson1", + params: { id: "1", name: "TestService" }, + body: "{\"id\":\"1\",\"name\":\"TestService\"}", + code: 200, + headers: { + "Content-Length": "31" + } + } +]) +operation RegisterService { + input: RegisterServiceInputRequest, + output: RegisterServiceOutputResponse, + errors: [ResourceAlreadyExists, ValidationException] +} + +@documentation("Service register input structure") +structure RegisterServiceInputRequest { + @required + @httpLabel + id: ServiceId, + name: ServiceName, +} + +@documentation("Service register output structure") +structure RegisterServiceOutputResponse { + @required + id: ServiceId, + name: ServiceName, +} + +@readonly +@http(uri: "/healthcheck", method: "GET") +@documentation("Read-only healthcheck operation") +operation Healthcheck { + input: HealthcheckInputRequest, + output: HealthcheckOutputResponse +} + +@documentation("Service healthcheck output structure") +structure HealthcheckInputRequest { + +} + +@documentation("Service healthcheck input structure") +structure HealthcheckOutputResponse { + +} + +@readonly +@http(method: "POST", uri: "/service/{id}/blob") +@documentation("Stores a blob for a service id") +operation StoreServiceBlob { + input: StoreServiceBlobInput, + output: StoreServiceBlobOutput, errors: [ValidationException] } -@input -structure Input { - @default(15) - int:ConstrainedInteger +@documentation("Store a blob for a service id input structure") +structure StoreServiceBlobInput { + @required + @httpLabel + id: ServiceId, + @required + @httpPayload + content: Blob, } -@default(15) -@range(min: 10, max: 29) -integer ConstrainedInteger +@documentation("Store a blob for a service id output structure") +structure StoreServiceBlobOutput { + +} From 31ed6c96e120c85a79f60dffc44bdd3dda5d9eae Mon Sep 17 00:00:00 2001 From: 82marbag <69267416+82marbag@users.noreply.github.com> Date: Wed, 21 Dec 2022 12:16:17 +0100 Subject: [PATCH 03/11] Fix fallible condition Signed-off-by: Daniele Ahmed Co-authored-by: david-perez --- .../ServerBuilderConstraintViolations.kt | 2 +- .../generators/ServerBuilderGenerator.kt | 126 ++++++++++++------ ...rGeneratorWithoutPublicConstrainedTypes.kt | 51 ++++--- .../ServerBuilderDefaultValuesTest.kt | 6 +- 4 files changed, 122 insertions(+), 63 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt index b416af4db7..774e78a7b4 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt @@ -62,7 +62,7 @@ class ServerBuilderConstraintViolations( nonExhaustive: Boolean, shouldRenderAsValidationExceptionFieldList: Boolean, ) { - check(all.isNotEmpty()) + check(all.isNotEmpty()) { shape } Attribute.Derives(setOf(RuntimeType.Debug, RuntimeType.PartialEq)).render(writer) writer.docs("Holds one variant for each of the ways the builder can fail.") diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index 59133e1e39..0d7c4dbb41 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -132,9 +132,24 @@ class ServerBuilderGenerator( takeInUnconstrainedTypes: Boolean, ): Boolean { val members = structureShape.members() - val allOptional = members.all { symbolProvider.toSymbol(it).isOptional() } - val allUnconstrainedDefault = members.all { it.hasNonNullDefault() && !it.canReachConstrainedShape(model, symbolProvider) } - val notFallible = allOptional || allUnconstrainedDefault + fun isOptional(member: MemberShape) = symbolProvider.toSymbol(member).isOptional() + fun hasDefault(member: MemberShape) = member.hasNonNullDefault() + fun isNotConstrained(member: MemberShape) = !member.canReachConstrainedShape(model, symbolProvider) + + val notFallible = members.all { + if (structureShape.isReachableFromOperationInput()) { + // When deserializing an input structure, constraints might not be satisfied. + // For this member not to be fallible, it must not be constrained (constraints in input must always be checked) + // and either optional (no need to set this; not required) + // or has a default value (some value will always be present) + isNotConstrained(it) && (isOptional(it) || hasDefault(it)) + } else { + // This structure will be constructed manually. + // Constraints will have to be dealt with + // before members are set in the builder + isOptional(it) || hasDefault(it) + } + } return if (takeInUnconstrainedTypes) { !notFallible && structureShape.canReachConstrainedShape(model, symbolProvider) @@ -176,26 +191,24 @@ class ServerBuilderGenerator( private fun renderBuilder(writer: RustWriter) { if (isBuilderFallible) { - if (!members.all { it.hasNonNullDefault() && !it.hasConstraintTraitOrTargetHasConstraintTrait(model, symbolProvider) }) { - serverBuilderConstraintViolations.render( - writer, - visibility, - nonExhaustive = true, - shouldRenderAsValidationExceptionFieldList = shape.isReachableFromOperationInput(), - ) - - // Only generate converter from `ConstraintViolation` into `RequestRejection` if the structure shape is - // an operation input shape. - if (shape.hasTrait()) { - renderImplFromConstraintViolationForRequestRejection(writer) - } + serverBuilderConstraintViolations.render( + writer, + visibility, + nonExhaustive = true, + shouldRenderAsValidationExceptionFieldList = shape.isReachableFromOperationInput(), + ) - if (takeInUnconstrainedTypes) { - renderImplFromBuilderForMaybeConstrained(writer) - } + // Only generate converter from `ConstraintViolation` into `RequestRejection` if the structure shape is + // an operation input shape. + if (shape.hasTrait()) { + renderImplFromConstraintViolationForRequestRejection(writer) + } - renderTryFromBuilderImpl(writer) + if (takeInUnconstrainedTypes) { + renderImplFromBuilderForMaybeConstrained(writer) } + + renderTryFromBuilderImpl(writer) } else { renderFromBuilderImpl(writer) } @@ -539,7 +552,12 @@ class ServerBuilderGenerator( // Use `unwrap_or_default` to make clippy happy. ".unwrap_or_default()" } else { - """.unwrap_or_else(|| $it.try_into().expect("This check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues"))""" + val into = if (!member.canReachConstrainedShape(model, symbolProvider)) { + "" + } else { + """.try_into().expect("This check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""" + } + """.unwrap_or_else(|| $it$into)""" } } @@ -596,21 +614,49 @@ class ServerBuilderGenerator( ".map(|v: #T| v.into())", constrainedShapeSymbolProvider.toSymbol(model.expectShape(member.target)), ) - } - if (member.hasNonNullDefault()) { - rustTemplate( - """#{Default:W}""", - "Default" to renderDefaultBuilder( - model, - runtimeConfig, - symbolProvider, - member, - wrapDefault, - ), - ) - if (!isBuilderFallible) { - // unwrap the Option - rust(".unwrap()") + // This is to avoid a `allow(clippy::useless_conversion)` on `try_into()` + // Removing this `if` and leaving the `else if` below a plain `if` will make no difference + // to the compilation, but to clippy. + if (member.hasNonNullDefault()) { + rustTemplate( + """#{Default:W}""", + "Default" to renderDefaultBuilder( + model, + runtimeConfig, + symbolProvider, + member, + ) { + if (member.isStreaming(model)) { + // We set ByteStream to Default::default() until it is easier to use the full namespace for python. + // Use `unwrap_or_default` to make clippy happy. + ".unwrap_or_default()" + } else { + // The conversion is done above + """.unwrap_or($it)""" + } + }, + ) + if (!isBuilderFallible) { + // Unwrap the Option + rust(".unwrap()") + } + } + } else { + if (member.hasNonNullDefault()) { + rustTemplate( + """#{Default:W}""", + "Default" to renderDefaultBuilder( + model, + runtimeConfig, + symbolProvider, + member, + wrapDefault, + ), + ) + if (!isBuilderFallible) { + // unwrap the Option + rust(".unwrap()") + } } } } @@ -713,8 +759,8 @@ fun renderDefaultBuilder(model: Model, runtimeConfig: RuntimeConfig, symbolProvi "SmithyTypes" to types, ) - is BooleanNode -> rust(wrap(node.value.toString())) - is StringNode -> rust(wrap("String::from(${node.value.dq()})")) + is BooleanNode -> rustTemplate(wrap("""#{SmithyTypes}::Document::Bool(${node.value})"""), "SmithyTypes" to types) + is StringNode -> rustTemplate(wrap("#{SmithyTypes}::Document::String(String::from(${node.value.dq()}))"), "SmithyTypes" to types) is NumberNode -> { val value = node.value.toString() val variant = when (node.value) { @@ -731,12 +777,12 @@ fun renderDefaultBuilder(model: Model, runtimeConfig: RuntimeConfig, symbolProvi is ArrayNode -> { check(node.isEmpty) - rust(wrap("Vec::new()")) + rustTemplate(wrap("""#{SmithyTypes}::Document::Array(Vec::new())"""), "SmithyTypes" to types) } is ObjectNode -> { check(node.isEmpty) - rust(wrap("std::collections::HashMap::new()")) + rustTemplate(wrap("#{SmithyTypes}::Document::Object(std::collections::HashMap::new())"), "SmithyTypes" to types) } else -> throw CodegenException("Default value $node for member shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt index 58e83735ab..2084c301f3 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt @@ -8,7 +8,10 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolProvider import software.amazon.smithy.model.Model +import software.amazon.smithy.model.shapes.BooleanShape +import software.amazon.smithy.model.shapes.EnumShape import software.amazon.smithy.model.shapes.MemberShape +import software.amazon.smithy.model.shapes.NumberShape import software.amazon.smithy.model.shapes.StructureShape import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter import software.amazon.smithy.rust.codegen.core.rustlang.Visibility @@ -29,8 +32,6 @@ import software.amazon.smithy.rust.codegen.core.smithy.module import software.amazon.smithy.rust.codegen.core.util.isStreaming import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType -import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape -import software.amazon.smithy.rust.codegen.server.smithy.hasConstraintTraitOrTargetHasConstraintTrait /** * Generates a builder for the Rust type associated with the [StructureShape]. @@ -62,9 +63,12 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( symbolProvider: SymbolProvider, ): Boolean { val members = structureShape.members() - val allOptional = members.all { symbolProvider.toSymbol(it).isOptional() } - val allUnconstrainedDefault = members.all { it.hasNonNullDefault() && !it.canReachConstrainedShape(model, symbolProvider) } - val notFallible = allOptional || allUnconstrainedDefault + fun isOptional(member: MemberShape) = symbolProvider.toSymbol(member).isOptional() + fun hasDefault(member: MemberShape) = member.hasNonNullDefault() + + val notFallible = members.all { + isOptional(it) || hasDefault(it) + } return !notFallible } @@ -99,16 +103,14 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( private fun renderBuilder(writer: RustWriter) { if (isBuilderFallible) { - if (!members.all { it.hasNonNullDefault() && !it.hasConstraintTraitOrTargetHasConstraintTrait(model, symbolProvider) }) { - serverBuilderConstraintViolations.render( - writer, - Visibility.PUBLIC, - nonExhaustive = false, - shouldRenderAsValidationExceptionFieldList = false, - ) + serverBuilderConstraintViolations.render( + writer, + Visibility.PUBLIC, + nonExhaustive = false, + shouldRenderAsValidationExceptionFieldList = false, + ) - renderTryFromBuilderImpl(writer) - } + renderTryFromBuilderImpl(writer) } else { renderFromBuilderImpl(writer) } @@ -170,10 +172,23 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( withBlock("$memberName: self.$memberName", ",") { if (member.hasNonNullDefault()) { - val into = if (member.isStreaming(model)) { - "" - } else { ".into()" } - rustTemplate("""#{default:W}""", "default" to renderDefaultBuilder(model, runtimeConfig, symbolProvider, member) { ".unwrap_or_else(|| $it$into)" }) + if (member.isStreaming(model)) { + rustTemplate(".unwrap_or_default()") + } else { + val unwrapOr = when (model.expectShape(member.target)) { + is NumberShape, is EnumShape, is BooleanShape -> ".unwrap_or(" + else -> ".unwrap_or_else(||" + } + rustTemplate( + """#{default:W}""", + "default" to renderDefaultBuilder( + model, + runtimeConfig, + symbolProvider, + member, + ) { "$unwrapOr $it)" }, + ) + } } serverBuilderConstraintViolations.forMember(member)?.also { rust(".ok_or(ConstraintViolation::${it.name()})?") diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt index f4df8daf8e..4fffc33766 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt @@ -97,9 +97,7 @@ class ServerBuilderDefaultValuesTest { val rustValues = setupRustValuesForTest(testConfig.assertValues) val applySetters = testConfig.applyDefaultValues val setters = if (applySetters) structSetters(rustValues, testConfig.nullDefault && !testConfig.requiredTrait) else writable { } - // unwrap when the builder is fallible - // enums are constrained - val unwrapBuilder = if ((testConfig.nullDefault && testConfig.requiredTrait) || (testConfig.applyDefaultValues && !testConfig.nullDefault)) ".unwrap()" else "" + val unwrapBuilder = if (testConfig.nullDefault && testConfig.requiredTrait && testConfig.applyDefaultValues) ".unwrap()" else "" unitTest( name = "generates_default_required_values", block = writable { @@ -135,7 +133,7 @@ class ServerBuilderDefaultValuesTest { "IntegerMap" to "std::collections::HashMap::::new()", "DocumentList" to "Vec::::new()", "DocumentMap" to "std::collections::HashMap::::new()", - ) + ) + valuesMap.filter { it.value?.startsWith("Document") == true }.map { it.key to "${it.value}.into()" } } private fun writeServerBuilderGeneratorWithoutPublicConstrainedTypes(writer: RustWriter, model: Model, symbolProvider: RustSymbolProvider) { From 1c4492c7a4d5006c8a1d50ba790f2e8907ee7410 Mon Sep 17 00:00:00 2001 From: 82marbag <69267416+82marbag@users.noreply.github.com> Date: Wed, 21 Dec 2022 15:50:39 +0100 Subject: [PATCH 04/11] Fix clippy Signed-off-by: Daniele Ahmed --- .../smithy/generators/ServerBuilderGenerator.kt | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index 0d7c4dbb41..c5e7c0b81c 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -28,6 +28,7 @@ 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.MemberShape +import software.amazon.smithy.model.shapes.NumberShape import software.amazon.smithy.model.shapes.ShortShape import software.amazon.smithy.model.shapes.StringShape import software.amazon.smithy.model.shapes.StructureShape @@ -552,12 +553,17 @@ class ServerBuilderGenerator( // Use `unwrap_or_default` to make clippy happy. ".unwrap_or_default()" } else { - val into = if (!member.canReachConstrainedShape(model, symbolProvider)) { - "" + if (!member.canReachConstrainedShape(model, symbolProvider)) { + val unwrapOr = when (model.expectShape(member.target)) { + is NumberShape, is EnumShape, is BooleanShape -> ".unwrap_or(" + else -> ".unwrap_or_else(||" + } + """$unwrapOr $it)""" } else { - """.try_into().expect("This check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""" + val into = + """.try_into().expect("This check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""" + """.unwrap_or_else(|| $it$into)""" } - """.unwrap_or_else(|| $it$into)""" } } From 760390a4e72b834d1f3f0cc8fc6bcc82ea5f76ea Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 22 Dec 2022 11:35:00 +0100 Subject: [PATCH 05/11] My minor nits --- .../ServerBuilderConstraintViolations.kt | 4 +- .../generators/ServerBuilderGenerator.kt | 74 +++-- ...rGeneratorWithoutPublicConstrainedTypes.kt | 12 +- .../ServerBuilderDefaultValuesTest.kt | 263 ++++++++++-------- 4 files changed, 194 insertions(+), 159 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt index 774e78a7b4..4d4efc4c7c 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt @@ -62,7 +62,9 @@ class ServerBuilderConstraintViolations( nonExhaustive: Boolean, shouldRenderAsValidationExceptionFieldList: Boolean, ) { - check(all.isNotEmpty()) { shape } + check(all.isNotEmpty()) { + "Attempted to render constraint violations for the builder for structure shape ${shape.id}, but calculation of the constraint violations resulted in no variants" + } Attribute.Derives(setOf(RuntimeType.Debug, RuntimeType.PartialEq)).render(writer) writer.docs("Holds one variant for each of the ways the builder can fail.") diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index c5e7c0b81c..137de7e14c 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -139,15 +139,15 @@ class ServerBuilderGenerator( val notFallible = members.all { if (structureShape.isReachableFromOperationInput()) { - // When deserializing an input structure, constraints might not be satisfied. - // For this member not to be fallible, it must not be constrained (constraints in input must always be checked) - // and either optional (no need to set this; not required) - // or has a default value (some value will always be present) + // When deserializing an input structure, constraints might not be satisfied by the data in the + // incoming request. + // For this builder not to be fallible, no members must be constrained (constraints in input must + // always be checked) and all members must _either_ be optional (no need to set it; not required) + // or have a default value. isNotConstrained(it) && (isOptional(it) || hasDefault(it)) } else { - // This structure will be constructed manually. - // Constraints will have to be dealt with - // before members are set in the builder + // This structure will be constructed manually by the user. + // Constraints will have to be dealt with before members are set in the builder. isOptional(it) || hasDefault(it) } } @@ -446,11 +446,10 @@ class ServerBuilderGenerator( } private fun renderTryFromBuilderImpl(writer: RustWriter) { - val errorType = if (!isBuilderFallible) "std::convert::Infallible" else "ConstraintViolation" writer.rustTemplate( """ impl #{TryFrom} for #{Structure} { - type Error = $errorType; + type Error = ConstraintViolation; fn try_from(builder: Builder) -> Result { builder.build() @@ -549,20 +548,18 @@ class ServerBuilderGenerator( withBlock("$memberName: self.$memberName", ",") { val wrapDefault: (String) -> String = { if (member.isStreaming(model)) { - // We set ByteStream to Default::default() until it is easier to use the full namespace for python. - // Use `unwrap_or_default` to make clippy happy. ".unwrap_or_default()" } else { - if (!member.canReachConstrainedShape(model, symbolProvider)) { + if (member.canReachConstrainedShape(model, symbolProvider)) { + val into = + """.try_into().expect("this check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""" + """.unwrap_or_else(|| $it$into)""" + } else { val unwrapOr = when (model.expectShape(member.target)) { is NumberShape, is EnumShape, is BooleanShape -> ".unwrap_or(" else -> ".unwrap_or_else(||" } """$unwrapOr $it)""" - } else { - val into = - """.try_into().expect("This check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""" - """.unwrap_or_else(|| $it$into)""" } } } @@ -622,10 +619,10 @@ class ServerBuilderGenerator( ) // This is to avoid a `allow(clippy::useless_conversion)` on `try_into()` // Removing this `if` and leaving the `else if` below a plain `if` will make no difference - // to the compilation, but to clippy. + // to the compilation, but to clippy. if (member.hasNonNullDefault()) { rustTemplate( - """#{Default:W}""", + "#{Default:W}", "Default" to renderDefaultBuilder( model, runtimeConfig, @@ -633,24 +630,22 @@ class ServerBuilderGenerator( member, ) { if (member.isStreaming(model)) { - // We set ByteStream to Default::default() until it is easier to use the full namespace for python. - // Use `unwrap_or_default` to make clippy happy. ".unwrap_or_default()" } else { - // The conversion is done above - """.unwrap_or($it)""" + ".unwrap_or($it)" } }, ) if (!isBuilderFallible) { - // Unwrap the Option + // Unwrap the `Option`. + // TODO This should be expect. rust(".unwrap()") } } } else { if (member.hasNonNullDefault()) { rustTemplate( - """#{Default:W}""", + "#{Default:W}", "Default" to renderDefaultBuilder( model, runtimeConfig, @@ -660,7 +655,8 @@ class ServerBuilderGenerator( ), ) if (!isBuilderFallible) { - // unwrap the Option + // Unwrap the `Option`. + // TODO This should be expect. rust(".unwrap()") } } @@ -674,7 +670,7 @@ class ServerBuilderGenerator( ) } } - // This won't run if there is a default value + // This won't run if there is a default value. serverBuilderConstraintViolations.forMember(member)?.also { rust(".ok_or(ConstraintViolation::${it.name()})?") } @@ -692,7 +688,13 @@ fun buildFnReturnType(isBuilderFallible: Boolean, structureSymbol: Symbol) = wri } } -fun renderDefaultBuilder(model: Model, runtimeConfig: RuntimeConfig, symbolProvider: RustSymbolProvider, member: MemberShape, wrap: (s: String) -> String = { it }): Writable { +fun renderDefaultBuilder( + model: Model, + runtimeConfig: RuntimeConfig, + symbolProvider: RustSymbolProvider, + member: MemberShape, + wrap: (s: String) -> String = { it }, +): Writable { return writable { val node = member.expectTrait().toNode()!! val name = member.memberName @@ -702,12 +704,14 @@ fun renderDefaultBuilder(model: Model, runtimeConfig: RuntimeConfig, symbolProvi val value = when (target) { is IntEnumShape -> node.expectNumberNode().value is EnumShape -> node.expectStringNode().value - else -> throw CodegenException("Default value for $name must be of EnumShape or IntEnumShape") + else -> throw CodegenException("Default value for shape ${target.id} must be of EnumShape or IntEnumShape") } val enumValues = when (target) { is IntEnumShape -> target.enumValues is EnumShape -> target.enumValues - else -> UNREACHABLE("It must be an [Int]EnumShape, otherwise it'd have failed above") + else -> UNREACHABLE( + "Target shape ${target.id} must be an `EnumShape` or an `IntEnumShape` at this point, otherwise it would have failed above", + ) } val variant = enumValues .entries @@ -795,17 +799,9 @@ fun renderDefaultBuilder(model: Model, runtimeConfig: RuntimeConfig, symbolProvi } } - is BlobShape -> { - val value = if (member.isStreaming(model)) { - /* ByteStream to work in Python and Rust without explicit dependency */ - "Default::default()" - } else { - "Vec::new()" - } - rust(wrap(value)) - } + is BlobShape -> rust(wrap("Default::default()")) - else -> throw CodegenException("Default value for $name is unsupported or cannot exist") + else -> throw CodegenException("Default value for shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") } } } diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt index 2084c301f3..b204f79080 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt @@ -7,7 +7,6 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolProvider -import software.amazon.smithy.model.Model import software.amazon.smithy.model.shapes.BooleanShape import software.amazon.smithy.model.shapes.EnumShape import software.amazon.smithy.model.shapes.MemberShape @@ -58,7 +57,6 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( * This builder only enforces the `required` trait. */ fun hasFallibleBuilder( - model: Model, structureShape: StructureShape, symbolProvider: SymbolProvider, ): Boolean { @@ -81,8 +79,7 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( private val structureSymbol = symbolProvider.toSymbol(shape) private val builderSymbol = shape.serverBuilderSymbol(symbolProvider, false) - private val moduleName = builderSymbol.namespace.split("::").last() - private val isBuilderFallible = hasFallibleBuilder(model, shape, symbolProvider) + private val isBuilderFallible = hasFallibleBuilder(shape, symbolProvider) private val serverBuilderConstraintViolations = ServerBuilderConstraintViolations(codegenContext, shape, builderTakesInUnconstrainedTypes = false) @@ -180,8 +177,8 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( else -> ".unwrap_or_else(||" } rustTemplate( - """#{default:W}""", - "default" to renderDefaultBuilder( + "#{Default:W}", + "Default" to renderDefaultBuilder( model, runtimeConfig, symbolProvider, @@ -236,11 +233,10 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( } private fun renderTryFromBuilderImpl(writer: RustWriter) { - val errorType = if (!isBuilderFallible) "std::convert::Infallible" else "ConstraintViolation" writer.rustTemplate( """ impl #{TryFrom} for #{Structure} { - type Error = $errorType; + type Error = ConstraintViolation; fn try_from(builder: Builder) -> Result { builder.build() diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt index 4fffc33766..3e0469ba10 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt @@ -14,9 +14,10 @@ import software.amazon.smithy.model.shapes.EnumShape import software.amazon.smithy.model.shapes.StructureShape import software.amazon.smithy.rust.codegen.core.rustlang.RustModule import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter -import software.amazon.smithy.rust.codegen.core.rustlang.Writable +import software.amazon.smithy.rust.codegen.core.rustlang.conditionalBlock import software.amazon.smithy.rust.codegen.core.rustlang.rust 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.RustSymbolProvider import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureGenerator @@ -35,7 +36,7 @@ import java.util.stream.Stream @TestInstance(TestInstance.Lifecycle.PER_CLASS) class ServerBuilderDefaultValuesTest { - // When defaults are used, the model will be generated with these default values + // When defaults are used, the model will be generated with these in the `@default` trait. private val defaultValues = mapOf( "Boolean" to "true", "String" to "foo".dq(), @@ -59,45 +60,62 @@ class ServerBuilderDefaultValuesTest { "DocumentMap" to "{}", ) - // When the test applies values to validate we honor custom values, use these values - private val customValues = - mapOf( - "Boolean" to "false", - "String" to "bar".dq(), - "Byte" to "6", - "Short" to "66", - "Integer" to "666", - "Long" to "6666", - "Float" to "0.6", - "Double" to "0.66", - "Timestamp" to "2022-11-25T17:30:50.00Z".dq(), - // "BigInteger" to "55555", "BigDecimal" to "0.555", // TODO(https://github.com/awslabs/smithy-rs/issues/312) - "StringList" to "[]", - "IntegerMap" to "{}", - "Language" to "fr".dq(), - "DocumentBoolean" to "false", - "DocumentString" to "bar".dq(), - "DocumentNumberPosInt" to "1000", - "DocumentNumberNegInt" to "-1000", - "DocumentNumberFloat" to "0.01", - "DocumentList" to "[]", - "DocumentMap" to "{}", - ) + // When the test applies values to validate we honor custom values, use these (different) values. + private val customValues = mapOf( + "Boolean" to "false", + "String" to "bar".dq(), + "Byte" to "6", + "Short" to "66", + "Integer" to "666", + "Long" to "6666", + "Float" to "0.6", + "Double" to "0.66", + "Timestamp" to "2022-11-25T17:30:50.00Z".dq(), + // "BigInteger" to "55555", "BigDecimal" to "0.555", // TODO(https://github.com/awslabs/smithy-rs/issues/312) + "StringList" to "[]", + "IntegerMap" to "{}", + "Language" to "fr".dq(), + "DocumentBoolean" to "false", + "DocumentString" to "bar".dq(), + "DocumentNumberPosInt" to "1000", + "DocumentNumberNegInt" to "-1000", + "DocumentNumberFloat" to "0.01", + "DocumentList" to "[]", + "DocumentMap" to "{}", + ) - @ParameterizedTest(name = "(#{index}) Generate default value. Required Trait: {1}, nulls: {2}, optionals: {3}") - @MethodSource("localParameters") - fun `default values are generated and builders respect default and overrides`(testConfig: TestConfig, setupFiles: (w: RustWriter, m: Model, s: RustSymbolProvider) -> Unit) { - val initialSetValues = defaultValues.mapValues { if (testConfig.nullDefault) null else it.value } - val model = generateModel(testConfig, initialSetValues) + @ParameterizedTest(name = "(#{index}) Server builders and default values. Params = requiredTrait: {0}, nullDefault: {1}, applyDefaultValues: {2}, builderGeneratorKind: {3}, assertValues: {4}") + @MethodSource("testParameters") + fun `default values are generated and builders respect default and overrides`( + requiredTrait: Boolean, + nullDefault: Boolean, + applyDefaultValues: Boolean, + builderGeneratorKind: BuilderGeneratorKind, + assertValues: Map, + ) { + println("Running test with params = requiredTrait: $requiredTrait, nullDefault: $nullDefault, applyDefaultValues: $applyDefaultValues, builderGeneratorKind: $builderGeneratorKind, assertValues: $assertValues"); + val initialSetValues = this.defaultValues.mapValues { if (nullDefault) null else it.value } + val model = generateModel(requiredTrait, applyDefaultValues, nullDefault, initialSetValues) val symbolProvider = serverTestSymbolProvider(model) val project = TestWorkspace.testProject(symbolProvider) + project.withModule(RustModule.public("model")) { - setupFiles(this, model, symbolProvider) + when (builderGeneratorKind) { + BuilderGeneratorKind.SERVER_BUILDER_GENERATOR -> { + writeServerBuilderGenerator(this, model, symbolProvider) + } + BuilderGeneratorKind.SERVER_BUILDER_GENERATOR_WITHOUT_PUBLIC_CONSTRAINED_TYPES -> { + writeServerBuilderGeneratorWithoutPublicConstrainedTypes(this, model, symbolProvider) + } + } - val rustValues = setupRustValuesForTest(testConfig.assertValues) - val applySetters = testConfig.applyDefaultValues - val setters = if (applySetters) structSetters(rustValues, testConfig.nullDefault && !testConfig.requiredTrait) else writable { } - val unwrapBuilder = if (testConfig.nullDefault && testConfig.requiredTrait && testConfig.applyDefaultValues) ".unwrap()" else "" + val rustValues = setupRustValuesForTest(assertValues) + val setters = if (applyDefaultValues) { + structSetters(rustValues, nullDefault && !requiredTrait) + } else { + writable { } + } + val unwrapBuilder = if (nullDefault && requiredTrait && applyDefaultValues) ".unwrap()" else "" unitTest( name = "generates_default_required_values", block = writable { @@ -105,17 +123,26 @@ class ServerBuilderDefaultValuesTest { """ let my_struct = MyStruct::builder() #{Setters:W} - .build()$unwrapBuilder; + .build() + $unwrapBuilder; #{Assertions:W} """, - "Assertions" to assertions(rustValues, applySetters, testConfig.nullDefault, testConfig.requiredTrait, testConfig.applyDefaultValues), + "Assertions" to assertions( + rustValues, + applyDefaultValues, + nullDefault, + requiredTrait, + applyDefaultValues + ), "Setters" to setters, ) }, ) } - project.compileAndTest() + + // Run clippy because the builder's code for handling `@default` is prone to upset it. + project.compileAndTest(runClippy = true) } private fun setupRustValuesForTest(valuesMap: Map): Map { @@ -133,11 +160,12 @@ class ServerBuilderDefaultValuesTest { "IntegerMap" to "std::collections::HashMap::::new()", "DocumentList" to "Vec::::new()", "DocumentMap" to "std::collections::HashMap::::new()", - ) + valuesMap.filter { it.value?.startsWith("Document") == true }.map { it.key to "${it.value}.into()" } + ) + valuesMap + .filter { it.value?.startsWith("Document") ?: false } + .map { it.key to "${it.value}.into()" } } private fun writeServerBuilderGeneratorWithoutPublicConstrainedTypes(writer: RustWriter, model: Model, symbolProvider: RustSymbolProvider) { - writer.rust("##![allow(deprecated)]") val struct = model.lookup("com.test#MyStruct") val codegenContext = serverTestCodegenContext(model) val builderGenerator = ServerBuilderGeneratorWithoutPublicConstrainedTypes(codegenContext, struct) @@ -152,7 +180,6 @@ class ServerBuilderDefaultValuesTest { } private fun writeServerBuilderGenerator(writer: RustWriter, model: Model, symbolProvider: RustSymbolProvider) { - writer.rust("##![allow(deprecated)]") val struct = model.lookup("com.test#MyStruct") val codegenContext = serverTestCodegenContext(model) val builderGenerator = ServerBuilderGenerator(codegenContext, struct) @@ -166,91 +193,92 @@ class ServerBuilderDefaultValuesTest { StructureGenerator(model, symbolProvider, writer, struct).render() } - private fun structSetters(values: Map, optional: Boolean): Writable { - return writable { - values.entries.forEach { - rust(".${it.key.toSnakeCase()}(") - if (optional) { - rust("Some(") - } - when (it.key) { - "String" -> { - rust("${it.value}.into()") - } - - "DocumentNull" -> - rust("aws_smithy_types::Document::Null") - - "DocumentString" -> { - rust("aws_smithy_types::Document::String(String::from(${it.value}))") - } - - else -> { - if (it.key.startsWith("DocumentNumber")) { - val type = it.key.replace("DocumentNumber", "") - rust("aws_smithy_types::Document::Number(aws_smithy_types::Number::$type(${it.value}))") - } else { - rust("${it.value}.into()") + private fun structSetters(values: Map, optional: Boolean) = writable { + for ((key, value) in values) { + withBlock(".${key.toSnakeCase()}(", ")") { + conditionalBlock("Some(", ")", optional) { + when (key) { + "String" -> rust("$value.into()") + "DocumentNull" -> rust("aws_smithy_types::Document::Null") + "DocumentString" -> rust("aws_smithy_types::Document::String(String::from($value))") + + else -> { + if (key.startsWith("DocumentNumber")) { + val type = key.replace("DocumentNumber", "") + rust("aws_smithy_types::Document::Number(aws_smithy_types::Number::$type($value))") + } else { + rust("$value.into()") + } } } } - if (optional) { - rust(")") - } - rust(")") } } } - private fun assertions(values: Map, hasSetValues: Boolean, hasNullValues: Boolean, requiredTrait: Boolean, hasDefaults: Boolean): Writable { - return writable { - for (it in values.entries) { - rust("assert_eq!(my_struct.${it.key.toSnakeCase()} ") - if (!hasSetValues) { - rust(".is_none(), true);") - continue - } - - val expected = if (it.key == "DocumentNull") { - "aws_smithy_types::Document::Null" - } else if (it.key == "DocumentString") { - "String::from(${it.value}).into()" - } else if (it.key.startsWith("DocumentNumber")) { - val type = it.key.replace("DocumentNumber", "") - "aws_smithy_types::Document::Number(aws_smithy_types::Number::$type(${it.value}))" - } else if (it.key.startsWith("Document")) { - "${it.value}.into()" - } else { - "${it.value}" + private fun assertions( + values: Map, + hasSetValues: Boolean, + hasNullValues: Boolean, + requiredTrait: Boolean, + hasDefaults: Boolean, + ) = writable { + for ((key, value) in values) { + val member = "my_struct.${key.toSnakeCase()}" + + if (!hasSetValues) { + rust("assert!($member.is_none());") + } else { + val actual = writable { + rust(member) + if (!requiredTrait && !(hasDefaults && !hasNullValues)) { + rust(".unwrap()") + } } - - if (!requiredTrait && !(hasDefaults && !hasNullValues)) { - rust(".unwrap()") + val expected = writable { + val expected = if (key == "DocumentNull") { + "aws_smithy_types::Document::Null" + } else if (key == "DocumentString") { + "String::from($value).into()" + } else if (key.startsWith("DocumentNumber")) { + val type = key.replace("DocumentNumber", "") + "aws_smithy_types::Document::Number(aws_smithy_types::Number::$type($value))" + } else if (key.startsWith("Document")) { + "$value.into()" + } else { + "$value" + } + rust(expected) } - - rust(", $expected);") + rustTemplate("assert_eq!(#{Actual:W}, #{Expected:W});", "Actual" to actual, "Expected" to expected) } } } - private fun generateModel(testConfig: TestConfig, values: Map): Model { - val requiredTrait = if (testConfig.requiredTrait) "@required" else "" + private fun generateModel( + requiredTrait: Boolean, + applyDefaultValues: Boolean, + nullDefault: Boolean, + values: Map, + ): Model { + val requiredOrNot = if (requiredTrait) "@required" else "" val members = values.entries.joinToString(", ") { - val value = if (testConfig.applyDefaultValues) { + val value = if (applyDefaultValues) { "= ${it.value}" - } else if (testConfig.nullDefault) { + } else if (nullDefault) { "= null" - } else { "" } + } else { + "" + } """ - $requiredTrait + $requiredOrNot ${it.key.toPascalCase()}: ${it.key} $value """ } val model = """ namespace com.test - use smithy.framework#ValidationException structure MyStruct { $members @@ -283,10 +311,21 @@ class ServerBuilderDefaultValuesTest { return model.asSmithyModel(smithyVersion = "2") } - private fun localParameters(): Stream { - val builderWriters = listOf( - ::writeServerBuilderGenerator, - ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes, + /** + * The builder generator we should test. + * We use an enum instead of directly passing in the closure so that JUnit can print a helpful string in the test + * report. + */ + enum class BuilderGeneratorKind { + SERVER_BUILDER_GENERATOR, + SERVER_BUILDER_GENERATOR_WITHOUT_PUBLIC_CONSTRAINED_TYPES, + } + + private fun testParameters(): Stream { + // TODO Use cross-product. + val builderGeneratorKindList = listOf( + BuilderGeneratorKind.SERVER_BUILDER_GENERATOR, + BuilderGeneratorKind.SERVER_BUILDER_GENERATOR_WITHOUT_PUBLIC_CONSTRAINED_TYPES, ) return Stream.of( TestConfig(defaultValues, requiredTrait = false, nullDefault = true, applyDefaultValues = true), @@ -305,14 +344,16 @@ class ServerBuilderDefaultValuesTest { TestConfig(customValues, requiredTrait = false, nullDefault = false, applyDefaultValues = false), TestConfig(defaultValues, requiredTrait = true, nullDefault = false, applyDefaultValues = true), - TestConfig(customValues, requiredTrait = true, nullDefault = false, applyDefaultValues = true), - - ).flatMap { builderWriters.stream().map { builderWriter -> Arguments.of(it, builderWriter) } } + ).flatMap { (assertValues, requiredTrait, nullDefault, applyDefaultValues) -> + builderGeneratorKindList.stream().map { builderGeneratorKind -> + Arguments.of(requiredTrait, nullDefault, applyDefaultValues, builderGeneratorKind, assertValues) + } + } } data class TestConfig( - // The values in the setters and assert!() calls + // The values in the `assert!()` calls and for the `@default` trait val assertValues: Map, // Whether to apply @required to all members val requiredTrait: Boolean, From 01b89578346be49d2b378ee5cafb67a9e68b3731 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 22 Dec 2022 19:23:35 +0100 Subject: [PATCH 06/11] Changes in ServerBuilderGenerator pass tests --- codegen-core/common-test-models/simple.smithy | 131 +----- .../generators/ServerBuilderGenerator.kt | 398 ++++++++++++------ 2 files changed, 292 insertions(+), 237 deletions(-) diff --git a/codegen-core/common-test-models/simple.smithy b/codegen-core/common-test-models/simple.smithy index 43c4bc6aca..823a513b1d 100644 --- a/codegen-core/common-test-models/simple.smithy +++ b/codegen-core/common-test-models/simple.smithy @@ -1,136 +1,39 @@ -$version: "1.0" +$version: "2.0" namespace com.amazonaws.simple use aws.protocols#restJson1 -use smithy.test#httpRequestTests -use smithy.test#httpResponseTests use smithy.framework#ValidationException @restJson1 -@title("SimpleService") -@documentation("A simple service example, with a Service resource that can be registered and a readonly healthcheck") service SimpleService { version: "2022-01-01", - resources: [ - Service, - ], operations: [ - Healthcheck, - StoreServiceBlob, + MyOperation, ], } -@documentation("Id of the service that will be registered") -string ServiceId - -@documentation("Name of the service that will be registered") -string ServiceName - -@error("client") -@documentation( - """ - Returned when a new resource cannot be created because one already exists. - """ -) -structure ResourceAlreadyExists { - @required - message: String -} - -@documentation("A resource that can register services") -resource Service { - identifiers: { id: ServiceId }, - put: RegisterService, -} - -@idempotent -@http(method: "PUT", uri: "/service/{id}") -@documentation("Service register operation") -@httpRequestTests([ - { - id: "RegisterServiceRequestTest", - protocol: "aws.protocols#restJson1", - uri: "/service/1", - headers: { - "Content-Type": "application/json", - }, - params: { id: "1", name: "TestService" }, - body: "{\"name\":\"TestService\"}", - method: "PUT", - } -]) -@httpResponseTests([ - { - id: "RegisterServiceResponseTest", - protocol: "aws.protocols#restJson1", - params: { id: "1", name: "TestService" }, - body: "{\"id\":\"1\",\"name\":\"TestService\"}", - code: 200, - headers: { - "Content-Length": "31" - } - } -]) -operation RegisterService { - input: RegisterServiceInputRequest, - output: RegisterServiceOutputResponse, - errors: [ResourceAlreadyExists, ValidationException] +@http(method: "POST", uri: "/my-operation") +operation MyOperation { + input: MyOperationInputOutput, + output: MyOperationInputOutput, + errors: [ValidationException] } -@documentation("Service register input structure") -structure RegisterServiceInputRequest { +structure MyOperationInputOutput { @required - @httpLabel - id: ServiceId, - name: ServiceName, -} + defaultNullAndRequired: String = null -@documentation("Service register output structure") -structure RegisterServiceOutputResponse { - @required - id: ServiceId, - name: ServiceName, -} + @default("ab") + defaultLengthString: DefaultLengthString, -@readonly -@http(uri: "/healthcheck", method: "GET") -@documentation("Read-only healthcheck operation") -operation Healthcheck { - input: HealthcheckInputRequest, - output: HealthcheckOutputResponse -} - -@documentation("Service healthcheck output structure") -structure HealthcheckInputRequest { - -} - -@documentation("Service healthcheck input structure") -structure HealthcheckOutputResponse { - -} - -@readonly -@http(method: "POST", uri: "/service/{id}/blob") -@documentation("Stores a blob for a service id") -operation StoreServiceBlob { - input: StoreServiceBlobInput, - output: StoreServiceBlobOutput, - errors: [ValidationException] -} - -@documentation("Store a blob for a service id input structure") -structure StoreServiceBlobInput { - @required - @httpLabel - id: ServiceId, @required - @httpPayload - content: Blob, + lengthString: LengthString, } -@documentation("Store a blob for a service id output structure") -structure StoreServiceBlobOutput { +@length(min: 69) +string LengthString -} +@length(max: 69) +@default("ab") +string DefaultLengthString diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index 137de7e14c..6fbdeaf03e 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -546,137 +546,180 @@ class ServerBuilderGenerator( val memberName = symbolProvider.toMemberName(member) withBlock("$memberName: self.$memberName", ",") { - val wrapDefault: (String) -> String = { - if (member.isStreaming(model)) { - ".unwrap_or_default()" - } else { - if (member.canReachConstrainedShape(model, symbolProvider)) { - val into = - """.try_into().expect("this check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""" - """.unwrap_or_else(|| $it$into)""" - } else { - val unwrapOr = when (model.expectShape(member.target)) { - is NumberShape, is EnumShape, is BooleanShape -> ".unwrap_or(" - else -> ".unwrap_or_else(||" - } - """$unwrapOr $it)""" - } - } - } - // Write the modifier(s). + + // 1. Enforce constraint traits of data from incoming requests. serverBuilderConstraintViolations.builderConstraintViolationForMember(member)?.also { constraintViolation -> - val hasBox = builderMemberSymbol(member) - .mapRustType { it.stripOuter() } - .isRustBoxed() - if (hasBox) { - rustTemplate( - """ - .map(|v| match *v { - #{MaybeConstrained}::Constrained(x) => Ok(Box::new(x)), - #{MaybeConstrained}::Unconstrained(x) => Ok(Box::new(x.try_into()?)), - }) - .map(|res| - res${ if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())" } - .map_err(|err| ConstraintViolation::${constraintViolation.name()}(Box::new(err))) - ) - .transpose()? - """, - *codegenScope, - ) - } else { - rustTemplate( - """ - .map(|v| match v { - #{MaybeConstrained}::Constrained(x) => Ok(x), - #{MaybeConstrained}::Unconstrained(x) => x.try_into(), - }) - """, - *codegenScope, - ) - if (isBuilderFallible) { - rustTemplate( - """ - .map(|res| - res${if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())"} - .map_err(ConstraintViolation::${constraintViolation.name()}) - ) - .transpose()? - """, - *codegenScope, - ) - } + enforceConstraints(this, member, constraintViolation) + } - // Constrained types are not public and this is a member shape that would have generated a - // public constrained type, were the setting to be enabled. - // We've just checked the constraints hold by going through the non-public - // constrained type, but the user wants to work with the unconstrained type, so we have to - // unwrap it. - if (!publicConstrainedTypes && member.wouldHaveConstrainedWrapperTupleTypeWerePublicConstrainedTypesEnabled(model)) { - rust( - ".map(|v: #T| v.into())", - constrainedShapeSymbolProvider.toSymbol(model.expectShape(member.target)), - ) - // This is to avoid a `allow(clippy::useless_conversion)` on `try_into()` - // Removing this `if` and leaving the `else if` below a plain `if` will make no difference - // to the compilation, but to clippy. - if (member.hasNonNullDefault()) { - rustTemplate( - "#{Default:W}", - "Default" to renderDefaultBuilder( - model, - runtimeConfig, - symbolProvider, - member, - ) { - if (member.isStreaming(model)) { - ".unwrap_or_default()" - } else { - ".unwrap_or($it)" - } - }, - ) - if (!isBuilderFallible) { - // Unwrap the `Option`. - // TODO This should be expect. - rust(".unwrap()") - } - } - } else { - if (member.hasNonNullDefault()) { - rustTemplate( - "#{Default:W}", - "Default" to renderDefaultBuilder( - model, - runtimeConfig, - symbolProvider, - member, - wrapDefault, - ), - ) - if (!isBuilderFallible) { - // Unwrap the `Option`. - // TODO This should be expect. - rust(".unwrap()") - } - } - } - } - } ?: run { - if (member.hasNonNullDefault()) { - rustTemplate( - "#{Default:W}", - "Default" to renderDefaultBuilder(model, runtimeConfig, symbolProvider, member, wrapDefault), - ) + if (member.hasNonNullDefault()) { + // 2a. If a `@default` value is modeled and the user did not set a value, fall back to using the + // default value. + fallbackToDefaultValue(this, member) + } else { + // 2b. If the member is `@required` and has no `@default` value, the user must set a value; + // otherwise, we fail with a `ConstraintViolation::Missing*` variant. + serverBuilderConstraintViolations.forMember(member)?.also { + rust(".ok_or(ConstraintViolation::${it.name()})?") } } - // This won't run if there is a default value. - serverBuilderConstraintViolations.forMember(member)?.also { - rust(".ok_or(ConstraintViolation::${it.name()})?") - } + + } + } + } + } + + private fun fallbackToDefaultValue(writer: RustWriter, member: MemberShape) { +// TODO Remove +// if (member.hasNonNullDefault()) { +// rustTemplate( +// "#{Default:W}", +// "Default" to renderDefaultBuilder( +// model, +// runtimeConfig, +// symbolProvider, +// member, +// ) { +// if (member.isStreaming(model)) { +// ".unwrap_or_default()" +// } else { +// ".unwrap_or($it)" +// } +// }, +// ) +// } +// } else if (member.hasNonNullDefault()) { +// rustTemplate( +// "#{Default:W}", +// "Default" to renderDefaultBuilder( +// model, +// runtimeConfig, +// symbolProvider, +// member, +// wrapDefault, +// ), +// ) +// } +// if (!isBuilderFallible) { +// // Unwrap the `Option`. +// // TODO This should be expect. +// rust(".unwrap()") +// } +// +// val wrapDefault: (String) -> String = { +// if (member.isStreaming(model)) { +// ".unwrap_or_default()" +// } else { +// val targetShape = model.expectShape(member.target) +// if (targetShape !is EnumShape && member.canReachConstrainedShape(model, symbolProvider)) { +// // TODO: Instead of panicking here, which will ungracefully shut down the service, potentially +// // causing data corruption, perform the `try_into()` check _once_ at service startup time, perhaps +// // storing the result in a `OnceCell` that could be reused. +// """ +// .unwrap_or_else(|| +// $it.try_into().expect("this check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") +// ) +// """ +// } else { +// val unwrapOr = when (model.expectShape(member.target)) { +// is NumberShape, is EnumShape, is BooleanShape -> ".unwrap_or(" +// else -> ".unwrap_or_else(||" +// } +// "$unwrapOr $it)" +// } +// } +// } + + val defaultValue = renderDefaultValue(model, runtimeConfig, symbolProvider, member) + val targetShape = model.expectShape(member.target) + + if (member.isStreaming(model)) { + writer.rust(".unwrap_or_default()") + } else if (targetShape !is EnumShape && member.canReachConstrainedShape(model, symbolProvider)) { + // TODO: Instead of panicking here, which will ungracefully shut down the service, potentially + // causing data corruption, perform the `try_into()` check _once_ at service startup time, perhaps + // storing the result in a `OnceCell` that could be reused. + writer.rustTemplate( + """ + .unwrap_or_else(|| + #{DefaultValue:W} + .try_into() + .expect("this check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") + ) + """, + "DefaultValue" to defaultValue, + ) + } else { + when (targetShape) { + is NumberShape, is EnumShape, is BooleanShape -> { + writer.rustTemplate(".unwrap_or(#{DefaultValue:W})", "DefaultValue" to defaultValue) + } + // Values for the Rust types of the rest of the shapes require heap allocations, so we calculate them + // in a (lazily-executed) closure for slight performance gains. + else -> { + writer.rustTemplate(".unwrap_or_else(|| #{DefaultValue:W})", "DefaultValue" to defaultValue) } } } + + // TODO Remove +// writer.rustTemplate( +// "#{Default:W}", +// "Default" to renderDefaultBuilder(model, runtimeConfig, symbolProvider, member, wrapDefault), +// ) + } + + private fun enforceConstraints(writer: RustWriter, member: MemberShape, constraintViolation: ConstraintViolation) { + // This member is constrained. Enforce the constraint traits on the value set in the builder. + // The code is slightly different in case the member is recursive, since it will be wrapped in + // `std::boxed::Box`. + val hasBox = builderMemberSymbol(member) + .mapRustType { it.stripOuter() } + .isRustBoxed() + if (hasBox) { + writer.rustTemplate( + """ + .map(|v| match *v { + #{MaybeConstrained}::Constrained(x) => Ok(Box::new(x)), + #{MaybeConstrained}::Unconstrained(x) => Ok(Box::new(x.try_into()?)), + }) + .map(|res| + res${ if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())" } + .map_err(|err| ConstraintViolation::${constraintViolation.name()}(Box::new(err))) + ) + .transpose()? + """, + *codegenScope, + ) + } else { + writer.rustTemplate( + """ + .map(|v| match v { + #{MaybeConstrained}::Constrained(x) => Ok(x), + #{MaybeConstrained}::Unconstrained(x) => x.try_into(), + }) + .map(|res| + res${if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())"} + .map_err(ConstraintViolation::${constraintViolation.name()}) + ) + .transpose()? + """, + *codegenScope, + ) + } + + // Constrained types are not public and this is a member shape that would have generated a + // public constrained type, were the setting to be enabled. + // We've just checked the constraints hold by going through the non-public + // constrained type, but the user wants to work with the unconstrained type, so we have to + // unwrap it. + if (!publicConstrainedTypes && member.wouldHaveConstrainedWrapperTupleTypeWerePublicConstrainedTypesEnabled(model)) { + writer.rust( + ".map(|v: #T| v.into())", + constrainedShapeSymbolProvider.toSymbol(model.expectShape(member.target)), + ) + } } } @@ -688,6 +731,7 @@ fun buildFnReturnType(isBuilderFallible: Boolean, structureSymbol: Symbol) = wri } } +// TODO Remove fun renderDefaultBuilder( model: Model, runtimeConfig: RuntimeConfig, @@ -805,3 +849,111 @@ fun renderDefaultBuilder( } } } + +fun renderDefaultValue( + model: Model, + runtimeConfig: RuntimeConfig, + symbolProvider: RustSymbolProvider, + member: MemberShape, +) = writable { + val node = member.expectTrait().toNode()!! + val types = ServerCargoDependency.smithyTypes(runtimeConfig).toType() + // Define the exception once for DRYness. + val unsupportedDefaultValueException = + CodegenException("Default value $node for member shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") + when (val target = model.expectShape(member.target)) { + is EnumShape, is IntEnumShape -> { + val value = when (target) { + is IntEnumShape -> node.expectNumberNode().value + is EnumShape -> node.expectStringNode().value + else -> throw CodegenException("Default value for shape ${target.id} must be of EnumShape or IntEnumShape") + } + val enumValues = when (target) { + is IntEnumShape -> target.enumValues + is EnumShape -> target.enumValues + else -> UNREACHABLE( + "Target shape ${target.id} must be an `EnumShape` or an `IntEnumShape` at this point, otherwise it would have failed above", + ) + } + val variant = enumValues + .entries + .filter { entry -> entry.value == value } + .map { entry -> + symbolProvider.toEnumVariantName( + EnumDefinition.builder().name(entry.key).value(entry.value.toString()).build(), + )!! + } + .first() + rust("#T::${variant.name}", symbolProvider.toSymbol(target)) + } + + is ByteShape -> rust(node.expectNumberNode().value.toString() + "i8") + is ShortShape -> rust(node.expectNumberNode().value.toString() + "i16") + is IntegerShape -> rust(node.expectNumberNode().value.toString() + "i32") + is LongShape -> rust(node.expectNumberNode().value.toString() + "i64") + is FloatShape -> rust(node.expectNumberNode().value.toFloat().toString() + "f32") + is DoubleShape -> rust(node.expectNumberNode().value.toDouble().toString() + "f64") + is BooleanShape -> rust(node.expectBooleanNode().value.toString()) + is StringShape -> rust("String::from(${node.expectStringNode().value.dq()})") + is TimestampShape -> when (node) { + is NumberNode -> rust(node.expectNumberNode().value.toString()) + is StringNode -> { + val value = node.expectStringNode().value + rustTemplate( + """ + #{SmithyTypes}::DateTime::from_str("$value", #{SmithyTypes}::date_time::Format::DateTime) + .expect("default value `$value` cannot be parsed into a valid date time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") + """, + "SmithyTypes" to types, + ) + } + else -> throw unsupportedDefaultValueException + } + is ListShape -> { + check(node is ArrayNode && node.isEmpty) + rust("Vec::new()") + } + is MapShape -> { + check(node is ObjectNode && node.isEmpty) + rust("std::collections::HashMap::new()") + } + is DocumentShape -> { + when (node) { + is NullNode -> rustTemplate( + "#{SmithyTypes}::Document::Null", + "SmithyTypes" to types, + ) + + is BooleanNode -> rustTemplate("""#{SmithyTypes}::Document::Bool(${node.value})""", "SmithyTypes" to types) + is StringNode -> rustTemplate("#{SmithyTypes}::Document::String(String::from(${node.value.dq()}))", "SmithyTypes" to types) + is NumberNode -> { + val value = node.value.toString() + val variant = when (node.value) { + is Float, is Double -> "Float" + else -> if (node.value.toLong() >= 0) "PosInt" else "NegInt" + } + rustTemplate( + "#{SmithyTypes}::Document::Number(#{SmithyTypes}::Number::$variant($value))", + "SmithyTypes" to types, + ) + } + + is ArrayNode -> { + check(node.isEmpty) + rustTemplate("""#{SmithyTypes}::Document::Array(Vec::new())""", "SmithyTypes" to types) + } + + is ObjectNode -> { + check(node.isEmpty) + rustTemplate("#{SmithyTypes}::Document::Object(std::collections::HashMap::new())", "SmithyTypes" to types) + } + + else -> throw unsupportedDefaultValueException + } + } + + is BlobShape -> rust("Default::default()") + + else -> throw unsupportedDefaultValueException + } +} From 9eb6a72b937412e4133df8715b1363ef301af73b Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 22 Dec 2022 19:45:06 +0100 Subject: [PATCH 07/11] Big refactor --- .../generators/ServerBuilderGenerator.kt | 370 +----------------- .../ServerBuilderGeneratorCommon.kt | 217 ++++++++++ ...rGeneratorWithoutPublicConstrainedTypes.kt | 31 +- 3 files changed, 226 insertions(+), 392 deletions(-) create mode 100644 codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index 6fbdeaf03e..aca84bf94c 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -5,42 +5,16 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators -import software.amazon.smithy.codegen.core.CodegenException import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolProvider import software.amazon.smithy.model.Model -import software.amazon.smithy.model.node.ArrayNode -import software.amazon.smithy.model.node.BooleanNode -import software.amazon.smithy.model.node.NullNode -import software.amazon.smithy.model.node.NumberNode -import software.amazon.smithy.model.node.ObjectNode -import software.amazon.smithy.model.node.StringNode -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.DocumentShape -import software.amazon.smithy.model.shapes.DoubleShape -import software.amazon.smithy.model.shapes.EnumShape -import software.amazon.smithy.model.shapes.FloatShape -import software.amazon.smithy.model.shapes.IntEnumShape -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.MemberShape -import software.amazon.smithy.model.shapes.NumberShape -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 import software.amazon.smithy.model.shapes.UnionShape -import software.amazon.smithy.model.traits.DefaultTrait -import software.amazon.smithy.model.traits.EnumDefinition import software.amazon.smithy.rust.codegen.core.rustlang.Attribute import software.amazon.smithy.rust.codegen.core.rustlang.RustType import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter import software.amazon.smithy.rust.codegen.core.rustlang.Visibility -import software.amazon.smithy.rust.codegen.core.rustlang.Writable import software.amazon.smithy.rust.codegen.core.rustlang.conditionalBlock import software.amazon.smithy.rust.codegen.core.rustlang.deprecatedShape import software.amazon.smithy.rust.codegen.core.rustlang.docs @@ -53,10 +27,7 @@ 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.stripOuter 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.RuntimeConfig import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType -import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata import software.amazon.smithy.rust.codegen.core.smithy.isOptional import software.amazon.smithy.rust.codegen.core.smithy.isRustBoxed @@ -67,15 +38,11 @@ import software.amazon.smithy.rust.codegen.core.smithy.mapRustType import software.amazon.smithy.rust.codegen.core.smithy.module import software.amazon.smithy.rust.codegen.core.smithy.rustType import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait -import software.amazon.smithy.rust.codegen.core.util.UNREACHABLE import software.amazon.smithy.rust.codegen.core.util.dq -import software.amazon.smithy.rust.codegen.core.util.expectTrait import software.amazon.smithy.rust.codegen.core.util.hasTrait -import software.amazon.smithy.rust.codegen.core.util.isStreaming import software.amazon.smithy.rust.codegen.core.util.letIf import software.amazon.smithy.rust.codegen.core.util.redactIfNecessary import software.amazon.smithy.rust.codegen.core.util.toSnakeCase -import software.amazon.smithy.rust.codegen.server.smithy.ServerCargoDependency import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape @@ -556,7 +523,7 @@ class ServerBuilderGenerator( if (member.hasNonNullDefault()) { // 2a. If a `@default` value is modeled and the user did not set a value, fall back to using the // default value. - fallbackToDefaultValue(this, member) + generateFallbackCodeToDefaultValue(this, member, model, runtimeConfig, symbolProvider) } else { // 2b. If the member is `@required` and has no `@default` value, the user must set a value; // otherwise, we fail with a `ConstraintViolation::Missing*` variant. @@ -570,106 +537,6 @@ class ServerBuilderGenerator( } } - private fun fallbackToDefaultValue(writer: RustWriter, member: MemberShape) { -// TODO Remove -// if (member.hasNonNullDefault()) { -// rustTemplate( -// "#{Default:W}", -// "Default" to renderDefaultBuilder( -// model, -// runtimeConfig, -// symbolProvider, -// member, -// ) { -// if (member.isStreaming(model)) { -// ".unwrap_or_default()" -// } else { -// ".unwrap_or($it)" -// } -// }, -// ) -// } -// } else if (member.hasNonNullDefault()) { -// rustTemplate( -// "#{Default:W}", -// "Default" to renderDefaultBuilder( -// model, -// runtimeConfig, -// symbolProvider, -// member, -// wrapDefault, -// ), -// ) -// } -// if (!isBuilderFallible) { -// // Unwrap the `Option`. -// // TODO This should be expect. -// rust(".unwrap()") -// } -// -// val wrapDefault: (String) -> String = { -// if (member.isStreaming(model)) { -// ".unwrap_or_default()" -// } else { -// val targetShape = model.expectShape(member.target) -// if (targetShape !is EnumShape && member.canReachConstrainedShape(model, symbolProvider)) { -// // TODO: Instead of panicking here, which will ungracefully shut down the service, potentially -// // causing data corruption, perform the `try_into()` check _once_ at service startup time, perhaps -// // storing the result in a `OnceCell` that could be reused. -// """ -// .unwrap_or_else(|| -// $it.try_into().expect("this check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") -// ) -// """ -// } else { -// val unwrapOr = when (model.expectShape(member.target)) { -// is NumberShape, is EnumShape, is BooleanShape -> ".unwrap_or(" -// else -> ".unwrap_or_else(||" -// } -// "$unwrapOr $it)" -// } -// } -// } - - val defaultValue = renderDefaultValue(model, runtimeConfig, symbolProvider, member) - val targetShape = model.expectShape(member.target) - - if (member.isStreaming(model)) { - writer.rust(".unwrap_or_default()") - } else if (targetShape !is EnumShape && member.canReachConstrainedShape(model, symbolProvider)) { - // TODO: Instead of panicking here, which will ungracefully shut down the service, potentially - // causing data corruption, perform the `try_into()` check _once_ at service startup time, perhaps - // storing the result in a `OnceCell` that could be reused. - writer.rustTemplate( - """ - .unwrap_or_else(|| - #{DefaultValue:W} - .try_into() - .expect("this check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") - ) - """, - "DefaultValue" to defaultValue, - ) - } else { - when (targetShape) { - is NumberShape, is EnumShape, is BooleanShape -> { - writer.rustTemplate(".unwrap_or(#{DefaultValue:W})", "DefaultValue" to defaultValue) - } - // Values for the Rust types of the rest of the shapes require heap allocations, so we calculate them - // in a (lazily-executed) closure for slight performance gains. - else -> { - writer.rustTemplate(".unwrap_or_else(|| #{DefaultValue:W})", "DefaultValue" to defaultValue) - } - } - } - - // TODO Remove -// writer.rustTemplate( -// "#{Default:W}", -// "Default" to renderDefaultBuilder(model, runtimeConfig, symbolProvider, member, wrapDefault), -// ) - } - private fun enforceConstraints(writer: RustWriter, member: MemberShape, constraintViolation: ConstraintViolation) { // This member is constrained. Enforce the constraint traits on the value set in the builder. // The code is slightly different in case the member is recursive, since it will be wrapped in @@ -722,238 +589,3 @@ class ServerBuilderGenerator( } } } - -fun buildFnReturnType(isBuilderFallible: Boolean, structureSymbol: Symbol) = writable { - if (isBuilderFallible) { - rust("Result<#T, ConstraintViolation>", structureSymbol) - } else { - rust("#T", structureSymbol) - } -} - -// TODO Remove -fun renderDefaultBuilder( - model: Model, - runtimeConfig: RuntimeConfig, - symbolProvider: RustSymbolProvider, - member: MemberShape, - wrap: (s: String) -> String = { it }, -): Writable { - return writable { - val node = member.expectTrait().toNode()!! - val name = member.memberName - val types = ServerCargoDependency.smithyTypes(runtimeConfig).toType() - when (val target = model.expectShape(member.target)) { - is EnumShape, is IntEnumShape -> { - val value = when (target) { - is IntEnumShape -> node.expectNumberNode().value - is EnumShape -> node.expectStringNode().value - else -> throw CodegenException("Default value for shape ${target.id} must be of EnumShape or IntEnumShape") - } - val enumValues = when (target) { - is IntEnumShape -> target.enumValues - is EnumShape -> target.enumValues - else -> UNREACHABLE( - "Target shape ${target.id} must be an `EnumShape` or an `IntEnumShape` at this point, otherwise it would have failed above", - ) - } - val variant = enumValues - .entries - .filter { entry -> entry.value == value } - .map { entry -> - symbolProvider.toEnumVariantName( - EnumDefinition.builder().name(entry.key).value(entry.value.toString()).build(), - )!! - } - .first() - val symbol = symbolProvider.toSymbol(target) - val result = "$symbol::${variant.name}" - rust(wrap(result)) - } - - is ByteShape -> rust(wrap(node.expectNumberNode().value.toString() + "i8")) - is ShortShape -> rust(wrap(node.expectNumberNode().value.toString() + "i16")) - is IntegerShape -> rust(wrap(node.expectNumberNode().value.toString() + "i32")) - is LongShape -> rust(wrap(node.expectNumberNode().value.toString() + "i64")) - is FloatShape -> rust(wrap(node.expectNumberNode().value.toFloat().toString() + "f32")) - is DoubleShape -> rust(wrap(node.expectNumberNode().value.toDouble().toString() + "f64")) - is BooleanShape -> rust(wrap(node.expectBooleanNode().value.toString())) - is StringShape -> rust(wrap("String::from(${node.expectStringNode().value.dq()})")) - is TimestampShape -> when (node) { - is NumberNode -> rust(wrap(node.expectNumberNode().value.toString())) - is StringNode -> { - val value = node.expectStringNode().value - rustTemplate( - wrap( - """ - #{SmithyTypes}::DateTime::from_str("$value", #{SmithyTypes}::date_time::Format::DateTime) - .expect("default value `$value` cannot be parsed into a valid date time; please file a bug report under https://github.com/awslabs/smithy-rs/issues")""", - ), - "SmithyTypes" to types, - ) - } - - else -> throw CodegenException("Default value for $name is unsupported") - } - - is ListShape -> { - check(node is ArrayNode && node.isEmpty) - rust(wrap("Vec::new()")) - } - - is MapShape -> { - check(node is ObjectNode && node.isEmpty) - rust(wrap("std::collections::HashMap::new()")) - } - - is DocumentShape -> { - when (node) { - is NullNode -> rustTemplate( - "#{SmithyTypes}::Document::Null", - "SmithyTypes" to types, - ) - - is BooleanNode -> rustTemplate(wrap("""#{SmithyTypes}::Document::Bool(${node.value})"""), "SmithyTypes" to types) - is StringNode -> rustTemplate(wrap("#{SmithyTypes}::Document::String(String::from(${node.value.dq()}))"), "SmithyTypes" to types) - is NumberNode -> { - val value = node.value.toString() - val variant = when (node.value) { - is Float, is Double -> "Float" - else -> if (node.value.toLong() >= 0) "PosInt" else "NegInt" - } - rustTemplate( - wrap( - "#{SmithyTypes}::Document::Number(#{SmithyTypes}::Number::$variant($value))", - ), - "SmithyTypes" to types, - ) - } - - is ArrayNode -> { - check(node.isEmpty) - rustTemplate(wrap("""#{SmithyTypes}::Document::Array(Vec::new())"""), "SmithyTypes" to types) - } - - is ObjectNode -> { - check(node.isEmpty) - rustTemplate(wrap("#{SmithyTypes}::Document::Object(std::collections::HashMap::new())"), "SmithyTypes" to types) - } - - else -> throw CodegenException("Default value $node for member shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") - } - } - - is BlobShape -> rust(wrap("Default::default()")) - - else -> throw CodegenException("Default value for shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") - } - } -} - -fun renderDefaultValue( - model: Model, - runtimeConfig: RuntimeConfig, - symbolProvider: RustSymbolProvider, - member: MemberShape, -) = writable { - val node = member.expectTrait().toNode()!! - val types = ServerCargoDependency.smithyTypes(runtimeConfig).toType() - // Define the exception once for DRYness. - val unsupportedDefaultValueException = - CodegenException("Default value $node for member shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") - when (val target = model.expectShape(member.target)) { - is EnumShape, is IntEnumShape -> { - val value = when (target) { - is IntEnumShape -> node.expectNumberNode().value - is EnumShape -> node.expectStringNode().value - else -> throw CodegenException("Default value for shape ${target.id} must be of EnumShape or IntEnumShape") - } - val enumValues = when (target) { - is IntEnumShape -> target.enumValues - is EnumShape -> target.enumValues - else -> UNREACHABLE( - "Target shape ${target.id} must be an `EnumShape` or an `IntEnumShape` at this point, otherwise it would have failed above", - ) - } - val variant = enumValues - .entries - .filter { entry -> entry.value == value } - .map { entry -> - symbolProvider.toEnumVariantName( - EnumDefinition.builder().name(entry.key).value(entry.value.toString()).build(), - )!! - } - .first() - rust("#T::${variant.name}", symbolProvider.toSymbol(target)) - } - - is ByteShape -> rust(node.expectNumberNode().value.toString() + "i8") - is ShortShape -> rust(node.expectNumberNode().value.toString() + "i16") - is IntegerShape -> rust(node.expectNumberNode().value.toString() + "i32") - is LongShape -> rust(node.expectNumberNode().value.toString() + "i64") - is FloatShape -> rust(node.expectNumberNode().value.toFloat().toString() + "f32") - is DoubleShape -> rust(node.expectNumberNode().value.toDouble().toString() + "f64") - is BooleanShape -> rust(node.expectBooleanNode().value.toString()) - is StringShape -> rust("String::from(${node.expectStringNode().value.dq()})") - is TimestampShape -> when (node) { - is NumberNode -> rust(node.expectNumberNode().value.toString()) - is StringNode -> { - val value = node.expectStringNode().value - rustTemplate( - """ - #{SmithyTypes}::DateTime::from_str("$value", #{SmithyTypes}::date_time::Format::DateTime) - .expect("default value `$value` cannot be parsed into a valid date time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") - """, - "SmithyTypes" to types, - ) - } - else -> throw unsupportedDefaultValueException - } - is ListShape -> { - check(node is ArrayNode && node.isEmpty) - rust("Vec::new()") - } - is MapShape -> { - check(node is ObjectNode && node.isEmpty) - rust("std::collections::HashMap::new()") - } - is DocumentShape -> { - when (node) { - is NullNode -> rustTemplate( - "#{SmithyTypes}::Document::Null", - "SmithyTypes" to types, - ) - - is BooleanNode -> rustTemplate("""#{SmithyTypes}::Document::Bool(${node.value})""", "SmithyTypes" to types) - is StringNode -> rustTemplate("#{SmithyTypes}::Document::String(String::from(${node.value.dq()}))", "SmithyTypes" to types) - is NumberNode -> { - val value = node.value.toString() - val variant = when (node.value) { - is Float, is Double -> "Float" - else -> if (node.value.toLong() >= 0) "PosInt" else "NegInt" - } - rustTemplate( - "#{SmithyTypes}::Document::Number(#{SmithyTypes}::Number::$variant($value))", - "SmithyTypes" to types, - ) - } - - is ArrayNode -> { - check(node.isEmpty) - rustTemplate("""#{SmithyTypes}::Document::Array(Vec::new())""", "SmithyTypes" to types) - } - - is ObjectNode -> { - check(node.isEmpty) - rustTemplate("#{SmithyTypes}::Document::Object(std::collections::HashMap::new())", "SmithyTypes" to types) - } - - else -> throw unsupportedDefaultValueException - } - } - - is BlobShape -> rust("Default::default()") - - else -> throw unsupportedDefaultValueException - } -} diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt new file mode 100644 index 0000000000..ad4fe2445b --- /dev/null +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt @@ -0,0 +1,217 @@ +package software.amazon.smithy.rust.codegen.server.smithy.generators + +import software.amazon.smithy.codegen.core.CodegenException +import software.amazon.smithy.codegen.core.Symbol +import software.amazon.smithy.model.Model +import software.amazon.smithy.model.node.ArrayNode +import software.amazon.smithy.model.node.BooleanNode +import software.amazon.smithy.model.node.NullNode +import software.amazon.smithy.model.node.NumberNode +import software.amazon.smithy.model.node.ObjectNode +import software.amazon.smithy.model.node.StringNode +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.DocumentShape +import software.amazon.smithy.model.shapes.DoubleShape +import software.amazon.smithy.model.shapes.EnumShape +import software.amazon.smithy.model.shapes.FloatShape +import software.amazon.smithy.model.shapes.IntEnumShape +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.MemberShape +import software.amazon.smithy.model.shapes.NumberShape +import software.amazon.smithy.model.shapes.ShortShape +import software.amazon.smithy.model.shapes.StringShape +import software.amazon.smithy.model.shapes.TimestampShape +import software.amazon.smithy.model.traits.DefaultTrait +import software.amazon.smithy.model.traits.EnumDefinition +import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter +import software.amazon.smithy.rust.codegen.core.rustlang.rust +import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate +import software.amazon.smithy.rust.codegen.core.rustlang.writable +import software.amazon.smithy.rust.codegen.core.smithy.RuntimeConfig +import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider +import software.amazon.smithy.rust.codegen.core.util.UNREACHABLE +import software.amazon.smithy.rust.codegen.core.util.dq +import software.amazon.smithy.rust.codegen.core.util.expectTrait +import software.amazon.smithy.rust.codegen.core.util.isStreaming +import software.amazon.smithy.rust.codegen.server.smithy.ServerCargoDependency +import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape + +/** + * Some common freestanding functions shared across: + * - [ServerBuilderGenerator]; and + * - [ServerBuilderGeneratorWithoutPublicConstrainedTypes], + * to keep them DRY and consistent. + */ + +/** + * Returns a writable to render the return type of the server builders' `build()` method. + */ +fun buildFnReturnType(isBuilderFallible: Boolean, structureSymbol: Symbol) = writable { + if (isBuilderFallible) { + rust("Result<#T, ConstraintViolation>", structureSymbol) + } else { + rust("#T", structureSymbol) + } +} + +/** + * Renders code to fallback to the modeled `@default` value on a [member] shape. + * The code is expected to be interpolated right after a value of type `Option`, where `T` is the type of the + * default value. + */ +fun generateFallbackCodeToDefaultValue( + writer: RustWriter, + member: MemberShape, + model: Model, + runtimeConfig: RuntimeConfig, + symbolProvider: RustSymbolProvider, +) { + val defaultValue = defaultValue(model, runtimeConfig, symbolProvider, member) + val targetShape = model.expectShape(member.target) + + if (member.isStreaming(model)) { + writer.rust(".unwrap_or_default()") + } else if (targetShape !is EnumShape && member.canReachConstrainedShape(model, symbolProvider)) { + // TODO: Instead of panicking here, which will ungracefully shut down the service, potentially + // causing data corruption, perform the `try_into()` check _once_ at service startup time, perhaps + // storing the result in a `OnceCell` that could be reused. + writer.rustTemplate( + """ + .unwrap_or_else(|| + #{DefaultValue:W} + .try_into() + .expect("this check should have failed at generation time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") + ) + """, + "DefaultValue" to defaultValue, + ) + } else { + when (targetShape) { + is NumberShape, is EnumShape, is BooleanShape -> { + writer.rustTemplate(".unwrap_or(#{DefaultValue:W})", "DefaultValue" to defaultValue) + } + // Values for the Rust types of the rest of the shapes require heap allocations, so we calculate them + // in a (lazily-executed) closure for slight performance gains. + else -> { + writer.rustTemplate(".unwrap_or_else(|| #{DefaultValue:W})", "DefaultValue" to defaultValue) + } + } + } +} + +/** + * Returns a writable to construct a Rust value of the correct type holding the modeled `@default` value on the + * [member] shape. + */ +fun defaultValue( + model: Model, + runtimeConfig: RuntimeConfig, + symbolProvider: RustSymbolProvider, + member: MemberShape, +) = writable { + val node = member.expectTrait().toNode()!! + val types = ServerCargoDependency.smithyTypes(runtimeConfig).toType() + // Define the exception once for DRYness. + val unsupportedDefaultValueException = + CodegenException("Default value $node for member shape ${member.id} is unsupported or cannot exist; please file a bug report under https://github.com/awslabs/smithy-rs/issues") + when (val target = model.expectShape(member.target)) { + is EnumShape, is IntEnumShape -> { + val value = when (target) { + is IntEnumShape -> node.expectNumberNode().value + is EnumShape -> node.expectStringNode().value + else -> throw CodegenException("Default value for shape ${target.id} must be of EnumShape or IntEnumShape") + } + val enumValues = when (target) { + is IntEnumShape -> target.enumValues + is EnumShape -> target.enumValues + else -> UNREACHABLE( + "Target shape ${target.id} must be an `EnumShape` or an `IntEnumShape` at this point, otherwise it would have failed above", + ) + } + val variant = enumValues + .entries + .filter { entry -> entry.value == value } + .map { entry -> + symbolProvider.toEnumVariantName( + EnumDefinition.builder().name(entry.key).value(entry.value.toString()).build(), + )!! + } + .first() + rust("#T::${variant.name}", symbolProvider.toSymbol(target)) + } + + is ByteShape -> rust(node.expectNumberNode().value.toString() + "i8") + is ShortShape -> rust(node.expectNumberNode().value.toString() + "i16") + is IntegerShape -> rust(node.expectNumberNode().value.toString() + "i32") + is LongShape -> rust(node.expectNumberNode().value.toString() + "i64") + is FloatShape -> rust(node.expectNumberNode().value.toFloat().toString() + "f32") + is DoubleShape -> rust(node.expectNumberNode().value.toDouble().toString() + "f64") + is BooleanShape -> rust(node.expectBooleanNode().value.toString()) + is StringShape -> rust("String::from(${node.expectStringNode().value.dq()})") + is TimestampShape -> when (node) { + is NumberNode -> rust(node.expectNumberNode().value.toString()) + is StringNode -> { + val value = node.expectStringNode().value + rustTemplate( + """ + #{SmithyTypes}::DateTime::from_str("$value", #{SmithyTypes}::date_time::Format::DateTime) + .expect("default value `$value` cannot be parsed into a valid date time; please file a bug report under https://github.com/awslabs/smithy-rs/issues") + """, + "SmithyTypes" to types, + ) + } + else -> throw unsupportedDefaultValueException + } + is ListShape -> { + check(node is ArrayNode && node.isEmpty) + rust("Vec::new()") + } + is MapShape -> { + check(node is ObjectNode && node.isEmpty) + rust("std::collections::HashMap::new()") + } + is DocumentShape -> { + when (node) { + is NullNode -> rustTemplate( + "#{SmithyTypes}::Document::Null", + "SmithyTypes" to types, + ) + + is BooleanNode -> rustTemplate("""#{SmithyTypes}::Document::Bool(${node.value})""", "SmithyTypes" to types) + is StringNode -> rustTemplate("#{SmithyTypes}::Document::String(String::from(${node.value.dq()}))", "SmithyTypes" to types) + is NumberNode -> { + val value = node.value.toString() + val variant = when (node.value) { + is Float, is Double -> "Float" + else -> if (node.value.toLong() >= 0) "PosInt" else "NegInt" + } + rustTemplate( + "#{SmithyTypes}::Document::Number(#{SmithyTypes}::Number::$variant($value))", + "SmithyTypes" to types, + ) + } + + is ArrayNode -> { + check(node.isEmpty) + rustTemplate("""#{SmithyTypes}::Document::Array(Vec::new())""", "SmithyTypes" to types) + } + + is ObjectNode -> { + check(node.isEmpty) + rustTemplate("#{SmithyTypes}::Document::Object(std::collections::HashMap::new())", "SmithyTypes" to types) + } + + else -> throw unsupportedDefaultValueException + } + } + + is BlobShape -> rust("Default::default()") + + else -> throw unsupportedDefaultValueException + } +} diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt index b204f79080..6a388193b1 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt @@ -7,10 +7,7 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolProvider -import software.amazon.smithy.model.shapes.BooleanShape -import software.amazon.smithy.model.shapes.EnumShape import software.amazon.smithy.model.shapes.MemberShape -import software.amazon.smithy.model.shapes.NumberShape import software.amazon.smithy.model.shapes.StructureShape import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter import software.amazon.smithy.rust.codegen.core.rustlang.Visibility @@ -28,7 +25,6 @@ import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata import software.amazon.smithy.rust.codegen.core.smithy.isOptional import software.amazon.smithy.rust.codegen.core.smithy.makeOptional import software.amazon.smithy.rust.codegen.core.smithy.module -import software.amazon.smithy.rust.codegen.core.util.isStreaming import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType @@ -169,27 +165,16 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( withBlock("$memberName: self.$memberName", ",") { if (member.hasNonNullDefault()) { - if (member.isStreaming(model)) { - rustTemplate(".unwrap_or_default()") - } else { - val unwrapOr = when (model.expectShape(member.target)) { - is NumberShape, is EnumShape, is BooleanShape -> ".unwrap_or(" - else -> ".unwrap_or_else(||" - } - rustTemplate( - "#{Default:W}", - "Default" to renderDefaultBuilder( - model, - runtimeConfig, - symbolProvider, - member, - ) { "$unwrapOr $it)" }, - ) + // 1a. If a `@default` value is modeled and the user did not set a value, fall back to using the + // default value. + generateFallbackCodeToDefaultValue(this, member, model, runtimeConfig, symbolProvider) + } else { + // 1b. If the member is `@required` and has no `@default` value, the user must set a value; + // otherwise, we fail with a `ConstraintViolation::Missing*` variant. + serverBuilderConstraintViolations.forMember(member)?.also { + rust(".ok_or(ConstraintViolation::${it.name()})?") } } - serverBuilderConstraintViolations.forMember(member)?.also { - rust(".ok_or(ConstraintViolation::${it.name()})?") - } } } } From a14086856a364e291ed26685fdda0145962f45d0 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 22 Dec 2022 19:45:59 +0100 Subject: [PATCH 08/11] Revert simple.smithy --- codegen-core/common-test-models/simple.smithy | 131 +++++++++++++++--- 1 file changed, 114 insertions(+), 17 deletions(-) diff --git a/codegen-core/common-test-models/simple.smithy b/codegen-core/common-test-models/simple.smithy index 823a513b1d..43c4bc6aca 100644 --- a/codegen-core/common-test-models/simple.smithy +++ b/codegen-core/common-test-models/simple.smithy @@ -1,39 +1,136 @@ -$version: "2.0" +$version: "1.0" namespace com.amazonaws.simple use aws.protocols#restJson1 +use smithy.test#httpRequestTests +use smithy.test#httpResponseTests use smithy.framework#ValidationException @restJson1 +@title("SimpleService") +@documentation("A simple service example, with a Service resource that can be registered and a readonly healthcheck") service SimpleService { version: "2022-01-01", + resources: [ + Service, + ], operations: [ - MyOperation, + Healthcheck, + StoreServiceBlob, ], } -@http(method: "POST", uri: "/my-operation") -operation MyOperation { - input: MyOperationInputOutput, - output: MyOperationInputOutput, - errors: [ValidationException] +@documentation("Id of the service that will be registered") +string ServiceId + +@documentation("Name of the service that will be registered") +string ServiceName + +@error("client") +@documentation( + """ + Returned when a new resource cannot be created because one already exists. + """ +) +structure ResourceAlreadyExists { + @required + message: String +} + +@documentation("A resource that can register services") +resource Service { + identifiers: { id: ServiceId }, + put: RegisterService, } -structure MyOperationInputOutput { +@idempotent +@http(method: "PUT", uri: "/service/{id}") +@documentation("Service register operation") +@httpRequestTests([ + { + id: "RegisterServiceRequestTest", + protocol: "aws.protocols#restJson1", + uri: "/service/1", + headers: { + "Content-Type": "application/json", + }, + params: { id: "1", name: "TestService" }, + body: "{\"name\":\"TestService\"}", + method: "PUT", + } +]) +@httpResponseTests([ + { + id: "RegisterServiceResponseTest", + protocol: "aws.protocols#restJson1", + params: { id: "1", name: "TestService" }, + body: "{\"id\":\"1\",\"name\":\"TestService\"}", + code: 200, + headers: { + "Content-Length": "31" + } + } +]) +operation RegisterService { + input: RegisterServiceInputRequest, + output: RegisterServiceOutputResponse, + errors: [ResourceAlreadyExists, ValidationException] +} + +@documentation("Service register input structure") +structure RegisterServiceInputRequest { @required - defaultNullAndRequired: String = null + @httpLabel + id: ServiceId, + name: ServiceName, +} - @default("ab") - defaultLengthString: DefaultLengthString, +@documentation("Service register output structure") +structure RegisterServiceOutputResponse { + @required + id: ServiceId, + name: ServiceName, +} +@readonly +@http(uri: "/healthcheck", method: "GET") +@documentation("Read-only healthcheck operation") +operation Healthcheck { + input: HealthcheckInputRequest, + output: HealthcheckOutputResponse +} + +@documentation("Service healthcheck output structure") +structure HealthcheckInputRequest { + +} + +@documentation("Service healthcheck input structure") +structure HealthcheckOutputResponse { + +} + +@readonly +@http(method: "POST", uri: "/service/{id}/blob") +@documentation("Stores a blob for a service id") +operation StoreServiceBlob { + input: StoreServiceBlobInput, + output: StoreServiceBlobOutput, + errors: [ValidationException] +} + +@documentation("Store a blob for a service id input structure") +structure StoreServiceBlobInput { + @required + @httpLabel + id: ServiceId, @required - lengthString: LengthString, + @httpPayload + content: Blob, } -@length(min: 69) -string LengthString +@documentation("Store a blob for a service id output structure") +structure StoreServiceBlobOutput { -@length(max: 69) -@default("ab") -string DefaultLengthString +} From 92bb829b881788073970f5b7f7f08ce408b29e5a Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 22 Dec 2022 20:12:28 +0100 Subject: [PATCH 09/11] ./gradlew ktlintFormat --- .../smithy/generators/ServerBuilderGenerator.kt | 1 - .../generators/ServerBuilderDefaultValuesTest.kt | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index aca84bf94c..8890bce146 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -531,7 +531,6 @@ class ServerBuilderGenerator( rust(".ok_or(ConstraintViolation::${it.name()})?") } } - } } } diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt index 3e0469ba10..d923d0a0c7 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt @@ -93,7 +93,7 @@ class ServerBuilderDefaultValuesTest { builderGeneratorKind: BuilderGeneratorKind, assertValues: Map, ) { - println("Running test with params = requiredTrait: $requiredTrait, nullDefault: $nullDefault, applyDefaultValues: $applyDefaultValues, builderGeneratorKind: $builderGeneratorKind, assertValues: $assertValues"); + println("Running test with params = requiredTrait: $requiredTrait, nullDefault: $nullDefault, applyDefaultValues: $applyDefaultValues, builderGeneratorKind: $builderGeneratorKind, assertValues: $assertValues") val initialSetValues = this.defaultValues.mapValues { if (nullDefault) null else it.value } val model = generateModel(requiredTrait, applyDefaultValues, nullDefault, initialSetValues) val symbolProvider = serverTestSymbolProvider(model) @@ -133,7 +133,7 @@ class ServerBuilderDefaultValuesTest { applyDefaultValues, nullDefault, requiredTrait, - applyDefaultValues + applyDefaultValues, ), "Setters" to setters, ) @@ -345,11 +345,11 @@ class ServerBuilderDefaultValuesTest { TestConfig(defaultValues, requiredTrait = true, nullDefault = false, applyDefaultValues = true), TestConfig(customValues, requiredTrait = true, nullDefault = false, applyDefaultValues = true), - ).flatMap { (assertValues, requiredTrait, nullDefault, applyDefaultValues) -> - builderGeneratorKindList.stream().map { builderGeneratorKind -> - Arguments.of(requiredTrait, nullDefault, applyDefaultValues, builderGeneratorKind, assertValues) - } + ).flatMap { (assertValues, requiredTrait, nullDefault, applyDefaultValues) -> + builderGeneratorKindList.stream().map { builderGeneratorKind -> + Arguments.of(requiredTrait, nullDefault, applyDefaultValues, builderGeneratorKind, assertValues) } + } } data class TestConfig( From ac9f4e57ad89ab5099757168d9049b53f0adbec5 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 22 Dec 2022 20:55:12 +0100 Subject: [PATCH 10/11] Fix constraints_without_public_constrained_types clippy, ktlint --- .../smithy/generators/ServerBuilderGenerator.kt | 9 ++++++++- .../generators/ServerBuilderGeneratorCommon.kt | 11 ++++++----- ...uilderGeneratorWithoutPublicConstrainedTypes.kt | 14 ++++++++++++-- .../generators/ServerBuilderDefaultValuesTest.kt | 1 - 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt index 8890bce146..d426717804 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt @@ -523,7 +523,14 @@ class ServerBuilderGenerator( if (member.hasNonNullDefault()) { // 2a. If a `@default` value is modeled and the user did not set a value, fall back to using the // default value. - generateFallbackCodeToDefaultValue(this, member, model, runtimeConfig, symbolProvider) + generateFallbackCodeToDefaultValue( + this, + member, + model, + runtimeConfig, + symbolProvider, + publicConstrainedTypes, + ) } else { // 2b. If the member is `@required` and has no `@default` value, the user must set a value; // otherwise, we fail with a `ConstraintViolation::Missing*` variant. diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt index ad4fe2445b..337db86cbb 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt @@ -39,7 +39,7 @@ import software.amazon.smithy.rust.codegen.core.util.dq import software.amazon.smithy.rust.codegen.core.util.expectTrait import software.amazon.smithy.rust.codegen.core.util.isStreaming import software.amazon.smithy.rust.codegen.server.smithy.ServerCargoDependency -import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape +import software.amazon.smithy.rust.codegen.server.smithy.hasPublicConstrainedWrapperTupleType /** * Some common freestanding functions shared across: @@ -60,7 +60,7 @@ fun buildFnReturnType(isBuilderFallible: Boolean, structureSymbol: Symbol) = wri } /** - * Renders code to fallback to the modeled `@default` value on a [member] shape. + * Renders code to fall back to the modeled `@default` value on a [member] shape. * The code is expected to be interpolated right after a value of type `Option`, where `T` is the type of the * default value. */ @@ -70,15 +70,16 @@ fun generateFallbackCodeToDefaultValue( model: Model, runtimeConfig: RuntimeConfig, symbolProvider: RustSymbolProvider, + publicConstrainedTypes: Boolean, ) { val defaultValue = defaultValue(model, runtimeConfig, symbolProvider, member) val targetShape = model.expectShape(member.target) if (member.isStreaming(model)) { writer.rust(".unwrap_or_default()") - } else if (targetShape !is EnumShape && member.canReachConstrainedShape(model, symbolProvider)) { - // TODO: Instead of panicking here, which will ungracefully shut down the service, potentially - // causing data corruption, perform the `try_into()` check _once_ at service startup time, perhaps + } else if (targetShape.hasPublicConstrainedWrapperTupleType(model, publicConstrainedTypes)) { + // TODO(https://github.com/awslabs/smithy-rs/issues/2134): Instead of panicking here, which will ungracefully + // shut down the service, perform the `try_into()` check _once_ at service startup time, perhaps // storing the result in a `OnceCell` that could be reused. writer.rustTemplate( """ diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt index 6a388193b1..3e03a9d1a9 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt @@ -42,7 +42,7 @@ import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType * when `publicConstrainedTypes` is false. */ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( - codegenContext: ServerCodegenContext, + private val codegenContext: ServerCodegenContext, shape: StructureShape, ) { companion object { @@ -88,6 +88,9 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( ) fun render(writer: RustWriter) { + check(!codegenContext.settings.codegenConfig.publicConstrainedTypes) { + "ServerBuilderGeneratorWithoutPublicConstrainedTypes should only be used when `publicConstrainedTypes` is false" + } writer.docs("See #D.", structureSymbol) writer.withInlineModule(builderSymbol.module()) { renderBuilder(this) @@ -167,7 +170,14 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes( if (member.hasNonNullDefault()) { // 1a. If a `@default` value is modeled and the user did not set a value, fall back to using the // default value. - generateFallbackCodeToDefaultValue(this, member, model, runtimeConfig, symbolProvider) + generateFallbackCodeToDefaultValue( + this, + member, + model, + runtimeConfig, + symbolProvider, + codegenContext.settings.codegenConfig.publicConstrainedTypes, + ) } else { // 1b. If the member is `@required` and has no `@default` value, the user must set a value; // otherwise, we fail with a `ConstraintViolation::Missing*` variant. diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt index d923d0a0c7..b6745e68d5 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt @@ -322,7 +322,6 @@ class ServerBuilderDefaultValuesTest { } private fun testParameters(): Stream { - // TODO Use cross-product. val builderGeneratorKindList = listOf( BuilderGeneratorKind.SERVER_BUILDER_GENERATOR, BuilderGeneratorKind.SERVER_BUILDER_GENERATOR_WITHOUT_PUBLIC_CONSTRAINED_TYPES, From 1f9ebc249a1acb7f43e8e7cb6eecfaf4091e0d88 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 22 Dec 2022 21:21:50 +0100 Subject: [PATCH 11/11] Fix ServerBuilderDefaultValuesTest, copyright header --- .../smithy/generators/ServerBuilderGeneratorCommon.kt | 5 +++++ .../smithy/generators/ServerBuilderDefaultValuesTest.kt | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt index 337db86cbb..56014b6d99 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt @@ -1,3 +1,8 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + package software.amazon.smithy.rust.codegen.server.smithy.generators import software.amazon.smithy.codegen.core.CodegenException diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt index b6745e68d5..2219c2e653 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderDefaultValuesTest.kt @@ -30,7 +30,9 @@ import software.amazon.smithy.rust.codegen.core.util.dq import software.amazon.smithy.rust.codegen.core.util.lookup import software.amazon.smithy.rust.codegen.core.util.toPascalCase import software.amazon.smithy.rust.codegen.core.util.toSnakeCase +import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenConfig import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestCodegenContext +import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestRustSettings import software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestSymbolProvider import java.util.stream.Stream @@ -167,7 +169,12 @@ class ServerBuilderDefaultValuesTest { private fun writeServerBuilderGeneratorWithoutPublicConstrainedTypes(writer: RustWriter, model: Model, symbolProvider: RustSymbolProvider) { val struct = model.lookup("com.test#MyStruct") - val codegenContext = serverTestCodegenContext(model) + val codegenContext = serverTestCodegenContext( + model, + settings = serverTestRustSettings( + codegenConfig = ServerCodegenConfig(publicConstrainedTypes = false), + ), + ) val builderGenerator = ServerBuilderGeneratorWithoutPublicConstrainedTypes(codegenContext, struct) writer.implBlock(struct, symbolProvider) {