deser: replace ParseError with LLDeserError

Previously, there was a cycle in error types. Precisely, the `value::BuiltinDeserializationErrorKind::GenericParseError` contained a `ParseError`. Meanwhile, ParseError contained a DeserializationError. This commit removes this cycle by removing the former dependency. The GenericParseError variant will be replaced by two new variants: - LowLevelDeserializationError (wrapping the corresponding error type) - CustomTypeNotSupported(String) The main issue was that `deser_cql_value` would return a ParseError. In this commit, we change it so it returns DeserializationError. Other functions from deserialization module are adjusted accordingly as well. We also changed the variant `TabletParsingError::Parse(ParseError)` to `TabletParsingError::DeserializationError(DeserializationError)`, because the tablets module makes use of `deser_cql_value`.
scylladb · Jul 1, 2024 · 34a047f · 34a047f
1 parent b3955c6
commit 34a047f
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 55 deletions.
diff --git a/scylla-cql/src/frame/response/result.rs b/scylla-cql/src/frame/response/result.rs
@@ -5,7 +5,9 @@ use crate::frame::value::{
 };
 use crate::frame::{frame_errors::ParseError, types};
 use crate::types::deserialize::result::{RowIterator, TypedRowIterator};
-use crate::types::deserialize::value::{DeserializeValue, MapIterator, UdtIterator};
+use crate::types::deserialize::value::{
+    mk_deser_err, BuiltinDeserializationErrorKind, DeserializeValue, MapIterator, UdtIterator,
+};
 use crate::types::deserialize::{DeserializationError, FrameSlice};
 use bytes::{Buf, Bytes};
 use std::borrow::Cow;
@@ -643,7 +645,10 @@ fn deser_prepared_metadata(buf: &mut &[u8]) -> StdResult<PreparedMetadata, Parse
     })
 }
 
-pub fn deser_cql_value(typ: &ColumnType, buf: &mut &[u8]) -> StdResult<CqlValue, ParseError> {
+pub fn deser_cql_value(
+    typ: &ColumnType,
+    buf: &mut &[u8],
+) -> StdResult<CqlValue, DeserializationError> {
     use ColumnType::*;
 
     if buf.is_empty() {
@@ -662,10 +667,10 @@ pub fn deser_cql_value(typ: &ColumnType, buf: &mut &[u8]) -> StdResult<CqlValue,
 
     Ok(match typ {
         Custom(type_str) => {
-            return Err(ParseError::BadIncomingData(format!(
-                "Support for custom types is not yet implemented: {}",
-                type_str
-            )));
+            return Err(mk_deser_err::<CqlValue>(
+                typ,
+                BuiltinDeserializationErrorKind::CustomTypeNotSupported(type_str.to_string()),
+            ))
         }
         Ascii => {
             let s = String::deserialize(typ, v)?;
@@ -784,16 +789,11 @@ pub fn deser_cql_value(typ: &ColumnType, buf: &mut &[u8]) -> StdResult<CqlValue,
         Tuple(type_names) => {
             let t = type_names
                 .iter()
-                .map(|typ| {
-                    types::read_bytes_opt(buf)
-                        .map_err(ParseError::from)
-                        .and_then(|v| {
-                            v.map(|v| {
-                                CqlValue::deserialize(typ, Some(FrameSlice::new_borrowed(v)))
-                                    .map_err(Into::into)
-                            })
-                            .transpose()
-                        })
+                .map(|typ| -> StdResult<_, DeserializationError> {
+                    let raw =
+                        types::read_bytes_opt(buf).map_err(|e| mk_deser_err::<CqlValue>(typ, e))?;
+                    raw.map(|v| CqlValue::deserialize(typ, Some(FrameSlice::new_borrowed(v))))
+                        .transpose()
                 })
                 .collect::<StdResult<_, _>>()?;
             CqlValue::Tuple(t)

diff --git a/scylla-cql/src/types/deserialize/frame_slice.rs b/scylla-cql/src/types/deserialize/frame_slice.rs
@@ -1,6 +1,6 @@
 use bytes::Bytes;
 
-use crate::frame::frame_errors::ParseError;
+use crate::frame::frame_errors::LowLevelDeserializationError;
 use crate::frame::types;
 
 /// A reference to a part of the frame.
@@ -139,7 +139,9 @@ impl<'frame> FrameSlice<'frame> {
     ///
     /// If the operation fails then the slice remains unchanged.
     #[inline]
-    pub(super) fn read_cql_bytes(&mut self) -> Result<Option<FrameSlice<'frame>>, ParseError> {
+    pub(super) fn read_cql_bytes(
+        &mut self,
+    ) -> Result<Option<FrameSlice<'frame>>, LowLevelDeserializationError> {
         // We copy the slice reference, not to mutate the FrameSlice in case of an error.
         let mut slice = self.frame_subslice;
 

diff --git a/scylla-cql/src/types/deserialize/row.rs b/scylla-cql/src/types/deserialize/row.rs
@@ -416,7 +416,6 @@ mod tests {
     use assert_matches::assert_matches;
     use bytes::Bytes;
 
-    use crate::frame::frame_errors::ParseError;
     use crate::frame::response::result::{ColumnSpec, ColumnType};
     use crate::types::deserialize::row::BuiltinDeserializationErrorKind;
     use crate::types::deserialize::{DeserializationError, FrameSlice};
@@ -651,13 +650,6 @@ mod tests {
             let err = super::super::value::tests::get_deser_err(err);
             assert_eq!(err.rust_name, std::any::type_name::<CqlValue>());
             assert_eq!(err.cql_type, ColumnType::BigInt);
-            let super::super::value::BuiltinDeserializationErrorKind::GenericParseError(
-                ParseError::DeserializationError(d),
-            ) = &err.kind
-            else {
-                panic!("unexpected error kind: {}", err.kind)
-            };
-            let err = super::super::value::tests::get_deser_err(d);
             let super::super::value::BuiltinDeserializationErrorKind::ByteLengthMismatch {
                 expected: 8,
                 got: 4,

diff --git a/scylla-cql/src/types/deserialize/value.rs b/scylla-cql/src/types/deserialize/value.rs
@@ -14,7 +14,7 @@ use std::fmt::Display;
 use thiserror::Error;
 
 use super::{make_error_replace_rust_name, DeserializationError, FrameSlice, TypeCheckError};
-use crate::frame::frame_errors::ParseError;
+use crate::frame::frame_errors::LowLevelDeserializationError;
 use crate::frame::response::result::{deser_cql_value, ColumnType, CqlValue};
 use crate::frame::types;
 use crate::frame::value::{
@@ -60,9 +60,7 @@ impl<'frame> DeserializeValue<'frame> for CqlValue {
         v: Option<FrameSlice<'frame>>,
     ) -> Result<Self, DeserializationError> {
         let mut val = ensure_not_null_slice::<Self>(typ, v)?;
-        let cql = deser_cql_value(typ, &mut val).map_err(|err| {
-            mk_deser_err::<Self>(typ, BuiltinDeserializationErrorKind::GenericParseError(err))
-        })?;
+        let cql = deser_cql_value(typ, &mut val).map_err(deser_error_replace_rust_name::<Self>)?;
         Ok(cql)
     }
 }
@@ -249,7 +247,7 @@ impl_emptiable_strict_type!(
         let scale = types::read_int(&mut val).map_err(|err| {
             mk_deser_err::<Self>(
                 typ,
-                BuiltinDeserializationErrorKind::GenericParseError(err.into()),
+                BuiltinDeserializationErrorKind::LowLevelDeserializationError(err.into()),
             )
         })?;
         Ok(CqlDecimal::from_signed_be_bytes_slice_and_exponent(
@@ -267,7 +265,7 @@ impl_emptiable_strict_type!(
         let scale = types::read_int(&mut val).map_err(|err| {
             mk_deser_err::<Self>(
                 typ,
-                BuiltinDeserializationErrorKind::GenericParseError(err.into()),
+                BuiltinDeserializationErrorKind::LowLevelDeserializationError(err.into()),
             )
         })? as i64;
         let int_value = bigdecimal_04::num_bigint::BigInt::from_signed_bytes_be(val);
@@ -381,25 +379,19 @@ impl_strict_type!(
         }
 
         let months_i64 = types::vint_decode(&mut val).map_err(|err| {
-            mk_err!(BuiltinDeserializationErrorKind::GenericParseError(
-                err.into()
-            ))
+            mk_err!(BuiltinDeserializationErrorKind::LowLevelDeserializationError(err.into()))
         })?;
         let months = i32::try_from(months_i64)
             .map_err(|_| mk_err!(BuiltinDeserializationErrorKind::ValueOverflow))?;
 
         let days_i64 = types::vint_decode(&mut val).map_err(|err| {
-            mk_err!(BuiltinDeserializationErrorKind::GenericParseError(
-                err.into()
-            ))
+            mk_err!(BuiltinDeserializationErrorKind::LowLevelDeserializationError(err.into()))
         })?;
         let days = i32::try_from(days_i64)
             .map_err(|_| mk_err!(BuiltinDeserializationErrorKind::ValueOverflow))?;
 
         let nanoseconds = types::vint_decode(&mut val).map_err(|err| {
-            mk_err!(BuiltinDeserializationErrorKind::GenericParseError(
-                err.into()
-            ))
+            mk_err!(BuiltinDeserializationErrorKind::LowLevelDeserializationError(err.into()))
         })?;
 
         Ok(CqlDuration {
@@ -727,7 +719,7 @@ where
         let raw = self.raw_iter.next()?.map_err(|err| {
             mk_deser_err::<Self>(
                 self.coll_typ,
-                BuiltinDeserializationErrorKind::GenericParseError(err),
+                BuiltinDeserializationErrorKind::LowLevelDeserializationError(err),
             )
         });
         Some(raw.and_then(|raw| {
@@ -906,7 +898,7 @@ where
             Some(Err(err)) => {
                 return Some(Err(mk_deser_err::<Self>(
                     self.coll_typ,
-                    BuiltinDeserializationErrorKind::GenericParseError(err),
+                    BuiltinDeserializationErrorKind::LowLevelDeserializationError(err),
                 )));
             }
             None => return None,
@@ -916,7 +908,7 @@ where
             Some(Err(err)) => {
                 return Some(Err(mk_deser_err::<Self>(
                     self.coll_typ,
-                    BuiltinDeserializationErrorKind::GenericParseError(err),
+                    BuiltinDeserializationErrorKind::LowLevelDeserializationError(err),
                 )));
             }
             None => return None,
@@ -1184,7 +1176,7 @@ impl<'frame> Iterator for UdtIterator<'frame> {
                     keyspace: self.keyspace.to_owned(),
                     field_types: self.all_fields.to_owned(),
                 },
-                BuiltinDeserializationErrorKind::GenericParseError(err),
+                BuiltinDeserializationErrorKind::LowLevelDeserializationError(err),
             )),
 
             // The field is just missing from the serialized form
@@ -1277,7 +1269,7 @@ impl<'frame> FixedLengthBytesSequenceIterator<'frame> {
 }
 
 impl<'frame> Iterator for FixedLengthBytesSequenceIterator<'frame> {
-    type Item = Result<Option<FrameSlice<'frame>>, ParseError>;
+    type Item = Result<Option<FrameSlice<'frame>>, LowLevelDeserializationError>;
 
     fn next(&mut self) -> Option<Self::Item> {
         self.remaining = self.remaining.checked_sub(1)?;
@@ -1307,7 +1299,7 @@ impl<'frame> From<FrameSlice<'frame>> for BytesSequenceIterator<'frame> {
 }
 
 impl<'frame> Iterator for BytesSequenceIterator<'frame> {
-    type Item = Result<Option<FrameSlice<'frame>>, ParseError>;
+    type Item = Result<Option<FrameSlice<'frame>>, LowLevelDeserializationError>;
 
     fn next(&mut self) -> Option<Self::Item> {
         if self.slice.as_slice().is_empty() {
@@ -1596,8 +1588,11 @@ fn mk_deser_err_named(
 #[derive(Debug)]
 #[non_exhaustive]
 pub enum BuiltinDeserializationErrorKind {
-    /// A generic deserialization failure - legacy error type.
-    GenericParseError(ParseError),
+    /// A low level deserialization failure.
+    LowLevelDeserializationError(LowLevelDeserializationError),
+
+    /// Returned on attempt to deserialize a value of custom type.
+    CustomTypeNotSupported(String),
 
     /// Expected non-null value, got null.
     ExpectedNonNull,
@@ -1631,7 +1626,7 @@ pub enum BuiltinDeserializationErrorKind {
 impl Display for BuiltinDeserializationErrorKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            BuiltinDeserializationErrorKind::GenericParseError(err) => err.fmt(f),
+            BuiltinDeserializationErrorKind::LowLevelDeserializationError(err) => err.fmt(f),
             BuiltinDeserializationErrorKind::ExpectedNonNull => {
                 f.write_str("expected a non-null value, got null")
             }
@@ -1656,10 +1651,18 @@ impl Display for BuiltinDeserializationErrorKind {
             BuiltinDeserializationErrorKind::SetOrListError(err) => err.fmt(f),
             BuiltinDeserializationErrorKind::MapError(err) => err.fmt(f),
             BuiltinDeserializationErrorKind::TupleError(err) => err.fmt(f),
+            BuiltinDeserializationErrorKind::CustomTypeNotSupported(typ) => write!(f, "Support for custom types is not yet implemented: {}", typ),
         }
     }
 }
 
+impl From<LowLevelDeserializationError> for BuiltinDeserializationErrorKind {
+    #[inline]
+    fn from(err: LowLevelDeserializationError) -> Self {
+        Self::LowLevelDeserializationError(err)
+    }
+}
+
 /// Describes why deserialization of a set or list type failed.
 #[derive(Debug)]
 #[non_exhaustive]
@@ -2356,7 +2359,10 @@ pub(super) mod tests {
             .map_err(|typecheck_err| DeserializationError(typecheck_err.0))?;
         let mut frame_slice = FrameSlice::new(bytes);
         let value = frame_slice.read_cql_bytes().map_err(|err| {
-            mk_deser_err::<T>(typ, BuiltinDeserializationErrorKind::GenericParseError(err))
+            mk_deser_err::<T>(
+                typ,
+                BuiltinDeserializationErrorKind::LowLevelDeserializationError(err),
+            )
         })?;
         <T as DeserializeValue<'frame>>::deserialize(typ, value)
     }

diff --git a/scylla/src/transport/locator/tablets.rs b/scylla/src/transport/locator/tablets.rs
@@ -1,8 +1,8 @@
 use itertools::Itertools;
 use lazy_static::lazy_static;
 use scylla_cql::cql_to_rust::FromCqlVal;
-use scylla_cql::frame::frame_errors::ParseError;
 use scylla_cql::frame::response::result::{deser_cql_value, ColumnType, TableSpec};
+use scylla_cql::types::deserialize::DeserializationError;
 use thiserror::Error;
 use tracing::warn;
 use uuid::Uuid;
@@ -16,7 +16,7 @@ use std::sync::Arc;
 #[derive(Error, Debug)]
 pub(crate) enum TabletParsingError {
     #[error(transparent)]
-    Parse(#[from] ParseError),
+    DeserializationError(#[from] DeserializationError),
     #[error("Shard id for tablet is negative: {0}")]
     ShardNum(i32),
 }
@@ -616,7 +616,7 @@ mod tests {
             HashMap::from([(CUSTOM_PAYLOAD_TABLETS_V1_KEY.to_string(), vec![1, 2, 3])]);
         assert_matches::assert_matches!(
             RawTablet::from_custom_payload(&custom_payload),
-            Some(Err(TabletParsingError::Parse(_)))
+            Some(Err(TabletParsingError::DeserializationError(_)))
         );
     }
 
@@ -646,7 +646,7 @@ mod tests {
 
         assert_matches::assert_matches!(
             RawTablet::from_custom_payload(&custom_payload),
-            Some(Err(TabletParsingError::Parse(_)))
+            Some(Err(TabletParsingError::DeserializationError(_)))
         );
     }