fix: reject null values in dense collections

.changelog/fix-dense-collection-null-handling.md

@@ -0,0 +1,9 @@
+---
+applies_to: ["client", "aws-sdk-rust"]
+authors: ["vcjana"]
+references: []
+breaking: false
+new_feature: false
+bug_fix: true
+---
+Fix null value handling in dense collections: SDK now correctly rejects null values in non-sparse collections instead of silently dropping them.

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ClientProtocolTestGenerator.kt

@@ -93,6 +93,11 @@ class ClientProtocolTestGenerator(
                 // https://github.com/smithy-lang/smithy-rs/issues/4184
                 FailingTest.RequestTest(REST_XML, "HttpEmptyPrefixHeadersRequestClient"),
                 FailingTest.RequestTest(REST_JSON, "RestJsonHttpEmptyPrefixHeadersRequestClient"),
+                // Smithy protocol tests expect null to be silently dropped, but we now correctly reject it.
+                // These tests will be removed in https://github.com/smithy-lang/smithy/pull/2972
+                FailingTest.ResponseTest(REST_JSON, "RestJsonDeserializesDenseSetMapAndSkipsNull"),
+                FailingTest.ResponseTest(RPC_V2_CBOR, "RpcV2CborDeserializesDenseSetMapAndSkipsNull"),
+                FailingTest.ResponseTest("aws.protocoltests.restjson#RestJsonExtras", "NullInNonSparse"),
             )
 
         private val BrokenTests:

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RpcV2Cbor.kt

@@ -188,13 +188,11 @@ open class RpcV2Cbor(
             codegenContext, httpBindingResolver,
             handleNullForNonSparseCollection = { collectionName: String ->
                 writable {
-                    // The client should drop a null value in a dense collection, see
-                    // https://github.com/smithy-lang/smithy/blob/6466fe77c65b8a17b219f0b0a60c767915205f95/smithy-protocol-tests/model/rpcv2Cbor/cbor-maps.smithy#L158
                     rustTemplate(
                         """
-                        decoder.null()?;
-                        return #{Ok}($collectionName)
+                        return #{Err}(#{Error}::custom("dense $collectionName cannot contain null values", decoder.position()))
                         """,
+                        "Error" to RuntimeType.smithyCbor(runtimeConfig).resolve("decode::DeserializeError"),
                         *RuntimeType.preludeScope,
                     )
                 }

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt

@@ -456,23 +456,16 @@ class JsonParserGenerator(
                                         withBlock("let value =", ";") {
                                             deserializeMember(shape.member)
                                         }
-                                        rust(
+                                        rustTemplate(
                                             """
                                             if let Some(value) = value {
                                                 items.push(value);
+                                            } else {
+                                                return Err(#{Error}::custom("dense list cannot contain null values"));
                                             }
                                             """,
+                                            *codegenScope,
                                         )
-                                        codegenTarget.ifServer {
-                                            rustTemplate(
-                                                """
-                                                else {
-                                                    return Err(#{Error}::custom("dense list cannot contain null values"));
-                                                }
-                                                """,
-                                                *codegenScope,
-                                            )
-                                        }
                                     }
                                 }
                             }
@@ -514,25 +507,14 @@ class JsonParserGenerator(
                             if (isSparse) {
                                 rust("map.insert(key, value);")
                             } else {
-                                codegenTarget.ifServer {
-                                    rustTemplate(
-                                        """
-                                        match value {
-                                            Some(value) => { map.insert(key, value); }
-                                            None => return Err(#{Error}::custom("dense map cannot contain null values"))
-                                            }""",
-                                        *codegenScope,
-                                    )
-                                }
-                                codegenTarget.ifClient {
-                                    rustTemplate(
-                                        """
-                                        if let Some(value) = value {
-                                            map.insert(key, value);
-                                        }
-                                        """,
-                                    )
-                                }
+                                rustTemplate(
+                                    """
+                                    match value {
+                                        Some(value) => { map.insert(key, value); }
+                                        None => return Err(#{Error}::custom("dense map cannot contain null values"))
+                                    }""",
+                                    *codegenScope,
+                                )
                             }
                         }
                         if (returnSymbolToParse.isUnconstrained) {

codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGeneratorTest.kt

@@ -220,6 +220,63 @@ class JsonParserGeneratorTest {
                 assert_eq!(error_output.message.expect("message should be set"), "hello");
                 """,
             )
+
+            unitTest(
+                "dense_list_rejects_null",
+                """
+                // dense list should reject null values
+                let input = br#"{ "top": { "choice": { "list": [{ "int": 5 }, null, { "int": 6 }] } } }"#;
+                let err = ${format(operationGenerator)}(input, test_output::OpOutput::builder()).expect_err("dense list cannot contain null");
+                assert!(err.to_string().contains("dense list cannot contain null values"));
+                """,
+            )
+
+            unitTest(
+                "dense_map_rejects_null",
+                """
+                // dense map should reject null values
+                let input = br#"{ "top": { "choice": { "map": { "a": { "int": 5 }, "b": null } } } }"#;
+                let err = ${format(operationGenerator)}(input, test_output::OpOutput::builder()).expect_err("dense map cannot contain null");
+                assert!(err.to_string().contains("dense map cannot contain null values"));
+                """,
+            )
+
+            unitTest(
+                "sparse_list_allows_null",
+                """
+                // sparse list should allow null values
+                let input = br#"{ "top": { "choice": { "listSparse": [{ "int": 5 }, null, { "int": 6 }] } } }"#;
+                let output = ${format(operationGenerator)}(input, test_output::OpOutput::builder()).unwrap().build();
+                use test_model::Choice;
+                match output.top.unwrap().choice {
+                    Choice::ListSparse(list) => {
+                        assert_eq!(list.len(), 3);
+                        assert_eq!(list[0], Some(Choice::Int(5)));
+                        assert_eq!(list[1], None);
+                        assert_eq!(list[2], Some(Choice::Int(6)));
+                    }
+                    _ => panic!("expected ListSparse variant"),
+                }
+                """,
+            )
+
+            unitTest(
+                "sparse_map_allows_null",
+                """
+                // sparse map should allow null values
+                let input = br#"{ "top": { "choice": { "mapSparse": { "a": { "int": 5 }, "b": null } } } }"#;
+                let output = ${format(operationGenerator)}(input, test_output::OpOutput::builder()).unwrap().build();
+                use test_model::Choice;
+                match output.top.unwrap().choice {
+                    Choice::MapSparse(map) => {
+                        assert_eq!(map.len(), 2);
+                        assert_eq!(map.get("a"), Some(&Some(Choice::Int(5))));
+                        assert_eq!(map.get("b"), Some(&None));
+                    }
+                    _ => panic!("expected MapSparse variant"),
+                }
+                """,
+            )
         }
         model.lookup<StructureShape>("test#Top").also { top ->
             top.renderWithModelBuilder(model, symbolProvider, project)