From 20ff1c7bbcf7829dded579523f5658a64608a737 Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Tue, 1 Oct 2024 16:33:43 +0100 Subject: [PATCH 1/8] Replace `serde_cbor` with `ciborium` due to security vulnerability - `serde_cbor` is unmaintained and has known [security issues](https://rustsec.org/advisories/RUSTSEC-2021-0127.html) - `ciborium` is an actively maintained CBOR library --- .../codegen/core/rustlang/CargoDependency.kt | 2 +- .../rust/codegen/serde/SerdeDecoratorTest.kt | 6 ++++-- ...rGeneratorSerdeRoundTripIntegrationTest.kt | 19 ++++++++++--------- .../aws-smithy-protocol-test/Cargo.toml | 2 +- .../aws-smithy-protocol-test/src/lib.rs | 18 +++++++++++++++--- 5 files changed, 31 insertions(+), 16 deletions(-) diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/CargoDependency.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/CargoDependency.kt index 3c82f3505d..1e9b161212 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/CargoDependency.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/CargoDependency.kt @@ -310,7 +310,7 @@ data class CargoDependency( val Hound: CargoDependency = CargoDependency("hound", CratesIo("3.4.0"), DependencyScope.Dev) val PrettyAssertions: CargoDependency = CargoDependency("pretty_assertions", CratesIo("1.3.0"), DependencyScope.Dev) - val SerdeCbor: CargoDependency = CargoDependency("serde_cbor", CratesIo("0.11"), DependencyScope.Dev) + val Ciborium: CargoDependency = CargoDependency("ciborium", CratesIo("0.2"), DependencyScope.Dev) val SerdeJson: CargoDependency = CargoDependency("serde_json", CratesIo("1.0.0"), DependencyScope.Dev) val Smol: CargoDependency = CargoDependency("smol", CratesIo("1.2.0"), DependencyScope.Dev) val TempFile: CargoDependency = CargoDependency("tempfile", CratesIo("3.2.0"), DependencyScope.Dev) diff --git a/codegen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecoratorTest.kt b/codegen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecoratorTest.kt index f9657f0003..1e419a3c76 100644 --- a/codegen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecoratorTest.kt +++ b/codegen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecoratorTest.kt @@ -341,7 +341,7 @@ class SerdeDecoratorTest { arrayOf( "crate" to RustType.Opaque(ctx.moduleUseName()), "serde_json" to CargoDependency("serde_json", CratesIo("1")).toDevDependency().toType(), - "serde_cbor" to CargoDependency("serde_cbor", CratesIo("0.11.2")).toDevDependency().toType(), + "ciborium" to CargoDependency.Ciborium.toType(), // we need the derive feature "serde" to CargoDependency.Serde.toDevDependency().toType(), ) @@ -437,7 +437,9 @@ class SerdeDecoratorTest { use #{crate}::serde::*; let input = StreamingInput::builder().data(ByteStream::from_static(b"123")).build().unwrap(); let settings = SerializationSettings::default(); - let serialized = #{serde_cbor}::to_vec(&input.serialize_ref(&settings)).expect("failed to serialize"); + let mut serialized = Vec::new(); + #{ciborium}::ser::into_writer(&input.serialize_ref(&settings), &mut serialized) + .expect("failed to serialize input into CBOR format using `ciborium`"); assert_eq!(serialized, b"\xa1ddataC123"); """, *codegenScope, diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/serialize/CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/serialize/CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest.kt index 2e92cde4e2..cafabe9558 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/serialize/CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/serialize/CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest.kt @@ -62,7 +62,7 @@ import java.util.logging.Logger /** * This lives in `codegen-server` because we want to run a full integration test for convenience, * but there's really nothing server-specific here. We're just testing that the CBOR (de)serializers work like - * the ones generated by `serde_cbor`. This is a good exhaustive litmus test for correctness, since `serde_cbor` + * the ones generated by `ciborium`. This is a good exhaustive litmus test for correctness, since `ciborium` * is battle-tested. */ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest { @@ -175,7 +175,7 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest { } @Test - fun `serde_cbor round trip`() { + fun `ciborium round trip`() { val addDeriveSerdeSerializeDeserializeDecorator = object : ServerCodegenDecorator { override val name: String = "Add `#[derive(serde::Serialize, serde::Deserialize)]`" @@ -240,7 +240,7 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest { val codegenScope = arrayOf( "AssertEq" to RuntimeType.PrettyAssertions.resolve("assert_eq!"), - "SerdeCbor" to CargoDependency.SerdeCbor.toType(), + "ciborium" to CargoDependency.Ciborium.toType(), ) val instantiator = ServerInstantiator(codegenContext, ignoreMissingMembers = true, withinTest = true) @@ -278,14 +278,14 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest { if (expectFail.contains(test.id)) { writeWithNoFormatting("#[should_panic]") } - unitTest("we_serialize_and_serde_cbor_deserializes_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") { + unitTest("we_serialize_and_ciborium_deserializes_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") { rustTemplate( """ let expected = #{InstantiateShape:W}; let bytes = #{SerializeFn}(&expected) .expect("our generated CBOR serializer failed"); - let actual = #{SerdeCbor}::from_slice(&bytes) - .expect("serde_cbor failed deserializing from bytes"); + let actual = #{ciborium}::from_reader(::std::io::Cursor::new(&bytes)) + .expect("failed to deserialize bytes with `ciborium`"); #{AssertEq}(expected, actual); """, "InstantiateShape" to instantiator.generate(targetShape, params), @@ -330,12 +330,13 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest { if (expectFail.contains(test.id)) { writeWithNoFormatting("#[should_panic]") } - unitTest("serde_cbor_serializes_and_we_deserialize_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") { + unitTest("ciborium_serializes_and_we_deserialize_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") { rustTemplate( """ let expected = #{InstantiateShape:W}; - let bytes: Vec = #{SerdeCbor}::to_vec(&expected) - .expect("serde_cbor failed serializing to `Vec`"); + let mut bytes = Vec::new(); + #{ciborium}::into_writer(&expected, &mut bytes) + .expect("failed to serialize to `Vec` with `ciborium`"); let input = #{InputBuilder}::default(); let input = #{DeserializeFn}(&bytes, input) .expect("our generated CBOR deserializer failed"); diff --git a/rust-runtime/aws-smithy-protocol-test/Cargo.toml b/rust-runtime/aws-smithy-protocol-test/Cargo.toml index 9f5189079a..2b4a119215 100644 --- a/rust-runtime/aws-smithy-protocol-test/Cargo.toml +++ b/rust-runtime/aws-smithy-protocol-test/Cargo.toml @@ -12,7 +12,7 @@ repository = "https://github.com/smithy-lang/smithy-rs" assert-json-diff = "1.1" base64-simd = "0.8" cbor-diag = "0.1.12" -serde_cbor = "0.11" +ciborium = "0.2" http = "0.2.1" pretty_assertions = "1.3" regex-lite = "0.1.5" diff --git a/rust-runtime/aws-smithy-protocol-test/src/lib.rs b/rust-runtime/aws-smithy-protocol-test/src/lib.rs index f6bef0d78c..5c6c7d4f37 100644 --- a/rust-runtime/aws-smithy-protocol-test/src/lib.rs +++ b/rust-runtime/aws-smithy-protocol-test/src/lib.rs @@ -422,9 +422,9 @@ fn try_cbor_eq + Debug>( let decoded = base64_simd::STANDARD .decode_to_vec(expected_body) .expect("smithy protocol test `body` property is not properly base64 encoded"); - let expected_cbor_value: serde_cbor::Value = - serde_cbor::from_slice(decoded.as_slice()).expect("expected value must be valid CBOR"); - let actual_cbor_value: serde_cbor::Value = serde_cbor::from_slice(actual_body.as_ref()) + let expected_cbor_value: ciborium::value::Value = + ciborium::from_reader(decoded.as_slice()).expect("expected value must be valid CBOR"); + let actual_cbor_value: ciborium::value::Value = ciborium::from_reader(actual_body.as_ref()) .map_err(|e| ProtocolTestFailure::InvalidBodyFormat { expected: "cbor".to_owned(), found: format!("{} {:?}", e, actual_body), @@ -599,6 +599,18 @@ mod tests { validate_body(actual, expected, MediaType::Json).expect_err("bodies do not match"); } + #[test] + fn test_validate_cbor_body() { + // The following is the CBOR representation of `{"abc": 5 }`. + let actual = [0xbf, 0x63, 0x61, 0x62, 0x63, 0x05, 0xff]; + // The following is the CBOR representation of `{"abc": 5 }` using a definite length map. + let expected = [0xA1, 0x63, 0x61, 0x62, 0x63, 0x05]; + let expected_base64 = base64_simd::STANDARD.encode_to_string(expected); + + validate_body(actual, expected_base64.as_str(), MediaType::Cbor) + .expect("expected base64-encoded CBOR value did not match"); + } + #[test] fn test_validate_xml_body() { let expected = r#" From 275f997db2288902cbabc839b8d0d35eaad0e7ba Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Tue, 1 Oct 2024 19:24:23 +0100 Subject: [PATCH 2/8] Fix map comparison: implement order-insensitive equality for `ciborium::Value::Map` --- .../aws-smithy-protocol-test/src/lib.rs | 98 +++++++++++++++++-- 1 file changed, 91 insertions(+), 7 deletions(-) diff --git a/rust-runtime/aws-smithy-protocol-test/src/lib.rs b/rust-runtime/aws-smithy-protocol-test/src/lib.rs index 5c6c7d4f37..b9a76d83b6 100644 --- a/rust-runtime/aws-smithy-protocol-test/src/lib.rs +++ b/rust-runtime/aws-smithy-protocol-test/src/lib.rs @@ -415,6 +415,68 @@ fn try_json_eq(expected: &str, actual: &str) -> Result<(), ProtocolTestFailure> } } +/// Compares two `ciborium::Value` instances for semantic equality. +/// +/// This function recursively compares two CBOR values, correctly handling arrays and maps +/// according to the CBOR specification. Arrays are compared element-wise in order, +/// while maps are compared without considering the order of key-value pairs. +fn cbor_values_equal(a: &ciborium::Value, b: &ciborium::Value) -> Result { + let result = match (a, b) { + (ciborium::Value::Array(a_array), ciborium::Value::Array(b_array)) => { + // Both arrays should be equal in size. + a_array.len() == b_array.len() && + // Compare arrays element-wise. + a_array.iter().zip(b_array.iter()).try_fold(true, |acc, (a_elem, b_elem)| { + cbor_values_equal(a_elem, b_elem).map(|equal| acc && equal) + })? + }, + + // Convert `ciborium::Map` to sorted `BTreeMap` and then compare the sorted maps. + (ciborium::Value::Map(a_map), ciborium::Value::Map(b_map)) => { + let a_btree = ciborium_map_to_btreemap(a_map)?; + let b_btree = ciborium_map_to_btreemap(b_map)?; + + if a_btree.len() != b_btree.len() { + false + } + else { + // Each key in `a` should exist in `b`, and the values should match. + a_btree.iter().try_fold(true, |acc, (a_key, a_value)| + b_btree.get(a_key) + .map(|b_value| cbor_values_equal(a_value, b_value).map(|equal| acc && equal)) + .unwrap_or(Ok(false)) + )? + } + }, + + _ => a == b, + }; + + Ok(result) +} + +/// Transforms a `ciborium::Value::Map` into a `BTreeMap<&String, &ciborium::Value>` sorted by keys. +/// +/// CBOR maps (`Value::Map`) are internally represented as vectors of key-value pairs, +/// and their direct comparison is affected by the order of these pairs. +/// Since CBOR specification treats maps as unordered collections, +/// this function converts the vector into a `BTreeMap`, which maintains +/// the entries in a sorted order based on the keys. +/// This allows for consistent, order-independent comparisons between maps. +fn ciborium_map_to_btreemap(cbor_map: &[(ciborium::Value, ciborium::Value)]) -> Result, ProtocolTestFailure> { + let mut btree = std::collections::BTreeMap::new(); + for (key, value) in cbor_map { + match key { + ciborium::Value::Text(key_str) => btree.insert(key_str, value), + _ => return Err(ProtocolTestFailure::InvalidBodyFormat { + expected: "a text key as map entry".to_string(), + found: format!("{:?}", key), + }) + }; + } + Ok(btree) +} + fn try_cbor_eq + Debug>( actual_body: T, expected_body: &str, @@ -422,16 +484,16 @@ fn try_cbor_eq + Debug>( let decoded = base64_simd::STANDARD .decode_to_vec(expected_body) .expect("smithy protocol test `body` property is not properly base64 encoded"); - let expected_cbor_value: ciborium::value::Value = + let expected_cbor_value: ciborium::Value = ciborium::from_reader(decoded.as_slice()).expect("expected value must be valid CBOR"); - let actual_cbor_value: ciborium::value::Value = ciborium::from_reader(actual_body.as_ref()) + let actual_cbor_value: ciborium::Value = ciborium::from_reader(actual_body.as_ref()) .map_err(|e| ProtocolTestFailure::InvalidBodyFormat { expected: "cbor".to_owned(), found: format!("{} {:?}", e, actual_body), })?; let actual_body_base64 = base64_simd::STANDARD.encode_to_string(&actual_body); - if expected_cbor_value != actual_cbor_value { + if !cbor_values_equal(&expected_cbor_value, &actual_cbor_value)? { let expected_body_annotated_hex: String = cbor_diag::parse_bytes(&decoded) .expect("smithy protocol test `body` property is not valid CBOR") .to_hex(); @@ -601,14 +663,36 @@ mod tests { #[test] fn test_validate_cbor_body() { + let base64_encode = |v : &[u8]| base64_simd::STANDARD.encode_to_string(v); + // The following is the CBOR representation of `{"abc": 5 }`. let actual = [0xbf, 0x63, 0x61, 0x62, 0x63, 0x05, 0xff]; - // The following is the CBOR representation of `{"abc": 5 }` using a definite length map. - let expected = [0xA1, 0x63, 0x61, 0x62, 0x63, 0x05]; - let expected_base64 = base64_simd::STANDARD.encode_to_string(expected); + // The following is the base64-encoded CBOR representation of `{"abc": 5 }` using a definite length map. + let expected_base64 = base64_encode(&[0xA1, 0x63, 0x61, 0x62, 0x63, 0x05]); + + validate_body(actual, expected_base64.as_str(), MediaType::Cbor) + .expect("unexpected mismatch between CBOR definite and indefinite map encodings"); + + // The following is the CBOR representation of `{"a":1, "b":2}`. + let actual = [0xBF, 0x61, 0x61, 0x01, 0x61, 0x62, 0x02, 0xFF]; + // The following is the base64-encoded CBOR representation of `{"b":2, "a":1}`. + let expected_base64 = base64_encode(&[0xBF, 0x61, 0x62, 0x02, 0x61, 0x61, 0x01, 0xFF]); + validate_body(actual, expected_base64.as_str(), MediaType::Cbor) + .expect("different ordering in CBOR decoded maps do not match"); + + // The following is the CBOR representation of `{"a":[1,2,{"b":3, "c":4}]}`. + let actual = [0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x62, 0x03, 0x61, 0x63, 0x04, 0xFF, 0xFF, 0xFF]; + // The following is the base64-encoded CBOR representation of `{"a":[1,2,{"c":4, "b":3}]}`. + let expected_base64 = base64_encode(&[0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x63, 0x04, 0x61, 0x62, 0x03, 0xFF, 0xFF, 0xFF]); + validate_body(actual, expected_base64.as_str(), MediaType::Cbor) + .expect("different ordering in CBOR decoded maps do not match"); + // The following is the CBOR representation of `{"a":[1,2]}`. + let actual = [0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xFF, 0xFF]; + // The following is the CBOR representation of `{"a":[2,1]}`. + let expected_base64 = base64_encode(&[0xBF, 0x61, 0x61, 0x9F, 0x02, 0x01, 0xFF, 0xFF]); validate_body(actual, expected_base64.as_str(), MediaType::Cbor) - .expect("expected base64-encoded CBOR value did not match"); + .expect_err("arrays in CBOR should follow strict ordering"); } #[test] From a2679acf30076c9033e9e1c277c39cca5695e2c7 Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Tue, 1 Oct 2024 20:08:12 +0100 Subject: [PATCH 3/8] Specifically, use float.is_nan() when comparing floats that may have NaN values. --- .../aws-smithy-protocol-test/src/lib.rs | 63 ++++++++++++------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/rust-runtime/aws-smithy-protocol-test/src/lib.rs b/rust-runtime/aws-smithy-protocol-test/src/lib.rs index b9a76d83b6..4976e67938 100644 --- a/rust-runtime/aws-smithy-protocol-test/src/lib.rs +++ b/rust-runtime/aws-smithy-protocol-test/src/lib.rs @@ -420,7 +420,10 @@ fn try_json_eq(expected: &str, actual: &str) -> Result<(), ProtocolTestFailure> /// This function recursively compares two CBOR values, correctly handling arrays and maps /// according to the CBOR specification. Arrays are compared element-wise in order, /// while maps are compared without considering the order of key-value pairs. -fn cbor_values_equal(a: &ciborium::Value, b: &ciborium::Value) -> Result { +fn cbor_values_equal( + a: &ciborium::Value, + b: &ciborium::Value, +) -> Result { let result = match (a, b) { (ciborium::Value::Array(a_array), ciborium::Value::Array(b_array)) => { // Both arrays should be equal in size. @@ -429,7 +432,7 @@ fn cbor_values_equal(a: &ciborium::Value, b: &ciborium::Value) -> Result { @@ -438,16 +441,22 @@ fn cbor_values_equal(a: &ciborium::Value, b: &ciborium::Value) -> Result { + a_float == b_float || a_float.is_nan() && b_float.is_nan() + } _ => a == b, }; @@ -463,15 +472,19 @@ fn cbor_values_equal(a: &ciborium::Value, b: &ciborium::Value) -> Result Result, ProtocolTestFailure> { +fn ciborium_map_to_btreemap( + cbor_map: &[(ciborium::Value, ciborium::Value)], +) -> Result, ProtocolTestFailure> { let mut btree = std::collections::BTreeMap::new(); for (key, value) in cbor_map { match key { ciborium::Value::Text(key_str) => btree.insert(key_str, value), - _ => return Err(ProtocolTestFailure::InvalidBodyFormat { - expected: "a text key as map entry".to_string(), - found: format!("{:?}", key), - }) + _ => { + return Err(ProtocolTestFailure::InvalidBodyFormat { + expected: "a text key as map entry".to_string(), + found: format!("{:?}", key), + }) + } }; } Ok(btree) @@ -486,10 +499,12 @@ fn try_cbor_eq + Debug>( .expect("smithy protocol test `body` property is not properly base64 encoded"); let expected_cbor_value: ciborium::Value = ciborium::from_reader(decoded.as_slice()).expect("expected value must be valid CBOR"); - let actual_cbor_value: ciborium::Value = ciborium::from_reader(actual_body.as_ref()) - .map_err(|e| ProtocolTestFailure::InvalidBodyFormat { - expected: "cbor".to_owned(), - found: format!("{} {:?}", e, actual_body), + let actual_cbor_value: ciborium::Value = + ciborium::from_reader(actual_body.as_ref()).map_err(|e| { + ProtocolTestFailure::InvalidBodyFormat { + expected: "cbor".to_owned(), + found: format!("{} {:?}", e, actual_body), + } })?; let actual_body_base64 = base64_simd::STANDARD.encode_to_string(&actual_body); @@ -663,7 +678,7 @@ mod tests { #[test] fn test_validate_cbor_body() { - let base64_encode = |v : &[u8]| base64_simd::STANDARD.encode_to_string(v); + let base64_encode = |v: &[u8]| base64_simd::STANDARD.encode_to_string(v); // The following is the CBOR representation of `{"abc": 5 }`. let actual = [0xbf, 0x63, 0x61, 0x62, 0x63, 0x05, 0xff]; @@ -681,9 +696,15 @@ mod tests { .expect("different ordering in CBOR decoded maps do not match"); // The following is the CBOR representation of `{"a":[1,2,{"b":3, "c":4}]}`. - let actual = [0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x62, 0x03, 0x61, 0x63, 0x04, 0xFF, 0xFF, 0xFF]; + let actual = [ + 0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x62, 0x03, 0x61, 0x63, 0x04, 0xFF, + 0xFF, 0xFF, + ]; // The following is the base64-encoded CBOR representation of `{"a":[1,2,{"c":4, "b":3}]}`. - let expected_base64 = base64_encode(&[0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x63, 0x04, 0x61, 0x62, 0x03, 0xFF, 0xFF, 0xFF]); + let expected_base64 = base64_encode(&[ + 0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x63, 0x04, 0x61, 0x62, 0x03, 0xFF, + 0xFF, 0xFF, + ]); validate_body(actual, expected_base64.as_str(), MediaType::Cbor) .expect("different ordering in CBOR decoded maps do not match"); From 7ae4517d746ece5751da90961ff8c81a0c25908a Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Tue, 1 Oct 2024 20:43:12 +0100 Subject: [PATCH 4/8] Use HashMap instead of BTreeMap --- .../aws-smithy-protocol-test/src/lib.rs | 48 +++++++++---------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/rust-runtime/aws-smithy-protocol-test/src/lib.rs b/rust-runtime/aws-smithy-protocol-test/src/lib.rs index 4976e67938..c1d0e8220b 100644 --- a/rust-runtime/aws-smithy-protocol-test/src/lib.rs +++ b/rust-runtime/aws-smithy-protocol-test/src/lib.rs @@ -436,15 +436,15 @@ fn cbor_values_equal( // Convert `ciborium::Map` to sorted `BTreeMap` and then compare the sorted maps. (ciborium::Value::Map(a_map), ciborium::Value::Map(b_map)) => { - let a_btree = ciborium_map_to_btreemap(a_map)?; - let b_btree = ciborium_map_to_btreemap(b_map)?; + let a_hashmap = ciborium_map_to_hashmap(a_map)?; + let b_hashmap = ciborium_map_to_hashmap(b_map)?; - if a_btree.len() != b_btree.len() { + if a_hashmap.len() != b_hashmap.len() { false } else { // Each key in `a` should exist in `b`, and the values should match. - a_btree.iter().try_fold(true, |acc, (a_key, a_value)| { - b_btree + a_hashmap.iter().try_fold(true, |acc, (a_key, a_value)| { + b_hashmap .get(a_key) .map(|b_value| { cbor_values_equal(a_value, b_value).map(|equal| acc && equal) @@ -464,30 +464,26 @@ fn cbor_values_equal( Ok(result) } -/// Transforms a `ciborium::Value::Map` into a `BTreeMap<&String, &ciborium::Value>` sorted by keys. +/// Converts a `ciborium::Value::Map` into a `HashMap<&String, &ciborium::Value>`. /// /// CBOR maps (`Value::Map`) are internally represented as vectors of key-value pairs, -/// and their direct comparison is affected by the order of these pairs. -/// Since CBOR specification treats maps as unordered collections, -/// this function converts the vector into a `BTreeMap`, which maintains -/// the entries in a sorted order based on the keys. -/// This allows for consistent, order-independent comparisons between maps. -fn ciborium_map_to_btreemap( +/// and direct comparison is affected by the order of these pairs. +/// Since the CBOR specification treats maps as unordered collections, +/// this function transforms the vector into a `HashMap`, for order-independent comparisons +/// between maps. +fn ciborium_map_to_hashmap( cbor_map: &[(ciborium::Value, ciborium::Value)], -) -> Result, ProtocolTestFailure> { - let mut btree = std::collections::BTreeMap::new(); - for (key, value) in cbor_map { - match key { - ciborium::Value::Text(key_str) => btree.insert(key_str, value), - _ => { - return Err(ProtocolTestFailure::InvalidBodyFormat { - expected: "a text key as map entry".to_string(), - found: format!("{:?}", key), - }) - } - }; - } - Ok(btree) +) -> Result, ProtocolTestFailure> { + cbor_map + .iter() + .map(|(key, value)| match key { + ciborium::Value::Text(key_str) => Ok((key_str, value)), + _ => Err(ProtocolTestFailure::InvalidBodyFormat { + expected: "a text key as map entry".to_string(), + found: format!("{:?}", key), + }), + }) + .collect() } fn try_cbor_eq + Debug>( From 0460c875f79c95c2a0fd62d85a0b74d66b14da48 Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Tue, 1 Oct 2024 21:26:20 +0100 Subject: [PATCH 5/8] Bump `aws-smithy-protocol-test` crate version to `0.63` --- rust-runtime/aws-smithy-protocol-test/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-runtime/aws-smithy-protocol-test/Cargo.toml b/rust-runtime/aws-smithy-protocol-test/Cargo.toml index 2b4a119215..d732b169e2 100644 --- a/rust-runtime/aws-smithy-protocol-test/Cargo.toml +++ b/rust-runtime/aws-smithy-protocol-test/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "aws-smithy-protocol-test" -version = "0.62.0" +version = "0.63.0" authors = ["AWS Rust SDK Team ", "Russell Cohen "] description = "A collection of library functions to validate HTTP requests against Smithy protocol tests." edition = "2021" From e4fe8fb1fb7a26fee8f9424ab1477aa449c52d39 Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Tue, 1 Oct 2024 21:33:58 +0100 Subject: [PATCH 6/8] Fix comment --- rust-runtime/aws-smithy-protocol-test/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rust-runtime/aws-smithy-protocol-test/src/lib.rs b/rust-runtime/aws-smithy-protocol-test/src/lib.rs index c1d0e8220b..ddd6f56dc6 100644 --- a/rust-runtime/aws-smithy-protocol-test/src/lib.rs +++ b/rust-runtime/aws-smithy-protocol-test/src/lib.rs @@ -434,7 +434,8 @@ fn cbor_values_equal( })? } - // Convert `ciborium::Map` to sorted `BTreeMap` and then compare the sorted maps. + // Convert `ciborium::Value::Map` to a `HashMap`, and then compare the values of + // each key in `a` with those in `b`. (ciborium::Value::Map(a_map), ciborium::Value::Map(b_map)) => { let a_hashmap = ciborium_map_to_hashmap(a_map)?; let b_hashmap = ciborium_map_to_hashmap(b_map)?; From fce2d6d696c528471e2c803190435a58b56b6167 Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Wed, 2 Oct 2024 01:36:35 +0100 Subject: [PATCH 7/8] Use fully qualified ciborium namespaces to satisfy the cargo minimal-versions check command. --- .../aws-smithy-protocol-test/src/lib.rs | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/rust-runtime/aws-smithy-protocol-test/src/lib.rs b/rust-runtime/aws-smithy-protocol-test/src/lib.rs index ddd6f56dc6..9a69e4109b 100644 --- a/rust-runtime/aws-smithy-protocol-test/src/lib.rs +++ b/rust-runtime/aws-smithy-protocol-test/src/lib.rs @@ -415,17 +415,17 @@ fn try_json_eq(expected: &str, actual: &str) -> Result<(), ProtocolTestFailure> } } -/// Compares two `ciborium::Value` instances for semantic equality. +/// Compares two `ciborium::value::Value` instances for semantic equality. /// /// This function recursively compares two CBOR values, correctly handling arrays and maps /// according to the CBOR specification. Arrays are compared element-wise in order, /// while maps are compared without considering the order of key-value pairs. fn cbor_values_equal( - a: &ciborium::Value, - b: &ciborium::Value, + a: &ciborium::value::Value, + b: &ciborium::value::Value, ) -> Result { let result = match (a, b) { - (ciborium::Value::Array(a_array), ciborium::Value::Array(b_array)) => { + (ciborium::value::Value::Array(a_array), ciborium::value::Value::Array(b_array)) => { // Both arrays should be equal in size. a_array.len() == b_array.len() && // Compare arrays element-wise. @@ -434,9 +434,9 @@ fn cbor_values_equal( })? } - // Convert `ciborium::Value::Map` to a `HashMap`, and then compare the values of + // Convert `ciborium::value::Value::Map` to a `HashMap`, and then compare the values of // each key in `a` with those in `b`. - (ciborium::Value::Map(a_map), ciborium::Value::Map(b_map)) => { + (ciborium::value::Value::Map(a_map), ciborium::value::Value::Map(b_map)) => { let a_hashmap = ciborium_map_to_hashmap(a_map)?; let b_hashmap = ciborium_map_to_hashmap(b_map)?; @@ -455,7 +455,7 @@ fn cbor_values_equal( } } - (ciborium::Value::Float(a_float), ciborium::Value::Float(b_float)) => { + (ciborium::value::Value::Float(a_float), ciborium::value::Value::Float(b_float)) => { a_float == b_float || a_float.is_nan() && b_float.is_nan() } @@ -465,7 +465,7 @@ fn cbor_values_equal( Ok(result) } -/// Converts a `ciborium::Value::Map` into a `HashMap<&String, &ciborium::Value>`. +/// Converts a `ciborium::value::Value::Map` into a `HashMap<&String, &ciborium::value::Value>`. /// /// CBOR maps (`Value::Map`) are internally represented as vectors of key-value pairs, /// and direct comparison is affected by the order of these pairs. @@ -473,12 +473,12 @@ fn cbor_values_equal( /// this function transforms the vector into a `HashMap`, for order-independent comparisons /// between maps. fn ciborium_map_to_hashmap( - cbor_map: &[(ciborium::Value, ciborium::Value)], -) -> Result, ProtocolTestFailure> { + cbor_map: &[(ciborium::value::Value, ciborium::value::Value)], +) -> Result, ProtocolTestFailure> { cbor_map .iter() .map(|(key, value)| match key { - ciborium::Value::Text(key_str) => Ok((key_str, value)), + ciborium::value::Value::Text(key_str) => Ok((key_str, value)), _ => Err(ProtocolTestFailure::InvalidBodyFormat { expected: "a text key as map entry".to_string(), found: format!("{:?}", key), @@ -494,14 +494,12 @@ fn try_cbor_eq + Debug>( let decoded = base64_simd::STANDARD .decode_to_vec(expected_body) .expect("smithy protocol test `body` property is not properly base64 encoded"); - let expected_cbor_value: ciborium::Value = - ciborium::from_reader(decoded.as_slice()).expect("expected value must be valid CBOR"); - let actual_cbor_value: ciborium::Value = - ciborium::from_reader(actual_body.as_ref()).map_err(|e| { - ProtocolTestFailure::InvalidBodyFormat { - expected: "cbor".to_owned(), - found: format!("{} {:?}", e, actual_body), - } + let expected_cbor_value: ciborium::value::Value = + ciborium::de::from_reader(decoded.as_slice()).expect("expected value must be valid CBOR"); + let actual_cbor_value: ciborium::value::Value = ciborium::de::from_reader(actual_body.as_ref()) + .map_err(|e| ProtocolTestFailure::InvalidBodyFormat { + expected: "cbor".to_owned(), + found: format!("{} {:?}", e, actual_body), })?; let actual_body_base64 = base64_simd::STANDARD.encode_to_string(&actual_body); From 755dd28c8d32b1f3b11807be5a4115182b9aec8f Mon Sep 17 00:00:00 2001 From: Fahad Zubair Date: Wed, 2 Oct 2024 08:21:25 +0100 Subject: [PATCH 8/8] Simplify the match statements --- .../aws-smithy-protocol-test/src/lib.rs | 78 +++++++++++-------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/rust-runtime/aws-smithy-protocol-test/src/lib.rs b/rust-runtime/aws-smithy-protocol-test/src/lib.rs index 9a69e4109b..4d5d00029f 100644 --- a/rust-runtime/aws-smithy-protocol-test/src/lib.rs +++ b/rust-runtime/aws-smithy-protocol-test/src/lib.rs @@ -424,45 +424,50 @@ fn cbor_values_equal( a: &ciborium::value::Value, b: &ciborium::value::Value, ) -> Result { - let result = match (a, b) { + match (a, b) { (ciborium::value::Value::Array(a_array), ciborium::value::Value::Array(b_array)) => { // Both arrays should be equal in size. - a_array.len() == b_array.len() && + if a_array.len() != b_array.len() { + return Ok(false); + } // Compare arrays element-wise. - a_array.iter().zip(b_array.iter()).try_fold(true, |acc, (a_elem, b_elem)| { - cbor_values_equal(a_elem, b_elem).map(|equal| acc && equal) - })? + for (a_elem, b_elem) in a_array.iter().zip(b_array.iter()) { + if !cbor_values_equal(a_elem, b_elem)? { + return Ok(false); + } + } + Ok(true) } // Convert `ciborium::value::Value::Map` to a `HashMap`, and then compare the values of // each key in `a` with those in `b`. (ciborium::value::Value::Map(a_map), ciborium::value::Value::Map(b_map)) => { - let a_hashmap = ciborium_map_to_hashmap(a_map)?; - let b_hashmap = ciborium_map_to_hashmap(b_map)?; + if a_map.len() != b_map.len() { + return Ok(false); + } - if a_hashmap.len() != b_hashmap.len() { - false - } else { - // Each key in `a` should exist in `b`, and the values should match. - a_hashmap.iter().try_fold(true, |acc, (a_key, a_value)| { - b_hashmap - .get(a_key) - .map(|b_value| { - cbor_values_equal(a_value, b_value).map(|equal| acc && equal) - }) - .unwrap_or(Ok(false)) - })? + let b_hashmap = ciborium_map_to_hashmap(b_map)?; + // Each key in `a` should exist in `b`, and the values should match. + for a_key_value in a_map.iter() { + let (a_key, a_value) = get_text_key_value(a_key_value)?; + match b_hashmap.get(a_key) { + Some(b_value) => { + if !cbor_values_equal(a_value, b_value)? { + return Ok(false); + } + } + None => return Ok(false), + } } + Ok(true) } (ciborium::value::Value::Float(a_float), ciborium::value::Value::Float(b_float)) => { - a_float == b_float || a_float.is_nan() && b_float.is_nan() + Ok(a_float == b_float || (a_float.is_nan() && b_float.is_nan())) } - _ => a == b, - }; - - Ok(result) + _ => Ok(a == b), + } } /// Converts a `ciborium::value::Value::Map` into a `HashMap<&String, &ciborium::value::Value>`. @@ -475,16 +480,21 @@ fn cbor_values_equal( fn ciborium_map_to_hashmap( cbor_map: &[(ciborium::value::Value, ciborium::value::Value)], ) -> Result, ProtocolTestFailure> { - cbor_map - .iter() - .map(|(key, value)| match key { - ciborium::value::Value::Text(key_str) => Ok((key_str, value)), - _ => Err(ProtocolTestFailure::InvalidBodyFormat { - expected: "a text key as map entry".to_string(), - found: format!("{:?}", key), - }), - }) - .collect() + cbor_map.iter().map(get_text_key_value).collect() +} + +/// Extracts a string key and its associated value from a CBOR key-value pair. +/// Returns a `ProtocolTestFailure::InvalidBodyFormat` error if the key is not a text value. +fn get_text_key_value( + (key, value): &(ciborium::value::Value, ciborium::value::Value), +) -> Result<(&String, &ciborium::value::Value), ProtocolTestFailure> { + match key { + ciborium::value::Value::Text(key_str) => Ok((key_str, value)), + _ => Err(ProtocolTestFailure::InvalidBodyFormat { + expected: "a text key as map entry".to_string(), + found: format!("{:?}", key), + }), + } } fn try_cbor_eq + Debug>(