Make routerify::Error as enum or as Box type

[rousan]

We should design the routerify::Error as enum type or as Box<dyn StdError + Send + Sync + 'static> and routerify::Result as pub type Result<T> = std::result::Result<T, Error>;

[gsserge]

@rousan If I understand you correctly, we already have

/// The error type used by the error handlers.
pub type RouteError = Box<dyn StdError + Send + Sync + 'static>;

I decided to keep the old string-based Error for compatibility reason, the whole discussion is here #55

The current situation is of course not ideal and I'm happy to discuss any improvements!

[rousan]

We have two error types right now, routerify::Error and routerify::RouteError and the routerify::Result is based on routerify::RouteError and it is very confusing. So, I was thinking to implement error type based on enum just like this.

use std::fmt::{self, Debug, Display, Formatter};

use derive_more::Display;

#[derive(Display)]
#[non_exhaustive]
pub enum Error {
    #[display(fmt = "unknown field received: {}", "field_name.as_deref().unwrap_or(\"<unknown>\")")]
    UnknownField { field_name: Option<String> },

    /// Failed to decode the field data as `JSON` in
    /// [`field.json()`](crate::Field::json) method.
    #[cfg(feature = "json")]
    #[cfg_attr(nightly, doc(cfg(feature = "json")))]
    #[display(fmt = "failed to decode field data as JSON: {}", _0)]
    DecodeJson(serde_json::Error),
}

impl Debug for Error {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        Display::fmt(self, f)
    }
}

impl std::error::Error for Error {}

impl PartialEq for Error {
    fn eq(&self, other: &Self) -> bool {
        self.to_string().eq(&other.to_string())
    }
}

impl Eq for Error {}

[gsserge]

We have two error types right now, routerify::Error and routerify::RouteError and the routerify::Result is based on routerify::RouteError and it is very confusing.

I agree. In retrospect, I should have made a breaking change for v2 and fixed routerify::Error without introducing routerify::RouteError.

Regarding the enum vs boxed error, I strongly prefer the maximum flexibility of boxed error. I think that in the context of an http router, the error type should be a kind of a container, capable of storing any error-like value. By not imposing any structure (like with enum and its fixed set of variants), we can give the users maximum freedom to define their own custom error enums representing their application needs (one simple example #42 (comment)), with routerify::Error, being a boxed error, simply carrying it between middleware and handlers.

Forcing users to define their own error type can definitely be an inconvenience and extra work in simple cases, but for more complex services they will have to do that anyway. Alternatively, something like anyhow::Error can be used as a quick and simple error type.

So, if you feel that compatibility with v1 is not an issue and you are ok with boxed error, then I think we can get rid of routerify::RouteError, and change routerify::Error to be

pub type Error = Box<dyn StdError + Send + Sync + 'static>;

This will of course be a breaking change and will force us to bump the major version to v3. And this could be a great opportunity to fix another issue with our error handling - drop the E generic parameter from Handler and friends and use routerify::Error as a concrete error type everywhere in Routerify. In addition to simplifying the codebase, this will allow us to decouple different middleware. #56 (comment) is a good example. There's no reason to force exactly the same concrete error type everywhere.

[rousan]

In general, If the library can provide the specific error types through enum values, the consumer can easily take appropriate action based on Error kind. If we look into hyper::Error or std::io::Error, they are using some sort of enum to define different types of possible errors.

[gsserge]

Hyper uses a combination of both https://docs.rs/hyper/0.14.10/src/hyper/error.rs.html#11-13. There's still a way to get to the original boxed error.

In general, I am not against a more structured error via enum. I'm just not sure how to pick a range of variants that is both sufficient and flexible. Should we approach it from the point of http status codes and create a variant per status? Or the other way around, trying to predict any possible cause, like the aforementioned DecodeJson, DatabaseError, etc.

I think tide's Error type is a nice middle ground https://docs.rs/http-types/2.11.1/src/http_types/error.rs.html#16-20

[seanpianka]

The difficulty in choosing variants for the public error type is real 😁

Take a look at the bottom of this comment in #13, I describe some helper methods used in an existing Routerify application and the associated error type definition.

enum HandlerError contains variants which broadly cover common HTTP endpoint handling issues, and which are roughly grouped by applicable status code. Personally, relating closely with HTTP status codes seems like the predictable behavior for an HTTP router's publicly exported error type, but I'm probably biased.

We should consider either creating a similarly broad error-type definition, or take the safe route with boxed error. Along the boxed error approach, I think using an implementation similar to Tide's sounds great.

This would also be a good opportunity to add some user documentation describing how to define a good error type for one's application.