errors: Low level (de)ser errors

[muzarski]

Ref: #519

Motivation

This commit is a small step forward to resolving the issues related to errors in rust driver. The main goal of this PR is to get rid of ParseError from frame::types module.

As a bonus, the newly introduced deserialization module is adjusted as well. Thanks to that, we remove the cycle in error types (DeserializationError <-> ParseError).

Changes

• introduced new enum error type returned from types module: LowLevelDeserializationError
• changed the return error type of types::read_* functions to LowLevelDeserializationError
• replaced value::BuiltinDeserializationErrorKind::GenericParseError with new variants
• changed the return error type of deser_cql_value to DeserializationError
• replaced TabletsParsingError::Parse with TabletsParsingError::DeserializationError

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]

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev aed3e443822d2f9241e1e74d8c0d02813d1b3921
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev aed3e443822d2f9241e1e74d8c0d02813d1b3921
     Cloning aed3e443822d2f9241e1e74d8c0d02813d1b3921
     Parsing scylla v0.13.0 (current)
      Parsed [  21.572s] (current)
     Parsing scylla v0.13.0 (baseline)
      Parsed [  19.725s] (baseline)
    Checking scylla v0.13.0 -> v0.13.0 (no change)
     Checked [   0.072s] 75 checks; 75 passed, 0 unnecessary
    Finished [  41.422s] scylla
     Parsing scylla-cql v0.2.0 (current)
      Parsed [  10.394s] (current)
     Parsing scylla-cql v0.2.0 (baseline)
      Parsed [  10.517s] (baseline)
    Checking scylla-cql v0.2.0 -> v0.2.0 (no change)
     Checked [   0.069s] 75 checks; 73 passed, 2 failed, 0 unnecessary

--- 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.32.0/src/lints/enum_variant_added.ron

Failed in:
  variant ParseError:LowLevelDeserializationError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/frame/frame_errors.rs:39

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

Failed in:
  variant BuiltinDeserializationErrorKind::GenericParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-aed3e443822d2f9241e1e74d8c0d02813d1b3921/0e1fe3201f7d097cab4f00e0d8de876117b2eade/scylla-cql/src/types/deserialize/value.rs:1600
     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  21.028s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

[muzarski]

v1.1: rebased on main

[muzarski]

v2:

• addressed review comments
• converted error type for all types::[read/write]_ to LL(De)SerializationError. Previously, I forgot to unify the return error types for all of the utility functions. This is helpful, so the higher level errors won't need to handle std::io::Error/std::num::TryFromIntError separately.

[wprzytula]

What about adding context to LLDeserError in new deserialization framework?

New deserialization framework needs more context.

[Lorak-mmk]

Please remind me, is the current plan to get rid of ParseError completely?
If not, we need to remember to add non_exhaustive to it, and other errors (like FrameError above).

[muzarski]

I ultimately would like to get rid of ParseError. It will be decoupled into multiple error types, which will be more specific and what's the most important - they will be typed. As of now, most of the ParseErrors are stringified (e.g. converted to QueryError::InvalidMessage).

There is a similar issue with a FrameError, however I'm not planning to remove it. I'd like to decouple it and introduce a distinction between FrameSerializationError (for client frames) and FrameDeserializationError (for server frames). These two could be then represented as variants of {Query/NewSession}Error (and not stringified to QueryError::InvalidMessage, which is something we currently do).

[Lorak-mmk]

I love it when github decides to post my review but without the main comment I wrote :) Posting again...

I think those are good changes in the right direction in general.

Please explain, because I don't remember from our discussion and I can't understand from commits: why do we have a distinction between deserialization error and low level deserialization error? The errors are a public API, why should a user of a library care which internal layer of the driver returned the error and not just what the error is?

Note: If we don't have something like this, whole error refactor should be finished with tests that will present and assert the error messages in various conditions. That way we can see them without writing one-off code to trigger those errors.

[wprzytula]

Please explain, because I don't remember from our discussion and I can't understand from commits: why do we have a distinction between deserialization error and low level deserialization error? The errors are a public API, why should a user of a library care which internal layer of the driver returned the error and not just what the error is?

LowLevelDeserializationError is strictly connected to the CQL protocol types deserialization layer. On top of it, there is DeserializationError, which carries the meaning of a logical error during deserialization.
For example, the underlying CQL protocol type structure can be perfectly valid (so no LowLevelDeserializationError involved), but invalid for the expected logical structure of, say, a CQL Set. (e.g., an int present when bytes or string was expected). While the example is not perfect, I hope you get the difference.

[muzarski]

v3:

• rebased on main
• removed LowLevelSerializationError at all. The reason is that types::write_* functions all return std::num::TryFromIntError, so currently there is no need to wrap it with an additional error type.
• narrowed the return type of types::read_* functions to std::io::Error (if possible)
• added more context to deserialization framework. Introduced three new variants which wrap LowLevelDeserializationError:
  • BadDate - deserialization of some date's field failed (months, days, or nanoseconds)
  • BadDecimalScale - deserialization of decimal's scale failed
  • RawCqlBytesReadError - failed to read raw cql value bytes from a frame slice

[Lorak-mmk]

* removed `LowLevelSerializationError` at all. The reason is that `types::write_*` functions all return `std::num::TryFromIntError`, so currently there is no need to wrap it with an additional error type.

I didn't read the changes yet, so my question may be inaccurate.
Isn't it possible for this situation to change in the future?
If it is, then LowLevelSerializationError is better because we can add variants to it.
If it can't then nvm

[muzarski]

* removed `LowLevelSerializationError` at all. The reason is that `types::write_*` functions all return `std::num::TryFromIntError`, so currently there is no need to wrap it with an additional error type.

I didn't read the changes yet, so my question may be inaccurate.

Isn't it possible for this situation to change in the future?

If it is, then LowLevelSerializationError is better because we can add variants to it.

If it can't then nvm

I don't think it will change in the future. I also wanted to introduce another layer represented as LowLevelSerializationError (marked as non_exhaustive). The problem is that, as of now we would unnecessarily broaden the error type (from std::num::TryFromIntError to LowLevelSerializationError) in functions, which contradicts the approach we took for LLDeserError.