-
Notifications
You must be signed in to change notification settings - Fork 146
Add DeserializeRow support for tuple structs #1531
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
base: main
Are you sure you want to change the base?
Conversation
|
|
369eede to
bc59c45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR extends the DeserializeRow derive macro to support tuple structs (structs with unnamed fields), enabling users to deserialize database rows into tuple structs like struct Point(f64, f64) or struct UserId(i32) using positional column mapping instead of name-based mapping.
Key changes:
- Added detection and routing logic for tuple structs in the derive macro entry point
- Implemented
deserialize_tuple_struct_derivefunction with order-based column-to-field mapping - Added test suite covering basic tuple struct scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scylla-macros/src/deserialize/row.rs | Added tuple struct detection in entry point and new implementation function with type checking and deserialization logic for positional field mapping |
| scylla-cql/src/types_tests.rs | Added test module covering single-field wrappers, multi-field tuples, type checking, and column count validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bc59c45 to
f27ff4d
Compare
| let input: syn::DeriveInput = syn::parse(tokens_input)?; | ||
|
|
||
| if let syn::Data::Struct(syn::DataStruct { | ||
| fields: syn::Fields::Unnamed(fields), | ||
| .. | ||
| }) = &input.data | ||
| { | ||
| return deserialize_tuple_struct_derive(&input, fields); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are skipping all of normal deserialization implementation.
I get that StructDesc doesn't support unnamed right now, but the solution is to fix that, not skip it altogether. Note that it even has a TODO about it! See lines 60-65 in deserialize/mod.rs. Otherwise you are introducing a lot of duplication, that we will need to handle with every future change to this code, and it also makes testing / review much more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example, when reviewing this I discovered that named impl has a serious bug. The following test fails on main:
#[derive(DeserializeRow, Debug, PartialEq)]
#[scylla(crate = "crate")]
struct GenericConstrainedStruct<T> {
a: T,
}
It is because in generated impl we use raw DeserializeValue, instead of using the path from _macro_internal. Fixing this surfaced another issue: we pass only one lifetime to DeserializeValue here, so it still doesn't compile.
I'm sure we have other bugs. I'm sure your impl also has some hidden bugs / missing parts. For example, it doesn't work in the following case:
trait MyTrait {}
#[derive(DeserializeRow, Debug, PartialEq)]
struct GenericTupleConstrained<T>(T)
where
T: MyTrait;If each such issue will need to be handled in multiple locations, fixing such problems will be extremely difficult and frustrating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to this PR I started to investigate generics support on our macros, and discovered a lot of bugs :D
| fn check_attributes_for_tuple_structs(attrs: &[syn::Attribute]) -> Result<(), syn::Error> { | ||
| for attr in attrs { | ||
| if !attr.path().is_ident("scylla") { | ||
| continue; | ||
| } | ||
|
|
||
| attr.parse_nested_meta(|meta| { | ||
| if meta.path.is_ident("flavor") || meta.path.is_ident("skip_name_checks") { | ||
| return Err(meta.error( | ||
| "struct-level attributes `flavor` and `skip_name_checks` are not supported for tuple structs; tuple structs always use order-based matching", | ||
| )); | ||
| } | ||
| if meta.input.peek(syn::Token![=]) { | ||
| let _ = meta.value()?.parse::<syn::Expr>()?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| })?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this manually parse attrs instead of relying on StructAttrs?
| let input: syn::DeriveInput = syn::parse(tokens_input)?; | ||
|
|
||
| if let syn::Data::Struct(syn::DataStruct { | ||
| fields: syn::Fields::Unnamed(fields), | ||
| .. | ||
| }) = &input.data | ||
| { | ||
| return deserialize_tuple_struct_derive(&input, fields); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Even if we retain the current duplicated approach, this is not a proper way to do this. Named and unnamed cases should be handled similarly. Now you treat it as an exceptional case, be having a check and delegating to another function, while the named case is handle here. Better option would be to have a match.
|
Also, we have one more file with macros tests. See |
Motivation
Users often define data structures using tuple structs (structs with unnamed fields), such as
struct Point(f64, f64)orstruct UserId(i32). Currently, theDeserializeRowderive macro only supports structs with named fields, where it maps database columns to Rust fields by name. This limitation forces users to either use named structs (which might not fit their domain model) or manually implement the deserialization traits, resulting in unnecessary boilerplate.Solution
This commit extends the
DeserializeRowderive macro to support tuple structs.When deriving
DeserializeRowfor a tuple struct, the macro generates an implementation that maps database columns to struct fields based on their order (index).What's done
Refactored
deserialize_row_derivesyn::Fields::Unnamed(tuple structs).deserialize_tuple_struct_derivefunction for clear separation from named struct logic.Implemented Tuple Logic
type_checkmethod iterates through the providedColumnSpecslice and the struct fields. It verifies that the database column type matches the expected Rust type for that position. It also verifies that the number of columns matches the number of fields.deserializemethod consumes theColumnIteratorsequentially, deserializing each column into the corresponding field of the tuple struct.Validation
What has been tested
Tests
Added a suite of unit tests covering:
struct Wrapper(i32)(Newtype pattern).struct MultiTuple(i32, String, f64)to verify correct ordering.type_checkcorrectly returns an error if the column type (e.g.,Text) does not match the field type (e.g.,Int).Fixes: #852
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.