Remove legacy serialization and deserialization APIs #1184
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lorak-mmk
force-pushed
the
remove-legacy-apis
branch
from
e2ee1fe to
6542a09
Compare
github-actions
bot
added
the
semver-checks-breaking
|
See the following report for details: cargo semver-checks output |
Lorak-mmk
force-pushed
the
remove-legacy-apis
branch
7 times, most recently
from
d0b0da8 to
52cc5c5
Compare
This error is not deprecated, this directive is clearly a leftover or mistake. It is probably caused by some merge, because I was not able to find the commit that caused this.
It was a leftover from the PR that migrated this test to new APIs.
It is no longer necessary after the PR that refactored this error.
Lorak-mmk
force-pushed
the
remove-legacy-apis
branch
2 times, most recently
from
555e7b1 to
9f20778
Compare
👍, let's move |
wprzytula
requested changes
Jan 30, 2025
Comment on lines
64
to
68
| # Setup Sphinx | ||
| def setup(sphinx): | ||
| lexers['rust'] = RustLexer() | ||
| lexers['rust,ignore'] = RustLexer() | ||
| lexers['toml'] = TOMLLexer() |
There was a problem hiding this comment.
📝 I wasn't aware that there is such a place where we can configure how ```<lang> is parsed.
There was a problem hiding this comment.
Me neither. In general I don't know much abut how Scylla documentation building works.
muzarski
reviewed
Jan 30, 2025
There was a problem hiding this comment.
Mainly some nits. I want to make sure that all removed tests for legacy API have their equivalent for new API (if applicable).
When it comes to the value module. I think that we should definitely make it a top-level module. Notice that deserialization tests are placed in deserialize::value_tests, while serialization tests are in frame::value_tests. So my proposition is to:
- move
value.rsto top-level (src/value.rs) - move
frame::value_tests.rstoserialize::value_tests.rs
Comment on lines
-996
to
-1057
| fn struct_from_row_wrong_size() { | ||
| #[derive(FromRow, PartialEq, Eq, Debug)] | ||
| struct MyRow { | ||
| a: i32, | ||
| b: Option<String>, | ||
| c: Option<Vec<i32>>, | ||
| } | ||
|
|
||
| let too_short_row = Row { | ||
| columns: vec![Some(CqlValue::Int(16)), None], | ||
| }; | ||
|
|
||
| let too_large_row = Row { | ||
| columns: vec![ | ||
| Some(CqlValue::Int(16)), | ||
| None, | ||
| Some(CqlValue::Set(vec![CqlValue::Int(1), CqlValue::Int(2)])), | ||
| Some(CqlValue::Set(vec![CqlValue::Int(1), CqlValue::Int(2)])), | ||
| ], | ||
| }; | ||
|
|
||
| assert_eq!( | ||
| MyRow::from_row(too_short_row), | ||
| Err(FromRowError::WrongRowSize { | ||
| expected: 3, | ||
| actual: 2 | ||
| }) | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| MyRow::from_row(too_large_row), | ||
| Err(FromRowError::WrongRowSize { | ||
| expected: 3, | ||
| actual: 4 | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| // Enabling `expect_used` clippy lint, | ||
| // validates that `derive(FromRow)` macro definition does do not violates such rule under the hood. | ||
| // Could be removed after such rule will be applied for the whole crate. | ||
| // <https://rust-lang.github.io/rust-clippy/master/index.html#/expect_used> | ||
| #[deny(clippy::expect_used)] | ||
| #[test] | ||
| fn unnamed_struct_from_row() { | ||
| #[derive(FromRow)] | ||
| struct MyRow(i32, Option<String>, Option<Vec<i32>>); | ||
|
|
||
| let row = Row { | ||
| columns: vec![ | ||
| Some(CqlValue::Int(16)), | ||
| None, | ||
| Some(CqlValue::Set(vec![CqlValue::Int(1), CqlValue::Int(2)])), | ||
| ], | ||
| }; | ||
|
|
||
| let my_row: MyRow = MyRow::from_row(row).unwrap(); | ||
|
|
||
| assert_eq!(my_row.0, 16); | ||
| assert_eq!(my_row.1, None); | ||
| assert_eq!(my_row.2, Some(vec![1, 2])); | ||
| } |
There was a problem hiding this comment.
Again - please make sure that analogous tests for new API exist (if applicable for new API).
There was a problem hiding this comment.
Those tests test old value "deserialization" (converting from CqlValue to other types), and to a lesser degree row deserialization (converting from Row = Vec<Option> to a concrete type).
New API has its own tests, which I believe to be extensive enough. See the scylla-cql/src/deserialize/value_tests.rs and scylla-cql/src/deserialize/row_tests.rs
There was a problem hiding this comment.
Ok, that's good enough. BTW, do we support structs with unnamed fields for DeserializeRow? I remember implementing that for FromRow : #985. I'm asking since unnamed_struct_from_row test checked that it works.
There was a problem hiding this comment.
I have no idea. @wprzytula ?
There was a problem hiding this comment.
No, there's no support for them. See respective TODO.
There was a problem hiding this comment.
Those tests test old value "deserialization" (converting from CqlValue to other types), and to a lesser degree row deserialization (converting from Row = Vec to a concrete type).
I believe that conversion of CqlValue into end types (using getters, e.g. as_bigint()) should be still well-tested, too, as it's a valid use case for, e.g., JS Driver. Is it already tested somewhere else?
There was a problem hiding this comment.
In serialize::value module there are some tests for CqlValue serialization, but I don't see tests for as_ method. I bielieve it is out of scope for this PR, so I'll open an issue about this.
It is used by mdbook for code blocks that are syntactically valid Rust, but should not be compiled (e.g. because they contain an intentional compile error).
Those are not used by the new macros.
It was made clonable before in order to accomodate sharing it between legacy and modern sessions. It is no longer necessary.
Lorak-mmk
force-pushed
the
remove-legacy-apis
branch
from
fe6be5c to
e894b24
Compare
Still need to move value and value_tests module. |
We want to move tests from this file into serialization module. In order for the move commit to be simpler, the tests should be made more similar before.
We want to move tests from this file into serialization module. In order for the move commit to be simpler, the tests should be made more similar before.
There is another equivalent function already: `col`.
That's how this helper function is named in serialize/row_tests.rs.
This allows us to finally get rid of frame/value_tests.rs!
|
Extracted value.rs and value_tests.rs from frame module. Note: this was not a force push, only new commits were added, so there is no need to re-review previous commits if you already reviewed them. |
muzarski
approved these changes
Jan 31, 2025
???!!!!!! |
wprzytula
requested changes
Feb 1, 2025
Comment on lines
+153
to
+155
| // rustfmt wants to have each tuple inside a tuple in a separate line | ||
| // so we end up with 170 lines of tuples | ||
| // FIXME: Is there some cargo fmt flag to fix this? |
There was a problem hiding this comment.
I'm perfectly OK with such formatting. This makes the tuples more readable for me.
Comment on lines
+24
to
+69
| 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() {} | ||
| } |
There was a problem hiding this comment.
❓
In my experience when writing them, the doc test runner does not enable test cfg flag. Therefore, these tests are skipped. That's why in the deserialization case I put doctests in the row.rs, not row_tests.rs.
Comment on lines
+26
to
+121
| 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() {} | ||
|
|
||
| /// ```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() {} | ||
|
|
||
| /// ``` | ||
| /// #[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() {} | ||
| } |
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR finally removes both of the old APIs.
Focus in review should be on making sure that I didn't miss anything that shall be removed.
Considerations
valuemoduleThis module in
scylla_cqlis placed inframemodule and contained multiple things:ValueandValueListtraits and their implementations)CqlValue,CqlTimeuuidetcAfter removing (1), and removing
cql_to_rustmodule which was also inframe, I find it a bit weirdfor this module to be placed in
frame.Serialization / deserialization are now a top-level modules in
scylla_cql, so maybe this one should be too?Session
I removed LegacySession, and de-genericized Session, making it a simple struct again.
However I did not inline the methods implementations (like inlining
do_batchintobatch).Those inlines should probably happen at some point, but it is not a blocker for 1.0 so I left it for now.
I performed de-genericization because it is a breaking change and thus a blocker for 1.0
Ser / deser tests
Those are all over scylla_cql crate. We should think about the structure for those and move them into a reasonable place.
I left it for another time because this does not affect the API
Fixes: #1167
Pre-review checklist
I added relevant tests for new features and bug fixes.I have provided docstrings for the public items that I want to introduce../docs/source/.Fixes:annotations to PR description.