Always write required query param keys, even if value is unset or empty (#1973)

* fix: issues when sending multipart-upload-related operations with no upload ID by making upload ID required
add: Mandatory trait
add: S3 customization to make Upload Ids mandatory

* add: CHANGELOG.next.toml entry

* update: strategy for tagging shapes as mandatory

* remove: Mandatory trait
undo: S3Decorator changes
add: codegen to generate "else" branch after required query string serialization `if let Some` check
add: missing tokio feature to `TokioTest`

* update: mandatory-query-param.rs tests

* format: run cargo fmt

* update: return BuildError for missing required query params
update: required-query-params.rs tests

* formatting: make RustWriter.paramList more readable
fix: code broken by merge from `main`

* fix: handling of enum strings

* add: codegen tests for required query params
update: required enum query param codegen
add: smithy-rs CHANGELOG.next.toml entry

* fix: presigning.rs test

* fix: use `toType` instead of `asType`
fix: indentation in test

Author: Zelda Hessler

CHANGELOG.next.toml

@@ -197,6 +197,27 @@ references = ["smithy-rs#1935"]
 meta = { "breaking" = true, "tada" = false, "bug" = false }
 author = "jdisanti"
 
+[[aws-sdk-rust]]
+message = """
+It was possible in some cases to send some S3 requests without a required upload ID, causing a risk of unintended data
+deletion and modification. Now, when an operation has query parameters that are marked as required, the omission of
+those query parameters will cause a BuildError, preventing the invalid operation from being sent.
+"""
+references = ["smithy-rs#1957"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "Velfi"
+
+[[smithy-rs]]
+message = """
+It was previously possible to send requests without setting query parameters modeled as required. Doing this may cause a
+service to interpret a request incorrectly instead of just sending back a 400 error. Now, when an operation has query
+parameters that are marked as required, the omission of those query parameters will cause a BuildError, preventing the
+invalid operation from being sent.
+"""
+references = ["smithy-rs#1957"]
+meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" }
+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

@@ -66,8 +66,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/presigning.rs

@@ -124,7 +124,7 @@ async fn test_presigned_upload_part() -> Result<(), Box<dyn Error>> {
         .build()?);
     assert_eq!(
         presigned.uri().to_string(),
-        "https://s3.us-east-1.amazonaws.com/bucket/key?x-id=UploadPart&uploadId=upload-id&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ANOTREAL%2F20090213%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20090213T233131Z&X-Amz-Expires=30&X-Amz-SignedHeaders=content-length%3Bhost&X-Amz-Signature=e50e1a4d1dae7465bb7731863a565bdf4137393e3ab4119b5764fb49f5f60b14&X-Amz-Security-Token=notarealsessiontoken"
+        "https://s3.us-east-1.amazonaws.com/bucket/key?x-id=UploadPart&partNumber=0&uploadId=upload-id&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ANOTREAL%2F20090213%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20090213T233131Z&X-Amz-Expires=30&X-Amz-SignedHeaders=content-length%3Bhost&X-Amz-Signature=59777f7ddd2f324dfe0749685e06b978433d03e6f090dceb96eb23cc9540c30c&X-Amz-Security-Token=notarealsessiontoken"
     );
     Ok(())
 }

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
@@ -32,6 +33,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 {
@@ -54,7 +56,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(
@@ -71,7 +73,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).toType().member("primitive::Encoder")
+    private val encoder = CargoDependency.smithyTypes(runtimeConfig).toType().member("primitive::Encoder")
 
     private val codegenScope = arrayOf(
         "BuildError" to runtimeConfig.operationBuildError(),
@@ -207,24 +209,58 @@ 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,
                         )
                     }
+
+                    // Strings that aren't enums must be checked to see if they're empty
+                    if (target.isStringShape && !target.hasTrait<EnumTrait>()) {
+                        rustBlock("if $derefName.is_empty()") {
+                            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
      */
@@ -234,18 +270,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()"
             }
         }
     }
@@ -272,17 +311,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

@@ -117,7 +117,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,
 )