errors: replace `RowsParseError` and narrow the return types after deserialization refactor #1117

muzarski · 2024-11-08T21:01:53Z

Motivation

After deserialization refactor, the RowsParseError was widely abused in multiple places, namely:

deser_rows() - initial deserialization of RESULT:Rows response
QueryResult::into_rows_result() - lazy deserialization of metadata
QueryResult::into_legacy_result() - lazy metadata deserialization, rows deserialization etc.
everytime we needed to convert Typecheck/DeserializationErrors to QueryError - widely abused in iterator API

This PR fixes that by narrowing the return types of functions originally depending on RowsParseError. The RowsParseError is removed at the end of this PR.

`QueryResult::into_rows_result()` return type

The last two commits adjust the API of QueryResult:

Flattened the return type of QueryResult::into_rows_result() to Result<_, IntoRowsResultError>. I believe that this API is cleaner than returning Option<Result<...>>.
[postponed] Introduced QueryResult::into_rows_result_with_recovery() - it does the same as into_rows_result(), but recovers self in case error occurred.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
I have adjusted the documentation in ./docs/source/.
I added appropriate Fixes: annotations to PR description.

github-actions · 2024-11-08T21:07:29Z

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 2054163

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 8122c6551a193e22693a72809ae95b1d6cb83acd
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 8122c6551a193e22693a72809ae95b1d6cb83acd
     Cloning 8122c6551a193e22693a72809ae95b1d6cb83acd
     Parsing scylla v0.14.0 (current)
      Parsed [  25.571s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  23.270s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.117s] 94 checks: 91 pass, 3 fail, 0 warn, 0 skip

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type PeersMetadataError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:458
  type PeersMetadataError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:458

--- failure enum_marked_non_exhaustive: enum marked #[non_exhaustive] ---

Description:
A public enum has been marked #[non_exhaustive]. Pattern-matching on it outside of its crate must now include a wildcard pattern like `_`, or it will fail to compile.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#attr-adding-non-exhaustive
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_marked_non_exhaustive.ron

Failed in:
  enum NextRowError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/iterator.rs:1057

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_variant_missing.ron

Failed in:
  variant SchemaVersionFetchError::ResultNotRows, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla/src/transport/errors.rs:355
  variant TracingProtocolError::TracesSessionNotRows, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla/src/transport/errors.rs:366
  variant TracingProtocolError::TracesEventsNotRows, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla/src/transport/errors.rs:378
  variant NextRowError::QueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla/src/transport/iterator.rs:1110
  variant NextRowError::FromRowError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla/src/transport/iterator.rs:1114

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  49.017s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [  11.545s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  11.668s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.110s] 94 checks: 92 pass, 2 fail, 0 warn, 0 skip

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_missing.ron

Failed in:
  enum scylla_cql::frame::frame_errors::RowsParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla-cql/src/frame/frame_errors.rs:313

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_variant_missing.ron

Failed in:
  variant CqlResultParseError::RowsParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla-cql/src/frame/frame_errors.rs:230
  variant ResultMetadataParseError::PkCountParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla-cql/src/frame/frame_errors.rs:339
  variant ResultMetadataParseError::PkIndexParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-8122c6551a193e22693a72809ae95b1d6cb83acd/de6e50567f0302726ec24e6e8c83b8141fe95de2/scylla-cql/src/frame/frame_errors.rs:341

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  23.373s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

muzarski · 2024-11-12T16:47:23Z

Rebased on main. Adjusted commit messages and PR description

scylla/src/transport/legacy_query_result.rs

scylla/src/transport/errors.rs

scylla/src/transport/iterator.rs

scylla/src/transport/query_result.rs

wprzytula · 2024-11-12T19:24:01Z

Apart from nits and comments about the last commit, looks good!

muzarski · 2024-11-12T20:30:52Z

Rebased on main - haven't addressed comments yet.

muzarski · 2024-11-12T20:47:02Z

v2: Addressed @wprzytula comments. Dropped the last commit that introduces into_rows_result_with_recovery. The commit can be found here muzarski@2174378 (cc: @Lorak-mmk)

muzarski · 2024-11-12T20:58:37Z

v2.1: adjusted book to new into_rows_result() API.

muzarski · 2024-11-12T21:17:08Z

v2.2: Adjusted book again..

scylla-cql/src/frame/response/result.rs

muzarski · 2024-11-13T15:00:28Z

Let me quickly rebase on main before merge. There are no conflicts with #1124, but I think the code won't compile (since there is an additional variant to QueryError which is not matched in speculative_execution::can_be_ignored

Lorak-mmk · 2024-11-13T15:04:18Z

Let me quickly rebase on main before merge. There are no conflicts with #1124, but I think the code won't compile (since there is an additional variant to QueryError which is not matched in speculative_execution::can_be_ignored

Please check. I thought Github runs CI on generated merge commit, so if it didn't compile with current main then CI would fail. Is that not the case?

Previously, the `ResultMetadataParseError` error type was common for both Result and Prepared metadata. It does not make much sense, though. Obviously, some of the error variants are shared between these two types, but not all of them.

An error that occurred during initial deserialization of `RESULT:Rows` response. Since the deserialization of rows is lazy, we initially only need to deserialize: - result metadata flags - column count (result metadata) - paging state response We ultimately want to get rid of current `RowsParseError` since its usage is abused in a lot of places throughout the scylla crate. We start by narrowing the return type of `deser_rows()` function.

muzarski · 2024-11-13T15:06:52Z

Let me quickly rebase on main before merge. There are no conflicts with #1124, but I think the code won't compile (since there is an additional variant to QueryError which is not matched in speculative_execution::can_be_ignored

Please check. I thought Github runs CI on generated merge commit, so if it didn't compile with current main then CI would fail. Is that not the case?

I checked (locally). It does not compile if I rebase without any additional changes.

An error returned by `QueryResult::into_legacy_result()`. Previously, the `QueryResult::into_legacy_result()` would return `RowsParseError`. Note that `RowsParseError` is (for now) included in new error type. This is because `RawMetadataAndRawRows::deserialize_metadata()` still returns `RowsParseError`. This will be adjusted in following commit once we introduce a new error type for `deserialize_metadata` method.

This could only happen if there is a serious bug in the driver. `deser_col_specs_generic` takes `column_count` as argument and deserializes the col specs `column_count` times in a loop. Thus, this check does not make sense.

…metadata Adjusted the error returned by `RawMetadataAndRawRows::deserialize_metadata()`. Again, to narrow the return error type of following methods: - RawMetadataAndRawRows::deserialize_metadata() - QueryResult::into_rows_result() Adjusted the callers of `QueryResult::into_rows_result` in topology.rs and session.rs and added corresponding variants for `TracingProtocolError` and `SchemaVersionFetchError` Temporarily, we need to introduce a corresponding variant to `QueryError` and `NewSessionError`. This is because internal API makes use of `into_rows_result()` in multiple places. The callers will be adjusted later in this PR, allowing us to remove the variant from `Query/NSError`.

We will re-introduce `NewRowError` as an error type returned by current async iterator API.

Lorak-mmk · 2024-11-13T15:10:19Z

Let me quickly rebase on main before merge. There are no conflicts with #1124, but I think the code won't compile (since there is an additional variant to QueryError which is not matched in speculative_execution::can_be_ignored

Please check. I thought Github runs CI on generated merge commit, so if it didn't compile with current main then CI would fail. Is that not the case?

I checked (locally). It does not compile if I rebase without any additional changes.

Weird

These are errors returned by async iterator API. Ultimately, we want them to be independent types returned by the public API. However, as of now, NextRowError needs to be wrapped in QueryError - to address that, further changes to iter API need to applied. These, however, are not in a scope of this PR. This partial change of iter API is introduced because we want to get rid of RowsParseError dependency in this module.

It's not used anymore after adjustments to iter API. It was introduced temporarily earlier, and can now be safely removed.

So we do not depend on RowsParseError anymore, new variant is introduced that represents a row deserialization failure.

Instead of returning Result<Option<_>, _>, we will simply return a Result. IntoRowsResultError is introduced specifically for this method. Adjusted all of usages of this API. Most of the changes were simply to replace `unwrap().unwrap()` to single `unwrap()`. There are 4 places that require more focus during review: - print_result() method in `cql-sh.rs` example. - changes to SchemaVersionFetchError (and corresponding code in connection.rs) - changes to TracingProtocolError (and corresponding code in session.rs) - adjustment to `scylla_supports_tablets` in test_utils.rs

Co-authored-by: Karol Baryła <[email protected]>

muzarski · 2024-11-13T15:17:31Z

Rebased on main (running clippy checks on each commit during rebase). Adjusted the speculative_execution::can_be_ignored accordingly.

muzarski marked this pull request as draft November 8, 2024 21:05

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Nov 8, 2024

muzarski self-assigned this Nov 8, 2024

muzarski force-pushed the rows-parse-error-refactor branch 2 times, most recently from 1d5ce8f to 22b6c36 Compare November 8, 2024 21:23

wprzytula added this to the 0.15.0 milestone Nov 10, 2024

muzarski marked this pull request as ready for review November 12, 2024 14:57

muzarski force-pushed the rows-parse-error-refactor branch from 22b6c36 to 74aea94 Compare November 12, 2024 16:46

muzarski requested review from wprzytula and Lorak-mmk November 12, 2024 16:47

muzarski force-pushed the rows-parse-error-refactor branch from 74aea94 to bd79e23 Compare November 12, 2024 16:53

wprzytula requested changes Nov 12, 2024

View reviewed changes

muzarski force-pushed the rows-parse-error-refactor branch from bd79e23 to 6861167 Compare November 12, 2024 20:30

muzarski force-pushed the rows-parse-error-refactor branch 2 times, most recently from 2174378 to 6eaabf2 Compare November 12, 2024 20:45

muzarski requested a review from wprzytula November 12, 2024 20:48

muzarski force-pushed the rows-parse-error-refactor branch from 6eaabf2 to 9b95760 Compare November 12, 2024 20:58

muzarski force-pushed the rows-parse-error-refactor branch from 9b95760 to 2ac12fe Compare November 12, 2024 21:16

wprzytula previously approved these changes Nov 13, 2024

View reviewed changes

wprzytula added the area/deserialization label Nov 13, 2024

Lorak-mmk reviewed Nov 13, 2024

View reviewed changes

scylla-cql/src/frame/response/result.rs Outdated Show resolved Hide resolved

Lorak-mmk dismissed wprzytula’s stale review via 7f8e749 November 13, 2024 12:36

muzarski force-pushed the rows-parse-error-refactor branch from 2ac12fe to 654a273 Compare November 13, 2024 14:22

muzarski added 2 commits November 13, 2024 16:05

f_errors: introduce PreparedMetadataParseError

52f0103

Previously, the `ResultMetadataParseError` error type was common for both Result and Prepared metadata. It does not make much sense, though. Obviously, some of the error variants are shared between these two types, but not all of them.

muzarski added 5 commits November 13, 2024 16:07

metadata: remove column count mismath check

9b625ef

This could only happen if there is a serious bug in the driver. `deser_col_specs_generic` takes `column_count` as argument and deserializes the col specs `column_count` times in a loop. Thus, this check does not make sense.

topology: remove RowsParseError usage from query_peers

e0e27c0

NextRowError -> LegacyNextRowError

6a137fd

We will re-introduce `NewRowError` as an error type returned by current async iterator API.

muzarski and others added 7 commits November 13, 2024 16:14

errors: remove QueryError::ResultMetadataParseError variant

c2a700f

It's not used anymore after adjustments to iter API. It was introduced temporarily earlier, and can now be safely removed.

legacy: adjust LegacyNextRowError

827dcb5

So we do not depend on RowsParseError anymore, new variant is introduced that represents a row deserialization failure.

errors: remove RowsParseError (finally!)

e9c446c

treewide: fix typo occure... -> occurre...

9f87616

qr: return self in into_rows_result if response is not Rows

2054163

Co-authored-by: Karol Baryła <[email protected]>

muzarski dismissed stale reviews from Lorak-mmk and wprzytula via 2054163 November 13, 2024 15:16

muzarski force-pushed the rows-parse-error-refactor branch from 9e9b55d to 2054163 Compare November 13, 2024 15:16

muzarski requested review from wprzytula and Lorak-mmk November 13, 2024 15:18

wprzytula approved these changes Nov 13, 2024

View reviewed changes

Lorak-mmk approved these changes Nov 13, 2024

View reviewed changes

wprzytula merged commit 8253681 into scylladb:main Nov 13, 2024
12 checks passed

This was referenced Nov 13, 2024

Aid in migration to the new deserialization API #1121

Merged

Unify macro attributes between serialization and deserialization derive macros #1119

Merged

Release 0.15 #1125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: replace `RowsParseError` and narrow the return types after deserialization refactor #1117

errors: replace `RowsParseError` and narrow the return types after deserialization refactor #1117

muzarski commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

muzarski commented Nov 12, 2024

wprzytula commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 13, 2024 •

edited

Loading

Lorak-mmk commented Nov 13, 2024

muzarski commented Nov 13, 2024 •

edited

Loading

Lorak-mmk commented Nov 13, 2024

muzarski commented Nov 13, 2024

errors: replace RowsParseError and narrow the return types after deserialization refactor #1117

errors: replace RowsParseError and narrow the return types after deserialization refactor #1117

Conversation

muzarski commented Nov 8, 2024 • edited Loading

Motivation

QueryResult::into_rows_result() return type

Pre-review checklist

github-actions bot commented Nov 8, 2024 • edited Loading

muzarski commented Nov 12, 2024

wprzytula commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 12, 2024

muzarski commented Nov 13, 2024 • edited Loading

Lorak-mmk commented Nov 13, 2024

muzarski commented Nov 13, 2024 • edited Loading

Lorak-mmk commented Nov 13, 2024

muzarski commented Nov 13, 2024

errors: replace `RowsParseError` and narrow the return types after deserialization refactor #1117

errors: replace `RowsParseError` and narrow the return types after deserialization refactor #1117

muzarski commented Nov 8, 2024 •

edited

Loading

`QueryResult::into_rows_result()` return type

github-actions bot commented Nov 8, 2024 •

edited

Loading

muzarski commented Nov 13, 2024 •

edited

Loading

muzarski commented Nov 13, 2024 •

edited

Loading