Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support @default #1879

Merged
merged 12 commits into from
Dec 23, 2022
Merged

Support @default #1879

merged 12 commits into from
Dec 23, 2022

Conversation

82marbag
Copy link
Contributor

@82marbag 82marbag commented Oct 19, 2022

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.

@82marbag 82marbag requested review from a team as code owners October 19, 2022 16:13
@@ -132,3 +132,22 @@ structure StoreServiceBlobInput {
structure StoreServiceBlobOutput {

}

@http(method: "POST", uri: "/default-value")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
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()}))")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Recommend using unwrap_or_else to avoid unnecessary allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@@ -268,4 +271,8 @@ class BuilderGenerator(
}
}
}

open fun renderDefaultBuilder(writer: RustWriter, member: MemberShape) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 25, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 25, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 25, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 26, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 26, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Oct 26, 2022
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -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";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. Implement presence tracking in this PR.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Nov 3, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Nov 4, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

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.

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

}

@http(method: "POST", uri: "/default-value")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

if (!codegenContext.settings.codegenConfig.publicConstrainedTypes) {
val serverBuilderGeneratorWithoutPublicConstrainedTypes =
ServerBuilderGeneratorWithoutPublicConstrainedTypes(codegenContext, shape)
ServerBuilderGeneratorWithoutPublicConstrainedTypes(codegenContext, shape, ::renderDefaultBuilder)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

We also need unit tests that exercise this builder type.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

private fun localParameters(): Stream<Arguments> {
val builderWriters = listOf(::writeServerBuilderGenerator, ::writeServerBuilderGeneratorWithoutPublicConstrainedTypes)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

setupFiles(this, model, symbolProvider)

val rustValues = setupValuesForTest(testConfig.values)
val setters = if (testConfig.requiredTrait) structSetters(rustValues) else writable { }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
@jdisanti jdisanti added the server Rust server SDK label Dec 20, 2022
82marbag and others added 2 commits December 21, 2022 15:34
Signed-off-by: Daniele Ahmed <[email protected]>
Co-authored-by: david-perez <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez
Copy link
Contributor

We need a changelog entry.

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

A new generated diff is ready to view.

A new doc preview is ready to view.

@82marbag 82marbag enabled auto-merge (squash) December 23, 2022 08:51
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@82marbag 82marbag merged commit 5363c41 into main Dec 23, 2022
@82marbag 82marbag deleted the 1860 branch December 23, 2022 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants