sea-orm
sea-orm copied to clipboard
SeaQL
Introducing sqlx-error feature
PR Info
Purpose of this feature is to not convert errors given from sqlx into strings to ease further analysis of the error and react to it accordingly. This implementation uses a feature switch and an additional error kind to avoid interfering with existing implementations without this feature enabled.
- Closes this discussion
Adds
- sqlx-error forwarding
@mohs8421 hello! Thank you for the PR, some tests have failed.
I saw that, but didn't have time to look deeper into these so far. Besides this change is more a suggestion, and @billy1624 already had a slightly different approach in mind, so it might even be better, if you take over what I did here. That decision is up to you.
After second thought, I will put this on hold until we come up with a way to generically represent common errors for different db backends. By common errors I mean, foreign key constraint error, unique constraint error...etc.
that means going for the "more sophisticated variant", this probably means more enum variants, it might not necessarily be more difficult than this approach. The sqlx error interpreting methods for the db drivers would need to check the error code and tell which variant it is, and perhaps provide a catch all fallback for error codes specific to the database which have not been implemented in sea-orm.
My main concern is that just throwing out the db depent error isn't ideal. Considering SeaORM can be used on multiple db backend.
My main concern is that just throwing out the db depent error isn't ideal. Considering SeaORM can be used on multiple db backend.
If I go at an other approach to this, I will take that thought into account. However, I think it is better to give users of SeaORM the possibility to cope this, instead of making it more difficult to get around that, we could introduce a source enum, which might help interpreting the error in the correct database context for this purpose. But I was thinking this is already available at some point.
The thing with abstraction layers always is that they are leaking pieces of other layers, and the question is how to deal with that. In my opinion it is better to accept this boundary and do this "leaking" intentionally at selective places where it is reasonable.
As a first thing I fixed the errors, that had occurred.
Now I took a look at the error conversion code again, and the thing is, that if I continue with that better approach, I would not be able to make it as a switchable feature, but as a breaking change, as the errors returned will look differently. Please let me know which way you prefer.
I don't think adding a new error variant is the way to go for now.
I prefer having the context approach (https://docs.rs/anyhow/latest/anyhow/trait.Context.html), so instead of Query(String) we should return a Query(QueryErr)
From which, we can hold a dyn Box of the SQLx error object, where the user should be able to retrieve via the ErrContext trait and downcast i.e. https://doc.rust-lang.org/std/any/index.html to the desired concrete error.
Meanwhile, we should think about mapping some common SQL errors to a enum, i.e. column not exist, constraint violation
Example of anyhow context with downcasting: https://github.com/dtolnay/anyhow/blob/master/tests/test_context.rs#L96-L118
Example of
anyhowcontext with downcasting: https://github.com/dtolnay/anyhow/blob/master/tests/test_context.rs#L96-L118
I still don't see how this would make it easier to implement the desired behaviour, can you help me out here so I can see what I was missing?
Okay, forget about anyhow. I don't think we need that on a library. We can adopt thiserror instead haha. Read https://nick.groenen.me/posts/rust-error-handling for more.
Enough talking. So, we'd update DbErr to be something like:
pub enum DbErr {
/// There was a problem with the database connection
Conn(ErrContext),
/// An operation did not execute successfully
Exec(ErrContext),
/// An error occurred while performing a query
Query(ErrContext),
...
}
pub struct ErrContext {
pub message: String,
pub code: Option<String>,
}
The part where SQLx errors are parsed into DbErr has to be changed as well. We can try to convert sqlx::errors into sqlx::DatabaseError. Then, we can get its error message and code out of it, and place it inside our own ErrContext.
- https://github.com/SeaQL/sea-orm/blob/d262501a44c048ed36a17bbf661ced3c5193f455/src/driver/sqlx_common.rs#L3-L16
Thanks, I guess that article about anyow and thiserror is really helpful on that regard. I will adapt my change to that when I have the opportunity to do this.
As the tests are passing now, what do you think about the current state? Do you want to pull it as it is, or should I continue by looking at the error codes first for some of the most common errors, or should we do that step afterwards? What remains a little puzzling in my opinion is the fact that postgres treats this as a query error, while the other databases issue an exec error. It should be aligned at some point. (look at the test I wrote)
Thank you for the contribution. This will be merged to a feature branch and I will work on a few details, in particular I think we should nesting the enum variant.