Flatten error handling, reducing generics

[smklein]

This is a clean-up enabled by #52

• Removes the ConnErr generic variant
• Propagates a raw diesel error type, rather than a "connection variant". The old variant was used to encapsulate either "a database error" or "a checkout error". Since callers now must checkout connections before issuing queries, these don't need to be intermingled.

[smklein]

See: oxidecomputer/omicron#4210 for usage

[sunshowers]

General question, is returning a DieselError directly correct or do we want to return a newtype on it (or maybe a DieselError with some augmented information?)

[sunshowers]

Looks good other than the question about whether we want to return our own type for future expandability.

[smklein]

General question, is returning a DieselError directly correct or do we want to return a newtype on it (or maybe a DieselError with some augmented information?)

Can I dig into what you're suggesting here? I don't necessarily disagree, but I want to get a better idea of "costs, pros, and cons".

It wasn't a simple "wrapper" newtype, but this is kinda what the ConnectionError was being used for, basically:

enum ConnectionError {
  Database(DieselError),
  Connection(WeFailedToPollTheConnectionPool),
}

Arguably we could have had additional layers, other errors here, etc... but this was painful from an ergonomics point-of-view, because most of the time when someone issues a request to the DB, error-handling is specific to "what did the DB say", meaning that callers want to match on the Database variant (see: oxidecomputer/omicron#4210 ).

We could add a wrapper type like:

enum AsyncBb8DieselError(DieselError);

But I dunno how advantageous that would be over just returning the type directly, since clients need to match on / decompose that DieselError, so we can't hide the type forever.

[sunshowers]

I do agree that this is a huge improvement over the current situation -- mixing connection and database errors doesn't seem quite right for the reasons you pointed out.

What about:

struct AsyncBb8DieselError {
    error: DieselError,
    // ... some extra fields, e.g. identifier for the connection in the pool}

I'm wondering if issues like #47 would have been ever so slightly easier to spot with that extra info.

But I certainly won't block on this.

[smklein]

I do agree that this is a huge improvement over the current situation -- mixing connection and database errors doesn't seem quite right for the reasons you pointed out.

What about:

struct AsyncBb8DieselError {
    error: DieselError,
    // ... some extra fields, e.g. identifier for the connection in the pool}

I'm wondering if issues like #47 would have been ever so slightly easier to spot with that extra info.

But I certainly won't block on this.

Ah, that makes sense. If we were sticking with async-bb8-diesel forever, I'd be strongly in favor of this direction.

I will throw out one more caveat -- this whole crate pre-dated diesel-async, and was kinda a stop-gap (see: oxidecomputer/omicron#4188 ). That crate directly returns DieselErrors, so aligning with it would make conversion easier. Similarly, breaking that convention makes conversion slightly harder.

With this in mind, I think I'm going to move forward on this PR as-is, but I really do like the proposal of "struct as a wrapper around an underlying error + easy access to underlying error, but extra auxiliary error info which can be propagated back up" too.