Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing the error UniqueConstraintViolation as a separate kind. (Recovered) #1110

Closed
wants to merge 2 commits into from

Conversation

mohs8421
Copy link
Contributor

Interpreting errors now is usually no longer independent of the database, so the error handling functions in sqlx_common.rs have to be split up. Since the interpretation depends on the backend, I provided a method there, which redirects to the driver implementations. I dropped the mentioned functions in sqlx_common, to avoid anyone relying on these, as that would no longer be valid without knowing the db context.

This PR continues #1040
changes from other people might still be missing here.

Interpreting errors now is usually no longer independent of the database, so the error handling functions in sqlx_common.rs have to be split up. Since the interpretation depends on the backend, I provided a method there, which redirects to the driver implementations.
I dropped the mentioned functions in sqlx_common, to avoid anyone relying on these, as that would no longer be valid without knowing the db context.
In mysql looking at the code alone was not sufficient to disambiguate different errors, as there exists the separate field `number`.
So I added a test for this additional case too.
Fixed some errors when compiling without sqlx.
@mohs8421
Copy link
Contributor Author

@billy1624 any update on the further process regarding this?

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mohs8421, sorry for the long delay!

@@ -108,21 +110,21 @@ macro_rules! try_getable_all {
QueryResultRow::SqlxMySql(row) => {
use sqlx::Row;
row.try_get::<Option<$type>, _>(column.as_str())
.map_err(|e| TryGetError::DbErr(crate::sqlx_error_to_query_err(e)))
.map_err(|e| TryGetError::DbErr(DbBackend::MySql.map_query_err(e)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to map the error into specific DB error at src/executor/query.rs because the query has been executed. We are parsing the result here. We can keep it as crate::sqlx_error_to_query_err(e).

Comment on lines -3 to -11
/// Converts an [sqlx::error] execution error to a [DbErr]
pub fn sqlx_error_to_exec_err(err: sqlx::Error) -> DbErr {
DbErr::Exec(RuntimeErr::SqlxError(err))
}

/// Converts an [sqlx::error] query error to a [DbErr]
pub fn sqlx_error_to_query_err(err: sqlx::Error) -> DbErr {
DbErr::Query(RuntimeErr::SqlxError(err))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally prefer keeping both method and take a DbBackend for converting the error to DB specific error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point in removing these from the common space was, that the implementations where no longer common, but separate for each backend. Hence it has to be in relation to the backend to see how an error has to be read. Since these methods where mostly wrappers before, I didn't see a loss in removing them.

@tyt2y3
Copy link
Member

tyt2y3 commented Dec 28, 2022

Sorry but I think I have detailed my plan/thought on fixing this issue here and thus this PR either needs a rewrite or to be closed

@mohs8421
Copy link
Contributor Author

Sorry but I think I have detailed my plan/thought on fixing this issue here and thus this PR either needs a rewrite or to be closed

I will look into this, but I need time, having other stuff on my workstack for now.

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 15, 2023

Closed in favour of #1707

@tyt2y3 tyt2y3 closed this Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants