Remove Trait bound From<Error> from Connection::transaction

[gliderkite]

I am working on a crate that makes use of an Error type defined in a dependency library, which I use as return type of a method that needs to be run as part of a transaction, such as for example:

use my_lib::Error;

fn foo() -> Result<(), Error>;

conn.transaction(|| { foo() });

This results in the following error when compiling:

error[E0277]: the trait bound my_lib::Error: std::convert::From<diesel::result::Error> is not satisfied

AFAIK it's it not possible for me to create the From<Error> implementation required by this method, because the my_lib::Error type is defined in another crate. And I have to create my own TransactionError dummy type just to map the my_lib::Error (and map it back at the end of the transaction).

Is there a strong reason to have this Trait bound in place?
Would it be possible to remove it and make the API more flexible?

[weiznich]

Please make a concrete suggestions how a more flexible API would look like if we remove that bound. Also keep in mind that every use case that is supported today should be supported in the future, that includes using a custom error type as return type of the transaction callback.

I'm closing this issue now, because it is currently not actionable for the diesel team. If you have any concrete suggestions we can talk about reopening this issue.

[gliderkite]

I wasn't aware of this policy in Diesel (this was the first issue I opened); in fact I see many open tickets that are not actionable today but still open, for example #2084, that by chance redirected me to this comment, that could be applied here as well #399 (comment). I'll reopen this issue in case I'll find the time to think to an alternative, and leave to you the job to redirect other people opening similar issues to this closed one.

[weiznich]

I probably should explain why I closed this issue a bit more in detail.
The corresponding code for the transaction function is here , to show what we are talking about. We have the following constraints there:

• We need to return some error type from that function because a transaction may fail
• A potential user should be able to provide domain specific callbacks there, this implies they may return their own error type
• We need to have some way to handle an internal error (like connection was lost, or something like that) wile doing the transaction handling.

This leave us in my point of view with exactly two choices:

1. The current way, saying that there must be a way to convert a diesel error to the user provided error type. As an requirement this needs the From<diesel::result::Error> bound there.
2. Wrapping the user provided error type somehow in a diesel provided error type. This would also require some bound on the user provided error type there to be useful, probably std::error::Error or something like that.

That means in the end both solutions would require a trait bound on the user provided error type. So in the general case non of those solutions will solve your problem.

in fact I see many open tickets that are not actionable today but still open, for example #2084,

I consider this issue as actionable, see #2257.

I'll reopen this issue in case I'll find the time to think to an alternative, and leave to you the job to redirect other people opening similar issues to this closed one.

To make it clear again: Closing a issue does not mean the discussion here is done, just that I do not see what could be done immediately to solve the issue. Basically all of the open issues should contain something a potential contributor could pick up and work on. (I know that is not necessarily the case for some older issue)

As a potential workaround for your concrete issue: You could just do your own error type that wraps diesel::result::Error and the third party error type into a common type. So something like

enum MyError<E> {
    DieselError(diesel::result::Error),
    LibraryError(E)
}

impl<E> From<diesel::result::Error> for MyError<E> {
    fn from(e: diesel::result::Error) -> Self {
        MyError::DieselError(e)
    }
}

• wrapping the callback to do a additional error transformation should just work for you.

[jacob-pro]

I'm facing the same issue, I'm using http-api-problem which provides an RFC7807 compliant error type in my web application, and naturally I want my to be able to return these errors from within a transaction.

This seems like a very reasonable use case to me, so I think the current API is very ununergonomic. I can suggest some possible solutions on the diesel side, otherwise creating all these wrappers means a lot of boilerplate code

1. A new transaction function that returns an enum like:

pub enum TransactionError<E> {
    DieselError(diesel::result::Error),
    UserError(E)
}
fn transaction<T, E, F>(&self, f: F) -> Result<T, TransactionError<E>>
    where
        F: FnOnce() -> Result<T, E>,

2. A transaction function that accepts a mapper function:

fn transaction<T, E, F, M>(&self, f: F, m: M) -> Result<T, E>
    where
        F: FnOnce() -> Result<T, E>,
        M: FnOnce(diesel::result::Error) -> E

[weiznich]

@jacob-pro Both of your proposals suffer from the same issue. If you want to handle any error inside of the transaction closure using the ? operator you need to have a From<diesel::result::Error> impl for the return type to convert any query error to your custom result type. Any possible solution need to provide this feature as this is basically mandatory for normal rust error handling code.
That written: You solution 1 is basically the implemented api. Just define that enum on your own and add a corresponding From impl.

Btw: Calling a specific API "unergonomic" just because it does not fit your special use case is not a good way to start any discussion, specially if you did not even try to include some concrete code why it is not working for your usecase.

[Ten0]

I'm answering #3102 (comment) here because I think this is a more appropriate place for it (as reference for others who may have the same issue).

1. So my usecase is when building an API (using actix), I want to return a standardised http_api_problem error (which is defined in an external crate) from each of the route handlers.

2. Defining the enum in my own crate does work, its just that then I have this boilerplate code that I either: (a) have to copy and paste everywhere, or (b) create a very tiny library just containing the code I have included in this PR, it would in my opinion be preferable to have this as part of the diesel API itself...

3. So this is a valid concern, but it the usecase I described the same problem exists even without transactions, i.e. say I want to perform just a simple database query, I already have to have my own function trait for converting the disel error to the http_api_problem error in the firstplace since I can't implement From.

Example: https://github.com/jacob-pro/calpol/blob/525fba9cce33cdb07460deaa34e748ff35d874a3/src/api/v1/users.rs#L171-L176

Specifically about:

i.e. say I want to perform just a simple database query, I already have to have my own function trait for converting the disel error to the http_api_problem error in the firstplace since I can't implement From.

I think a better interface than https://github.com/jacob-pro/calpol/blob/525fba9cce33cdb07460deaa34e748ff35d874a3/src/api/error.rs to solve this issue would be to do this:

pub struct CalpolError {
   // For instance, but could also be an enum of all the things that could go wrong, or box dyn stuff...
   api_error: ApiError
}
pub fn internal_server_error<E: std::fmt::Display>(prefix: &str, error: E) -> CalpolError {
    let builder = ApiError::builder(StatusCode::INTERNAL_SERVER_ERROR);
    CalpolError {
        api_error: if cfg!(debug_assertions) {
            builder.message(format!("{}: {}", prefix, error)).finish()
        } else {
            builder.finish()
        }
    }
}
impl From<actix_utils::real_ip::RealIpAddressError> for CalpolError {
    fn from(e: actix_utils::real_ip::RealIpAddressError) -> CalpolError {
        internal_server_error("RealIpAddressError", self)
    }
}
impl From<CalpolError> for ApiError {
    fn form(e: CalpolError) -> ApiError {
        e.api_error
    }
}

You would then only need to update your response_mapper (https://github.com/jacob-pro/calpol/blob/525fba9cce33cdb07460deaa34e748ff35d874a3/src/api/mod.rs#L29-L37) into:

pub fn response_mapper<T, E>(response: Result<T, E>) -> Result<HttpResponse, ApiError>
where
    T: Serialize,
    E: Into<ApiError>,
{
    response
        .map(|value| HttpResponse::Ok().json(value))
        .map_err(|e| e.into())
}

and you could then use your CalpolError type everywhere else in your app instead of ApiError, which would:

• Remove all the map_api_error() boilerplate across your project, since thanks to the From implementations you could just use the ?.
• Provide you with more flexibility for future evolution (maybe someday you want to move out of ApiError or store something else inside? Maybe you'll want to report some of the errors to an error reporting system such as Sentry before sending error 500 to the end-user?)
• Also as side effect (though not so random) solve this issue because you would already have the From you need.

While this may seem complex at first sight because you introduce your own type, I'm pretty confident that you'll find this to be a more comfortable design in the end.

then I have this boilerplate code that I either: (a) have to copy and paste everywhere, or (b) create a very tiny library just containing the code I have included in this PR, it would in my opinion be preferable to have this as part of the diesel API itself...

This code: https://github.com/jacob-pro/calpol/blob/525fba9cce33cdb07460deaa34e748ff35d874a3/src/api/error.rs
that you're using to solve 3. already generates more boilerplate (especially through all the map_api_error calls it enforces) than the above proposed design.

In conclusion, I believe introducing the code proposed in #3102 would generally ease using a design that isn't optimal instead of encouraging users to use a more appropriate rust-idiomatic design, so I would conclude we should not do that (apart from the typo fix ^^).

[jacob-pro]

@Ten0 this is a really good idea! Thank you!!

Feel free to close the associated PR :)