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

Fix bug where string builtIns were not supported #2150

Merged
merged 1 commit into from Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.next.toml
Expand Up @@ -130,3 +130,9 @@ let sdk_config = aws_config::from_env()
.await;
/// ```
"""

[[smithy-rs]]
message = "Fix bug where string default values were not supported for endpoint parameters"
references = ["smithy-rs#2150"]
meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client"}
author = "rcoh"
Expand Up @@ -28,6 +28,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationCustomization
import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationSection
import software.amazon.smithy.rust.codegen.core.util.PANIC
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.orNull

Expand Down Expand Up @@ -167,6 +168,17 @@ class EndpointsDecorator : ClientCodegenDecorator {
}
}

private fun Node.toWritable(): Writable {
val node = this
return writable {
when (node) {
is StringNode -> rust("Some(${node.value.dq()}.to_string())")
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the fix? I fail to see it 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is an unrelated refactoring

is BooleanNode -> rust("Some(${node.value})")
else -> PANIC("unsupported default value: $node")
}
}
}

private fun builderFields(params: Parameters, section: OperationSection.MutateInput) = writable {
val memberParams = idx.getContextParams(operationShape)
val builtInParams = params.toList().filter { it.isBuiltIn }
Expand All @@ -189,13 +201,7 @@ class EndpointsDecorator : ClientCodegenDecorator {

idx.getStaticContextParams(operationShape).orNull()?.parameters?.forEach { (name, param) ->
val setterName = EndpointParamsGenerator.setterName(name)
val value = writable {
when (val v = param.value) {
is BooleanNode -> rust("Some(${v.value})")
is StringNode -> rust("Some(${v.value.dq()}.to_string())")
else -> TODO("Unexpected static value type: $v")
}
}
val value = param.value.toWritable()
rust(".$setterName(#W)", value)
}

Expand Down
Expand Up @@ -255,10 +255,11 @@ internal class EndpointParamsGenerator(private val parameters: Parameters) {
"ParamsError" to paramsError(),
) {
val params = writable {
Attribute.Custom("allow(clippy::unnecessary_lazy_evaluations)").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.

Why is this needed? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when it's a boolean value, the generated code isn't a function invocation

rustBlockTemplate("#{Params}", "Params" to paramsStruct()) {
parameters.toList().forEach { parameter ->
rust("${parameter.memberName()}: self.${parameter.memberName()}")
parameter.default.orNull()?.also { default -> rust(".or(Some(${value(default)}))") }
parameter.default.orNull()?.also { default -> rust(".or_else(||Some(${value(default)}))") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space

Suggested change
parameter.default.orNull()?.also { default -> rust(".or_else(||Some(${value(default)}))") }
parameter.default.orNull()?.also { default -> rust(".or_else(|| Some(${value(default)}))") }

if (parameter.isRequired) {
rustTemplate(
".ok_or_else(||#{Error}::missing(${parameter.memberName().dq()}))?",
Expand Down
Expand Up @@ -56,6 +56,8 @@ class EndpointsDecoratorTest {
"parameters": {
"Bucket": { "required": false, "type": "String" },
"Region": { "required": false, "type": "String", "builtIn": "AWS::Region" },
"BuiltInWithDefault": { "required": true, "type": "String", "builtIn": "AWS::DefaultBuiltIn", "default": "some-default" },
"BoolBuiltInWithDefault": { "required": true, "type": "Boolean", "builtIn": "AWS::FooBar", "default": true },
"AStringParam": { "required": false, "type": "String" },
"ABoolParam": { "required": false, "type": "Boolean" }
}
Expand Down Expand Up @@ -129,13 +131,15 @@ class EndpointsDecoratorTest {
use $moduleName::endpoint::{Params};
use aws_smithy_http::endpoint::Result;
let props = operation.properties();
let endpoint_result = dbg!(props.get::<Result>().expect("endpoint result in the bag"));
let endpoint_params = props.get::<Params>().expect("endpoint params in the bag");
let endpoint_result = props.get::<Result>().expect("endpoint result in the bag");
let endpoint = endpoint_result.as_ref().expect("endpoint resolved properly");
assert_eq!(
endpoint_params,
&Params::builder()
.bucket("bucket-name".to_string())
.built_in_with_default("some-default")
.bool_built_in_with_default(true)
.a_bool_param(false)
.a_string_param("hello".to_string())
.region("us-east-2".to_string())
Expand Down