-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
Added enum for SQLx Error and basic functions for it #1707
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Ivan, nice! The sql_err
method looks a lot cleaner than the first draft! Some comments...
src/error.rs
Outdated
"23000" => return Some(SqlErr::UniqueConstraintViolation()), | ||
"1586" => return Some(SqlErr::UniqueConstraintViolation()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we have two MySQL error codes representing the same unique constraint violation error?
Can we have a comment documenting the corresponding web page of each database's error code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I put 23000 since I saw it in the error testing file, but later I found that 23000 is the sqlstate for both unique/foreign key constraint violation. Therefore I cannot use sqlstate there. I would update the error codes again once I make a foreign key constraint test.
Thank you for spotting this though!
let error_number = e | ||
.try_downcast_ref::<sqlx::mysql::MySqlDatabaseError>()? | ||
.number(); | ||
match error_number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @darkmmon, I wonder why for MySQL we use error_number
to determine the error type instead of using _error_code_expanded
just like other databases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billy1624
The reason is that the error code for MySQL is ambiguous. If I wanted to do it with .code() like for other databases, I would not be able to distinguish the violation between unique constraint and foreign key constraint (they both return 23000).
I can only think of using the exact error number for distinguishing these errors, leading to the clumsy code. If there is an elegant way it would definitely be welcomed!
src/error.rs
Outdated
ForeignKeyConstraintViolation(), | ||
/// error for duplicate record in unique field or primary key field | ||
#[error("Unique Constraint Violated")] | ||
UniqueConstraintViolation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My Question is why remove the ()
? It is a placeholder, in case some day, we'd like to add some content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe if in the future we want to add content, then we will need to update everything no matter if we place the ()
there. So I thought it would be better for us to remove the bracket first and add it back if it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I thought we shipped this enum somewhere already.
But then can we actually shove a little more context inside them?
Even just &'static str
something like "Duplicate entry".
But of course more structure is welcomed.
src/error.rs
Outdated
#[non_exhaustive] | ||
pub enum SqlErr { | ||
/// error for duplicate record in unique field or primary key field | ||
#[error("Unique Constraint Violated")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can then do #[error("Unique Constraint Violated: {0}")]
tests/sql_err_tests.rs
Outdated
assert_eq!( | ||
error.sql_err(), | ||
Some(SqlErr::UniqueConstraintViolation(String::from( | ||
error_message | ||
))) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content is out of our control, so may be
assert!(matches!(error.sql_err(), Some(SqlErr::UniqueConstraintViolation(_)));
would suffice.
* Optional Field SeaQL/sea-orm#1513 * .gitignore SeaQL/sea-orm#1334 * migration table custom name SeaQL/sea-orm#1511 * OR condition relation SeaQL/sea-orm#1433 * DerivePartialModel SeaQL/sea-orm#1597 * space for migration file naming SeaQL/sea-orm#1570 * composite key up to 12 SeaQL/sea-orm#1508 * seaography integration SeaQL/sea-orm#1599 * QuerySelect SimpleExpr SeaQL/sea-orm#1702 * sqlErr SeaQL/sea-orm#1707 * migration check SeaQL/sea-orm#1519 * postgres array SeaQL/sea-orm#1565 * param intoString SeaQL/sea-orm#1439 * **skipped** re-export SeaQL/sea-orm#1661 * ping SeaQL/sea-orm#1627 * on empty do nothing SeaQL/sea-orm#1708 * on conflict do nothing SeaQL/sea-orm#1712 * **skipped** upgrade versions * active enum fail safe SeaQL/sea-orm#1374 * relation generation check SeaQL/sea-orm#1435 * entity generation bug SeaQL/sea-schema#105 * **skipped** bug fix that does not require edits * EnumIter change SeaQL/sea-orm#1535 * completed and fixed a previous todo SeaQL/sea-orm#1570 * amended wordings and structures * Edit * Remove temp file --------- Co-authored-by: Billy Chan <ccw.billy.123@gmail.com>
🎉 Released In 0.12.1 🎉Thank you everyone for the contribution! |
PR Info
New Features
Changes