From aa5ccb9216cab1e98c408325efc2f4c4e7f02072 Mon Sep 17 00:00:00 2001 From: Billy Chan Date: Fri, 15 Oct 2021 17:41:59 +0800 Subject: [PATCH 1/6] Hotfix - `ActiveModel::insert()` trigger `ActiveModelBehavior::after_save()` --- src/entity/active_model.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/entity/active_model.rs b/src/entity/active_model.rs index 9006e385..f952f721 100644 --- a/src/entity/active_model.rs +++ b/src/entity/active_model.rs @@ -114,10 +114,11 @@ pub trait ActiveModelTrait: Clone + Debug { let found = ::find_by_id(res.last_insert_id) .one(db) .await?; - match found { - Some(model) => Ok(model.into_active_model()), - None => Err(DbErr::Exec("Failed to find inserted item".to_owned())), - } + let am = match found { + Some(model) => model.into_active_model(), + None => return Err(DbErr::Exec("Failed to find inserted item".to_owned())), + }; + ActiveModelBehavior::after_save(am, true) } async fn update<'a, C>(self, db: &'a C) -> Result From cc96bb783cbea8097d49e16e37f7e9be34a14f85 Mon Sep 17 00:00:00 2001 From: Billy Chan Date: Fri, 15 Oct 2021 17:42:50 +0800 Subject: [PATCH 2/6] Test `ActiveModelBehavior` --- tests/common/bakery_chain/cake.rs | 74 ++++++++++++++++++++++- tests/transaction_tests.rs | 99 ++++++++++++++++++++++++++++++- 2 files changed, 170 insertions(+), 3 deletions(-) diff --git a/tests/common/bakery_chain/cake.rs b/tests/common/bakery_chain/cake.rs index 4730394b..cdae08af 100644 --- a/tests/common/bakery_chain/cake.rs +++ b/tests/common/bakery_chain/cake.rs @@ -49,4 +49,76 @@ impl Related for Entity { } } -impl ActiveModelBehavior for ActiveModel {} +impl ActiveModelBehavior for ActiveModel { + fn new() -> Self { + use sea_orm::Set; + Self { + serial: Set(Uuid::new_v4()), + ..ActiveModelTrait::default() + } + } + + fn before_save(self, insert: bool) -> Result { + use rust_decimal_macros::dec; + if let Some(price) = self.price.clone().take() { + if price == dec!(0) { + Err(DbErr::Custom(format!( + "[before_save] Invalid Price, insert: {}", + insert + ))) + } else { + Ok(self) + } + } else { + Err(DbErr::Custom(format!( + "[before_save] Price must be set, insert: {}", + insert + ))) + } + } + + fn after_save(self, insert: bool) -> Result { + use rust_decimal_macros::dec; + if let Some(price) = dbg!(self.price.clone()).take() { + if price < dec!(0) { + Err(DbErr::Custom(format!( + "[after_save] Invalid Price, insert: {}", + insert + ))) + } else { + Ok(self) + } + } else { + Err(DbErr::Custom(format!( + "[after_save] Price must be set, insert: {}", + insert + ))) + } + } + + fn before_delete(self) -> Result { + if let Some(name) = self.name.clone().take() { + if name.contains("(err_on_before_delete)") { + Err(DbErr::Custom( + "[before_delete] Cannot be deleted".to_owned(), + )) + } else { + Ok(self) + } + } else { + Err(DbErr::Custom("[before_delete] Name must be set".to_owned())) + } + } + + fn after_delete(self) -> Result { + if let Some(name) = self.name.clone().take() { + if name.contains("(err_on_after_delete)") { + Err(DbErr::Custom("[after_delete] Cannot be deleted".to_owned())) + } else { + Ok(self) + } + } else { + Err(DbErr::Custom("[after_delete] Name must be set".to_owned())) + } + } +} diff --git a/tests/transaction_tests.rs b/tests/transaction_tests.rs index 6a713e4d..72e20368 100644 --- a/tests/transaction_tests.rs +++ b/tests/transaction_tests.rs @@ -1,9 +1,9 @@ pub mod common; pub use common::{bakery_chain::*, setup::*, TestContext}; +use pretty_assertions::assert_eq; pub use sea_orm::entity::*; -pub use sea_orm::{ConnectionTrait, QueryFilter}; -use sea_orm::{DatabaseTransaction, DbErr}; +pub use sea_orm::*; #[sea_orm_macros::test] #[cfg(any( @@ -105,6 +105,101 @@ fn _transaction_with_reference<'a>( }) } +#[sea_orm_macros::test] +#[cfg(any( + feature = "sqlx-mysql", + feature = "sqlx-sqlite", + feature = "sqlx-postgres" +))] + +#[sea_orm_macros::test] +#[cfg(any( + feature = "sqlx-mysql", + feature = "sqlx-sqlite", + feature = "sqlx-postgres" +))] +pub async fn transaction_with_active_model_behaviour() -> Result<(), DbErr> { + use rust_decimal_macros::dec; + let ctx = TestContext::new("transaction_with_active_model_behaviour_test").await; + + if let Ok(txn) = ctx.db.begin().await { + assert_eq!( + cake::ActiveModel { + name: Set("Cake with invalid price".to_owned()), + price: Set(dec!(0)), + gluten_free: Set(false), + ..Default::default() + } + .save(&txn) + .await, + Err(DbErr::Custom( + "[before_save] Invalid Price, insert: true".to_owned() + )) + ); + + assert_eq!(cake::Entity::find().all(&txn).await?.len(), 0); + + assert_eq!( + cake::ActiveModel { + name: Set("Cake with invalid price".to_owned()), + price: Set(dec!(-10)), + gluten_free: Set(false), + ..Default::default() + } + .save(&txn) + .await, + Err(DbErr::Custom( + "[after_save] Invalid Price, insert: true".to_owned() + )) + ); + + assert_eq!(cake::Entity::find().all(&txn).await?.len(), 1); + + let readonly_cake_1 = cake::ActiveModel { + name: Set("Readonly cake (err_on_before_delete)".to_owned()), + price: Set(dec!(10)), + gluten_free: Set(true), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(cake::Entity::find().all(&txn).await?.len(), 2); + + assert_eq!( + readonly_cake_1.delete(&txn).await.err(), + Some(DbErr::Custom( + "[before_delete] Cannot be deleted".to_owned() + )) + ); + + assert_eq!(cake::Entity::find().all(&txn).await?.len(), 2); + + let readonly_cake_2 = cake::ActiveModel { + name: Set("Readonly cake (err_on_after_delete)".to_owned()), + price: Set(dec!(10)), + gluten_free: Set(true), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(cake::Entity::find().all(&txn).await?.len(), 3); + + assert_eq!( + readonly_cake_2.delete(&txn).await.err(), + Some(DbErr::Custom("[after_delete] Cannot be deleted".to_owned())) + ); + + assert_eq!(cake::Entity::find().all(&txn).await?.len(), 2); + } + + assert_eq!(cake::Entity::find().all(&ctx.db).await?.len(), 0); + + ctx.delete().await; + Ok(()) +} + #[sea_orm_macros::test] #[cfg(any( feature = "sqlx-mysql", From 13f5792f8691fbc8c939de3f4b66d2f0bb3cc64f Mon Sep 17 00:00:00 2001 From: Billy Chan Date: Fri, 15 Oct 2021 17:43:01 +0800 Subject: [PATCH 3/6] Test transaction --- tests/transaction_tests.rs | 235 +++++++++++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+) diff --git a/tests/transaction_tests.rs b/tests/transaction_tests.rs index 72e20368..78186222 100644 --- a/tests/transaction_tests.rs +++ b/tests/transaction_tests.rs @@ -111,6 +111,241 @@ fn _transaction_with_reference<'a>( feature = "sqlx-sqlite", feature = "sqlx-postgres" ))] +pub async fn transaction_begin_out_of_scope() -> Result<(), DbErr> { + let ctx = TestContext::new("transaction_begin_out_of_scope_test").await; + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + { + // Transaction begin in this scope + let txn = ctx.db.begin().await?; + + bakery::ActiveModel { + name: Set("SeaSide Bakery".to_owned()), + profit_margin: Set(10.4), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(bakery::Entity::find().all(&txn).await?.len(), 1); + + bakery::ActiveModel { + name: Set("Top Bakery".to_owned()), + profit_margin: Set(15.0), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(bakery::Entity::find().all(&txn).await?.len(), 2); + + // The scope ended and transaction is dropped without commit + } + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + ctx.delete().await; + Ok(()) +} + +#[sea_orm_macros::test] +#[cfg(any( + feature = "sqlx-mysql", + feature = "sqlx-sqlite", + feature = "sqlx-postgres" +))] +pub async fn transaction_begin_commit() -> Result<(), DbErr> { + let ctx = TestContext::new("transaction_begin_commit_test").await; + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + { + // Transaction begin in this scope + let txn = ctx.db.begin().await?; + + bakery::ActiveModel { + name: Set("SeaSide Bakery".to_owned()), + profit_margin: Set(10.4), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(bakery::Entity::find().all(&txn).await?.len(), 1); + + bakery::ActiveModel { + name: Set("Top Bakery".to_owned()), + profit_margin: Set(15.0), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(bakery::Entity::find().all(&txn).await?.len(), 2); + + // Commit changes before the end of scope + txn.commit().await?; + } + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 2); + + ctx.delete().await; + Ok(()) +} + +#[sea_orm_macros::test] +#[cfg(any( + feature = "sqlx-mysql", + feature = "sqlx-sqlite", + feature = "sqlx-postgres" +))] +pub async fn transaction_begin_rollback() -> Result<(), DbErr> { + let ctx = TestContext::new("transaction_begin_rollback_test").await; + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + { + // Transaction begin in this scope + let txn = ctx.db.begin().await?; + + bakery::ActiveModel { + name: Set("SeaSide Bakery".to_owned()), + profit_margin: Set(10.4), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(bakery::Entity::find().all(&txn).await?.len(), 1); + + bakery::ActiveModel { + name: Set("Top Bakery".to_owned()), + profit_margin: Set(15.0), + ..Default::default() + } + .save(&txn) + .await?; + + assert_eq!(bakery::Entity::find().all(&txn).await?.len(), 2); + + // Rollback changes before the end of scope + txn.rollback().await?; + } + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + ctx.delete().await; + Ok(()) +} + +#[sea_orm_macros::test] +#[cfg(any( + feature = "sqlx-mysql", + feature = "sqlx-sqlite", + feature = "sqlx-postgres" +))] +pub async fn transaction_closure_commit() -> Result<(), DbErr> { + let ctx = TestContext::new("transaction_closure_commit_test").await; + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + let res = ctx + .db + .transaction::<_, _, DbErr>(|txn| { + Box::pin(async move { + bakery::ActiveModel { + name: Set("SeaSide Bakery".to_owned()), + profit_margin: Set(10.4), + ..Default::default() + } + .save(txn) + .await?; + + assert_eq!(bakery::Entity::find().all(txn).await?.len(), 1); + + bakery::ActiveModel { + name: Set("Top Bakery".to_owned()), + profit_margin: Set(15.0), + ..Default::default() + } + .save(txn) + .await?; + + assert_eq!(bakery::Entity::find().all(txn).await?.len(), 2); + + Ok(()) + }) + }) + .await; + + assert!(res.is_ok()); + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 2); + + ctx.delete().await; + Ok(()) +} + +#[sea_orm_macros::test] +#[cfg(any( + feature = "sqlx-mysql", + feature = "sqlx-sqlite", + feature = "sqlx-postgres" +))] +pub async fn transaction_closure_rollback() -> Result<(), DbErr> { + let ctx = TestContext::new("transaction_closure_commit_test").await; + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + let res = ctx + .db + .transaction::<_, _, DbErr>(|txn| { + Box::pin(async move { + bakery::ActiveModel { + name: Set("SeaSide Bakery".to_owned()), + profit_margin: Set(10.4), + ..Default::default() + } + .save(txn) + .await?; + + assert_eq!(bakery::Entity::find().all(txn).await?.len(), 1); + + bakery::ActiveModel { + name: Set("Top Bakery".to_owned()), + profit_margin: Set(15.0), + ..Default::default() + } + .save(txn) + .await?; + + assert_eq!(bakery::Entity::find().all(txn).await?.len(), 2); + + bakery::ActiveModel { + id: Set(1), + name: Set("Duplicated primary key".to_owned()), + profit_margin: Set(20.0), + ..Default::default() + } + .insert(txn) + .await?; // Throw error and rollback + + // This line won't be reached + assert!(false); + + Ok(()) + }) + }) + .await; + + assert!(res.is_err()); + + assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); + + ctx.delete().await; + Ok(()) +} #[sea_orm_macros::test] #[cfg(any( From 7c18e19f56ec98da755925208b9be3a1679d2887 Mon Sep 17 00:00:00 2001 From: Billy Chan Date: Fri, 15 Oct 2021 17:52:46 +0800 Subject: [PATCH 4/6] Fixup --- tests/transaction_tests.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/transaction_tests.rs b/tests/transaction_tests.rs index 78186222..1bf4a241 100644 --- a/tests/transaction_tests.rs +++ b/tests/transaction_tests.rs @@ -113,6 +113,7 @@ fn _transaction_with_reference<'a>( ))] pub async fn transaction_begin_out_of_scope() -> Result<(), DbErr> { let ctx = TestContext::new("transaction_begin_out_of_scope_test").await; + create_tables(&ctx.db).await?; assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); @@ -157,6 +158,7 @@ pub async fn transaction_begin_out_of_scope() -> Result<(), DbErr> { ))] pub async fn transaction_begin_commit() -> Result<(), DbErr> { let ctx = TestContext::new("transaction_begin_commit_test").await; + create_tables(&ctx.db).await?; assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); @@ -202,6 +204,7 @@ pub async fn transaction_begin_commit() -> Result<(), DbErr> { ))] pub async fn transaction_begin_rollback() -> Result<(), DbErr> { let ctx = TestContext::new("transaction_begin_rollback_test").await; + create_tables(&ctx.db).await?; assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); @@ -247,6 +250,7 @@ pub async fn transaction_begin_rollback() -> Result<(), DbErr> { ))] pub async fn transaction_closure_commit() -> Result<(), DbErr> { let ctx = TestContext::new("transaction_closure_commit_test").await; + create_tables(&ctx.db).await?; assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); @@ -295,6 +299,7 @@ pub async fn transaction_closure_commit() -> Result<(), DbErr> { ))] pub async fn transaction_closure_rollback() -> Result<(), DbErr> { let ctx = TestContext::new("transaction_closure_commit_test").await; + create_tables(&ctx.db).await?; assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); @@ -356,6 +361,7 @@ pub async fn transaction_closure_rollback() -> Result<(), DbErr> { pub async fn transaction_with_active_model_behaviour() -> Result<(), DbErr> { use rust_decimal_macros::dec; let ctx = TestContext::new("transaction_with_active_model_behaviour_test").await; + create_tables(&ctx.db).await?; if let Ok(txn) = ctx.db.begin().await { assert_eq!( From 23155dced1131c1d2abb8ec10559217ef07a2f3c Mon Sep 17 00:00:00 2001 From: Billy Chan Date: Fri, 15 Oct 2021 18:04:49 +0800 Subject: [PATCH 5/6] Fixup --- tests/transaction_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/transaction_tests.rs b/tests/transaction_tests.rs index 1bf4a241..7845843e 100644 --- a/tests/transaction_tests.rs +++ b/tests/transaction_tests.rs @@ -298,7 +298,7 @@ pub async fn transaction_closure_commit() -> Result<(), DbErr> { feature = "sqlx-postgres" ))] pub async fn transaction_closure_rollback() -> Result<(), DbErr> { - let ctx = TestContext::new("transaction_closure_commit_test").await; + let ctx = TestContext::new("transaction_closure_rollback_test").await; create_tables(&ctx.db).await?; assert_eq!(bakery::Entity::find().all(&ctx.db).await?.len(), 0); From 351a78c7842ac19a2809fbfa7765a2a3bce7522a Mon Sep 17 00:00:00 2001 From: Billy Chan Date: Fri, 15 Oct 2021 22:48:49 +0800 Subject: [PATCH 6/6] Try as_ref --- tests/common/bakery_chain/cake.rs | 54 +++++++++---------------------- 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/tests/common/bakery_chain/cake.rs b/tests/common/bakery_chain/cake.rs index cdae08af..0ecaf5de 100644 --- a/tests/common/bakery_chain/cake.rs +++ b/tests/common/bakery_chain/cake.rs @@ -60,65 +60,43 @@ impl ActiveModelBehavior for ActiveModel { fn before_save(self, insert: bool) -> Result { use rust_decimal_macros::dec; - if let Some(price) = self.price.clone().take() { - if price == dec!(0) { - Err(DbErr::Custom(format!( - "[before_save] Invalid Price, insert: {}", - insert - ))) - } else { - Ok(self) - } - } else { + if self.price.as_ref() == &dec!(0) { Err(DbErr::Custom(format!( - "[before_save] Price must be set, insert: {}", + "[before_save] Invalid Price, insert: {}", insert ))) + } else { + Ok(self) } } fn after_save(self, insert: bool) -> Result { use rust_decimal_macros::dec; - if let Some(price) = dbg!(self.price.clone()).take() { - if price < dec!(0) { - Err(DbErr::Custom(format!( - "[after_save] Invalid Price, insert: {}", - insert - ))) - } else { - Ok(self) - } - } else { + if self.price.as_ref() < &dec!(0) { Err(DbErr::Custom(format!( - "[after_save] Price must be set, insert: {}", + "[after_save] Invalid Price, insert: {}", insert ))) + } else { + Ok(self) } } fn before_delete(self) -> Result { - if let Some(name) = self.name.clone().take() { - if name.contains("(err_on_before_delete)") { - Err(DbErr::Custom( - "[before_delete] Cannot be deleted".to_owned(), - )) - } else { - Ok(self) - } + if self.name.as_ref().contains("(err_on_before_delete)") { + Err(DbErr::Custom( + "[before_delete] Cannot be deleted".to_owned(), + )) } else { - Err(DbErr::Custom("[before_delete] Name must be set".to_owned())) + Ok(self) } } fn after_delete(self) -> Result { - if let Some(name) = self.name.clone().take() { - if name.contains("(err_on_after_delete)") { - Err(DbErr::Custom("[after_delete] Cannot be deleted".to_owned())) - } else { - Ok(self) - } + if self.name.as_ref().contains("(err_on_after_delete)") { + Err(DbErr::Custom("[after_delete] Cannot be deleted".to_owned())) } else { - Err(DbErr::Custom("[after_delete] Name must be set".to_owned())) + Ok(self) } } }