errors: Replace `ParseError` with `CqlResponseParseError` in `frame::response` module #1026

muzarski · 2024-07-01T11:34:40Z

Depends on: #1017

Motivation

We ultimately want to get rid of the ParseError. This error type is overloaded with a lot of variants which do not provide much context to the users. In addition, a lot of errors are stringified.

This PR gets rid of ParseError from the frame::response module which is responsible for CQL response deserialization. This is the place where the usage of ParseError is abused the most.

Changes

Types of response

There are multiple types of responses. We introduce a new error type for each of them, e.g. CqlResultParseError or CqlSupportedParseError. I follow a bottom-to-top approach, starting from implementing an error types for CQL column type deserialization.

Each commit introduces a new error type, which is firstly encapsulated by ParseError (so the higher level functions returning ParseError can apply ? operator). Once we exhaust all of the error variants that can be returned by some function, we introduce a new error type which encapsulates error types introduced in previous commits. These error types are then moved out of the ParseError (if possible, sometimes the functions are not exclusive to frame::response module which is the only module addressed in this PR).

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-07-01T11:39:30Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 45f02889b859606d2e378bcabb303fdfb03498d1
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 45f02889b859606d2e378bcabb303fdfb03498d1
     Cloning 45f02889b859606d2e378bcabb303fdfb03498d1
     Parsing scylla v0.13.0 (current)
      Parsed [  19.975s] (current)
     Parsing scylla v0.13.0 (baseline)
      Parsed [  19.610s] (baseline)
    Checking scylla v0.13.0 -> v0.13.0 (no change)
     Checked [   0.073s] 79 checks: 79 pass, 0 skip
     Summary no semver update required
    Finished [  39.703s] scylla
     Parsing scylla-cql v0.2.0 (current)
      Parsed [   9.920s] (current)
     Parsing scylla-cql v0.2.0 (baseline)
      Parsed [   9.934s] (baseline)
    Checking scylla-cql v0.2.0 -> v0.2.0 (no change)
     Checked [   0.074s] 79 checks: 77 pass, 2 fail, 0 warn, 0 skip

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/enum_variant_added.ron

Failed in:
  variant NewSessionError:CqlResponseParseError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/errors.rs:390
  variant QueryError:CqlResponseParseError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/errors.rs:26

--- 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.34.0/src/lints/enum_variant_missing.ron

Failed in:
  variant ParseError::TypeNotImplemented, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-45f02889b859606d2e378bcabb303fdfb03498d1/f6fc9a0a71d023aeebba1e44dad73c6172ad77a9/scylla-cql/src/frame/frame_errors.rs:49

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

wprzytula · 2024-07-02T10:46:57Z

You haven't checked nor crossed out the I added relevant tests for new features and bug fixes option. Do you believe any tests are relevant here?

wprzytula

Awesome changes!

Mainly some nits.

scylla-cql/src/frame/frame_errors.rs

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

scylla-cql/src/frame/frame_errors.rs

muzarski · 2024-07-15T14:18:21Z

v2:

rebased on current version of low_lever_deser_errors
addressed review comments

wprzytula

Awesome work! This is a huge step in direction of clean and informative error handling.

scylla-cql/src/frame/frame_errors.rs

scylla-cql/src/frame/request/auth_response.rs

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

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

muzarski · 2024-08-06T17:49:15Z

Rebased on main

muzarski · 2024-08-06T18:12:28Z

v2.1: addressed nit review comments

wprzytula

LGTM, thank you for finally making errors orderly!

Lorak-mmk · 2024-08-07T12:08:37Z

scylla-cql/src/frame/frame_errors.rs

+    #[error("'Schema_change' response deserialization failed: {0}")]
+    SchemaChangeEventParseError(#[from] SchemaChangeEventParseError),


@wprzytula @muzarski Do you think that this variant should be present here?
CQL events are always sent on a separate channel (-1) which handles only that - so there should be no way for a user to get this error

I think that you meant CqlResponseParseError::CqlEventParseError variant. I think that you are right. If so, then I'll remove this variant from public API.

I see no easy way to decouple event handling from ordinary response handling. Thus, we need a common error type for both.

It's true. Now the question is whether we want to introduce new public error type excluding this specific variant? I'm not sure if there is much sense to do so.

I'd leave it as is for now.

I see your point (why would handling events have its own error variants in QueryError), but the size of such refactor makes it out of this PR's scope IMO.

It's not only about Events. CqlResponseParseError has the following variants:

CqlErrorParseError CqlAuthChallengeParseError CqlAuthSuccessParseError CqlAuthenticateParseError CqlSupportedParseError CqlEventParseError CqlResultParseError

If I'm not mistaken only first and last are possible for the user to encounter in QueryError (other ones only possible in NewSessionError). I understand that it may increase the amount of changes, but I think we should avoid creating such types anyway. It makes it much harder for users who want to handle errors in a different way than just printing them.

OK, this is quite convincing. I suggest that @muzarski analyse all possible error routes in the driver and try to redesign the error hierarchy so that user-facing error types don't contain variants that the user is never supposed to see.

One idea is that we can introduce two layers of error handling:

a lower-lever one, resembling one introduced in this PR,

a higher-level one, restricting variants to those that a user really might see. It should be organised with the user's perspective in mind rather than reflect the driver's inner architecture.

scylla-cql/src/frame/frame_errors.rs

wprzytula · 2024-08-13T13:39:36Z

ref: #1056

muzarski · 2024-08-20T14:37:43Z

Rebased on main

The Display implementation for this error already says: "could not convert slice to array". No need to provide more context.

Introduced an error type returned when we fail to deserialize a RESULT response type to SET KEYSPACE request.

Introduced an error type returned when we fail to deserialize a RESULT::Schema_change response.

The `allow` is not needed anymore.

Introduced an error type returned when deserialization of table spec fails.

Introduced an error type returned when we fail to deserialize a CQL type.

An error type returned when we fail to deserialize table's column specs.

Replaced `read_int().try_into()` with a utility function that does exactly the same. This way, the caller doesn't have to handle two separate errors.

Introduced an error returned when deserialization of Result/PreparedMetadata fails.

Error returned when we fail to deserialize a RESULT::Prepared CQL response.

An error type returned when deserialization of RESULT::Rows response fails.

An error type returned when RESULT response deserialization fails.

Introduced an error type returned when we fail to deserialize either StatusChangeEvent or TopologyChangeEvent.

Introduced an error type returned when we fail to deserialize an EVENT response.

In the following commit we want to introduce a CqlResponseParseError type which will be a new variant of `[Query/NewSession]Error`. These two error types implement a `Clone` trait. They require a `Clone`, since there is some cloning of errors in `history` module. This is why, we need to derive Clone for all of the types introduces in previous commits. Unfortunately, the `std::io::Error` does not implement Clone. This is why we need to wrap any appearing `std::io::Error` with `Arc`, so the enum types encapsulating it can derive Clone.

muzarski self-assigned this Jul 1, 2024

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

muzarski requested review from wprzytula and Lorak-mmk July 1, 2024 11:55

wprzytula requested changes Jul 2, 2024

View reviewed changes

wprzytula self-assigned this Jul 10, 2024

muzarski force-pushed the remove_parse_error branch 2 times, most recently from 9093ca9 to bc37fbe Compare July 15, 2024 14:17

wprzytula reviewed Jul 31, 2024

View reviewed changes

wprzytula removed their assignment Aug 1, 2024

muzarski force-pushed the remove_parse_error branch from bc37fbe to fe2f625 Compare August 6, 2024 17:49

muzarski force-pushed the remove_parse_error branch from fe2f625 to 0b1bf1e Compare August 6, 2024 18:10

muzarski requested a review from wprzytula August 6, 2024 18:12

wprzytula approved these changes Aug 7, 2024

View reviewed changes

muzarski mentioned this pull request Aug 7, 2024

errors: remove ParseError #1054

Merged

6 tasks

Lorak-mmk reviewed Aug 7, 2024

View reviewed changes

muzarski force-pushed the remove_parse_error branch from 0b1bf1e to b29d0fc Compare August 7, 2024 13:42

muzarski requested a review from Lorak-mmk August 7, 2024 13:44

wprzytula added this to the 0.14.0 milestone Aug 20, 2024

muzarski force-pushed the remove_parse_error branch from b29d0fc to b5f48d5 Compare August 20, 2024 14:37

muzarski added 3 commits August 22, 2024 11:38

f_errors: add information about accepted bytes lengths for inet

d786f29

f_errors: remove context for TryFromSliceError

1c2c5f2

The Display implementation for this error already says: "could not convert slice to array". No need to provide more context.

f_errors: SetKeyspaceParseError

3f50fe9

Introduced an error type returned when we fail to deserialize a RESULT response type to SET KEYSPACE request.

muzarski added 19 commits August 22, 2024 11:38

f_errors: SchemaChangeEventParseError

c55298c

Introduced an error type returned when we fail to deserialize a RESULT::Schema_change response.

frame/result: enable clippy lint

b2504a8

The `allow` is not needed anymore.

f_errors: TableSpecParseError

866b79d

Introduced an error type returned when deserialization of table spec fails.

f_errors: CqlTypeParseError

8a22e09

Introduced an error type returned when we fail to deserialize a CQL type.

f_errors: ColumnSpecsParseError

335e05b

An error type returned when we fail to deserialize table's column specs.

result: read_int().try_into() -> read_int_length()

85ac6be

Replaced `read_int().try_into()` with a utility function that does exactly the same. This way, the caller doesn't have to handle two separate errors.

f_errors: ResultMetadataParseError

215df16

Introduced an error returned when deserialization of Result/PreparedMetadata fails.

f_errors: PreparedParseError

94d7d9f

Error returned when we fail to deserialize a RESULT::Prepared CQL response.

f_errors: RowsParseError

51b69f4

An error type returned when deserialization of RESULT::Rows response fails.

f_errors: CqlResultParseError

ff3d733

An error type returned when RESULT response deserialization fails.

f_errors: ClusterChangeParseError

94f90b5

Introduced an error type returned when we fail to deserialize either StatusChangeEvent or TopologyChangeEvent.

f_errors: CqlEventParseError

fdb6f50

Introduced an error type returned when we fail to deserialize an EVENT response.

f_errors: CqlSupportedParseError

a816963

f_errors: CqlAuthenticateParseError

2d9e955

f_errors: CqlAuthSuccessParseError

f1c92cc

f_errors: CqlAuthChallengeParseError

6fcbe9a

f_errors: CqlErrorParseError

fc8fe80

f_errors: CqlResponseParseError

46d639b

muzarski force-pushed the remove_parse_error branch from b5f48d5 to 46d639b Compare August 22, 2024 09:38

Lorak-mmk approved these changes Aug 22, 2024

View reviewed changes

Lorak-mmk merged commit 9dcc2f1 into scylladb:main Aug 22, 2024
11 checks passed

muzarski deleted the remove_parse_error branch October 29, 2024 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: Replace `ParseError` with `CqlResponseParseError` in `frame::response` module #1026

errors: Replace `ParseError` with `CqlResponseParseError` in `frame::response` module #1026

muzarski commented Jul 1, 2024 •

edited

Loading

github-actions bot commented Jul 1, 2024 •

edited

Loading

wprzytula commented Jul 2, 2024

wprzytula left a comment

muzarski commented Jul 15, 2024

wprzytula left a comment

muzarski commented Aug 6, 2024

muzarski commented Aug 6, 2024

wprzytula left a comment

Lorak-mmk Aug 7, 2024

muzarski Aug 7, 2024

wprzytula Aug 7, 2024

muzarski Aug 7, 2024

muzarski Aug 7, 2024

wprzytula Aug 7, 2024

Lorak-mmk Aug 19, 2024 •

edited

Loading

wprzytula Aug 19, 2024 •

edited

Loading

wprzytula commented Aug 13, 2024

muzarski commented Aug 20, 2024

		#[error("'Schema_change' response deserialization failed: {0}")]
		SchemaChangeEventParseError(#[from] SchemaChangeEventParseError),

errors: Replace ParseError with CqlResponseParseError in frame::response module #1026

errors: Replace ParseError with CqlResponseParseError in frame::response module #1026

Conversation

muzarski commented Jul 1, 2024 • edited Loading

Motivation

Changes

Types of response

Pre-review checklist

github-actions bot commented Jul 1, 2024 • edited Loading

wprzytula commented Jul 2, 2024

wprzytula left a comment

Choose a reason for hiding this comment

muzarski commented Jul 15, 2024

wprzytula left a comment

Choose a reason for hiding this comment

muzarski commented Aug 6, 2024

muzarski commented Aug 6, 2024

wprzytula left a comment

Choose a reason for hiding this comment

Lorak-mmk Aug 7, 2024

Choose a reason for hiding this comment

muzarski Aug 7, 2024

Choose a reason for hiding this comment

wprzytula Aug 7, 2024

Choose a reason for hiding this comment

muzarski Aug 7, 2024

Choose a reason for hiding this comment

muzarski Aug 7, 2024

Choose a reason for hiding this comment

wprzytula Aug 7, 2024

Choose a reason for hiding this comment

Lorak-mmk Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

wprzytula Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

wprzytula commented Aug 13, 2024

muzarski commented Aug 20, 2024

errors: Replace `ParseError` with `CqlResponseParseError` in `frame::response` module #1026

errors: Replace `ParseError` with `CqlResponseParseError` in `frame::response` module #1026

muzarski commented Jul 1, 2024 •

edited

Loading

github-actions bot commented Jul 1, 2024 •

edited

Loading

Lorak-mmk Aug 19, 2024 •

edited

Loading

wprzytula Aug 19, 2024 •

edited

Loading