Custom error type in transactions

[Lur1an]

In production we write quite a bit of code inside of EdgeDB Transactions, however the limitation of having to return an edgedb_tokio::Error is starting to weigh down on us as we don't want to convert them all to the same error type and then afterwards to cast them back to the correct type.

I introduced the new function transaction_with_err that allows the closure to return any error type as long as it:

• can be turned into edgedb_tokio::Error
• if it wraps a edgedb_tokio::Error inside (since we want to mix application code into the transaction), it returns a Option<&Error> ref to it, this is expressed through the EdgeDBErrorRef trait.

Here an example how you could use this:

 #[derive(thiserror::Error, Debug)]
 enum MyError {
     #[error(transparent)]
     EdgeDBError(#[from] edgedb_tokio::Error),
     #[error("Something went wrong and it wasn't EdgeDB")]
     SomethingWentWrong,
 }

 impl EdgeDBErrorRef for MyError {
     fn as_edge_db_error(&self) -> Option<&edgedb_tokio::Error> {
         match self {
             MyError::EdgeDBError(e) => Some(e),
             _ => None,
         }
     }
 }

 let conn = edgedb_tokio::create_client().await?;
 let val = conn.transaction_with_err::<_, _, _, MyError>(|mut tx| async move {
     tx.query_required_single::<i64, _>("
         WITH C := UPDATE Counter SET { value := .value + 1}
         SELECT C.value LIMIT 1
     ", &()
     ).await
}).await?;

Implementations for EdgeDBErrorRef for anyhow::Error and edgedb_tokio::Error are provided out of the box.

Let me know what you think of this idea!

[edgedb-cla[bot]]

All commit authors signed the Contributor License Agreement.

[Lur1an]

I added transaction_no_retry to allow transactions that don't retry the closure body under any circumstance, leaving this up to the user. This also empowers us to pass in closures that consume captured values. I think this is desirable in many instances where on failure we also need to rollback a bigger operation through publishing events or calling other API's before attempting everything again inside a transaction.

[aljazerzen]

Ok, that has taken me some time to digest. First of all, thanks for the PR.

Your problem is a valid use-case: returning errors from withing the transaction closure should be easier.

---

For a starter, there is a workaround with nested Results: you could return Result<Result<T, YourError>, edgedb_tokio::Error> from the transaction closure:

let conn = edgedb_tokio::create_client().await?;
let val = conn.transaction(|mut tx| async move {
    let my_number = tx.query_required_single::<i64, _>("...", &()).await?;

    if my_number < 0 {
        // let's assume this is an error
        Ok(Err(YourError::ResultIsNegative))
    } else {
        Ok(Ok(my_number))
    }
}).await?;

I know it makes using ? harder, but it works.

---

Given that there is an easy workaround, transaction_with_err seems a bit excessive, since it does add quite a lot of code.

---

transaction_no_retry can be achieved via .with_retry_options(RetryOptions::new(0, |_| Duration::ZERO)) (for which we could have a better helper). But as you point out, the benefit of the closure being FnOnce is significant, so it is worth adding I think.

---

Looking at all this, I'm tempted to try to redesign to have something like this:

impl Client {
    fn start_transaction() -> Result<Transaction, Error> { ... }

    /// this is what is called .transaction right now
    fn within_transaction(body: FnMut -> Future<...>) -> Result<...> { ... }
}

impl Transaction {
    /// Commit (can retry only network errors)
    async fn commit() -> Result<...>
}

impl Drop for Transaction {
    /// Does the rollback
}

This way, you could have:

let conn = edgedb_tokio::create_client().await?;
let my_number = {
    let tx = conn.start_transaction().await;

    let my_number = tx.query_required_single::<i64, _>("...", &()).await?;
    if my_number < 0 {
        return Err(YourError::ResultIsNegative)
    }

    tx.commit().await;

    my_number
};

Errors are easier to handle and one has full control over retries.

[Lur1an]

Your redesign for the transaction would check off all boxes for which I created this PR.

Is the no_retry function still valuable then? It's the one that adds the most code, as it duplicates the original transaction function without the inner retry loop.

Close this PR and start working on the transaction redesign?

[aljazerzen]

Is the no_retry function still valuable then? It's the one that adds the most code, as it duplicates the original transaction function without the inner retry loop.

I'd say no. That would be too much overlapping functionality.

---

Close this PR and start working on the transaction redesign?

Yes, please :D