Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scylla-macros: attributes for better control over name checks #882

Merged
merged 5 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions scylla-cql/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,15 @@ pub use scylla_macros::ValueList;
/// macro itself, so in those cases the user must provide an alternative path
/// to either the `scylla` or `scylla-cql` crate.
///
/// `#[scylla(skip_name_checks)]
///
/// _Specific only to the `enforce_order` flavor._
///
/// Skips checking Rust field names against names of the UDT fields. With this
/// annotation, the generated implementation will allow mismatch between Rust
/// 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.
///
Comment on lines +90 to +98
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it wouldn't be better to make this a new flavor instead of an attribute - one less place to make a mistake for a user. But seeing that it's a compile time error, maybe it doesn't matter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure either; implementation-wise it was more natural to add an attribute as it introduces very slight changes to how the enforce_order flavor generates code.

One argument that I see for leaving it as a separate attribute is that it disables some safety checks, while both current flavors don't compromise on safety. It's good to make the less safe option more verbose IMO.

In the future we should consider adding support for tuple structs which very natually fit into the enforce_order + skip_name_checks pattern, we could probably remove the current skip_name_checks attribute when doing that.

/// # Field attributes
///
/// `#[scylla(rename = "name_in_the_udt")]`
Expand Down
39 changes: 39 additions & 0 deletions scylla-cql/src/types/serialize/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2434,4 +2434,43 @@ mod tests {

assert_eq!(reference, udt);
}

#[derive(SerializeCql, Debug)]
#[scylla(crate = crate, flavor = "enforce_order", skip_name_checks)]
struct TestUdtWithSkippedNameChecks {
a: String,
b: i32,
}

#[test]
fn test_udt_serialization_with_skipped_name_checks() {
let typ = ColumnType::UserDefinedType {
type_name: "typ".to_string(),
keyspace: "ks".to_string(),
field_types: vec![
("a".to_string(), ColumnType::Text),
("x".to_string(), ColumnType::Int),
],
};

let mut reference = Vec::new();
// Total length of the struct is 23
reference.extend_from_slice(&23i32.to_be_bytes());
// Field 'a'
reference.extend_from_slice(&("Ala ma kota".len() as i32).to_be_bytes());
reference.extend_from_slice("Ala ma kota".as_bytes());
// Field 'x'
reference.extend_from_slice(&4i32.to_be_bytes());
reference.extend_from_slice(&42i32.to_be_bytes());

let udt = do_serialize(
TestUdtWithFieldRenameAndEnforceOrder {
a: "Ala ma kota".to_owned(),
b: 42,
},
&typ,
);

assert_eq!(reference, udt);
}
}
36 changes: 33 additions & 3 deletions scylla-macros/src/serialize/cql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ struct Attributes {

#[darling(default)]
flavor: Flavor,

#[darling(default)]
skip_name_checks: bool,
}

impl Attributes {
Expand Down Expand Up @@ -74,7 +77,7 @@ pub fn derive_serialize_cql(tokens_input: TokenStream) -> Result<syn::ItemImpl,
})
.collect::<Result<_, _>>()?;
let ctx = Context { attributes, fields };
ctx.validate()?;
ctx.validate(&input.ident)?;

let gen: Box<dyn Generator> = match ctx.attributes.flavor {
Flavor::MatchByName => Box::new(FieldSortingGenerator { ctx: &ctx }),
Expand All @@ -92,9 +95,31 @@ pub fn derive_serialize_cql(tokens_input: TokenStream) -> Result<syn::ItemImpl,
}

impl Context {
fn validate(&self) -> Result<(), syn::Error> {
fn validate(&self, struct_ident: &syn::Ident) -> Result<(), syn::Error> {
let mut errors = darling::Error::accumulator();

if self.attributes.skip_name_checks {
// Skipping name checks is only available in enforce_order mode
if self.attributes.flavor != Flavor::EnforceOrder {
let err = darling::Error::custom(
"the `skip_name_checks` attribute is only allowed with the `enforce_order` flavor",
)
.with_span(struct_ident);
errors.push(err);
}

// `rename` annotations don't make sense with skipped name checks
for field in self.fields.iter() {
if field.attrs.rename.is_some() {
let err = darling::Error::custom(
"the `rename` annotations don't make sense with `skip_name_checks` attribute",
)
.with_span(&field.ident);
errors.push(err);
}
}
}

// Check for name collisions
let mut used_names = HashMap::<String, &Field>::new();
for field in self.fields.iter() {
Expand Down Expand Up @@ -330,10 +355,15 @@ impl<'a> Generator for FieldOrderedGenerator<'a> {
let rust_field_ident = &field.ident;
let rust_field_name = field.field_name();
let typ = &field.ty;
let name_check_expression: syn::Expr = if !self.ctx.attributes.skip_name_checks {
parse_quote! { field_name == #rust_field_name }
} else {
parse_quote! { true }
};
statements.push(parse_quote! {
match field_iter.next() {
Some((field_name, typ)) => {
if field_name == #rust_field_name {
if #name_check_expression {
let sub_builder = #crate_path::CellValueBuilder::make_sub_writer(&mut builder);
match <#typ as #crate_path::SerializeCql>::serialize(&self.#rust_field_ident, typ, sub_builder) {
Ok(_proof) => {},
Expand Down