edgedb / edgedb-rust

The official Rust binding for EdgeDB
https://edgedb.com
Apache License 2.0
215 stars 26 forks source link

Add `AsEdgedbError` trait and `TransactionError` type #208

Closed tailhook closed 1 year ago

tailhook commented 1 year ago

Trying to improve the situation with errors in transaction.

This adds a trait and instead of requiring concrete type from transaction it returns a trait. So you can use different types for transactions.

Inspired by discussion in #171

tailhook commented 1 year ago

This looks quite unergonomic and unlikely to be applied. Here I'll try to note the list of issues:

  1. Return type has to be specified explicitly in the closure to have question mark ? operator work (this could be not the case if TransactionError is used right away not as a trait)
  2. .map_err must still be used on user errors
  3. Inacurate wrapping of errors is dangerous and easy to get (see example below)

Error Wrapping Issue

async fn do_transaction(tx: &Transaction) -> anyhow::Result<()> {
    tx.query("# [... snip ...]").await?;
    Ok(())
}
async fn handle_request(conn: &Client) -> TransactionError {
   conn.transaction(|| async move {
      do_transaction(tx)
          // Since it returns generic anyhow::Error you can only wrap it into a
          // user transaction.
          .map_err(TransactionError::User)
   })
}

In this case, retries will not work as retry code will only check TransactionError::Edgedb errors.

While with the current code we check all sources of errors so. do_transaction(tx).map_err(UserError::from_source) will work fine for retries. (Although, perhaps will not be printed nice enough by default).

Also current codebase in the main branch has impl<T> From<T> for Error where T: AsRef<dyn StdError + Send + Sync + 'static>. Which makes it possible to use ? for anyhow errors (see example). Although, I'm not sure if it will create issues in the future.

tailhook commented 1 year ago

Closing as it looks like it creates more issues than solves.