Support @default #1879

82marbag · 2022-10-19T16:13:15Z

Motivation and Context

The @default trait is not supported.

Description

Add support for the @default trait.

Issue: #1860
Reference for the trait:
https://awslabs.github.io/smithy/2.0/spec/type-refinement-traits.html

I have updated the simple model to use version 2. If it is better to split in two and keep the current at version 1, I will change this.

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

...c/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt

jdisanti · 2022-10-19T16:26:49Z

codegen-core/common-test-models/simple.smithy

@@ -132,3 +132,22 @@ structure StoreServiceBlobInput {
 structure StoreServiceBlobOutput {

 }
+
+@http(method: "POST", uri: "/default-value")


This should go into a Kotlin unit test for the BuilderGenerator so that it can be more thoroughly tested.

Agreed. @82marbag let's move this to a Kotlin unit test.

It needs to test @default on all supported shapes. And not only that compilation succeeds, but that when using the builders, the default value is used when none is provided.

jdisanti · 2022-10-19T16:27:21Z

...c/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt

+                                }
+                                is IntegerShape, is FloatShape -> rust(".unwrap_or(${node.expectNumberNode().value})")
+                                is BooleanShape -> rust(".unwrap_or(${node.expectBooleanNode().value})")
+                                is StringShape -> rust(".unwrap_or(String::from(${node.expectStringNode().value.dq()}))")


Recommend using unwrap_or_else to avoid unnecessary allocations.

github-actions · 2022-10-20T14:40:45Z

A new generated diff is ready to view.

A new doc preview is ready to view.

jdisanti · 2022-10-20T17:56:29Z

...c/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt

@@ -258,6 +260,7 @@ class BuilderGenerator(
                withBlock("$memberName: self.$memberName", ",") {
                    // Write the modifier
                    when {
+                        !memberSymbol.isOptional() && member.hasTrait<DefaultTrait>() -> renderDefaultBuilder(writer, member)


Thinking through this on the client side... Everything in an input is always optional (at present), so this won't set a value for a customer on input. So this would only set a default when a server didn't respond with a default value, which is the incorrect behavior for the server.

I think this should probably be an error case. These builders are fallible, right? If so, then this should definitely just emit an error. If they're not fallible, then perhaps we need more discussion.

My apologies, I was wrong on this. I talked to the Smithy folks, and the client should actually set a default value if a value is not present. If a value is given by the default trait, then it should use that. Otherwise, it should use the zero value.

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/TestHelpers.kt

jdisanti · 2022-10-20T18:06:27Z

...c/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt

@@ -268,4 +271,8 @@ class BuilderGenerator(
            }
        }
    }
+
+    open fun renderDefaultBuilder(writer: RustWriter, member: MemberShape) {


Can we do this with composition instead of inheritance by passing in a function for handling defaults as a constructor argument?

It seems like the implementation of what it does for clients should also live in codegen-client rather than being a default for codegen-core.

We'll have a ServerBuilderGenerator when builders of builders is merged. Do we still want to refactor this or can we merge this with the new generator (and add one for the client)?

@david-perez and @hlbarber - Can you weigh in on this? I don't think I have enough context to make this call.

I believe this is not relevant anymore since it's the same code for both client and server

This is no longer relevant. Now that the server has its own builders, we can place the implementation of default there until the client implements presence tracking.

github-actions · 2022-10-26T11:10:11Z

A new generated diff is ready to view.

A new doc preview is ready to view.

jdisanti · 2022-10-26T18:15:46Z

aws/sdk/integration-tests/ec2/tests/paginators.rs

@@ -19,7 +19,7 @@ fn stub_config() -> Config {
 /// EC2 replies with `<nextToken></nextToken>` which our XML parser parses as empty string and not "none"
 #[tokio::test]
 async fn paginators_handle_empty_tokens() {
-    let request= "Action=DescribeSpotPriceHistory&Version=2016-11-15&AvailabilityZone=eu-north-1a&InstanceType.1=g5.48xlarge&ProductDescription.1=Linux%2FUNIX";
+    let request= "Action=DescribeSpotPriceHistory&Version=2016-11-15&AvailabilityZone=eu-north-1a&DryRun=false&InstanceType.1=g5.48xlarge&MaxResults=0&ProductDescription.1=Linux%2FUNIX";


Apologies again, I should have been clearer. For clients, the default should be set for responses, but not for requests. From the spec:

To allow servers to change default values if necessary, clients SHOULD NOT serialize default values unless the member is explicitly set to the default value or marked with the default trait. This implies that clients SHOULD implement a kind of "presence tracking" of defaulted members so that the member is only serialized if it is explicitly set to the default value.

There shouldn't be any changes to these tests.

We need to implement presence tracking anyway for nullability support: #1767

I think you have a couple of options for moving forward:

Feature flag handling the default trait for clients and have it disabled for now until presence tracking is implemented. File an issue to track finishing it.

Implement presence tracking in this PR.

...c/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt

github-actions · 2022-10-31T12:37:19Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-02T15:37:11Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-03T17:10:59Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-04T13:11:37Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-21T17:12:16Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-22T11:33:46Z

A new generated diff is ready to view.

A new doc preview is ready to view.

david-perez

The implementation is a good start, but needs more work and we especially need a lot more testing to be confident that it is correct.

david-perez · 2022-11-24T14:03:25Z

codegen-core/common-test-models/simple.smithy

@@ -132,3 +132,22 @@ structure StoreServiceBlobInput {
 structure StoreServiceBlobOutput {

 }
+
+@http(method: "POST", uri: "/default-value")


Agreed. @82marbag let's move this to a Kotlin unit test.

It needs to test @default on all supported shapes. And not only that compilation succeeds, but that when using the builders, the default value is used when none is provided.

...c/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt

david-perez · 2022-11-24T14:06:06Z

...er/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCodegenVisitor.kt

@@ -275,7 +294,7 @@ open class ServerCodegenVisitor(

        if (!codegenContext.settings.codegenConfig.publicConstrainedTypes) {
            val serverBuilderGeneratorWithoutPublicConstrainedTypes =
-                ServerBuilderGeneratorWithoutPublicConstrainedTypes(codegenContext, shape)
+                ServerBuilderGeneratorWithoutPublicConstrainedTypes(codegenContext, shape, ::renderDefaultBuilder)


No need to pass in this function. The server builder generators should host it.

...er/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCodegenVisitor.kt

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt

david-perez · 2022-11-24T15:38:05Z

...rust/codegen/server/smithy/generators/ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt

@@ -160,6 +164,10 @@ class ServerBuilderGeneratorWithoutPublicConstrainedTypes(
                withBlock("$memberName: self.$memberName", ",") {
                    serverBuilderConstraintViolations.forMember(member)?.also {
                        rust(".ok_or(ConstraintViolation::${it.name()})?")
+                    } ?: run {


Same here, we need to inject the writable before constraint checking.

We also need unit tests that exercise this builder type.

.../software/amazon/smithy/rust/codegen/server/smithy/generators/UnconstrainedUnionGenerator.kt

...n/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt

github-actions · 2022-11-29T12:53:39Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-30T15:05:45Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-30T17:33:45Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-12-12T19:21:08Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-12-12T20:02:06Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-12-13T12:08:50Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-12-13T13:20:35Z

A new generated diff is ready to view.

A new doc preview is ready to view.

...are/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt

david-perez · 2022-12-14T11:19:02Z

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt

@@ -257,6 +290,9 @@ class ServerBuilderGenerator(
            "fn build_enforcing_all_constraints(self) -> #{ReturnType:W}",
            "ReturnType" to buildFnReturnType(isBuilderFallible, structureSymbol),
        ) {
+            if (members.mapNotNull { serverBuilderConstraintViolations.forDefaultMember(it) }.isNotEmpty()) {
+                Attribute.Custom("allow(clippy::useless_conversion)").render(this)


What model is Clippy complaining about if we don't opt out of this lint?

I made codegen a bit more complex to remove this and left a comment where needed

david-perez · 2022-12-14T12:24:03Z

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt

-                            .isRustBoxed()
-                        if (hasBox) {
-                            rustTemplate(
+                    val defaultWrap: (inner: String) -> String = { inner ->


With this model:

structure ConstrainedShapesOperationInputOutput { @default("ab") defaultLengthString: DefaultLengthString, @required lengthString: LengthString, }

This is generating:

fn build_enforcing_all_constraints( self, ) -> Result<crate::input::ConstrainedShapesOperationInput, ConstraintViolation> { Ok(crate::input::ConstrainedShapesOperationInput { default_length_string: self .default_length_string .map(Ok) .unwrap_or_else(|| { String::from("ab") .try_into() .map_err(|_| ConstraintViolation::InvalidDefaultForDefaultLengthString) }) .map(|v| match v { crate::constrained::MaybeConstrained::Constrained(x) => Ok(x), crate::constrained::MaybeConstrained::Unconstrained(x) => x.try_into(), }) .map(|res| res.map_err(ConstraintViolation::DefaultLengthString))??, length_string: self .length_string .map(|v| match v { crate::constrained::MaybeConstrained::Constrained(x) => Ok(x), crate::constrained::MaybeConstrained::Unconstrained(x) => x.try_into(), }) .map(|res| res.map_err(ConstraintViolation::LengthString)) .transpose()? .ok_or(ConstraintViolation::MissingLengthString)?, }) }

This technically works, but note how the default_length_string code is less efficient. It's artificially wrapping the Option in a Result via map(Ok) only to then unwrap it in the next line, to then wrap the Result in a Result that gets doubly unwrapped via ?? at the end!

I tried for a long time to rewrite this more efficiently but still using a functional approach (i.e. chaining method calls all the way through so that the codegen code is easily composable), but I couldn't get it to work.

The chain was getting long and complex to understand anyway, so here's what I came up with:

default_length_string: { let v = self.default_length_string.unwrap_or_else(|| { crate::constrained::MaybeConstrained::Unconstrained(String::from("ab")) }); match v { crate::constrained::MaybeConstrained::Constrained(x) => Ok(x), crate::constrained::MaybeConstrained::Unconstrained(x) => x.try_into(), } .map_err(ConstraintViolation::DefaultLengthString)? },

Not only is this more efficient and easier to understand, I think it generalizes better to the other cases. For instance, here's how length_string would look like:

length_string: { let v = self .length_string .ok_or(ConstraintViolation::MissingLengthString)?; match v { crate::constrained::MaybeConstrained::Constrained(x) => Ok(x), crate::constrained::MaybeConstrained::Unconstrained(x) => x.try_into(), } .map_err(ConstraintViolation::LengthString)? },

You also don't end up needing to special-case @default-handling like we're doing right now with the ??.

So, the general approach would be:

open a scope {} where we can have intermediate variable bindings.

check for @required or @default first.

enforce the rest of the constraint traits last.

Talked over this offline: we decided to revert back to the strategy of relying on java/kotlin checks (generation time) for constrained default values. The current path would lead to complexity and maintainability issues

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt

david-perez · 2022-12-14T12:43:17Z

...n/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt

+    }
+
+    private fun localParameters(): Stream<Arguments> {
+        val builderWriters = listOf(::writeServerBuilderGenerator, ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes)


Suggested change

val builderWriters = listOf(::writeServerBuilderGenerator, ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes)

val builderWriters = Stream.of(::writeServerBuilderGenerator, ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes)

You then don't need the stream() call below.

I can't because the stream is consumed after the first use, that's why it's constructed again

...n/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt

david-perez · 2022-12-14T13:22:56Z

...n/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt

+            setupFiles(this, model, symbolProvider)
+
+            val rustValues = setupValuesForTest(testConfig.values)
+            val setters = if (testConfig.requiredTrait) structSetters(rustValues) else writable { }


This seems odd. It's only setting the values on the builder when the members are @required. What we want is to not set them when the members are @default, and test that the built structure uses the modeled default values.

@required should have no bearing on our assertions / generated code. Note the spec says both @default and @required should generate non-Optional structure fields, so we're only testing that case out of an abundance of caution.

...n/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt

Signed-off-by: Daniele Ahmed <[email protected]>

Signed-off-by: Daniele Ahmed <[email protected]> Co-authored-by: david-perez <[email protected]>

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-12-21T15:07:09Z

A new generated diff is ready to view.

A new doc preview is ready to view.

david-perez · 2022-12-22T19:02:16Z

We need a changelog entry.

github-actions · 2022-12-22T20:36:47Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-12-23T09:05:23Z

A new generated diff is ready to view.

A new doc preview is ready to view.

82marbag requested review from a team as code owners October 19, 2022 16:13

jdisanti reviewed Oct 19, 2022

View reviewed changes

82marbag force-pushed the 1860 branch from 40d8625 to 5aa8b94 Compare October 20, 2022 14:13

smithy-lang deleted a comment from github-actions bot Oct 20, 2022

jdisanti reviewed Oct 20, 2022

View reviewed changes

smithy-lang deleted a comment from github-actions bot Oct 25, 2022

smithy-lang deleted a comment from github-actions bot Oct 26, 2022

jdisanti reviewed Oct 26, 2022

View reviewed changes

82marbag force-pushed the 1860 branch from 3771452 to a1825e1 Compare October 31, 2022 12:11

david-perez requested changes Nov 24, 2022

View reviewed changes

82marbag force-pushed the 1860 branch from 57c6ff5 to 605d33a Compare November 30, 2022 14:53

82marbag requested a review from a team as a code owner November 30, 2022 14:53

david-perez requested changes Dec 14, 2022

View reviewed changes

Default trait in server

77d5a51

Signed-off-by: Daniele Ahmed <[email protected]>

82marbag force-pushed the 1860 branch from 5aefa4d to 77d5a51 Compare December 20, 2022 16:26

Revert .smithy changes

6c6e863

Signed-off-by: Daniele Ahmed <[email protected]>

jdisanti added the server Rust server SDK label Dec 20, 2022

82marbag and others added 2 commits December 21, 2022 15:34

Fix fallible condition

31ed6c9

Signed-off-by: Daniele Ahmed <[email protected]> Co-authored-by: david-perez <[email protected]>

Fix clippy

1c4492c

Signed-off-by: Daniele Ahmed <[email protected]>

david-perez added 4 commits December 22, 2022 16:55

My minor nits

760390a

Changes in ServerBuilderGenerator pass tests

01b8957

Big refactor

9eb6a72

Revert simple.smithy

a140868

./gradlew ktlintFormat

92bb829

david-perez mentioned this pull request Dec 22, 2022

Convert @default values of constrained types only once #2134

Open

david-perez added 2 commits December 22, 2022 20:55

Fix constraints_without_public_constrained_types clippy, ktlint

ac9f4e5

Fix ServerBuilderDefaultValuesTest, copyright header

1f9ebc2

smithy-lang deleted a comment from github-actions bot Dec 22, 2022

david-perez approved these changes Dec 22, 2022

View reviewed changes

82marbag enabled auto-merge (squash) December 23, 2022 08:51

Merge branch 'main' into 1860

28855c9

82marbag merged commit 5363c41 into main Dec 23, 2022

82marbag deleted the 1860 branch December 23, 2022 09:20

	val builderWriters = listOf(::writeServerBuilderGenerator, ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes)
	val builderWriters = Stream.of(::writeServerBuilderGenerator, ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes)

Support @default #1879

Support @default #1879

Conversation

82marbag commented Oct 19, 2022 • edited Loading

Motivation and Context

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 31, 2022

github-actions bot commented Nov 2, 2022

github-actions bot commented Nov 3, 2022

github-actions bot commented Nov 4, 2022

github-actions bot commented Nov 21, 2022

github-actions bot commented Nov 22, 2022

david-perez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2022

github-actions bot commented Nov 30, 2022

github-actions bot commented Nov 30, 2022

github-actions bot commented Dec 12, 2022

github-actions bot commented Dec 12, 2022

github-actions bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2022

david-perez commented Dec 22, 2022

github-actions bot commented Dec 22, 2022

github-actions bot commented Dec 23, 2022

82marbag commented Oct 19, 2022 •

edited

Loading