Error<Err> type pattern prevent building wrapping abstraction

[touilleMan]

Having to specify the custom error type in the Database brings a drawback: it is not possible to have multiple methods using different custom error.

This is typically an issue when wrapping the database to abstract it¹, consider the following:

struct WrapperDatabase( Database<anyhow::Error>);  // The error is settled here...
struct WrapperTransaction(Transaction<anyhow::Error>);

impl WrapperTransaction {
  // Implement business-specific methods
}

impl WrapperDatabase {
    async fn for_update<R, E>(
        &mut self,
        cb: impl AsyncFnOnce(WrapperTransaction) -> Result<R, E>, // ...but we want to be generic over the error type !
    ) -> anyhow::Result<Result<R, E>> {
        self.0.transaction(&["store"])
            .rw()
            .run(async move |transaction| {
                let wrapper = WrapperTransaction(transaction);
                cb(updater).await // Error ! expected Result<Result<R, E>, Error<Error>>, found Result<R, E>
            })
            .await
            .map_err(|e| anyhow::anyhow!("{e:?}"))
    }
 }

A solution to overcome this issue is to rely on type erasing:

struct WrapperDatabase( Database<Box<dyn std::any::Any>>);
struct WrapperTransaction(Transaction<Box<dyn std::any::Any>>);

impl WrapperDatabase {
    async fn for_update<R, E>(
        &mut self,
        cb: impl AsyncFnOnce(WrapperTransaction) -> Result<R, E>,
    ) -> anyhow::Result<Result<R, E>> {
        let outcome = self.0.transaction(&["store"])
            .rw()
            .run(async move |transaction| {
                let wrapper = WrapperTransaction(transaction);
                cb(wrapper)
                .await
                .map_err(|e| {
                    let boxed = Box::new(e) as Box<dyn std::any::Any>;
                    boxed.into()
                })
            })
            .await;
        match outcome {
            Ok(ok) => Ok(Ok(ok)),
            Err(err) => match err {
                indexed_db::Error::User(err) => Ok(Err(err.downcast().unwrap())),
                err => Err(anyhow::anyhow!("{err:?}")),
            }
        }
    }
 }

However this is rather cumbersome, and force to Box the error...

Do you see any better approach for this ?

Footnotes

1. I'm doing this to have a codebase using SQLite on native and IndexedDb on web ↩

[Ekleog]

Usually, I'd expect users to choose their own type and stick through it — be it a thiserror enum or just using Database<anyhow::Error>, both ways would work fine with the current scheme. So depending on whether you can choose one specific error or not, you'd use one or the other. Actually, maybe for convenience it'd make sense to add a default to anyhow::Error for this generic argument? But OTOH it'd add a dependency to anyhow, that is currently not required.

As for what you could do, the thing I'd do on my end would probably be to also take <Err> on your wrapper structs. This way, your users could also have the error types that they prefer

I'm not sure I understand your question with all its specificities, but hopefully this answers your question?

BTW, if you're trying to abstract SQLite and IndexedDb, you may be interested in the project I'm working on right now: sakuhiki, an abstraction layer for KV stores like indexed-db, rocksdb or tikv 😄

[Ekleog]

Oh and also: the reason why I'm using a Factory-wide Err type is so that type inference works better. I originally started with an Err type specified on each run call, but then I'd need to explicitly list the Err type on all runs.

But… maybe that'd be a lesser evil? I'm actually rethinking it, especially now that I'll have to think about releasing a 0.5 with #5.

WDYT about this alternative? It might be easier to handle for you too?

[touilleMan]

Oh and also: the reason why I'm using a Factory-wide Err type is so that type inference works better

I don't have enough experience on this part, so I might be missing some important tradeoff here compared to my current needs ^^

WDYT about this alternative? It might be easier to handle for you too?

I'd rather have each call to TransactionBuilder::run have it own parametrize on error type, so typically returning a Result<Result<R, E>, indexed_db::Error>

This way it would be easier to have per-method business error:

async fn add_user(id: u32, name: String) -> AddUserError { ... }

async fn get_user(id: u32) -> Restul<String, GetUserError> {
    db.transaction(&["data"]).run(async move |t| {
        match t.get(&id).await? {
            Some(user) => Ok(user),
            None => Err(GetUserError::NotFound),
        }
    })
    .await
    .map_err(|e| GetUserError::DatabaseError(e))?
   // Typically with `DatabaseError(#[transparent] anyhow::Error)`
}

(on top of being able to write middleware wrapper as my original post was showcasing)

[Ekleog]

Hmm so I'm still thinking of returning a Result<R, Error<E>> from the transaction's run function, but just not have that E be enforced-the-same for literally all the transactions.

That's because this way I can have the From implementations required to allow the user to just use ? basically everywhere in their code: if I did it the way you suggest, your example require Err(Err(GetUserError::NotFound)), which is much less pleasant — especially when trying to use ?

[touilleMan]

I agree the biggest issue for me is the fact the error type is binded to the Database object.

On the other hand, having to do something like is fine:

async fn get_user(id: u32) -> Restul<String, GetUserError> {
    db.transaction(&["data"]).run(async move |t| {
        match t.get(&id).await? {
            Some(user) => Ok(user),
            None => Err(indexed_db::Error::Custom(GetUserError::NotFound)),
        }
    })
    .await
    .map_err(|e| match e {
        indexed_db::Error::Custom(e) => e
        e => GetUserError::DatabaseError(e)
    })
}

[Ekleog]

Got it! I'll go forward with this soon then; plus thanks to the From implementation you can just do .foo()? where foo returns GetUserError and it'll automagically cast to indexed_db::Error<GetUserError> — that's my whole reason for using an Error<Err> struct 😄

[Ekleog]

I just killed as much as possible while still keeping the From implementations — unfortunately it means that we'll only be able to kill the remaining generics once never_type gets stabilized.

I also took advantage of that to move object store creation from the Database to the VersionChangeEvent: this way it'll be harder to misuse, as it's supposed to only be used there anyway 😄

Can you confirm b3380b0 looks good to you, and I can close this issue?

[touilleMan]

Can you confirm b3380b0 looks good to you, and I can close this issue?

I've played a bit and opened a PR with my findings ;-)
See #15