From f8e1db6d8cda5491ebe85d28d6c2efb0da4adeb8 Mon Sep 17 00:00:00 2001 From: Jiacai Liu Date: Wed, 4 Sep 2024 15:35:27 +0800 Subject: [PATCH] refactor(rust): simplify error handle (#1823) ## What does this PR do? Make error easy to use. In most case, users don't care about error details, so too many fields in enum is hard to write, and hard to use. So I refactor it to include an `Other` field to be used as a general Error, most error can be mapped to it directly. `Ref` is a special case, so I leave it as it's now. ## Related issues ## Does this PR introduce any user-facing change? - [ ] Does this PR introduce any public API change? - [ ] Does this PR introduce any binary protocol compatibility change? ## Benchmark --- rust/fury-core/Cargo.toml | 1 + rust/fury-core/src/error.rs | 85 ++++++++--------------- rust/fury-core/src/meta/meta_string.rs | 59 ++++++++++------ rust/fury-core/src/meta/type_meta.rs | 5 +- rust/fury-core/src/resolver/context.rs | 3 +- rust/fury-core/src/row/row.rs | 20 +++--- rust/fury-core/src/serializer/any.rs | 5 +- rust/fury-core/src/serializer/datetime.rs | 20 +++--- rust/fury-core/src/serializer/mod.rs | 20 +++--- rust/fury-core/src/serializer/option.rs | 18 ++--- rust/fury-core/src/types.rs | 3 +- rust/fury-derive/src/object/read.rs | 4 +- 12 files changed, 122 insertions(+), 121 deletions(-) diff --git a/rust/fury-core/Cargo.toml b/rust/fury-core/Cargo.toml index 5e8f8b774c..0e4b876023 100644 --- a/rust/fury-core/Cargo.toml +++ b/rust/fury-core/Cargo.toml @@ -28,6 +28,7 @@ quote = { default-features = false, version = "1.0" } byteorder = { version = "1.4" } chrono = "0.4" thiserror = { default-features = false, version = "1.0" } +anyhow = "1" num_enum = "0.5.1" diff --git a/rust/fury-core/src/error.rs b/rust/fury-core/src/error.rs index e2b2e25142..de54d3bc5e 100644 --- a/rust/fury-core/src/error.rs +++ b/rust/fury-core/src/error.rs @@ -15,67 +15,38 @@ // specific language governing permissions and limitations // under the License. -use super::types::Language; +use thiserror::Error; -#[derive(thiserror::Error, Debug)] +#[derive(Error, Debug)] +#[non_exhaustive] pub enum Error { - #[error("Field is not Option type, can't be deserialize of None")] - Null, - #[error("Fury on Rust not support Ref type")] Ref, - #[error("Fury on Rust not support RefValue type")] - RefValue, - - #[error("BadRefFlag")] - BadRefFlag, - - #[error("Bad FieldType; expected: {expected:?}, actual: {actual:?}")] - FieldType { expected: i16, actual: i16 }, - - #[error("Bad timestamp; out-of-range number of milliseconds")] - NaiveDateTime, - - #[error("Bad date; out-of-range")] - NaiveDate, - - #[error("Schema is not consistent; expected: {expected:?}, actual: {actual:?}")] - StructHash { expected: u32, actual: u32 }, - - #[error("Bad Tag Type: {0}")] - TagType(u8), - - #[error("Only Xlang supported; receive: {language:?}")] - UnsupportedLanguage { language: Language }, - - #[error("Unsupported Language Code; receive: {code:?}")] - UnsupportedLanguageCode { code: u8 }, - - #[error("Unsupported encoding of field name in type meta; receive: {code:?}")] - UnsupportedTypeMetaFieldNameEncoding { code: u8 }, - - #[error("encoded_data cannot be empty")] - EncodedDataEmpty, - - #[error("Long meta string than 32767 is not allowed")] - LengthExceed, - - #[error("Non-ASCII characters in meta string are not allowed")] - OnlyAllowASCII, - - #[error("Unsupported character for LOWER_SPECIAL encoding: {ch:?}")] - UnsupportedLowerSpecialCharacter { ch: char }, - - #[error("Unsupported character for LOWER_UPPER_DIGIT_SPECIAL encoding: {ch:?}")] - UnsupportedLowerUpperDigitSpecialCharacter { ch: char }, - - #[error("Invalid character value for LOWER_SPECIAL decoding: {value:?}")] - InvalidLowerSpecialValue { value: u8 }, - - #[error("Invalid character value for LOWER_UPPER_DIGIT_SPECIAL decoding: {value:?}")] - InvalidLowerUpperDigitSpecialValue { value: u8 }, + #[error(transparent)] + Other(#[from] anyhow::Error), +} - #[error("Unregistered type when serializing or deserializing object of Any type: {value:?}")] - UnregisteredType { value: u32 }, +/// Works like anyhow's [ensure](https://docs.rs/anyhow/latest/anyhow/macro.ensure.html) +/// But return `Return` +#[macro_export] +macro_rules! ensure { + ($cond:expr, $msg:literal) => { + if !$cond { + return Err(anyhow::anyhow!($msg).into()); + } + }; + ($cond:expr, $err:expr) => { + if !$cond { + return Err($err.into()); + } + }; + ($cond:expr, $fmt:expr, $($arg:tt)*) => { + if !$cond { + return Err(anyhow::anyhow!($fmt, $($arg)*).into()); + } + }; } + +// Re-export anyhow::Error since it may appear in the public API. +pub use anyhow::Error as AnyhowError; diff --git a/rust/fury-core/src/meta/meta_string.rs b/rust/fury-core/src/meta/meta_string.rs index 282b1ab549..df74c384ac 100644 --- a/rust/fury-core/src/meta/meta_string.rs +++ b/rust/fury-core/src/meta/meta_string.rs @@ -15,9 +15,15 @@ // specific language governing permissions and limitations // under the License. +use anyhow::anyhow; + +use crate::ensure; use crate::error::Error; use crate::meta::string_util; +// equal to "std::i16::MAX" +const SHORT_MAX_VALUE: usize = 32767; + #[derive(Debug, PartialEq)] pub enum Encoding { Utf8 = 0x00, @@ -39,9 +45,8 @@ impl MetaString { pub fn new(original: String, encoding: Encoding, bytes: Vec) -> Result { let mut strip_last_char = false; if encoding != Encoding::Utf8 { - if bytes.is_empty() { - return Err(Error::EncodedDataEmpty); - } + ensure!(!bytes.is_empty(), anyhow!("Encoded data cannot be empty")); + strip_last_char = (bytes[0] & 0x80) != 0; } Ok(MetaString { @@ -88,11 +93,13 @@ impl MetaStringEncoder { if input.is_empty() { return MetaString::new(input.to_string(), Encoding::Utf8, vec![]); } - // equal to "std::i16::MAX" - const SHORT_MAX_VALUE: usize = 32767; - if input.len() >= SHORT_MAX_VALUE { - return Err(Error::LengthExceed); - } + ensure!( + input.len() < SHORT_MAX_VALUE, + anyhow!( + "Meta string is too long, max:{SHORT_MAX_VALUE}, current:{}", + input.len() + ) + ); if !self.is_latin(input) { return MetaString::new(input.to_string(), Encoding::Utf8, input.as_bytes().to_vec()); } @@ -164,14 +171,18 @@ impl MetaStringEncoder { if input.is_empty() { return MetaString::new(input.to_string(), Encoding::Utf8, vec![]); } - // equal to "std::i16::MAX" - const SHORT_MAX_VALUE: usize = 32767; - if input.len() >= SHORT_MAX_VALUE { - return Err(Error::LengthExceed); - } - if encoding != Encoding::Utf8 && !self.is_latin(input) { - return Err(Error::OnlyAllowASCII); - }; + ensure!( + input.len() < SHORT_MAX_VALUE, + anyhow!( + "Meta string is too long, max:{SHORT_MAX_VALUE}, current:{}", + input.len() + ) + ); + ensure!( + encoding == Encoding::Utf8 || self.is_latin(input), + anyhow!("Non-ASCII characters in meta string are not allowed") + ); + if input.is_empty() { return MetaString::new(input.to_string(), Encoding::Utf8, vec![]); }; @@ -261,7 +272,9 @@ impl MetaStringEncoder { '_' => Ok(27), '$' => Ok(28), '|' => Ok(29), - _ => Err(Error::UnsupportedLowerSpecialCharacter { ch: c }), + _ => Err(anyhow!( + "Unsupported character for LOWER_UPPER_DIGIT_SPECIAL encoding: {c}" + ))?, }, 6 => match c { 'a'..='z' => Ok(c as u8 - b'a'), @@ -273,7 +286,9 @@ impl MetaStringEncoder { } else if c == '_' { Ok(63) } else { - Err(Error::UnsupportedLowerUpperDigitSpecialCharacter { ch: c }) + Err(anyhow!( + "Invalid character value for LOWER_SPECIAL decoding: {c:?}" + ))? } } }, @@ -362,7 +377,9 @@ impl MetaStringDecoder { 27 => Ok('_'), 28 => Ok('$'), 29 => Ok('|'), - _ => Err(Error::InvalidLowerSpecialValue { value: char_value }), + _ => Err(anyhow!( + "Invalid character value for LOWER_SPECIAL decoding: {char_value}" + ))?, } } @@ -373,7 +390,9 @@ impl MetaStringDecoder { 52..=61 => Ok((b'0' + char_value - 52) as char), 62 => Ok('.'), 63 => Ok('_'), - _ => Err(Error::InvalidLowerUpperDigitSpecialValue { value: char_value }), + _ => Err(anyhow!( + "Invalid character value for LOWER_UPPER_DIGIT_SPECIAL decoding: {char_value}" + ))?, } } diff --git a/rust/fury-core/src/meta/type_meta.rs b/rust/fury-core/src/meta/type_meta.rs index c1086e0f25..7c784f8d6d 100644 --- a/rust/fury-core/src/meta/type_meta.rs +++ b/rust/fury-core/src/meta/type_meta.rs @@ -19,6 +19,7 @@ use super::meta_string::MetaStringEncoder; use crate::buffer::{Reader, Writer}; use crate::error::Error; use crate::meta::{Encoding, MetaStringDecoder}; +use anyhow::anyhow; pub struct FieldInfo { field_name: String, @@ -38,7 +39,9 @@ impl FieldInfo { 0x00 => Ok(Encoding::Utf8), 0x01 => Ok(Encoding::AllToLowerSpecial), 0x02 => Ok(Encoding::LowerUpperDigitSpecial), - _ => Err(Error::UnsupportedTypeMetaFieldNameEncoding { code: value }), + _ => Err(anyhow!( + "Unsupported encoding of field name in type meta, value:{value}" + ))?, } } diff --git a/rust/fury-core/src/resolver/context.rs b/rust/fury-core/src/resolver/context.rs index 635d161d92..d7c0341910 100644 --- a/rust/fury-core/src/resolver/context.rs +++ b/rust/fury-core/src/resolver/context.rs @@ -18,6 +18,7 @@ use crate::buffer::{Reader, Writer}; use crate::error::Error; use crate::fury::Fury; +use anyhow::anyhow; use crate::meta::TypeMeta; use crate::resolver::meta_resolver::{MetaReaderResolver, MetaWriterResolver}; @@ -119,7 +120,7 @@ impl<'de, 'bf: 'de> ReadContext<'de, 'bf> { self.tags.push(tag); Ok(tag) } else { - Err(Error::TagType(tag_type)) + Err(anyhow!("Unknown tag type, value:{tag_type}"))? } } } diff --git a/rust/fury-core/src/row/row.rs b/rust/fury-core/src/row/row.rs index f9e995b1cf..f71f7bc328 100644 --- a/rust/fury-core/src/row/row.rs +++ b/rust/fury-core/src/row/row.rs @@ -17,6 +17,7 @@ use crate::util::EPOCH; use crate::{buffer::Writer, error::Error}; +use anyhow::anyhow; use byteorder::{ByteOrder, LittleEndian}; use chrono::{DateTime, Days, NaiveDate, NaiveDateTime}; use std::collections::BTreeMap; @@ -95,10 +96,11 @@ impl<'a> Row<'a> for NaiveDate { fn cast(bytes: &[u8]) -> Self::ReadResult { let days = LittleEndian::read_u32(bytes); - match EPOCH.checked_add_days(Days::new(days.into())) { - Some(value) => Ok(value), - None => Err(Error::NaiveDate), - } + EPOCH + .checked_add_days(Days::new(days.into())) + .ok_or(Error::from(anyhow!( + "Date out of range, {days} days since epoch" + ))) } } @@ -111,11 +113,11 @@ impl<'a> Row<'a> for NaiveDateTime { fn cast(bytes: &[u8]) -> Self::ReadResult { let timestamp = LittleEndian::read_u64(bytes); - let ret = DateTime::from_timestamp_millis(timestamp as i64).map(|dt| dt.naive_utc()); - match ret { - Some(r) => Ok(r), - None => Err(Error::NaiveDateTime), - } + DateTime::from_timestamp_millis(timestamp as i64) + .map(|dt| dt.naive_utc()) + .ok_or(Error::from(anyhow!( + "Date out of range, timestamp:{timestamp}" + ))) } } diff --git a/rust/fury-core/src/serializer/any.rs b/rust/fury-core/src/serializer/any.rs index f05c435c09..64a275d956 100644 --- a/rust/fury-core/src/serializer/any.rs +++ b/rust/fury-core/src/serializer/any.rs @@ -20,6 +20,7 @@ use crate::fury::Fury; use crate::resolver::context::{ReadContext, WriteContext}; use crate::serializer::Serializer; use crate::types::{FieldType, Mode, RefFlag}; +use anyhow::anyhow; use std::any::Any; impl Serializer for Box { @@ -75,13 +76,13 @@ impl Serializer for Box { .get_deserializer()(context) } } else if ref_flag == (RefFlag::Null as i8) { - Err(Error::Null) + Err(anyhow!("Try to deserialize `any` to null"))? } else if ref_flag == (RefFlag::Ref as i8) { reset_cursor(&mut context.reader); Err(Error::Ref) } else { reset_cursor(&mut context.reader); - Err(Error::BadRefFlag) + Err(anyhow!("Unknown ref flag, value:{ref_flag}"))? } } } diff --git a/rust/fury-core/src/serializer/datetime.rs b/rust/fury-core/src/serializer/datetime.rs index f6bfa736cf..e2f557a974 100644 --- a/rust/fury-core/src/serializer/datetime.rs +++ b/rust/fury-core/src/serializer/datetime.rs @@ -22,17 +22,18 @@ use crate::resolver::context::WriteContext; use crate::serializer::Serializer; use crate::types::{FieldType, FuryGeneralList}; use crate::util::EPOCH; +use anyhow::anyhow; use chrono::{DateTime, Days, NaiveDate, NaiveDateTime}; use std::mem; impl Serializer for NaiveDateTime { fn read(context: &mut ReadContext) -> Result { let timestamp = context.reader.u64(); - let ret = DateTime::from_timestamp_millis(timestamp as i64).map(|dt| dt.naive_utc()); - match ret { - Some(r) => Ok(r), - None => Err(Error::NaiveDateTime), - } + DateTime::from_timestamp_millis(timestamp as i64) + .map(|dt| dt.naive_utc()) + .ok_or(Error::from(anyhow!( + "Date out of range, timestamp:{timestamp}" + ))) } fn write(&self, context: &mut WriteContext) { @@ -62,10 +63,11 @@ impl Serializer for NaiveDate { fn read(context: &mut ReadContext) -> Result { let days = context.reader.u64(); - match EPOCH.checked_add_days(Days::new(days)) { - Some(value) => Ok(value), - None => Err(Error::NaiveDate), - } + EPOCH + .checked_add_days(Days::new(days)) + .ok_or(Error::from(anyhow!( + "Date out of range, {days} days since epoch" + ))) } fn get_type_id(_fury: &Fury) -> i16 { diff --git a/rust/fury-core/src/serializer/mod.rs b/rust/fury-core/src/serializer/mod.rs index 20f02093c5..0083b60516 100644 --- a/rust/fury-core/src/serializer/mod.rs +++ b/rust/fury-core/src/serializer/mod.rs @@ -15,10 +15,12 @@ // specific language governing permissions and limitations // under the License. +use crate::ensure; use crate::error::Error; use crate::fury::Fury; use crate::resolver::context::{ReadContext, WriteContext}; use crate::types::RefFlag; +use anyhow::anyhow; mod any; mod bool; @@ -46,20 +48,18 @@ pub fn deserialize(context: &mut ReadContext) -> Result if ref_flag == (RefFlag::NotNullValue as i8) || ref_flag == (RefFlag::RefValue as i8) { let actual_type_id = context.reader.i16(); let expected_type_id = T::get_type_id(context.get_fury()); - if actual_type_id != expected_type_id { - Err(Error::FieldType { - expected: expected_type_id, - actual: actual_type_id, - }) - } else { - Ok(T::read(context)?) - } + ensure!( + actual_type_id == expected_type_id, + anyhow!("Invalid field type, expected:{expected_type_id}, actual:{actual_type_id}") + ); + + T::read(context) } else if ref_flag == (RefFlag::Null as i8) { - Err(Error::Null) + Err(anyhow!("Try to deserialize non-option type to null"))? } else if ref_flag == (RefFlag::Ref as i8) { Err(Error::Ref) } else { - Err(Error::BadRefFlag) + Err(anyhow!("Unknown ref flag, value:{ref_flag}"))? } } diff --git a/rust/fury-core/src/serializer/option.rs b/rust/fury-core/src/serializer/option.rs index dfd607bf4e..3af8764d85 100644 --- a/rust/fury-core/src/serializer/option.rs +++ b/rust/fury-core/src/serializer/option.rs @@ -15,12 +15,14 @@ // specific language governing permissions and limitations // under the License. +use crate::ensure; use crate::error::Error; use crate::fury::Fury; use crate::resolver::context::ReadContext; use crate::resolver::context::WriteContext; use crate::serializer::Serializer; use crate::types::{FuryGeneralList, RefFlag}; +use anyhow::anyhow; impl Serializer for Option { fn read(context: &mut ReadContext) -> Result { @@ -35,20 +37,18 @@ impl Serializer for Option { // type_id let actual_type_id = context.reader.i16(); let expected_type_id = T::get_type_id(context.get_fury()); - if actual_type_id != expected_type_id { - Err(Error::FieldType { - expected: expected_type_id, - actual: actual_type_id, - }) - } else { - Ok(Some(T::read(context)?)) - } + ensure!( + actual_type_id == expected_type_id, + anyhow!("Invalid field type, expected:{expected_type_id}, actual:{actual_type_id}") + ); + + Ok(Some(T::read(context)?)) } else if ref_flag == (RefFlag::Null as i8) { Ok(None) } else if ref_flag == (RefFlag::Ref as i8) { Err(Error::Ref) } else { - Err(Error::BadRefFlag) + Err(anyhow!("Unknown ref flag, value:{ref_flag}"))? } } diff --git a/rust/fury-core/src/types.rs b/rust/fury-core/src/types.rs index 195adfb04e..29a7e441eb 100644 --- a/rust/fury-core/src/types.rs +++ b/rust/fury-core/src/types.rs @@ -16,6 +16,7 @@ // under the License. use crate::error::Error; +use anyhow::anyhow; use num_enum::{IntoPrimitive, TryFromPrimitive}; use std::mem; @@ -163,7 +164,7 @@ impl TryFrom for Language { 4 => Ok(Language::Go), 5 => Ok(Language::Javascript), 6 => Ok(Language::Rust), - _ => Err(Error::UnsupportedLanguageCode { code: num }), + _ => Err(anyhow!("Unsupported language code, value:{num}"))?, } } } diff --git a/rust/fury-derive/src/object/read.rs b/rust/fury-derive/src/object/read.rs index 7249609a81..6d3e0666db 100644 --- a/rust/fury-derive/src/object/read.rs +++ b/rust/fury-derive/src/object/read.rs @@ -98,11 +98,11 @@ fn deserialize_compatible(fields: &[&Field]) -> TokenStream { #(#create),* }) } else if ref_flag == (fury_core::types::RefFlag::Null as i8) { - Err(fury_core::error::Error::Null) + Err(fury_core::error::AnyhowError::msg("Try to deserialize non-option type to null"))? } else if ref_flag == (fury_core::types::RefFlag::Ref as i8) { Err(fury_core::error::Error::Ref) } else { - Err(fury_core::error::Error::BadRefFlag) + Err(fury_core::error::AnyhowError::msg("Unknown ref flag, value:{ref_flag}"))? } } }