A bunch of minor refactors - type safety, readability, robustness #345
Conversation
This file contains hidden or 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
requested changes
Jul 15, 2025
Before our FFI framework was introduced, I had written some heuristic safeguards in the code to avoid use-after-free errors. They are no longer needed now, as the borrow checker fully guarantees that no UAF can happen in those cases.
The CPP Driver unsafely dereferences the callback pointer even if it is null. Our Rust wrapper `unwrap()`s the Option with the callback, so it panics in such case. This commit changes the code to return an error upon passing a null callback pointer to `cass_future_set_callback`. Then, the callback is always non-null if set in the driver, so no panic is possible. This should be considered an *improvement* compared to the CPP Driver.
let-else is the idiomatic way to handle early error returns in Rust, which in general makes the code cleaner and more readable.
wprzytula
force-pushed
the
minor-refactors
branch
from
a56e7d5 to
ff64af1
Compare
The code of `cass_statement_set_paging_state_token` was unsetting the paging state on a statement if a null paging state pointer was passed, but it also returned an error in this case. This is inconsistent with the behavior of other setters, which either preserve the existing state and return an error if a null pointer is passed, or unset the state and return CASS_OK. For consistency and ease of use, the code is changed to unset paging state when null is provided. As a bonus, to mimic CPP Driver behaviour, empty strings now also unset the paging state. Note: CPP Driver does not check the passed pointer for null at all.
Result is more idiomatic in Rust to represent a fallible operation. Converting an integer to a Rust enum can definitely fail, so using `Result` is more appropriate than `Option`.
let-else is the idiomatic way to handle early error returns in Rust, which can make the code cleaner and more readable.
The driver was using the C enum `CassCollectionType` directly in its `CassCollection` struct, which led to necessity of handling the "else" case (values other than the explicitly listed variants) in many places. To bring type safety to the driver internals, this commit introduces a new Rust `CollectionType` enum, which is used internally instead of the C enum.
After introducing CollectionType and ensuring type safety for collections, we can simplify the conversion from `&CassCollection` to `CassCqlValue` by removing the `TryFrom` trait and using `From` instead. This change reflects that the conversion is always valid as long as the collection is well-formed.
There existed an `external.rs` file that only contained the `cass_error_desc` function. As other error-related functions reside in `execution_error.rs`, it makes sense to move it there as well. The `external.rs` file is now deleted.
The unwrap can be avoided by reordering the code slightly and operating on the `val` directly, not through the `CassBorrowedSharedPtr`.
This commit refactors `cass_iterator_*` functions to use let-else syntax for better readability.
The error message should be constructed by the caller if needed, and explain the context of the error in more abstract terms, not such low-level wrapper implementation details.
wprzytula
force-pushed
the
minor-refactors
branch
from
ff64af1 to
e5df40b
Compare
|
Altered the commit with changes to |
Lorak-mmk
reviewed
Jul 16, 2025
Lorak-mmk
approved these changes
Jul 16, 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 is a bunch of small independent refactors that aim to increase type safety, readability and robustness.
Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced.[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.