Consider revisiting `DeserializationError` design

[PhilippGackstatter]

Quite frequently, we write deserialization code like:

let field = <FieldType>::read_from(source)?;

Self::new(field)
.map_err(|err| DeserializationError::InvalidValue(err.to_string()))

There are two potential issues with this:

• This includes the source error as a stringified error into the deserialization error. If err is actually an error with a source error (i.e. an error chain) then the error chain will be discarded and only the string representation of err is included, so we loose valuable debugging information.
• There is no context attached to the read_from call. In other words, deserializing FieldType directly or as part of the larger struct here produces the same error, because we just propagate the error upwards with ?. For deeply nested structs this can result in quite unhelpful errors.

Redesign

We could redesign the deserialization error to allow for source errors and make attaching context very easy, e.g.:

#[derive(Debug, thiserror::Error)]
pub enum DeserializationError {
    /// An end of input was reached before a valid value could be deserialized.
    #[error("unexpected end of file")]
    UnexpectedEOF,
    /// Deserialization has finished but not all bytes have been consumed.
    #[error("unconsumed bytes")]
    UnconsumedBytes,
    /// A custom error.
    #[error("{error_msg}")]
    Other {
        error_msg: Box<str>,
        // thiserror will return this when calling `Error::source` on `DeserializationError`.
        source: Option<Box<dyn CoreError + Send + Sync + 'static>>,
    },
}

impl DeserializationError {
    /// Creates a custom error using the [`DeserializationError::Other`] variant from an error
    /// message.
    pub fn other(message: impl Into<String>) -> Self {
        let message: String = message.into();
        Self::Other { error_msg: message.into(), source: None }
    }

    /// Creates a custom error using the [`DeserializationError::Other`] variant from an error
    /// message and a source error.
    pub fn other_with_source(
        message: impl Into<String>,
        source: impl CoreError + Send + Sync + 'static,
    ) -> Self {
        let message: String = message.into();
        Self::Other {
            error_msg: message.into(),
            source: Some(Box::new(source)),
        }
    }
}

pub trait DeserializationErrorContext {
    fn context(self, message: impl Into<String>) -> Self;
}

impl<O> DeserializationErrorContext for Result<O, DeserializationError> {
    fn context(self, message: impl Into<String>) -> Self {
        self.map_err(|source| DeserializationError::other_with_source(message, source))
    }
}

struct Transaction {
    id: [u8; 32],
}
struct Batch {
    tx: Transaction,
}

impl Transaction {
    fn read_from<R: ByteReader>(source: &mut R) -> Result<Self, DeserializationError> {
        // Note that this calls the existing method which is why the manual map_err is necessary.
        // Once converted, the context method like below could be used.
        let id = <[u8; 32]>::read_from(source).map_err(|source| {
            DeserializationError::other_with_source("failed to deserialize id", source)
        })?;

        Ok(Transaction { id })
    }
}

impl Batch {
    fn read_from<R: ByteReader>(source: &mut R) -> Result<Self, DeserializationError> {
        let tx = Transaction::read_from(source).context("failed to deserialize transaction")?;
        Ok(Batch { tx })
    }
}

#[test]
fn deserialize_test() -> anyhow::Result<()> {
    let mut reader = SliceReader::new(&[5]);
    Batch::read_from(&mut reader).context("failed to deserialize batch")?;
    Ok(())
}

Running this test results in a nice error report (courtesy of anyhow):

Error: failed to deserialize batch

Caused by:
    0: failed to deserialize transaction
    1: failed to deserialize id
    2: unexpected EOF

This results in a much better error because it contains more information that helps pinpoint the cause of the error. Without the context, we would only get "unexpected EOF" which, in a deeply nested struct, could have been returned anywhere and make locating the cause of the error much harder.

[manchain]

I propose using a thiserror-based DeserializationError with a context method (e.g., self.map_err(|source| DeserializationError::other_with_source(message, source))) to preserve chains and add trace info like 'failed to deserialize batch > transaction > id > unexpected EOF'