Always write required query param keys, even if value is unset or empty

CHANGELOG.next.toml

@@ -197,6 +197,12 @@ references = ["smithy-rs#1935"]
 meta = { "breaking" = true, "tada" = false, "bug" = false }
 author = "jdisanti"
 
+[[aws-sdk-rust]]
+message = "Neglecting to include an upload ID when sending an AbortMultipartUpload request will no longer result in accidental deletion of data."
+references = ["smithy-rs#1957"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "Velfi"
+
 [[smithy-rs]]
 message = "Upgrade to Smithy 1.26.2"
 references = ["smithy-rs#1972"]

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/s3/S3Decorator.kt

@@ -67,8 +67,8 @@ class S3Decorator : RustCodegenDecorator<ClientProtocolGenerator, ClientCodegenC
         return model.letIf(applies(service.id)) {
             ModelTransformer.create().mapShapes(model) { shape ->
                 shape.letIf(isInInvalidXmlRootAllowList(shape)) {
-                    logger.info("Adding AllowInvalidXmlRoot trait to $shape")
-                    (shape as StructureShape).toBuilder().addTrait(AllowInvalidXmlRoot()).build()
+                    logger.info("Adding AllowInvalidXmlRoot trait to $it")
+                    (it as StructureShape).toBuilder().addTrait(AllowInvalidXmlRoot()).build()
                 }
             }
         }

aws/sdk/integration-tests/s3/tests/required-query-params.rs

@@ -0,0 +1,50 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+use aws_sdk_s3::operation::AbortMultipartUpload;
+use aws_sdk_s3::Region;
+use aws_smithy_http::operation::error::BuildError;
+
+#[tokio::test]
+async fn test_error_when_required_query_param_is_unset() {
+    let conf = aws_sdk_s3::Config::builder()
+        .region(Region::new("us-east-1"))
+        .build();
+
+    let err = AbortMultipartUpload::builder()
+        .bucket("test-bucket")
+        .key("test.txt")
+        .build()
+        .unwrap()
+        .make_operation(&conf)
+        .await
+        .unwrap_err();
+
+    assert_eq!(
+        BuildError::missing_field("upload_id", "cannot be empty or unset").to_string(),
+        err.to_string(),
+    )
+}
+
+#[tokio::test]
+async fn test_error_when_required_query_param_is_set_but_empty() {
+    let conf = aws_sdk_s3::Config::builder()
+        .region(Region::new("us-east-1"))
+        .build();
+    let err = AbortMultipartUpload::builder()
+        .bucket("test-bucket")
+        .key("test.txt")
+        .upload_id("")
+        .build()
+        .unwrap()
+        .make_operation(&conf)
+        .await
+        .unwrap_err();
+
+    assert_eq!(
+        BuildError::missing_field("upload_id", "cannot be empty or unset").to_string(),
+        err.to_string(),
+    )
+}

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/RequestBindingGenerator.kt

@@ -14,6 +14,7 @@ import software.amazon.smithy.model.shapes.MemberShape
 import software.amazon.smithy.model.shapes.OperationShape
 import software.amazon.smithy.model.shapes.Shape
 import software.amazon.smithy.model.shapes.StructureShape
+import software.amazon.smithy.model.traits.EnumTrait
 import software.amazon.smithy.model.traits.HttpTrait
 import software.amazon.smithy.rust.codegen.core.rustlang.Attribute
 import software.amazon.smithy.rust.codegen.core.rustlang.CargoDependency
@@ -23,7 +24,9 @@ import software.amazon.smithy.rust.codegen.core.rustlang.autoDeref
 import software.amazon.smithy.rust.codegen.core.rustlang.rust
 import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
 import software.amazon.smithy.rust.codegen.core.rustlang.rustBlockTemplate
+import software.amazon.smithy.rust.codegen.core.rustlang.rustInlineTemplate
 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.CodegenContext
 import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
 import software.amazon.smithy.rust.codegen.core.smithy.generators.OperationBuildError
@@ -33,6 +36,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.isOptional
 import software.amazon.smithy.rust.codegen.core.smithy.protocols.Protocol
 import software.amazon.smithy.rust.codegen.core.util.dq
 import software.amazon.smithy.rust.codegen.core.util.expectMember
+import software.amazon.smithy.rust.codegen.core.util.hasTrait
 import software.amazon.smithy.rust.codegen.core.util.inputShape
 
 fun HttpTrait.uriFormatString(): String {
@@ -55,7 +59,7 @@ fun SmithyPattern.rustFormatString(prefix: String, separator: String): String {
  * Generates methods to serialize and deserialize requests based on the HTTP trait. Specifically:
  * 1. `fn update_http_request(builder: http::request::Builder) -> Builder`
  *
- * This method takes a builder (perhaps pre configured with some headers) from the caller and sets the HTTP
+ * This method takes a builder (perhaps pre-configured with some headers) from the caller and sets the HTTP
  * headers & URL based on the HTTP trait implementation.
  */
 class RequestBindingGenerator(
@@ -72,7 +76,7 @@ class RequestBindingGenerator(
     private val httpBindingGenerator =
         HttpBindingGenerator(protocol, codegenContext, codegenContext.symbolProvider, operationShape, ::builderSymbol)
     private val index = HttpBindingIndex.of(model)
-    private val Encoder = CargoDependency.SmithyTypes(runtimeConfig).asType().member("primitive::Encoder")
+    private val encoder = CargoDependency.SmithyTypes(runtimeConfig).asType().member("primitive::Encoder")
 
     private val codegenScope = arrayOf(
         "BuildError" to runtimeConfig.operationBuildError(),
@@ -208,24 +212,64 @@ class RequestBindingGenerator(
                 val memberShape = param.member
                 val memberSymbol = symbolProvider.toSymbol(memberShape)
                 val memberName = symbolProvider.toMemberName(memberShape)
-                val outerTarget = model.expectShape(memberShape.target)
-                ifSet(outerTarget, memberSymbol, "&_input.$memberName") { field ->
-                    // if `param` is a list, generate another level of iteration
-                    listForEach(outerTarget, field) { innerField, targetId ->
-                        val target = model.expectShape(targetId)
-                        rust(
-                            "query.push_kv(${param.locationName.dq()}, ${
-                            paramFmtFun(writer, target, memberShape, innerField)
-                            });",
+                val target = model.expectShape(memberShape.target)
+
+                if (memberShape.isRequired) {
+                    val codegenScope = arrayOf(
+                        "BuildError" to OperationBuildError(runtimeConfig).missingField(
+                            memberName,
+                            "cannot be empty or unset",
+                        ),
+                    )
+                    val derefName = safeName("inner")
+                    rust("let $derefName = &_input.$memberName;")
+                    if (memberSymbol.isOptional()) {
+                        rustTemplate(
+                            "let $derefName = $derefName.as_ref().ok_or_else(|| #{BuildError:W})?;",
+                            *codegenScope,
                         )
                     }
+
+                    when {
+                        target.isStringShape -> {
+                            val asStr = writable {
+                                if (target.hasTrait<EnumTrait>()) {
+                                    rustInlineTemplate(".as_str()")
+                                }
+                            }
+                            rustBlock("if $derefName#W.is_empty()", asStr) {
+                                rustTemplate("return Err(#{BuildError:W});", *codegenScope)
+                            }
+                        }
+                    }
+
+                    paramList(target, derefName, param, writer, memberShape)
+                } else {
+                    ifSet(target, memberSymbol, "&_input.$memberName") { field ->
+                        // if `param` is a list, generate another level of iteration
+                        paramList(target, field, param, writer, memberShape)
+                    }
                 }
             }
             writer.rust("Ok(())")
         }
         return true
     }
 
+    private fun RustWriter.paramList(
+        outerTarget: Shape,
+        field: String,
+        param: HttpBinding,
+        writer: RustWriter,
+        memberShape: MemberShape,
+    ) {
+        listForEach(outerTarget, field) { innerField, targetId ->
+            val target = model.expectShape(targetId)
+            val value = paramFmtFun(writer, target, memberShape, innerField)
+            rust("""query.push_kv("${param.locationName}", $value);""")
+        }
+    }
+
     /**
      * Format [member] when used as a queryParam
      */
@@ -235,18 +279,21 @@ class RequestBindingGenerator(
                 val func = writer.format(RuntimeType.QueryFormat(runtimeConfig, "fmt_string"))
                 "&$func(&$targetName)"
             }
+
             target.isTimestampShape -> {
                 val timestampFormat =
                     index.determineTimestampFormat(member, HttpBinding.Location.QUERY, protocol.defaultTimestampFormat)
                 val timestampFormatType = RuntimeType.TimestampFormat(runtimeConfig, timestampFormat)
                 val func = writer.format(RuntimeType.QueryFormat(runtimeConfig, "fmt_timestamp"))
                 "&$func($targetName, ${writer.format(timestampFormatType)})?"
             }
+
             target.isListShape || target.isMemberShape -> {
                 throw IllegalArgumentException("lists should be handled at a higher level")
             }
+
             else -> {
-                "${writer.format(Encoder)}::from(${autoDeref(targetName)}).encode()"
+                "${writer.format(encoder)}::from(${autoDeref(targetName)}).encode()"
             }
         }
     }
@@ -273,17 +320,19 @@ class RequestBindingGenerator(
                 }
                 rust("let $outputVar = $func($input, #T);", encodingStrategy)
             }
+
             target.isTimestampShape -> {
                 val timestampFormat =
                     index.determineTimestampFormat(member, HttpBinding.Location.LABEL, protocol.defaultTimestampFormat)
                 val timestampFormatType = RuntimeType.TimestampFormat(runtimeConfig, timestampFormat)
                 val func = format(RuntimeType.LabelFormat(runtimeConfig, "fmt_timestamp"))
                 rust("let $outputVar = $func($input, ${format(timestampFormatType)})?;")
             }
+
             else -> {
                 rust(
                     "let mut ${outputVar}_encoder = #T::from(${autoDeref(input)}); let $outputVar = ${outputVar}_encoder.encode();",
-                    Encoder,
+                    encoder,
                 )
             }
         }

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

@@ -118,7 +118,7 @@ fun StructureShape.renderWithModelBuilder(
 val TokioWithTestMacros = CargoDependency(
     "tokio",
     CratesIo("1"),
-    features = setOf("macros", "test-util", "rt"),
+    features = setOf("macros", "test-util", "rt", "rt-multi-thread"),
     scope = DependencyScope.Dev,
 )