Remove legacy serialization and deserialization APIs #1184
Merged
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
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.
Lorak-mmk
force-pushed
the
remove-legacy-apis
branch
from
fe6be5c to
e894b24
Compare
Still need to move value and value_tests module. |
|
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.
There was a problem hiding this comment.
I did not write this comment, it was just part of the moved code. I dislike the formatting a bit, but no matter if we want to change it or not I think it is out of scope for this PR.
This is because of docstrings to |
Yep, I understood that a bit after posting my comment, I just didn't have time to fix the PR yey. |
After legacy session removal it is no longer used.
This was the only leftover from the old deserialization interface.
Those are not used by the new macros.
Lorak-mmk
force-pushed
the
remove-legacy-apis
branch
from
95bb00b to
841a0c8
Compare
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
841a0c8 to
c714f59
Compare
Now that I think of it I might as well do it here. |
value.rs contains serializable/deserializable value types introduced in the driver (e.g. CqlVarint). It makes perfect sense to put CqlValue there as well. |
|
Ok this PR is now +6k lines -11k lines and 37 commits. This is a signal that it got too big and should be split. |
Lorak-mmk
force-pushed
the
remove-legacy-apis
branch
from
f69cf2f to
9cc52c9
Compare
|
Removed cleanup commits. Now this PR contains only the API removal. |
8 tasks
|
Opened #1198 |
wprzytula
approved these changes
Feb 4, 2025
Comment on lines
+557
to
559
| pub async fn build(&self) -> Result<Session, NewSessionError> { | ||
| Session::connect(self.config.clone()).await | ||
| } |
There was a problem hiding this comment.
📌 Why is build borrowing instead of consuming the SessionBuilder? This makes no sense, as SessionBuilder implements Clone, so it could always be cloned before building to retain it. Now there is needless cloning for the usual case.
There was a problem hiding this comment.
I can change that in the follow-up in order to not expand the scope of this PR
muzarski
approved these changes
Feb 4, 2025
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.
Removing old APIs makes it clear that some cleanups in scylla-cql shall be done.
Those will be described and done in a follow-up PR because this one is already big enough.
I'm delibarately not marking this as fixing #1167 - the follow-up PR that deals with the consequences of API removal will do that.
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.