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

Added enum for SQLx Error and basic functions for it #1707

Merged
merged 26 commits into from Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
01d6050
Added enum for SQL Error and basic functions for it
darkmmon Jun 13, 2023
e973517
fmt
darkmmon Jun 13, 2023
6de76f9
Restructured code to allow multiple database system simultaneously
darkmmon Jun 14, 2023
fd337e1
updated code error and moved Eq to derive by compiler
darkmmon Jun 14, 2023
268c6f2
added ForeignKeyViolation implementation
darkmmon Jun 14, 2023
2230575
fmt
darkmmon Jun 14, 2023
015172c
added type param
darkmmon Jun 14, 2023
96201b8
changed type param and used unwrap_or_default instead of unwrap
darkmmon Jun 14, 2023
1d84145
fmt
darkmmon Jun 14, 2023
f19f28b
refactor code and update documentation
darkmmon Jun 14, 2023
90c109f
updated some error code and fixed formatting
darkmmon Jun 14, 2023
64b5176
create SQL Err test for UniqueConstraintViolation
darkmmon Jun 14, 2023
610a470
fmt
darkmmon Jun 14, 2023
bdd432f
updated error codes for mysql errors
darkmmon Jun 15, 2023
b693813
added test for ForeignKeyConstraintViolation and removed unused impor…
darkmmon Jun 15, 2023
acc9ed4
fmt
darkmmon Jun 15, 2023
96e566e
updated doc and error message
darkmmon Jun 15, 2023
df52415
added static str in SqlErr
darkmmon Jun 16, 2023
02dceff
changed to add error message into SqlErr as String
darkmmon Jun 16, 2023
5fb5128
updated test file to check database type through connection
darkmmon Jun 16, 2023
90279ae
fmt
darkmmon Jun 16, 2023
6fe8c5b
updated test for SqlErr to match the error type only
darkmmon Jun 19, 2023
ae8067f
Removed comment and fixed grammar mistake
darkmmon Jun 19, 2023
144976d
fmt
darkmmon Jun 19, 2023
da0660b
Update src/error.rs
tyt2y3 Jun 19, 2023
156e577
Refactoring
billy1624 Jun 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 37 additions & 19 deletions src/error.rs
Expand Up @@ -144,18 +144,17 @@ where
#[derive(Error, Debug, PartialEq, Eq)]
tyt2y3 marked this conversation as resolved.
Show resolved Hide resolved
#[non_exhaustive]
pub enum SqlErr {
/// error for inserting a record with a key that already exists in the table
#[error("Cannot have record with same key")]
UniqueConstraintViolation(),
/// error for Foreign key is not primary key
#[error("Cannot add non-primary key from other table")]
ForeignKeyConstraintViolation(),
/// error for duplicate record in unique field or primary key field
#[error("Unique Constraint Violated")]
Copy link
Member

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}")]

UniqueConstraintViolation,
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@tyt2y3 tyt2y3 Jun 15, 2023

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.

/// error for Foreign key constraint
#[error("Foreign Key Constraint Violated")]
ForeignKeyConstraintViolation,
}

#[allow(dead_code)]
impl DbErr {
/// convert generic DbErr by sqlx to SqlErr
/// return none if input is not any of SqlErr
/// Convert generic DbErr by sqlx to SqlErr, return none if the error is not any type of SqlErr
pub fn sql_err(&self) -> Option<SqlErr> {
#[cfg(any(
feature = "sqlx-mysql",
Expand All @@ -168,33 +167,52 @@ impl DbErr {
| DbErr::Query(RuntimeErr::SqlxError(sqlx::Error::Database(e))) = self
{
let error_code = e.code().unwrap_or_default();
let error_code_expanded = error_code.deref();
let _error_code_expanded = error_code.deref();
#[cfg(feature = "sqlx-mysql")]
if e.try_downcast_ref::<sqlx::mysql::MySqlDatabaseError>()
.is_some()
{
match error_code_expanded {
"23000" => return Some(SqlErr::UniqueConstraintViolation()),
"1586" => return Some(SqlErr::UniqueConstraintViolation()),
"1452" => return Some(SqlErr::ForeignKeyConstraintViolation()),
let error_number = e
.try_downcast_ref::<sqlx::mysql::MySqlDatabaseError>()?
.number();
match error_number {
Comment on lines +175 to +178
Copy link
Member

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?

Copy link
Contributor Author

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!

// 1022 Can't write; duplicate key in table '%s'
// 1062 Duplicate entry '%s' for key %d
// 1169 Can't write, because of unique constraint, to table '%s'
// 1586 Duplicate entry '%s' for key '%s'
1022 | 1062 | 1169 | 1586 => {
return Some(SqlErr::UniqueConstraintViolation)
}
// 1216 Cannot add or update a child row: a foreign key constraint fails
// 1217 Cannot delete or update a parent row: a foreign key constraint fails
// 1451 Cannot delete or update a parent row: a foreign key constraint fails (%s)
// 1452 Cannot add or update a child row: a foreign key constraint fails (%s)
// 1557 Upholding foreign key constraints for table '%s', entry '%s', key %d would lead to a duplicate entry
// 1761 Foreign key constraint for table '%s', record '%s' would lead to a duplicate entry in table '%s', key '%s'
// 1762 Foreign key constraint for table '%s', record '%s' would lead to a duplicate entry in a child table
1216 | 1217 | 1451 | 1452 | 1557 | 1761 | 1762 => {
return Some(SqlErr::ForeignKeyConstraintViolation)
}
_ => return None,
}
}
#[cfg(feature = "sqlx-postgres")]
if e.try_downcast_ref::<sqlx::postgres::PgDatabaseError>()
.is_some()
{
match error_code_expanded {
"23505" => return Some(SqlErr::UniqueConstraintViolation()),
"23503" => return Some(SqlErr::ForeignKeyConstraintViolation()),
match _error_code_expanded {
"23505" => return Some(SqlErr::UniqueConstraintViolation),
"23503" => return Some(SqlErr::ForeignKeyConstraintViolation),
_ => return None,
}
}
#[cfg(feature = "sqlx-sqlite")]
if e.try_downcast_ref::<sqlx::sqlite::SqliteError>().is_some() {
match error_code_expanded {
"1555" => return Some(SqlErr::UniqueConstraintViolation()),
"787" => return Some(SqlErr::ForeignKeyConstraintViolation()),
match _error_code_expanded {
// error code 1555 refers to the primary key's unique constraint violation
// error code 2067 refers to the UNIQUE unique constraint violation
"1555" | "2067" => return Some(SqlErr::UniqueConstraintViolation),
"787" => return Some(SqlErr::ForeignKeyConstraintViolation),
_ => return None,
}
}
Expand Down
43 changes: 22 additions & 21 deletions tests/sql_err_tests.rs
@@ -1,17 +1,9 @@
pub mod common;
mod crud;
use rust_decimal_macros::dec;
pub use sea_orm::entity::*;
use sea_orm::error::*;
pub use sea_orm::{sea_query, tests_cfg, ConnectionTrait, EntityName, QueryFilter, QuerySelect};
// use sea_query::ForeignKey;
use uuid::Uuid;

pub use common::{bakery_chain::*, setup::*, TestContext};
use pretty_assertions::assert_eq;

pub use crud::*;
use sea_orm::DatabaseConnection;
use rust_decimal_macros::dec;
pub use sea_orm::{entity::*, error::*, tests_cfg, DatabaseConnection, EntityName, ExecResult};
use uuid::Uuid;

#[sea_orm_macros::test]
#[cfg(any(
Expand All @@ -24,11 +16,6 @@ async fn main() {
create_tables(&ctx.db).await.unwrap();
test_error(&ctx.db).await;
ctx.delete().await;

// let ctx = TestContext::new("bakery_chain_sql_err_tests_2").await;
// create_tables(&ctx.db2).await.unwrap();
// test_error_foreign(&ctx.db1, &ctx.db2).await;
// ctx.delete().await;
}

pub async fn test_error(db: &DatabaseConnection) {
Expand All @@ -52,10 +39,24 @@ pub async fn test_error(db: &DatabaseConnection) {
.await
.expect_err("inserting should fail due to duplicate primary key");

let sqlerr = error.sql_err();
assert_eq!(sqlerr.unwrap(), SqlErr::UniqueConstraintViolation())
}
assert_eq!(error.sql_err(), Some(SqlErr::UniqueConstraintViolation));

// pub async fn test_error_foreign(db1: &DatabaseConnection, db2: &DatabaseConnection) {
let fk_cake = cake::ActiveModel {
name: Set("fk error Cake".to_owned()),
price: Set(dec!(10.25)),
gluten_free: Set(false),
serial: Set(Uuid::new_v4()),
bakery_id: Set(Some(1000)),
..Default::default()
};

// }
let fk_error = fk_cake
.insert(db)
.await
.expect_err("create foreign key should fail with non-primary key");

assert_eq!(
fk_error.sql_err(),
Some(SqlErr::ForeignKeyConstraintViolation)
);
}