Skip to content

RUST-1406 Add type-specific errors to standard error type #555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
isabelatkinson merged 13 commits into from
Jun 18, 2025
Merged

RUST-1406 Add type-specific errors to standard error type #555

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6380979
datetime and oid
isabelatkinson Jun 12, 2025
a10b680
binary
isabelatkinson Jun 12, 2025
33a6a6f
maintain uuid structure
isabelatkinson Jun 12, 2025
950965c
maintain datetime structure
isabelatkinson Jun 12, 2025
d7e05d4
restructure, maintain oid structure
isabelatkinson Jun 13, 2025
d368d4c
maintain binary structure
isabelatkinson Jun 13, 2025
ec54947
api cleanup
isabelatkinson Jun 13, 2025
a74e72e
remove invalid value
isabelatkinson Jun 13, 2025
3d8e887
docs
isabelatkinson Jun 13, 2025
54c7dbb
collapse datetime
isabelatkinson Jun 16, 2025
2176ad6
decimal128
isabelatkinson Jun 16, 2025
5b0057c
refactor message
isabelatkinson Jun 18, 2025
1ee3a41
merge main
isabelatkinson Jun 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use crate::{
base64,
error::{Error, Result},
spec::BinarySubtype,
Document,
RawBinaryRef,
};

Expand Down
34 changes: 15 additions & 19 deletions src/decimal128.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl Coefficient {
bits[prefix_len..].copy_from_bitslice(src_suffix);
let out = Self(bytes);
if out.value() > Self::MAX_VALUE {
Err(Error::decimal128(Decimal128ErrorKind::Overflow))
Err(Error::decimal128(Decimal128ErrorKind::Overflow {}))
} else {
Ok(out)
}
Expand Down Expand Up @@ -349,17 +349,15 @@ impl std::str::FromStr for ParsedDecimal128 {
exp_str = "0";
}
Some((_, "")) => {
return Err(Error::decimal128(Decimal128ErrorKind::EmptyExponent))
return Err(Error::decimal128(Decimal128ErrorKind::EmptyExponent {}))
}
Some((pre, post)) => {
decimal_str = pre;
exp_str = post;
}
}
let mut exp = exp_str.parse::<i16>().map_err(|e| {
Error::decimal128(Decimal128ErrorKind::InvalidExponent {
message: e.to_string(),
})
Error::decimal128(Decimal128ErrorKind::InvalidExponent {}).with_message(e)
})?;

// Remove decimal point and adjust exponent
Expand All @@ -368,10 +366,10 @@ impl std::str::FromStr for ParsedDecimal128 {
let exp_adj = post
.len()
.try_into()
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Underflow))?;
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Underflow {}))?;
exp = exp
.checked_sub(exp_adj)
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Underflow))?;
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Underflow {}))?;
joined_str = format!("{}{}", pre, post);
decimal_str = &joined_str;
}
Expand All @@ -387,10 +385,10 @@ impl std::str::FromStr for ParsedDecimal128 {
decimal_str = round_decimal_str(decimal_str, Coefficient::MAX_DIGITS)?;
let exp_adj = (len - decimal_str.len())
.try_into()
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Overflow))?;
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Overflow {}))?;
exp = exp
.checked_add(exp_adj)
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Overflow))?;
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Overflow {}))?;
}
}

Expand All @@ -399,11 +397,11 @@ impl std::str::FromStr for ParsedDecimal128 {
if decimal_str != "0" {
let delta = (Exponent::TINY - exp)
.try_into()
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Overflow))?;
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Overflow {}))?;
let new_precision = decimal_str
.len()
.checked_sub(delta)
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Overflow))?;
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Overflow {}))?;
decimal_str = round_decimal_str(decimal_str, new_precision)?;
}
exp = Exponent::TINY;
Expand All @@ -413,14 +411,14 @@ impl std::str::FromStr for ParsedDecimal128 {
if decimal_str != "0" {
let delta = (exp - Exponent::MAX)
.try_into()
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Overflow))?;
.map_err(|_| Error::decimal128(Decimal128ErrorKind::Overflow {}))?;
if decimal_str
.len()
.checked_add(delta)
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Overflow))?
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Overflow {}))?
> Coefficient::MAX_DIGITS
{
return Err(Error::decimal128(Decimal128ErrorKind::Overflow));
return Err(Error::decimal128(Decimal128ErrorKind::Overflow {}));
}
padded_str = format!("{}{}", decimal_str, "0".repeat(delta));
decimal_str = &padded_str;
Expand All @@ -431,9 +429,7 @@ impl std::str::FromStr for ParsedDecimal128 {
// Assemble the final value
let exponent = Exponent::from_native(exp);
let coeff: u128 = decimal_str.parse().map_err(|e: ParseIntError| {
Error::decimal128(Decimal128ErrorKind::InvalidCoefficient {
message: e.to_string(),
})
Error::decimal128(Decimal128ErrorKind::InvalidCoefficient {}).with_message(e)
})?;
let coefficient = Coefficient::from_native(coeff);
Decimal128Kind::Finite {
Expand All @@ -450,10 +446,10 @@ impl std::str::FromStr for ParsedDecimal128 {
fn round_decimal_str(s: &str, precision: usize) -> Result<&str> {
let (pre, post) = s
.split_at_checked(precision)
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Unparseable))?;
.ok_or_else(|| Error::decimal128(Decimal128ErrorKind::Unparseable {}))?;
// Any nonzero trimmed digits mean it would be an imprecise round.
if post.chars().any(|c| c != '0') {
return Err(Error::decimal128(Decimal128ErrorKind::InexactRounding));
return Err(Error::decimal128(Decimal128ErrorKind::InexactRounding {}));
}
Ok(pre)
}
76 changes: 40 additions & 36 deletions src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ pub struct Error {
/// The kind of error that occurred.
pub kind: ErrorKind,

/// An optional message describing the error.
pub message: Option<String>,

/// The document key associated with the error, if any.
pub key: Option<String>,

Expand All @@ -28,13 +31,19 @@ pub struct Error {

impl std::fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "BSON error")?;

if let Some(key) = self.key.as_deref() {
write!(f, "Error at key \"{key}\": ")?;
write!(f, " at key \"{key}\"")?;
} else if let Some(index) = self.index {
write!(f, "Error at array index {index}: ")?;
write!(f, " at array index {index}")?;
}

write!(f, "{}", self.kind)
write!(f, ". Kind: {}", self.kind)?;
if let Some(ref message) = self.message {
write!(f, ". Message: {}", message)?;
}
write!(f, ".")
}
}

Expand All @@ -43,47 +52,44 @@ impl std::fmt::Display for Error {
#[non_exhaustive]
pub enum ErrorKind {
/// An error related to the [`Binary`](crate::Binary) type occurred.
Copy link
Contributor

@abr-egn abr-egn Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exposing how arbitrary it is which of our errors carry a string and which carry a subkind enum; that's got a bit of a code smell to it but I can't decide if it's actually a problem worth addressing. Do you have thoughts here?

Copy link
Contributor Author

@isabelatkinson isabelatkinson Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't love this design; I agree that it's arbitrary and inconsistent. My concern, however, is that users who do have some kind of programmatic use for the values in these errors will be less inclined to migrate to the new version, so my instinct here is to err on the side of portability for lower-priority changes. Certainly open to other designs here if you had something else in mind though!

Copy link
Contributor

@abr-egn abr-egn Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sketch of a proposal:

  • Hoist message to an optional field of Error, with corresponding code in the Display impl to append it if it's present.
  • All variants of ErrorKind are required to be #[non_exhaustive] struct variants, even if they're empty (which a lot now will be with message stripped out)
  • Any sub-Kind types follow the same pattern (sub-Kinds also don't need to have message since they're ultimately attached to an Error!)

This makes the error space a lot more uniform, gives us a lot of flexibility for future error evolution, and doesn't introduce substantial migration burden.

Copy link
Contributor Author

@isabelatkinson isabelatkinson Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot more - updated with your suggestions!

#[error("A Binary-related error occurred: {message}")]
Binary {
/// A message describing the error.
message: String,
},
#[error("A Binary-related error occurred")]
#[non_exhaustive]
Binary {},

/// An error related to the [`DateTime`](crate::DateTime) type occurred.
#[error("A DateTime-related error occurred: {message}")]
DateTime {
/// A message describing the error.
message: String,
},
#[error("A DateTime-related error occurred")]
#[non_exhaustive]
DateTime {},

/// An error related to the [`Decimal128`](crate::Decimal128) type occurred.
#[error("A Decimal128-related error occurred: {kind}")]
#[non_exhaustive]
Decimal128 {
/// The kind of error that occurred.
kind: Decimal128ErrorKind,
},

/// Malformed BSON bytes were encountered.
#[error("Malformed BSON bytes: {message}")]
#[error("Malformed BSON bytes")]
#[non_exhaustive]
MalformedBytes {
/// A message describing the error.
message: String,
},
MalformedBytes {},

/// An error related to the [`ObjectId`](crate::oid::ObjectId) type occurred.
#[error("An ObjectId-related error occurred: {kind}")]
#[non_exhaustive]
ObjectId {
/// The kind of error that occurred.
kind: ObjectIdErrorKind,
},

/// Invalid UTF-8 bytes were encountered.
#[error("Invalid UTF-8")]
Utf8Encoding,
#[non_exhaustive]
Utf8Encoding {},

/// An error related to the [`Uuid`](crate::uuid::Uuid) type occurred.
#[error("A UUID-related error occurred: {kind}")]
#[non_exhaustive]
Uuid {
/// The kind of error that occurred.
kind: UuidErrorKind,
Expand All @@ -98,8 +104,9 @@ pub enum ErrorKind {
},

/// A [`std::io::Error`] occurred.
#[error("An IO error occurred: {0}")]
Io(std::io::Error),
#[error("An IO error occurred")]
#[non_exhaustive]
Io {},
Copy link
Contributor Author

@isabelatkinson isabelatkinson Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched this over to a stringified version of the error - std::io::Error is not Clone, and although I'm not worried about the stability of a standard library error, keeping external types out of our API allows for more flexibility in situations like RUST-1798

Copy link
Contributor

@abr-egn abr-egn Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to carry the std::io::ErrorKind but that can be added later if it's asked for :)


/// A wrapped deserialization error.
/// TODO RUST-1406: collapse this
Expand All @@ -114,13 +121,14 @@ impl From<ErrorKind> for Error {
kind,
key: None,
index: None,
message: None,
}
}
}

impl From<std::io::Error> for Error {
fn from(value: std::io::Error) -> Self {
ErrorKind::Io(value).into()
Error::from(ErrorKind::Io {}).with_message(value)
}
}

Expand All @@ -142,29 +150,25 @@ impl Error {
self
}

pub(crate) fn with_message(mut self, message: impl ToString) -> Self {
self.message = Some(message.to_string());
self
}

pub(crate) fn binary(message: impl ToString) -> Self {
ErrorKind::Binary {
message: message.to_string(),
}
.into()
Self::from(ErrorKind::Binary {}).with_message(message)
}

pub(crate) fn datetime(message: impl ToString) -> Self {
ErrorKind::DateTime {
message: message.to_string(),
}
.into()
Self::from(ErrorKind::DateTime {}).with_message(message)
}

pub(crate) fn malformed_bytes(message: impl ToString) -> Self {
ErrorKind::MalformedBytes {
message: message.to_string(),
}
.into()
Self::from(ErrorKind::MalformedBytes {}).with_message(message)
}

#[cfg(all(test, feature = "serde"))]
pub(crate) fn is_malformed_value(&self) -> bool {
matches!(self.kind, ErrorKind::MalformedValue { .. },)
pub(crate) fn is_malformed_bytes(&self) -> bool {
matches!(self.kind, ErrorKind::MalformedBytes { .. },)
}
}
31 changes: 15 additions & 16 deletions src/error/decimal128.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,39 +8,38 @@ use crate::error::{Error, ErrorKind};
pub enum Decimal128ErrorKind {
/// Empty exponent.
#[error("empty exponent")]
EmptyExponent,
#[non_exhaustive]
EmptyExponent {},

/// Invalid exponent.
#[error("invalid exponent: {message}")]
#[error("invalid exponent")]
#[non_exhaustive]
InvalidExponent {
/// A message describing the error.
message: String,
},
InvalidExponent {},

/// Invalid coefficient.
#[error("invalid coefficient: {message}")]
#[error("invalid coefficient")]
#[non_exhaustive]
InvalidCoefficient {
/// A message describing the error.
message: String,
},
InvalidCoefficient {},

/// Overflow.
#[error("overflow")]
Overflow,
#[non_exhaustive]
Overflow {},

/// Underflow.
#[error("underflow")]
Underflow,
#[non_exhaustive]
Underflow {},

/// Inexact rounding.
#[error("inexact rounding")]
InexactRounding,
#[non_exhaustive]
InexactRounding {},

/// Unparseable.
#[error("unparseable")]
Unparseable,
#[non_exhaustive]
Unparseable {},
}

impl Error {
Expand All @@ -53,7 +52,7 @@ impl Error {
matches!(
self.kind,
ErrorKind::Decimal128 {
kind: Decimal128ErrorKind::Unparseable,
kind: Decimal128ErrorKind::Unparseable {},
}
)
}
Expand Down
17 changes: 6 additions & 11 deletions src/error/uuid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,9 @@ use crate::{
#[non_exhaustive]
pub enum UuidErrorKind {
/// An invalid string was used to construct a UUID.
#[error("{message}")]
#[error("invalid UUID string")]
#[non_exhaustive]
InvalidString {
/// A message describing the error.
message: String,
},
InvalidString {},

/// The requested `UuidRepresentation` does not match the binary subtype of a `Binary`
/// value.
Expand Down Expand Up @@ -47,12 +44,10 @@ pub enum UuidErrorKind {

impl Error {
pub(crate) fn invalid_uuid_string(message: impl ToString) -> Self {
ErrorKind::Uuid {
kind: UuidErrorKind::InvalidString {
message: message.to_string(),
},
}
.into()
Self::from(ErrorKind::Uuid {
kind: UuidErrorKind::InvalidString {},
})
.with_message(message)
}

pub(crate) fn uuid_representation_mismatch(
Expand Down
Loading