From 703f08d00a70661a263d780e6abb064174f5f8d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Mon, 11 Nov 2024 10:00:49 +0100 Subject: [PATCH 01/12] macros: SerializeValue force_exact_match -> forbid_excess_udt_fields This unifies the name of the attribute across SerializeValue and DeserializeValue. `forbid_excess_udt_fields` is arguably a better name for what it does than `force_exact_match`. --- scylla-cql/src/types/serialize/value.rs | 4 +-- scylla-macros/src/serialize/value.rs | 44 +++++++++++++++---------- scylla/src/macros.rs | 6 ++-- 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 22c3372c6..78bed4efc 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -2591,7 +2591,7 @@ mod tests { } #[derive(SerializeValue, Debug, PartialEq, Eq, Default)] - #[scylla(crate = crate, force_exact_match)] + #[scylla(crate = crate, forbid_excess_udt_fields)] struct TestStrictUdtWithFieldSorting { a: String, b: i32, @@ -2647,7 +2647,7 @@ mod tests { } #[derive(SerializeValue, Debug, PartialEq, Eq, Default)] - #[scylla(crate = crate, flavor = "enforce_order", force_exact_match)] + #[scylla(crate = crate, flavor = "enforce_order", forbid_excess_udt_fields)] struct TestStrictUdtWithEnforcedOrder { a: String, b: i32, diff --git a/scylla-macros/src/serialize/value.rs b/scylla-macros/src/serialize/value.rs index 6922d57dc..ea8b11add 100644 --- a/scylla-macros/src/serialize/value.rs +++ b/scylla-macros/src/serialize/value.rs @@ -19,8 +19,14 @@ struct Attributes { #[darling(default)] skip_name_checks: bool, + // If true, then the type checking code will require that the UDT does not + // contain excess fields at its suffix. Otherwise, if UDT has some fields + // at its suffix that do not correspond to Rust struct's fields, + // they will be sent as NULLs (if they are in the middle of the UDT) or not + // sent at all (if they are in the prefix of the UDT), which means that + // the DB will interpret them as NULLs anyway. #[darling(default)] - force_exact_match: bool, + forbid_excess_udt_fields: bool, } impl Attributes { @@ -227,21 +233,25 @@ impl<'a> Generator for FieldSortingGenerator<'a> { let udt_field_names = rust_field_names.clone(); // For now, it's the same let field_types = self.ctx.fields.iter().map(|f| &f.ty).collect::>(); - let missing_rust_field_expression: syn::Expr = if self.ctx.attributes.force_exact_match { - parse_quote! { - return ::std::result::Result::Err(mk_typck_err( - #crate_path::UdtTypeCheckErrorKind::NoSuchFieldInUdt { - field_name: <_ as ::std::clone::Clone>::clone(field_name).into_owned(), - } - )) - } - } else { - parse_quote! { - skipped_fields += 1 - } - }; + let missing_rust_field_expression: syn::Expr = + if self.ctx.attributes.forbid_excess_udt_fields { + parse_quote! { + return ::std::result::Result::Err(mk_typck_err( + #crate_path::UdtTypeCheckErrorKind::NoSuchFieldInUdt { + field_name: <_ as ::std::clone::Clone>::clone(field_name).into_owned(), + } + )) + } + } else { + parse_quote! { + skipped_fields += 1 + } + }; - let serialize_missing_nulls_statement: syn::Stmt = if self.ctx.attributes.force_exact_match + let serialize_missing_nulls_statement: syn::Stmt = if self + .ctx + .attributes + .forbid_excess_udt_fields { // Not sure if there is better way to create no-op statement // parse_quote!{} / parse_quote!{ ; } doesn't work @@ -287,7 +297,7 @@ impl<'a> Generator for FieldSortingGenerator<'a> { // nothing for those fields at the end of UDT. While executing the loop // we don't know if there will be any more present fields. The solution is // to count how many fields we missed and send them when we find any present field. - if !self.ctx.attributes.force_exact_match { + if !self.ctx.attributes.forbid_excess_udt_fields { statements.push(parse_quote! { let mut skipped_fields = 0; }); @@ -445,7 +455,7 @@ impl<'a> Generator for FieldOrderedGenerator<'a> { }); } - if self.ctx.attributes.force_exact_match { + if self.ctx.attributes.forbid_excess_udt_fields { // Check whether there are some fields remaining statements.push(parse_quote! { if let Some((field_name, typ)) = field_iter.next() { diff --git a/scylla/src/macros.rs b/scylla/src/macros.rs index f5bef190f..f08c29b66 100644 --- a/scylla/src/macros.rs +++ b/scylla/src/macros.rs @@ -44,12 +44,12 @@ pub use scylla_cql::macros::IntoUserType; /// - serialization will succeed in "match_by_name" flavor (default). Missing /// fields in the middle of UDT will be sent as NULLs, missing fields at the end will not be sent /// at all. -/// - serialization will succed if suffix of UDT fields is missing. If there are missing fields in the +/// - serialization will succeed if suffix of UDT fields is missing. If there are missing fields in the /// middle it will fail. Note that if "skip_name_checks" is enabled, and the types happen to match, /// it is possible for serialization to succeed with unexpected result. /// /// This behavior is the default to support ALTERing UDTs by adding new fields. -/// You can require exact match of fields using `force_exact_match` attribute. +/// You can forbid excess fields in the UDT using `forbid_excess_udt_fields` attribute. /// /// In case of failure, either [`BuiltinTypeCheckError`](crate::serialize::value::BuiltinTypeCheckError) /// or [`BuiltinSerializationError`](crate::serialize::value::BuiltinSerializationError) @@ -125,7 +125,7 @@ pub use scylla_cql::macros::IntoUserType; /// struct field names and UDT field names, i.e. it's OK if i-th field has a /// different name in Rust and in the UDT. Fields are still being type-checked. /// -/// `#[scylla(force_exact_match)]` +/// `#[scylla(forbid_excess_udt_fields)]` /// /// Forces Rust struct to have all the fields present in UDT, otherwise /// serialization fails. From 382dc1e0c9470e23a11acec85b1c04ebf28275ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 09:30:19 +0100 Subject: [PATCH 02/12] macros: add tests for attribute verification in SerializeValue They are taken from DeserializeValue. --- scylla-cql/src/types/serialize/value.rs | 46 +++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 78bed4efc..39bc35ac2 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -1496,6 +1496,52 @@ pub enum ValueToSerializeValueAdapterError { }, } +mod doctests { + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql, skip_name_checks)] + /// struct TestUdt {} + /// ``` + fn _test_udt_bad_attributes_skip_name_check_requires_enforce_order() {} + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql, flavor = "enforce_order", skip_name_checks)] + /// struct TestUdt { + /// #[scylla(rename = "b")] + /// a: i32, + /// } + /// ``` + fn _test_udt_bad_attributes_skip_name_check_conflicts_with_rename() {} + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql)] + /// struct TestUdt { + /// #[scylla(rename = "b")] + /// a: i32, + /// b: String, + /// } + /// ``` + fn _test_udt_bad_attributes_rename_collision_with_field() {} + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql)] + /// struct TestUdt { + /// #[scylla(rename = "c")] + /// a: i32, + /// #[scylla(rename = "c")] + /// b: String, + /// } + /// ``` + fn _test_udt_bad_attributes_rename_collision_with_another_rename() {} +} + #[cfg(test)] mod tests { use std::collections::BTreeMap; From 0399b2f60986d5a38492e37c21c39dee7a315310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 09:27:58 +0100 Subject: [PATCH 03/12] macros: add tests for attribute verification in SerializeRow They are taken from DeserializeRow. --- scylla-cql/src/types/serialize/row.rs | 47 +++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index 3888a122b..9e4aa949d 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -867,6 +867,53 @@ impl<'a> Iterator for SerializedValuesIterator<'a> { } } +mod doctests { + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeRow)] + /// #[scylla(crate = scylla_cql, skip_name_checks)] + /// struct TestRow {} + /// ``` + fn _test_struct_deserialization_name_check_skip_requires_enforce_order() {} + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeRow)] + /// #[scylla(crate = scylla_cql, skip_name_checks)] + /// struct TestRow { + /// #[scylla(rename = "b")] + /// a: i32, + /// } + /// ``` + fn _test_struct_deserialization_skip_name_check_conflicts_with_rename() {} + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeRow)] + /// #[scylla(crate = scylla_cql)] + /// struct TestRow { + /// #[scylla(rename = "b")] + /// a: i32, + /// b: String, + /// } + /// ``` + fn _test_struct_deserialization_skip_rename_collision_with_field() {} + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeRow)] + /// #[scylla(crate = scylla_cql)] + /// struct TestRow { + /// #[scylla(rename = "c")] + /// a: i32, + /// #[scylla(rename = "c")] + /// b: String, + /// } + /// ``` + fn _test_struct_deserialization_rename_collision_with_another_rename() {} +} + #[cfg(test)] mod tests { use std::borrow::Cow; From 580bfd696080b9a0c2a5d306379fc8e367cf3bfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Mon, 11 Nov 2024 10:06:58 +0100 Subject: [PATCH 04/12] macros: share Flavor between ser and deser macros --- scylla-macros/src/lib.rs | 20 +++++++++++++++++++- scylla-macros/src/serialize/mod.rs | 19 ------------------- scylla-macros/src/serialize/row.rs | 2 +- scylla-macros/src/serialize/value.rs | 2 +- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/scylla-macros/src/lib.rs b/scylla-macros/src/lib.rs index 05e24362f..05c22a32f 100644 --- a/scylla-macros/src/lib.rs +++ b/scylla-macros/src/lib.rs @@ -1,4 +1,4 @@ -use darling::ToTokens; +use darling::{FromMeta, ToTokens}; use proc_macro::TokenStream; mod from_row; @@ -7,6 +7,24 @@ mod into_user_type; mod parser; mod value_list; +// Flavor of serialization/deserialization macros ({De,S}erialize{Value,Row}). +#[derive(Copy, Clone, PartialEq, Eq, Default)] +enum Flavor { + #[default] + MatchByName, + EnforceOrder, +} + +impl FromMeta for Flavor { + fn from_string(value: &str) -> darling::Result { + match value { + "match_by_name" => Ok(Self::MatchByName), + "enforce_order" => Ok(Self::EnforceOrder), + _ => Err(darling::Error::unknown_value(value)), + } + } +} + mod serialize; /// Documentation for this macro can only be found diff --git a/scylla-macros/src/serialize/mod.rs b/scylla-macros/src/serialize/mod.rs index b9c1fa985..f294d590b 100644 --- a/scylla-macros/src/serialize/mod.rs +++ b/scylla-macros/src/serialize/mod.rs @@ -1,21 +1,2 @@ -use darling::FromMeta; - pub(crate) mod row; pub(crate) mod value; - -#[derive(Copy, Clone, PartialEq, Eq, Default)] -enum Flavor { - #[default] - MatchByName, - EnforceOrder, -} - -impl FromMeta for Flavor { - fn from_string(value: &str) -> darling::Result { - match value { - "match_by_name" => Ok(Self::MatchByName), - "enforce_order" => Ok(Self::EnforceOrder), - _ => Err(darling::Error::unknown_value(value)), - } - } -} diff --git a/scylla-macros/src/serialize/row.rs b/scylla-macros/src/serialize/row.rs index aef1c7db4..9e4a991f6 100644 --- a/scylla-macros/src/serialize/row.rs +++ b/scylla-macros/src/serialize/row.rs @@ -5,7 +5,7 @@ use proc_macro::TokenStream; use proc_macro2::Span; use syn::parse_quote; -use super::Flavor; +use crate::Flavor; #[derive(FromAttributes)] #[darling(attributes(scylla))] diff --git a/scylla-macros/src/serialize/value.rs b/scylla-macros/src/serialize/value.rs index ea8b11add..75e2449df 100644 --- a/scylla-macros/src/serialize/value.rs +++ b/scylla-macros/src/serialize/value.rs @@ -5,7 +5,7 @@ use proc_macro::TokenStream; use proc_macro2::Span; use syn::parse_quote; -use super::Flavor; +use crate::Flavor; #[derive(FromAttributes)] #[darling(attributes(scylla))] From 8539e16a6d3ee81a7984b7fdbd8f405d64761105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Mon, 11 Nov 2024 10:14:11 +0100 Subject: [PATCH 05/12] macros: DeserializeValue uses Flavor Instead of taking `enforce_order` as boolean flag, DeserializeValue now takes a string (e.g., `flavor = "enforce_order"`) and converts it to a variant of the Flavor enum. This way DeserializeValue is unified with SerializeValue wrt flavor selection. --- scylla-cql/src/types/deserialize/value.rs | 4 +-- .../src/types/deserialize/value_tests.rs | 10 +++---- scylla-macros/src/deserialize/value.rs | 29 +++++++++---------- scylla/src/macros.rs | 26 ++++++++++------- 4 files changed, 36 insertions(+), 33 deletions(-) diff --git a/scylla-cql/src/types/deserialize/value.rs b/scylla-cql/src/types/deserialize/value.rs index 492984c3f..2191fd9be 100644 --- a/scylla-cql/src/types/deserialize/value.rs +++ b/scylla-cql/src/types/deserialize/value.rs @@ -1963,7 +1963,7 @@ fn _test_udt_bad_attributes_skip_name_check_requires_enforce_order() {} /// ```compile_fail /// /// #[derive(scylla_macros::DeserializeValue)] -/// #[scylla(crate = scylla_cql, enforce_order, skip_name_checks)] +/// #[scylla(crate = scylla_cql, flavor = "enforce_order", skip_name_checks)] /// struct TestUdt { /// #[scylla(rename = "b")] /// a: i32, @@ -1999,7 +1999,7 @@ fn _test_udt_bad_attributes_rename_collision_with_another_rename() {} /// ```compile_fail /// /// #[derive(scylla_macros::DeserializeValue)] -/// #[scylla(crate = scylla_cql, enforce_order, skip_name_checks)] +/// #[scylla(crate = scylla_cql, flavor = "enforce_order", skip_name_checks)] /// struct TestUdt { /// a: i32, /// #[scylla(allow_missing)] diff --git a/scylla-cql/src/types/deserialize/value_tests.rs b/scylla-cql/src/types/deserialize/value_tests.rs index 2ce5d0ea4..0b507cb13 100644 --- a/scylla-cql/src/types/deserialize/value_tests.rs +++ b/scylla-cql/src/types/deserialize/value_tests.rs @@ -632,7 +632,7 @@ struct TestUdtWithNoFieldsUnordered {} #[allow(unused)] #[derive(scylla_macros::DeserializeValue)] -#[scylla(crate = crate, enforce_order)] +#[scylla(crate = crate, flavor = "enforce_order")] struct TestUdtWithNoFieldsOrdered {} #[test] @@ -786,7 +786,7 @@ fn test_udt_loose_ordering() { #[test] fn test_udt_strict_ordering() { #[derive(scylla_macros::DeserializeValue, PartialEq, Eq, Debug)] - #[scylla(crate = "crate", enforce_order)] + #[scylla(crate = "crate", flavor = "enforce_order")] struct Udt<'a> { #[scylla(default_when_null)] a: &'a str, @@ -860,7 +860,7 @@ fn test_udt_strict_ordering() { // An excess field at the end of UDT, when such are forbidden { #[derive(scylla_macros::DeserializeValue, PartialEq, Eq, Debug)] - #[scylla(crate = "crate", enforce_order, forbid_excess_udt_fields)] + #[scylla(crate = "crate", flavor = "enforce_order", forbid_excess_udt_fields)] struct Udt<'a> { a: &'a str, #[scylla(skip)] @@ -934,7 +934,7 @@ fn test_udt_strict_ordering() { #[test] fn test_udt_no_name_check() { #[derive(scylla_macros::DeserializeValue, PartialEq, Eq, Debug)] - #[scylla(crate = "crate", enforce_order, skip_name_checks)] + #[scylla(crate = "crate", flavor = "enforce_order", skip_name_checks)] struct Udt<'a> { a: &'a str, #[scylla(skip)] @@ -1762,7 +1762,7 @@ fn test_udt_errors() { // Strict ordering { #[derive(scylla_macros::DeserializeValue, PartialEq, Eq, Debug)] - #[scylla(crate = "crate", enforce_order, forbid_excess_udt_fields)] + #[scylla(crate = "crate", flavor = "enforce_order", forbid_excess_udt_fields)] struct Udt<'a> { a: &'a str, #[scylla(skip)] diff --git a/scylla-macros/src/deserialize/value.rs b/scylla-macros/src/deserialize/value.rs index 1e1efdf0a..de7a752e6 100644 --- a/scylla-macros/src/deserialize/value.rs +++ b/scylla-macros/src/deserialize/value.rs @@ -5,6 +5,8 @@ use proc_macro::TokenStream; use proc_macro2::Span; use syn::{ext::IdentExt, parse_quote}; +use crate::Flavor; + use super::{DeserializeCommonFieldAttrs, DeserializeCommonStructAttrs}; #[derive(FromAttributes)] @@ -13,12 +15,8 @@ struct StructAttrs { #[darling(rename = "crate")] crate_path: Option, - // If true, then the type checking code will require the order of the fields - // to be the same in both the Rust struct and the UDT. This allows the - // deserialization to be slightly faster because looking struct fields up - // by name can be avoided, though it is less convenient. #[darling(default)] - enforce_order: bool, + flavor: Flavor, // If true, then the type checking code won't verify the UDT field names. // UDT fields will be matched to struct fields based solely on the order. @@ -112,9 +110,10 @@ fn validate_attrs(attrs: &StructAttrs, fields: &[Field]) -> Result<(), darling:: if attrs.skip_name_checks { // Skipping name checks is only available in enforce_order mode - if !attrs.enforce_order { - let error = - darling::Error::custom("attribute requires ."); + if attrs.flavor != Flavor::EnforceOrder { + let error = darling::Error::custom( + "attribute requires .", + ); errors.push(error); } @@ -207,18 +206,16 @@ impl StructDesc { } fn generate_type_check_method(&self) -> syn::ImplItemFn { - if self.attrs.enforce_order { - TypeCheckAssumeOrderGenerator(self).generate() - } else { - TypeCheckUnorderedGenerator(self).generate() + match self.attrs.flavor { + Flavor::MatchByName => TypeCheckUnorderedGenerator(self).generate(), + Flavor::EnforceOrder => TypeCheckAssumeOrderGenerator(self).generate(), } } fn generate_deserialize_method(&self) -> syn::ImplItemFn { - if self.attrs.enforce_order { - DeserializeAssumeOrderGenerator(self).generate() - } else { - DeserializeUnorderedGenerator(self).generate() + match self.attrs.flavor { + Flavor::MatchByName => DeserializeUnorderedGenerator(self).generate(), + Flavor::EnforceOrder => DeserializeAssumeOrderGenerator(self).generate(), } } } diff --git a/scylla/src/macros.rs b/scylla/src/macros.rs index f08c29b66..d21349d09 100644 --- a/scylla/src/macros.rs +++ b/scylla/src/macros.rs @@ -307,20 +307,26 @@ pub use scylla_cql::macros::SerializeRow; /// macro itself, so in those cases the user must provide an alternative path /// to either the `scylla` or `scylla-cql` crate. /// -/// `#[scylla(enforce_order)]` /// -/// By default, the generated deserialization code will be insensitive -/// to the UDT field order - when processing a field, it will look it up -/// in the Rust struct with the corresponding field and set it. However, -/// if the UDT field order is known to be the same both in the UDT -/// and the Rust struct, then the `enforce_order` annotation can be used -/// so that a more efficient implementation that does not perform lookups -/// is be generated. The UDT field names will still be checked during the -/// type check phase. +/// `#[scylla(flavor = "flavor_name")]` +/// +/// Allows to choose one of the possible "flavors", i.e. the way how the +/// generated code will approach deserialization. Possible flavors are: +/// +/// - `"match_by_name"` (default) - the generated implementation _does not +/// require_ the fields in the Rust struct to be in the same order as the +/// fields in the UDT. During deserialization, the implementation will take +/// care to deserialize the fields in the order which the database expects. +/// - `"enforce_order"` - the generated implementation _requires_ the fields +/// in the Rust struct to be in the same order as the fields in the UDT. +/// If the order is incorrect, type checking/deserialization will fail. +/// This is a less robust flavor than `"match_by_name"`, but should be +/// slightly more performant as it doesn't need to perform lookups by name. +/// The UDT field names will still be checked during the type check phase. /// /// #[(scylla(skip_name_checks))] /// -/// This attribute only works when used with `enforce_order`. +/// This attribute only works when used with `flavor = "enforce_order"`. /// /// If set, the generated implementation will not verify the UDT field names at /// all. Because it only works with `enforce_order`, it will deserialize first From 66b0172595f42dd2849998dd35874b54bee888ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Mon, 11 Nov 2024 10:21:31 +0100 Subject: [PATCH 06/12] macros: DeserializeRow uses Flavor Instead of taking `enforce_order` as boolean flag, DeserializeRow now takes a string (e.g., `flavor = "enforce_order"`) and converts it to a variant of the Flavor enum. This way DeserializeRow is unified with SerializeRow wrt flavor selection. --- scylla-cql/src/types/deserialize/row_tests.rs | 8 +++---- scylla-macros/src/deserialize/row.rs | 24 ++++++++----------- scylla/src/macros.rs | 24 ++++++++++++------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/scylla-cql/src/types/deserialize/row_tests.rs b/scylla-cql/src/types/deserialize/row_tests.rs index 9fdbc8586..eb10e3f03 100644 --- a/scylla-cql/src/types/deserialize/row_tests.rs +++ b/scylla-cql/src/types/deserialize/row_tests.rs @@ -91,7 +91,7 @@ struct TestUdtWithNoFieldsUnordered {} #[allow(unused)] #[derive(DeserializeRow)] -#[scylla(crate = crate, enforce_order)] +#[scylla(crate = crate, flavor = "enforce_order")] struct TestUdtWithNoFieldsOrdered {} #[test] @@ -143,7 +143,7 @@ fn test_struct_deserialization_loose_ordering() { #[test] fn test_struct_deserialization_strict_ordering() { #[derive(DeserializeRow, PartialEq, Eq, Debug)] - #[scylla(crate = "crate", enforce_order)] + #[scylla(crate = "crate", flavor = "enforce_order")] struct MyRow<'a> { a: &'a str, b: Option, @@ -180,7 +180,7 @@ fn test_struct_deserialization_strict_ordering() { #[test] fn test_struct_deserialization_no_name_check() { #[derive(DeserializeRow, PartialEq, Eq, Debug)] - #[scylla(crate = "crate", enforce_order, skip_name_checks)] + #[scylla(crate = "crate", flavor = "enforce_order", skip_name_checks)] struct MyRow<'a> { a: &'a str, b: Option, @@ -623,7 +623,7 @@ fn test_struct_deserialization_errors() { // Strict ordering { #[derive(scylla_macros::DeserializeRow, PartialEq, Eq, Debug)] - #[scylla(crate = "crate", enforce_order)] + #[scylla(crate = "crate", flavor = "enforce_order")] struct MyRow<'a> { a: &'a str, #[scylla(skip)] diff --git a/scylla-macros/src/deserialize/row.rs b/scylla-macros/src/deserialize/row.rs index 7fe56edd2..01ba2a0e4 100644 --- a/scylla-macros/src/deserialize/row.rs +++ b/scylla-macros/src/deserialize/row.rs @@ -5,6 +5,8 @@ use proc_macro2::Span; use syn::ext::IdentExt; use syn::parse_quote; +use crate::Flavor; + use super::{DeserializeCommonFieldAttrs, DeserializeCommonStructAttrs}; #[derive(FromAttributes)] @@ -13,12 +15,8 @@ struct StructAttrs { #[darling(rename = "crate")] crate_path: Option, - // If true, then the type checking code will require the order of the fields - // to be the same in both the Rust struct and the columns. This allows the - // deserialization to be slightly faster because looking struct fields up - // by name can be avoided, though it is less convenient. #[darling(default)] - enforce_order: bool, + flavor: Flavor, // If true, then the type checking code won't verify the column names. // Columns will be matched to struct fields based solely on the order. @@ -94,7 +92,7 @@ fn validate_attrs(attrs: &StructAttrs, fields: &[Field]) -> Result<(), darling:: if attrs.skip_name_checks { // Skipping name checks is only available in enforce_order mode - if !attrs.enforce_order { + if attrs.flavor != Flavor::EnforceOrder { let error = darling::Error::custom("attribute requires ."); errors.push(error); @@ -153,18 +151,16 @@ type StructDesc = super::StructDescForDeserialize; impl StructDesc { fn generate_type_check_method(&self) -> syn::ImplItemFn { - if self.attrs.enforce_order { - TypeCheckAssumeOrderGenerator(self).generate() - } else { - TypeCheckUnorderedGenerator(self).generate() + match self.attrs.flavor { + Flavor::MatchByName => TypeCheckUnorderedGenerator(self).generate(), + Flavor::EnforceOrder => TypeCheckAssumeOrderGenerator(self).generate(), } } fn generate_deserialize_method(&self) -> syn::ImplItemFn { - if self.attrs.enforce_order { - DeserializeAssumeOrderGenerator(self).generate() - } else { - DeserializeUnorderedGenerator(self).generate() + match self.attrs.flavor { + Flavor::MatchByName => DeserializeUnorderedGenerator(self).generate(), + Flavor::EnforceOrder => DeserializeAssumeOrderGenerator(self).generate(), } } } diff --git a/scylla/src/macros.rs b/scylla/src/macros.rs index d21349d09..6549507be 100644 --- a/scylla/src/macros.rs +++ b/scylla/src/macros.rs @@ -443,19 +443,25 @@ pub use scylla_macros::DeserializeValue; /// macro itself, so in those cases the user must provide an alternative path /// to either the `scylla` or `scylla-cql` crate. /// -/// `#[scylla(enforce_order)]` +/// `#[scylla(flavor = "flavor_name")]` /// -/// By default, the generated deserialization code will be insensitive -/// to the column order - when processing a column, the corresponding Rust field -/// will be looked up and the column will be deserialized based on its type. -/// However, if the column order and the Rust field order is known to be the -/// same, then the `enforce_order` annotation can be used so that a more -/// efficient implementation that does not perform lookups is be generated. -/// The generated code will still check that the column and field names match. +/// Allows to choose one of the possible "flavors", i.e. the way how the +/// generated code will approach deserialization. Possible flavors are: +/// +/// - `"match_by_name"` (default) - the generated implementation _does not +/// require_ the fields in the Rust struct to be in the same order as the +/// columns in the row. During deserialization, the implementation will take +/// care to deserialize the columns in the order which the database provided. +/// - `"enforce_order"` - the generated implementation _requires_ the fields +/// in the Rust struct to be in the same order as the columns in the row. +/// If the order is incorrect, type checking/deserialization will fail. +/// This is a less robust flavor than `"match_by_name"`, but should be +/// slightly more performant as it doesn't need to perform lookups by name. +/// The generated code will still check that the column and field names match. /// /// #[(scylla(skip_name_checks))] /// -/// This attribute only works when used with `enforce_order`. +/// This attribute only works when used with `flavor = "enforce_order"`. /// /// If set, the generated implementation will not verify the column names at /// all. Because it only works with `enforce_order`, it will deserialize first From 941684ae35a0012ba725068529bb7c39b7bf198d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 09:36:46 +0100 Subject: [PATCH 07/12] macros: SerializeValue supports #[allow_missing] The `#[allow_missing]` attibute is intented to allow a flexible transition period when schema is altered. Namely, if a UDT is extended with a new field and the transition begins with migrating clients to the new schema (extending the Rust struct with a new field), then server may still have the old schema and provide metadata that is missing the new field. In such case, `#[allow_missing]` can be attached to the new Rust struct's field and this way handle the situation: - in deserialization, the field missing from DB metadata will be default-initialized, - in serialization, such a field will be ignored and not serialized. This commit adds support for this attribute: for parsing it using `darling`, and for its semantics in both `match_by_name` and `enforce_order` flavors. Also, corresponding (doc)tests for attribute sanity verification are added, fully based on those from DeserializeValue. --- scylla-cql/src/types/serialize/value.rs | 39 +++++++++++ scylla-macros/src/serialize/value.rs | 87 +++++++++++++++++++++---- 2 files changed, 113 insertions(+), 13 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 39bc35ac2..5497baeb8 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -1540,6 +1540,45 @@ mod doctests { /// } /// ``` fn _test_udt_bad_attributes_rename_collision_with_another_rename() {} + + /// ```compile_fail + /// + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql, flavor = "enforce_order", skip_name_checks)] + /// struct TestUdt { + /// a: i32, + /// #[scylla(allow_missing)] + /// b: bool, + /// c: String, + /// } + /// ``` + fn _test_udt_bad_attributes_name_skip_name_checks_limitations_on_allow_missing() {} + + /// ``` + /// + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql, flavor = "enforce_order", skip_name_checks)] + /// struct TestUdt { + /// a: i32, + /// #[scylla(allow_missing)] + /// b: bool, + /// #[scylla(allow_missing)] + /// c: String, + /// } + /// ``` + fn _test_udt_good_attributes_name_skip_name_checks_limitations_on_allow_missing() {} + + /// ``` + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql)] + /// struct TestUdt { + /// a: i32, + /// #[scylla(allow_missing)] + /// b: bool, + /// c: String, + /// } + /// ``` + fn _test_udt_unordered_flavour_no_limitations_on_allow_missing() {} } #[cfg(test)] diff --git a/scylla-macros/src/serialize/value.rs b/scylla-macros/src/serialize/value.rs index 75e2449df..59055592b 100644 --- a/scylla-macros/src/serialize/value.rs +++ b/scylla-macros/src/serialize/value.rs @@ -51,6 +51,12 @@ impl Field { None => self.ident.to_string(), } } + + // Returns whether this field must be serialized (can't be ignored in case + // that there is no corresponding field in UDT). + fn is_required(&self) -> bool { + !self.attrs.skip && !self.attrs.ignore_missing + } } #[derive(FromAttributes)] @@ -60,6 +66,12 @@ struct FieldAttributes { #[darling(default)] skip: bool, + + // If true, then - if this field is missing from the UDT fields metadata + // - it will be ignored during serialization. + #[darling(default)] + #[darling(rename = "allow_missing")] + ignore_missing: bool, } struct Context { @@ -125,6 +137,28 @@ impl Context { errors.push(err); } + // When name checks are skipped, fields with `allow_missing` are only + // permitted at the end of the struct, i.e. no field without + // `allow_missing` and `skip` is allowed to be after any field + // with `allow_missing`. + let invalid_default_when_missing_field = self + .fields + .iter() + .rev() + // Skip the whole suffix of and . + .skip_while(|field| !field.is_required()) + // skip_while finished either because the iterator is empty or it found a field without both and . + // In either case, there aren't allowed to be any more fields with `allow_missing`. + .find(|field| field.attrs.ignore_missing); + if let Some(invalid) = invalid_default_when_missing_field { + let error = + darling::Error::custom( + "when `skip_name_checks` is on, fields with `allow_missing` are only permitted at the end of the struct, \ + i.e. no field without `allow_missing` and `skip` is allowed to be after any field with `allow_missing`." + ).with_span(&invalid.ident); + errors.push(error); + } + // `rename` annotations don't make sense with skipped name checks for field in self.fields.iter() { if field.attrs.rename.is_some() { @@ -230,6 +264,8 @@ impl<'a> Generator for FieldSortingGenerator<'a> { .iter() .map(|f| f.field_name()) .collect::>(); + let rust_field_ignore_missing_flags = + self.ctx.fields.iter().map(|f| f.attrs.ignore_missing); let udt_field_names = rust_field_names.clone(); // For now, it's the same let field_types = self.ctx.fields.iter().map(|f| &f.ty).collect::>(); @@ -278,15 +314,32 @@ impl<'a> Generator for FieldSortingGenerator<'a> { .generate_udt_type_match(parse_quote!(#crate_path::UdtTypeCheckErrorKind::NotUdt)), ); + fn make_visited_flag_ident(field_name: &str) -> syn::Ident { + syn::Ident::new(&format!("visited_flag_{}", field_name), Span::call_site()) + } + // Generate a "visited" flag for each field let visited_flag_names = rust_field_names .iter() - .map(|s| syn::Ident::new(&format!("visited_flag_{}", s), Span::call_site())) + .map(|s| make_visited_flag_ident(s)) .collect::>(); statements.extend::>(parse_quote! { #(let mut #visited_flag_names = false;)* }); + // An iterator over names of Rust fields that can't be ignored + // (i.e., if UDT misses a corresponding field, an error should be raised). + let nonignorable_rust_field_names = self + .ctx + .fields + .iter() + .filter(|f| !f.attrs.ignore_missing) + .map(|f| f.field_name()); + // An iterator over visited flags of Rust fields that can't be ignored + // (i.e., if UDT misses a corresponding field, an error should be raised). + let nonignorable_visited_flag_names = + nonignorable_rust_field_names.map(|s| make_visited_flag_ident(&s)); + // Generate a variable that counts down visited fields. let field_count = self.ctx.fields.len(); statements.push(parse_quote! { @@ -340,11 +393,12 @@ impl<'a> Generator for FieldSortingGenerator<'a> { }); // Finally, check that all fields were consumed. - // If there are some missing fields, return an error + // If there are some missing fields that don't have the `#[allow_missing]` + // attribute on them, return an error. statements.push(parse_quote! { if remaining_count > 0 { #( - if !#visited_flag_names { + if !#nonignorable_visited_flag_names && !#rust_field_ignore_missing_flags { return ::std::result::Result::Err(mk_typck_err( #crate_path::UdtTypeCheckErrorKind::ValueMissingForUdtField { field_name: <_ as ::std::string::ToString>::to_string(#rust_field_names), @@ -352,7 +406,6 @@ impl<'a> Generator for FieldSortingGenerator<'a> { )); } )* - ::std::unreachable!() } }); @@ -404,15 +457,16 @@ impl<'a> Generator for FieldOrderedGenerator<'a> { let mut builder = #crate_path::CellWriter::into_value_builder(writer); }); - // Create an iterator over fields + // Create a peekable iterator over fields. statements.push(parse_quote! { - let mut field_iter = field_types.iter(); + let mut field_iter = field_types.iter().peekable(); }); // Serialize each field for field in self.ctx.fields.iter() { let rust_field_ident = &field.ident; let rust_field_name = field.field_name(); + let field_can_be_ignored = field.attrs.ignore_missing; let typ = &field.ty; let name_check_expression: syn::Expr = if !self.ctx.attributes.skip_name_checks { parse_quote! { field_name == #rust_field_name } @@ -420,9 +474,12 @@ impl<'a> Generator for FieldOrderedGenerator<'a> { parse_quote! { true } }; statements.push(parse_quote! { - match field_iter.next() { + match field_iter.peek() { Some((field_name, typ)) => { if #name_check_expression { + // Advance the iterator. + field_iter.next(); + let sub_builder = #crate_path::CellValueBuilder::make_sub_writer(&mut builder); match <#typ as #crate_path::SerializeValue>::serialize(&self.#rust_field_ident, typ, sub_builder) { Ok(_proof) => {}, @@ -435,7 +492,7 @@ impl<'a> Generator for FieldOrderedGenerator<'a> { )); } } - } else { + } else if !#field_can_be_ignored { return ::std::result::Result::Err(mk_typck_err( #crate_path::UdtTypeCheckErrorKind::FieldNameMismatch { rust_field_name: <_ as ::std::string::ToString>::to_string(#rust_field_name), @@ -443,13 +500,17 @@ impl<'a> Generator for FieldOrderedGenerator<'a> { } )); } + // Else simply ignore the field. } None => { - return ::std::result::Result::Err(mk_typck_err( - #crate_path::UdtTypeCheckErrorKind::ValueMissingForUdtField { - field_name: <_ as ::std::string::ToString>::to_string(#rust_field_name), - } - )); + if !#field_can_be_ignored { + return ::std::result::Result::Err(mk_typck_err( + #crate_path::UdtTypeCheckErrorKind::ValueMissingForUdtField { + field_name: <_ as ::std::string::ToString>::to_string(#rust_field_name), + } + )); + } + // Else the field is ignored and we continue with other fields. } } }); From 1269002174e2bff4849ad56f8d11434f27bdf022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 09:42:54 +0100 Subject: [PATCH 08/12] macros: SerializeValue supports #[default_when_null] The `#[default_when_null]` attibute is intented to allow a flexible transition period when schema is altered. Namely, if a UDT is extended with a new field, then server will populate the new column with NULLs. If the data model does not abstractly permit NULLs in that column, one may not want to represent the field with `Option`, not to have to handle the case of `None`. In such case, `#[default_when_null]` can be attached to the new Rust struct's field and this way handle the situation: - in deserialization, the NULL field received from the DB will be default-initialized, - in serialization, there is no corresponding situation, so this attribute does nothing. This commit adds support for this attribute: for parsing it using `darling`, and that's it - because it's a no-op. Also, a corresponding (doc)test is added to confirm that the attibute is parsed correctly. --- scylla-cql/src/types/serialize/value.rs | 12 ++++++++++++ scylla-macros/src/serialize/value.rs | 5 +++++ 2 files changed, 17 insertions(+) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 5497baeb8..32ed0cd54 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -1579,6 +1579,18 @@ mod doctests { /// } /// ``` fn _test_udt_unordered_flavour_no_limitations_on_allow_missing() {} + + /// ``` + /// #[derive(scylla_macros::SerializeValue)] + /// #[scylla(crate = scylla_cql)] + /// struct TestUdt { + /// a: i32, + /// #[scylla(default_when_null)] + /// b: bool, + /// c: String, + /// } + /// ``` + fn _test_udt_default_when_null_is_accepted() {} } #[cfg(test)] diff --git a/scylla-macros/src/serialize/value.rs b/scylla-macros/src/serialize/value.rs index 59055592b..101d1c244 100644 --- a/scylla-macros/src/serialize/value.rs +++ b/scylla-macros/src/serialize/value.rs @@ -72,6 +72,11 @@ struct FieldAttributes { #[darling(default)] #[darling(rename = "allow_missing")] ignore_missing: bool, + + // Used for deserialization only. Ignored in serialization. + #[darling(default)] + #[darling(rename = "default_when_null")] + _default_when_null: bool, } struct Context { From d553f94bb675b961d03b29e7fd577b3c06dcb189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 09:48:32 +0100 Subject: [PATCH 09/12] macros: add tests for {Ser,De}Value macros integration As those macros are complex and feature many switches and flavors, tests are added that verify that when a UDT struct is derived both SerializeValue and DeserializeValue: - attributes are parsed correctly and trait impls are generated without errors, - after serialization and deserialization, the result is what is expected. --- scylla-cql/src/types/deserialize/value.rs | 2 +- .../src/types/deserialize/value_tests.rs | 4 +- scylla-cql/src/types/mod.rs | 3 + scylla-cql/src/types/serialize/value.rs | 4 +- scylla-cql/src/types/types_tests.rs | 152 ++++++++++++++++++ 5 files changed, 160 insertions(+), 5 deletions(-) create mode 100644 scylla-cql/src/types/types_tests.rs diff --git a/scylla-cql/src/types/deserialize/value.rs b/scylla-cql/src/types/deserialize/value.rs index 2191fd9be..2ff5d228e 100644 --- a/scylla-cql/src/types/deserialize/value.rs +++ b/scylla-cql/src/types/deserialize/value.rs @@ -1950,7 +1950,7 @@ impl From for BuiltinDeserializationErrorKind { #[cfg(test)] #[path = "value_tests.rs"] -pub(super) mod tests; +pub(crate) mod tests; /// ```compile_fail /// diff --git a/scylla-cql/src/types/deserialize/value_tests.rs b/scylla-cql/src/types/deserialize/value_tests.rs index 0b507cb13..e02915588 100644 --- a/scylla-cql/src/types/deserialize/value_tests.rs +++ b/scylla-cql/src/types/deserialize/value_tests.rs @@ -585,7 +585,7 @@ fn test_tuples() { ); } -fn udt_def_with_fields( +pub(crate) fn udt_def_with_fields( fields: impl IntoIterator>, ColumnType<'static>)>, ) -> ColumnType<'static> { ColumnType::UserDefinedType { @@ -1044,7 +1044,7 @@ fn test_custom_type_parser() { assert_eq!(tup, SwappedPair("foo", 42)); } -fn deserialize<'frame, 'metadata, T>( +pub(crate) fn deserialize<'frame, 'metadata, T>( typ: &'metadata ColumnType<'metadata>, bytes: &'frame Bytes, ) -> Result diff --git a/scylla-cql/src/types/mod.rs b/scylla-cql/src/types/mod.rs index 59bce9522..5fd9eb6ef 100644 --- a/scylla-cql/src/types/mod.rs +++ b/scylla-cql/src/types/mod.rs @@ -1,2 +1,5 @@ pub mod deserialize; pub mod serialize; + +#[cfg(test)] +mod types_tests; diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 32ed0cd54..0e7fba669 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -1594,7 +1594,7 @@ mod doctests { } #[cfg(test)] -mod tests { +pub(crate) mod tests { use std::collections::BTreeMap; use crate::frame::response::result::{ColumnType, CqlValue}; @@ -1654,7 +1654,7 @@ mod tests { t.serialize(typ, writer).map(|_| ()).map(|()| ret) } - fn do_serialize(t: T, typ: &ColumnType) -> Vec { + pub(crate) fn do_serialize(t: T, typ: &ColumnType) -> Vec { do_serialize_result(t, typ).unwrap() } diff --git a/scylla-cql/src/types/types_tests.rs b/scylla-cql/src/types/types_tests.rs new file mode 100644 index 000000000..002caaae5 --- /dev/null +++ b/scylla-cql/src/types/types_tests.rs @@ -0,0 +1,152 @@ +mod derive_macros_integration { + mod value { + use bytes::Bytes; + + use crate::frame::response::result::ColumnType; + use crate::types::deserialize::value::tests::{deserialize, udt_def_with_fields}; + use crate::types::serialize::value::tests::do_serialize; + + #[test] + fn derive_serialize_and_deserialize_value_loose_ordering() { + #[derive( + scylla_macros::DeserializeValue, scylla_macros::SerializeValue, PartialEq, Eq, Debug, + )] + #[scylla(crate = "crate")] + struct Udt<'a> { + a: &'a str, + #[scylla(skip)] + x: String, + #[scylla(allow_missing)] + b: Option, + #[scylla(default_when_null)] + c: i64, + } + + let original_udt = Udt { + a: "The quick brown fox", + x: String::from("THIS SHOULD NOT BE (DE)SERIALIZED"), + b: Some(42), + c: 2137, + }; + + let tests = [ + // All fields present + ( + udt_def_with_fields([ + ("a", ColumnType::Text), + ("b", ColumnType::Int), + ("c", ColumnType::BigInt), + ]), + Udt { + x: String::new(), + ..original_udt + }, + ), + // + // One field missing: + // - ignored during serialization, + // - default-initialized during deserialization. + ( + udt_def_with_fields([("a", ColumnType::Text), ("c", ColumnType::BigInt)]), + Udt { + x: String::new(), + b: None, + ..original_udt + }, + ), + // + // UDT fields switched - should still work. + ( + udt_def_with_fields([ + ("b", ColumnType::Int), + ("a", ColumnType::Text), + ("c", ColumnType::BigInt), + ]), + Udt { + x: String::new(), + ..original_udt + }, + ), + ]; + for (typ, expected_udt) in tests { + let serialized_udt = Bytes::from(do_serialize(&original_udt, &typ)); + let deserialized_udt = deserialize::>(&typ, &serialized_udt).unwrap(); + + assert_eq!(deserialized_udt, expected_udt); + } + } + + #[test] + fn derive_serialize_and_deserialize_value_strict_ordering() { + #[derive( + scylla_macros::DeserializeValue, scylla_macros::SerializeValue, PartialEq, Eq, Debug, + )] + #[scylla(crate = "crate", flavor = "enforce_order")] + struct Udt<'a> { + #[scylla(allow_missing)] + #[scylla(default_when_null)] + a: &'a str, + #[scylla(skip)] + x: String, + #[scylla(allow_missing)] + b: Option, + } + + let original_udt = Udt { + a: "The quick brown fox", + x: String::from("THIS SHOULD NOT BE (DE)SERIALIZED"), + b: Some(42), + }; + + let tests = [ + // All fields present + ( + udt_def_with_fields([("a", ColumnType::Text), ("b", ColumnType::Int)]), + Udt { + x: String::new(), + ..original_udt + }, + ), + // + // An excess field at the end of UDT + ( + udt_def_with_fields([ + ("a", ColumnType::Text), + ("b", ColumnType::Int), + ("d", ColumnType::Boolean), + ]), + Udt { + x: String::new(), + ..original_udt + }, + ), + // + // Missing non-required fields + ( + udt_def_with_fields([("a", ColumnType::Text)]), + Udt { + x: String::new(), + b: None, + ..original_udt + }, + ), + // + // An excess field at the end of UDT instead of non-required fields + ( + udt_def_with_fields([("d", ColumnType::Boolean)]), + Udt { + x: String::new(), + a: "", + b: None, + }, + ), + ]; + for (typ, expected_udt) in tests { + let serialized_udt = Bytes::from(do_serialize(&original_udt, &typ)); + let deserialized_udt = deserialize::>(&typ, &serialized_udt).unwrap(); + + assert_eq!(deserialized_udt, expected_udt); + } + } + } +} From cdcbad248cbabf65aa2e9576e9ab7e8679992b90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 10:21:47 +0100 Subject: [PATCH 10/12] macros: add tests for {Ser,De}Row macros integration Even though these macros are less complex and feature fewer switches and flavors than their {Ser,De}Value counterparts, tests are added that verify that when a row struct is derived both SerializeRow and DeserializeRow: - attributes are parsed correctly and trait impls are generated without errors, - after serialization and deserialization, the result is what is expected. --- scylla-cql/src/types/deserialize/mod.rs | 4 +- scylla-cql/src/types/deserialize/row.rs | 2 +- scylla-cql/src/types/deserialize/row_tests.rs | 2 +- scylla-cql/src/types/serialize/row.rs | 4 +- scylla-cql/src/types/types_tests.rs | 102 ++++++++++++++++++ 5 files changed, 108 insertions(+), 6 deletions(-) diff --git a/scylla-cql/src/types/deserialize/mod.rs b/scylla-cql/src/types/deserialize/mod.rs index affc2c0fc..3cd2dbccd 100644 --- a/scylla-cql/src/types/deserialize/mod.rs +++ b/scylla-cql/src/types/deserialize/mod.rs @@ -323,7 +323,7 @@ macro_rules! make_error_replace_rust_name { use make_error_replace_rust_name; #[cfg(test)] -mod tests { +pub(crate) mod tests { use bytes::{Bytes, BytesMut}; use crate::frame::response::result::{ColumnSpec, ColumnType, TableSpec}; @@ -342,7 +342,7 @@ mod tests { bytes.freeze() } - pub(super) const fn spec<'a>(name: &'a str, typ: ColumnType<'a>) -> ColumnSpec<'a> { + pub(crate) const fn spec<'a>(name: &'a str, typ: ColumnType<'a>) -> ColumnSpec<'a> { ColumnSpec::borrowed(name, typ, TableSpec::borrowed("ks", "tbl")) } } diff --git a/scylla-cql/src/types/deserialize/row.rs b/scylla-cql/src/types/deserialize/row.rs index 8de97489d..dd9ac1747 100644 --- a/scylla-cql/src/types/deserialize/row.rs +++ b/scylla-cql/src/types/deserialize/row.rs @@ -496,7 +496,7 @@ impl Display for BuiltinDeserializationErrorKind { #[cfg(test)] #[path = "row_tests.rs"] -mod tests; +pub(crate) mod tests; /// ```compile_fail /// diff --git a/scylla-cql/src/types/deserialize/row_tests.rs b/scylla-cql/src/types/deserialize/row_tests.rs index eb10e3f03..1ca26f02c 100644 --- a/scylla-cql/src/types/deserialize/row_tests.rs +++ b/scylla-cql/src/types/deserialize/row_tests.rs @@ -251,7 +251,7 @@ fn val_str(s: &str) -> Option> { Some(s.as_bytes().to_vec()) } -fn deserialize<'frame, 'metadata, R>( +pub(crate) fn deserialize<'frame, 'metadata, R>( specs: &'metadata [ColumnSpec<'metadata>], byts: &'frame Bytes, ) -> Result diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index 9e4aa949d..09d3ba43c 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -915,7 +915,7 @@ mod doctests { } #[cfg(test)] -mod tests { +pub(crate) mod tests { use std::borrow::Cow; use std::collections::BTreeMap; @@ -1037,7 +1037,7 @@ mod tests { assert_eq!(typed_data, erased_data); } - fn do_serialize(t: T, columns: &[ColumnSpec]) -> Vec { + pub(crate) fn do_serialize(t: T, columns: &[ColumnSpec]) -> Vec { let ctx = RowSerializationContext { columns }; let mut ret = Vec::new(); let mut builder = RowWriter::new(&mut ret); diff --git a/scylla-cql/src/types/types_tests.rs b/scylla-cql/src/types/types_tests.rs index 002caaae5..d54cc4dda 100644 --- a/scylla-cql/src/types/types_tests.rs +++ b/scylla-cql/src/types/types_tests.rs @@ -149,4 +149,106 @@ mod derive_macros_integration { } } } + + mod row { + use bytes::Bytes; + + use crate::frame::response::result::ColumnType; + use crate::types::deserialize::row::tests::deserialize; + use crate::types::deserialize::tests::spec; + use crate::types::serialize::row::tests::do_serialize; + + #[test] + fn derive_serialize_and_deserialize_row_loose_ordering() { + #[derive( + scylla_macros::DeserializeRow, scylla_macros::SerializeRow, PartialEq, Eq, Debug, + )] + #[scylla(crate = "crate")] + struct MyRow<'a> { + a: &'a str, + #[scylla(skip)] + x: String, + b: Option, + c: i64, + } + + let original_row = MyRow { + a: "The quick brown fox", + x: String::from("THIS SHOULD NOT BE (DE)SERIALIZED"), + b: Some(42), + c: 2137, + }; + + let tests = [ + // All columns present + ( + &[ + spec("a", ColumnType::Text), + spec("b", ColumnType::Int), + spec("c", ColumnType::BigInt), + ][..], + MyRow { + x: String::new(), + ..original_row + }, + ), + // + // Columns switched - should still work. + ( + &[ + spec("b", ColumnType::Int), + spec("a", ColumnType::Text), + spec("c", ColumnType::BigInt), + ], + MyRow { + x: String::new(), + ..original_row + }, + ), + ]; + for (typ, expected_row) in tests { + let serialized_row = Bytes::from(do_serialize(&original_row, typ)); + let deserialized_row = deserialize::>(typ, &serialized_row).unwrap(); + + assert_eq!(deserialized_row, expected_row); + } + } + + #[test] + fn derive_serialize_and_deserialize_row_strict_ordering() { + #[derive( + scylla_macros::DeserializeRow, scylla_macros::SerializeRow, PartialEq, Eq, Debug, + )] + #[scylla(crate = "crate", flavor = "enforce_order")] + struct MyRow<'a> { + a: &'a str, + #[scylla(skip)] + x: String, + b: Option, + } + + let original_row = MyRow { + a: "The quick brown fox", + x: String::from("THIS SHOULD NOT BE (DE)SERIALIZED"), + b: Some(42), + }; + + let tests = [ + // All columns present + ( + &[spec("a", ColumnType::Text), spec("b", ColumnType::Int)][..], + MyRow { + x: String::new(), + ..original_row + }, + ), + ]; + for (typ, expected_row) in tests { + let serialized_row = Bytes::from(do_serialize(&original_row, typ)); + let deserialized_row = deserialize::>(typ, &serialized_row).unwrap(); + + assert_eq!(deserialized_row, expected_row); + } + } + } } From 35f1c5801439d1d751391bc8d63defabe79b67d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 10:40:51 +0100 Subject: [PATCH 11/12] book: update examples to use DeserializeValue Also, no longer mention the limitations of old derive macros. The new macros have no limitations compared to Serialize macros. --- docs/source/data-types/udt.md | 9 +++++---- docs/source/migration-guides/0.11-serialization.md | 2 -- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/source/data-types/udt.md b/docs/source/data-types/udt.md index 4a8f91b13..ddafb748e 100644 --- a/docs/source/data-types/udt.md +++ b/docs/source/data-types/udt.md @@ -20,13 +20,14 @@ and `DeserializeValue` macros documentation. ```rust # extern crate scylla; # async fn check_only_compiles() { -use scylla::macros::{FromUserType, SerializeValue}; +use scylla::macros::{DeserializeValue, SerializeValue}; // Define a custom struct that matches the User Defined Type created earlier. -// Fields must be in the same order as they are in the database and also -// have the same names. +// Fields don't have to be in the same order as they are in the database. +// By default, they must have the same names, but this can be worked around +// using `#[rename] field attribute. // Wrapping a field in Option will gracefully handle null field values. -#[derive(Debug, FromUserType, SerializeValue)] +#[derive(Debug, DeserializeValue, SerializeValue)] struct MyType { int_val: i32, text_val: Option, diff --git a/docs/source/migration-guides/0.11-serialization.md b/docs/source/migration-guides/0.11-serialization.md index 77004b03d..601be0da2 100644 --- a/docs/source/migration-guides/0.11-serialization.md +++ b/docs/source/migration-guides/0.11-serialization.md @@ -42,8 +42,6 @@ The driver comes a set of `impl`s of those traits which allow to represent any C By default, the `SerializeRow` and `SerializeValue` **will match the fields in the Rust struct by name to bind marker names** (in case of `SerializeRow`) **or UDT field names** (in case of `SerializeValue`). This is different from the old `ValueList` and `IntoUserType` macros which did not look at the field names at all and would expect the user to order the fields correctly. While the new behavior is much more ergonomic, you might have reasons not to use it. -> **NOTE:** The deserialization macro counterparts `FromRow` and `FromUserType` have the same limitation as the old serialization macros - they require struct fields to be properly ordered. While a similar rework is planned for the deserialization traits in a future release, for the time being it might not be worth keeping the column names in sync with the database. - In order to bring the old behavior to the new macros (the only difference being type checking which cannot be disabled right now) you can configure it using attributes, as shown in the snippet below: ```rust From 0c9a8cc438758998af300bc294825f9f04f924ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 13 Nov 2024 21:31:50 +0100 Subject: [PATCH 12/12] macros: add missing comments to Serialize* attrs Some comments were missing, so I imported them from DeserializeValue. --- scylla-macros/src/serialize/row.rs | 8 ++++++++ scylla-macros/src/serialize/value.rs | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/scylla-macros/src/serialize/row.rs b/scylla-macros/src/serialize/row.rs index 9e4a991f6..a5449c9be 100644 --- a/scylla-macros/src/serialize/row.rs +++ b/scylla-macros/src/serialize/row.rs @@ -16,6 +16,10 @@ struct Attributes { #[darling(default)] flavor: Flavor, + // If true, then the type checking code won't verify the column names. + // Columns will be matched to struct fields based solely on the order. + // + // This annotation only works if `enforce_order` flavor is specified. #[darling(default)] skip_name_checks: bool, } @@ -47,8 +51,12 @@ impl Field { #[derive(FromAttributes)] #[darling(attributes(scylla))] struct FieldAttributes { + // If set, then serializes from the column with this particular name + // instead of the Rust field name. rename: Option, + // If true, then the field is not serialized at all, but simply ignored. + // All other attributes are ignored. #[darling(default)] skip: bool, } diff --git a/scylla-macros/src/serialize/value.rs b/scylla-macros/src/serialize/value.rs index 101d1c244..14ca2e7b9 100644 --- a/scylla-macros/src/serialize/value.rs +++ b/scylla-macros/src/serialize/value.rs @@ -16,6 +16,10 @@ struct Attributes { #[darling(default)] flavor: Flavor, + // If true, then the type checking code won't verify the UDT field names. + // UDT fields will be matched to struct fields based solely on the order. + // + // This annotation only works if `enforce_order` flavor is specified. #[darling(default)] skip_name_checks: bool, @@ -62,8 +66,12 @@ impl Field { #[derive(FromAttributes)] #[darling(attributes(scylla))] struct FieldAttributes { + // If set, then serializes from the UDT field with this particular name + // instead of the Rust field name. rename: Option, + // If true, then the field is not serialized at all, but simply ignored. + // All other attributes are ignored. #[darling(default)] skip: bool,