User defined error from transactions?

[kriswuollett]

It seemed like in #2342 that a change proposal maybe could have been accepted if both Box-ing and NewType-ing the error would have been okay? In general the use case seems valid as it seems like they are performing some "check" in a third-party library. Similar to that is my use case of checking the "etag" or "epoch" of an entity before allowing an update.

Short of being able to box an error within a diesel error, it seems like it should be acceptable usage for me to just simply do the following and try extracting the error from something that implements DatabaseErrorInformation like String?

let etag: String = things.find(id).select(etag).first(conn)?;
if !etag.eq(expected_etag) {
    return Err(Error::DatabaseError(
        // Or use DatabaseErrorKind::Unknown?
        // Or should there be a DatabaseErrorKind::Custom?
        DatabaseErrorKind::CheckViolation,
        // User expected to retry with new etag
        Box::new(String::from("Aborted/etag mismatch")),
 ));

I would however find it nice to be able to do something like the following instead and try downcasting the error after the transaction:

let etag: String = things.find(id).select(etag).first(conn)?;
if !etag.eq(expected_etag) {
    return Err(Error::DatabaseError(
        DatabaseErrorKind::Custom,
        // User expected to retry with new etag
        Box::new(Status::aborted("etag mismatch"),
 ));

Perhaps of course this perhaps could be done as a trigger where possible, but the DatabaseErrorInformation would still need to be parsed in order to create the downstream API error.

Is something like DatabaseErrorKind::Custom worth adding?

The complication I see with creating a concrete implementation of From<diesel::result::Error> is that what error to return can be context dependent. For example an end-user that is able to write to an object should get an invalid argument error on a check failure (HTTP 400), but an unauthorized user should always get a not found error (HTTP 400 NOT 403)... of course for this example all the logic is done within the transaction function & database.

[weiznich]

I'm not sure I can follow you there.

The documentation lists the following function signature:

fn transaction<F, R, E>(conn: &mut Conn, callback: F) -> Result<R, E>
where
    F: FnOnce(&mut Conn) -> Result<R, E>,
    E: From<Error>,

As can be easily seen there there is no constraint there to return a diesel::result::Error or even a specific error type from there. The only constraint there is to return any error type that satisfies E: From<diesel::result::Error>, which can be any custom error type you define including such a boxed field or a match all type like anyhow::Error. You don't even need to return the same error for all your calls to transaction, you can use different error types for different function calls. Using a diesel::result::Error there is certainly possible for simple cases, but it is just not the expected solution for anything that's not trivial.

Finally that all written: Leaving beside the details above, your post only describes a problem. It doesn't propose a solution, it's not even clear from your posting how such a solution could even look like, as it essentially only says: It doesn't work like I want it to work, which is not helpful for anyone but you. For a formal feature request I would expect that you outline the problem, describe how that problem affects a significant part of the diesel user base, outline possible solutions and there advantages/disadvantages. Most of that is missing here. If you don't have such details, please fill such questions as support question in the future.

[kriswuollett]

Perhaps I buried the lede in the explanation.

The use case:

Return an error to the downstream API user with a type like tonic::Status based on a read within a transaction that was aborted.

The problem:

Rollback transaction with user defined error that is not technically a database error, nor something that is desired to be implemented in a constraint or trigger.

Implementing From<diesel’s Error> does not make sense since it seems to be only for actual database generated errors.

The workaround (1at code example):

User defined error or new type error should implement DatabaseErrorInformation,return it with code
DatabaseErrorKind::Unknown, and downcast the box on the returned error.

Possible solutions:

1. Add DatabaseErrorKind::Custom to officially support the method above.

2. Add another Error type for user defined error of Box and any field(s) needed by diesel whether to retry or rollback the transaction.

I’m not currently blocked since I’m using the workaround, but I thought it may be something that could be more directly supported.

[weiznich]

I'm still not able to follow you on why you need to use diesel::result::Error as error type there. It is designed to be used as diesel error type, not as general error type for any other application. From this design perspective the first suggested solution doesn't seem meaningful.

For me the second solution is completely unclear.

Implementing From<diesel’s Error> does not make sense since it seems to be only for actual database generated errors.

To clarify that: This E: From<diesel::result::Error> impl is exactly used for that is case of an error generated by interacting the the transaction (and not the closure) itself. That can also fail due to network errors, etc like any other database query. If you return a custom type from that closure diesel needs to be able to insert these errors there.

Rollback transaction with user defined error that is not technically a database error, nor something that is desired to be implemented in a constraint or trigger.

This might be the point where you missed an important part of the documentation, although given the still unstructured way you express this it's not 100% clear for me if you are not aware of that. Anyway let me cite from the documentation again:

This function executes the provided closure f inside a database transaction. If there is already an open transaction for the current connection savepoints will be used instead. The connection is committed if the closure returns Ok(), **it will be rolled back if it returns Err()**. For both cases the original result value will be returned from this function.

Which just means you don't need to use diesel::result::Error in your closure at all, especially not Error::RollbackTransaction. You can just use any error there and the transaction will be still rolled back. That's also pointed out in the relevant example code, so I really wonder whether you missed it or not.

[kriswuollett]

Yes, that explanation helps. I believe it was just quickly reading the method comments and signature and assuming I had to care about the specific diesel::result::Error and not seeing example code or guide for transactions to realize what the type signature really meant.

Looks like all I have to do is just use the following:

#[cfg(feature = "diesel")]
impl From<diesel::result::Error> for MyError {
    fn from(value: diesel::result::Error) -> Self {
        Self::internal(value) // display
    }
}

[kriswuollett]

If anything the code doc comments doesn't mention this use case so it is easy to not not realize what the where E: From<diesel::result::Error> gives you after having to map_err with other libraries.

[weiznich]

Glad that it is resolved 👍

If you have something specific in mind how to improve the documentation we would be happy to receive a PR for that. Given that you are not the first person having problems with this method, it might be useful to try improving the documentation.

[kriswuollett]

For sure, I'd be happy to give an attempt at adding some docs/examples once I'm done with my initial diesel-using library to see if there is also anything else that would be good to add some docs for too.