From 34a047fd81d87a0dadf24b5095eb2d25e59c8fe1 Mon Sep 17 00:00:00 2001 From: muzarski Date: Wed, 26 Jun 2024 09:46:46 +0200 Subject: [PATCH] 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`. --- scylla-cql/src/frame/response/result.rs | 32 +++++------ .../src/types/deserialize/frame_slice.rs | 6 +- scylla-cql/src/types/deserialize/row.rs | 8 --- scylla-cql/src/types/deserialize/value.rs | 56 ++++++++++--------- scylla/src/transport/locator/tablets.rs | 8 +-- 5 files changed, 55 insertions(+), 55 deletions(-) diff --git a/scylla-cql/src/frame/response/result.rs b/scylla-cql/src/frame/response/result.rs index 2a69ab2d87..0d73991f39 100644 --- 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 StdResult { +pub fn deser_cql_value( + typ: &ColumnType, + buf: &mut &[u8], +) -> StdResult { use ColumnType::*; if buf.is_empty() { @@ -662,10 +667,10 @@ pub fn deser_cql_value(typ: &ColumnType, buf: &mut &[u8]) -> StdResult { - return Err(ParseError::BadIncomingData(format!( - "Support for custom types is not yet implemented: {}", - type_str - ))); + return Err(mk_deser_err::( + 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 { 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::(typ, e))?; + raw.map(|v| CqlValue::deserialize(typ, Some(FrameSlice::new_borrowed(v)))) + .transpose() }) .collect::>()?; CqlValue::Tuple(t) diff --git a/scylla-cql/src/types/deserialize/frame_slice.rs b/scylla-cql/src/types/deserialize/frame_slice.rs index cfc98d5ce5..4471960a03 100644 --- 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>, ParseError> { + pub(super) fn read_cql_bytes( + &mut self, + ) -> Result>, 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 index c66f3c7328..5dfec4b12a 100644 --- 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::()); 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 index 8ac66a6914..465586b6a8 100644 --- 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>, ) -> Result { let mut val = ensure_not_null_slice::(typ, v)?; - let cql = deser_cql_value(typ, &mut val).map_err(|err| { - mk_deser_err::(typ, BuiltinDeserializationErrorKind::GenericParseError(err)) - })?; + let cql = deser_cql_value(typ, &mut val).map_err(deser_error_replace_rust_name::)?; Ok(cql) } } @@ -249,7 +247,7 @@ impl_emptiable_strict_type!( let scale = types::read_int(&mut val).map_err(|err| { mk_deser_err::( 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::( 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.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.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.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>, ParseError>; + type Item = Result>, LowLevelDeserializationError>; fn next(&mut self) -> Option { self.remaining = self.remaining.checked_sub(1)?; @@ -1307,7 +1299,7 @@ impl<'frame> From> for BytesSequenceIterator<'frame> { } impl<'frame> Iterator for BytesSequenceIterator<'frame> { - type Item = Result>, ParseError>; + type Item = Result>, LowLevelDeserializationError>; fn next(&mut self) -> Option { 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 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::(typ, BuiltinDeserializationErrorKind::GenericParseError(err)) + mk_deser_err::( + typ, + BuiltinDeserializationErrorKind::LowLevelDeserializationError(err), + ) })?; >::deserialize(typ, value) } diff --git a/scylla/src/transport/locator/tablets.rs b/scylla/src/transport/locator/tablets.rs index 14a3ea2b32..5549bed2f6 100644 --- 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(_))) ); }