diff --git a/.changelog/fix-dense-collection-null-handling.md b/.changelog/fix-dense-collection-null-handling.md new file mode 100644 index 0000000000..28209280ee --- /dev/null +++ b/.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. diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ClientProtocolTestGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ClientProtocolTestGenerator.kt index 33ada3f3a3..50f5fad52b 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ClientProtocolTestGenerator.kt +++ b/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: diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RpcV2Cbor.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RpcV2Cbor.kt index 40570a847d..72fcbde0a2 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RpcV2Cbor.kt +++ b/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, ) } diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt index 7295494c07..b5b6cf72d3 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt +++ b/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) { diff --git a/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGeneratorTest.kt b/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGeneratorTest.kt index 0a5c8376c4..86a9fb258e 100644 --- a/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGeneratorTest.kt +++ b/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("test#Top").also { top -> top.renderWithModelBuilder(model, symbolProvider, project)