From e5b69ebb73c1502ede9c825eb7024cff1d5bbefc Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Wed, 18 Jan 2023 19:06:06 +0800 Subject: [PATCH 1/6] Don't call last_insert_id if not needed --- src/executor/insert.rs | 65 ++++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/src/executor/insert.rs b/src/executor/insert.rs index 796cd362..9a686997 100644 --- a/src/executor/insert.rs +++ b/src/executor/insert.rs @@ -1,6 +1,7 @@ use crate::{ - error::*, ActiveModelTrait, ColumnTrait, ConnectionTrait, EntityTrait, Insert, IntoActiveModel, - Iterable, PrimaryKeyTrait, SelectModel, SelectorRaw, Statement, TryFromU64, + error::*, ActiveModelTrait, ColumnTrait, ConnectionTrait, EntityTrait, ExecResult, Insert, + IntoActiveModel, Iterable, PrimaryKeyTrait, QueryResult, SelectModel, SelectorRaw, Statement, + TryFromU64, }; use sea_query::{Expr, FromValueTuple, Iden, InsertStatement, IntoColumnRef, Query, ValueTuple}; use std::{future::Future, marker::PhantomData}; @@ -137,29 +138,49 @@ where { type PrimaryKey = <::Entity as EntityTrait>::PrimaryKey; type ValueTypeOf = as PrimaryKeyTrait>::ValueType; - let last_insert_id_opt = match db.support_returning() { - true => { - let cols = PrimaryKey::::iter() - .map(|col| col.to_string()) - .collect::>(); - let rows = db.query_all(statement).await?; - let res = rows.last().ok_or_else(|| { - DbErr::RecordNotInserted("None of the records are being inserted".to_owned()) - })?; - res.try_get_many("", cols.as_ref()).ok() - } - false => { - let last_insert_id = db.execute(statement).await?.last_insert_id(); - ValueTypeOf::::try_from_u64(last_insert_id).ok() + + enum QueryOrExecResult { + Query(QueryResult), + Exec(ExecResult), + } + + let insert_result = if db.support_returning() { + let mut rows = db.query_all(statement).await?; + if rows.is_empty() { + return Err(DbErr::RecordNotInserted( + "None of the records are being inserted".to_owned(), + )); } + QueryOrExecResult::Query(rows.remove(rows.len() - 1)) + } else { + QueryOrExecResult::Exec(db.execute(statement).await?) }; + let last_insert_id = match primary_key { - Some(value_tuple) => FromValueTuple::from_value_tuple(value_tuple), - None => match last_insert_id_opt { - Some(last_insert_id) => last_insert_id, - None => return Err(DbErr::UnpackInsertId), - }, - }; + Some(value_tuple) => Ok(FromValueTuple::from_value_tuple(value_tuple)), + None => { + if db.support_returning() { + match insert_result { + QueryOrExecResult::Query(row) => { + let cols = PrimaryKey::::iter() + .map(|col| col.to_string()) + .collect::>(); + row.try_get_many("", cols.as_ref()) + } + _ => unreachable!(), + } + } else { + match insert_result { + QueryOrExecResult::Exec(res) => { + let last_insert_id = res.last_insert_id(); + ValueTypeOf::::try_from_u64(last_insert_id) + } + _ => unreachable!(), + } + } + } + } + .map_err(|_| DbErr::UnpackInsertId)?; Ok(InsertResult { last_insert_id }) } From f50dc1dd1c6eefa464b6fbc3bdf622281a8da564 Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Wed, 18 Jan 2023 19:23:29 +0800 Subject: [PATCH 2/6] [issues] test case --- issues/1357/Cargo.toml | 18 +++++++++++++++++ issues/1357/src/entity.rs | 14 +++++++++++++ issues/1357/src/main.rs | 42 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 issues/1357/Cargo.toml create mode 100644 issues/1357/src/entity.rs create mode 100644 issues/1357/src/main.rs diff --git a/issues/1357/Cargo.toml b/issues/1357/Cargo.toml new file mode 100644 index 00000000..55e7bfc3 --- /dev/null +++ b/issues/1357/Cargo.toml @@ -0,0 +1,18 @@ +[workspace] +# A separate workspace + +[package] +name = "sea-orm-issues-1357" +version = "0.1.0" +edition = "2021" +publish = false + +[dependencies] +anyhow = "1" +serde = "1" +tokio = { version = "1", features = ["rt", "rt-multi-thread", "macros"] } + +[dependencies.sea-orm] +path = "../../" +default-features = false +features = ["macros", "runtime-tokio-native-tls", "sqlx-sqlite"] diff --git a/issues/1357/src/entity.rs b/issues/1357/src/entity.rs new file mode 100644 index 00000000..3ad25f9f --- /dev/null +++ b/issues/1357/src/entity.rs @@ -0,0 +1,14 @@ +use sea_orm::entity::prelude::*; + +#[derive(Clone, Debug, PartialEq, DeriveEntityModel, Eq)] +#[sea_orm(table_name = "pool")] +pub struct Model { + #[sea_orm(primary_key, auto_increment = false)] + pub id: i64, + pub name: String, +} + +#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)] +pub enum Relation {} + +impl ActiveModelBehavior for ActiveModel {} diff --git a/issues/1357/src/main.rs b/issues/1357/src/main.rs new file mode 100644 index 00000000..f7488beb --- /dev/null +++ b/issues/1357/src/main.rs @@ -0,0 +1,42 @@ +use anyhow::Result; +use sea_orm::{ConnectionTrait, Database, EntityTrait, IntoActiveModel, Schema}; + +mod entity; + +use entity::*; + +#[tokio::main] +async fn main() -> Result<()> { + let db = Database::connect("sqlite::memory:").await.unwrap(); + + let builder = db.get_database_backend(); + let schema = Schema::new(builder); + let stmt = schema.create_table_from_entity(Entity); + db.execute(builder.build(&stmt)).await?; + + let model = Model { + id: 100, + name: "Hello".to_owned(), + }; + + let res = Entity::insert(model.clone().into_active_model()) + .exec(&db) + .await?; + + assert_eq!(Entity::find().one(&db).await?, Some(model.clone())); + assert_eq!(res.last_insert_id, model.id); + + let model = Model { + id: -10, + name: "World".to_owned(), + }; + + let res = Entity::insert(model.clone().into_active_model()) + .exec(&db) + .await?; + + assert_eq!(Entity::find().one(&db).await?, Some(model.clone())); + assert_eq!(res.last_insert_id, model.id); + + Ok(()) +} From 07d5f781ca4ceb5158f94f552c8ef2113f98a132 Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Wed, 18 Jan 2023 20:07:42 +0800 Subject: [PATCH 3/6] Refactor --- src/executor/insert.rs | 64 +++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/src/executor/insert.rs b/src/executor/insert.rs index 9a686997..6d7ab426 100644 --- a/src/executor/insert.rs +++ b/src/executor/insert.rs @@ -1,7 +1,6 @@ use crate::{ - error::*, ActiveModelTrait, ColumnTrait, ConnectionTrait, EntityTrait, ExecResult, Insert, - IntoActiveModel, Iterable, PrimaryKeyTrait, QueryResult, SelectModel, SelectorRaw, Statement, - TryFromU64, + error::*, ActiveModelTrait, ColumnTrait, ConnectionTrait, EntityTrait, Insert, IntoActiveModel, + Iterable, PrimaryKeyTrait, SelectModel, SelectorRaw, Statement, TryFromU64, }; use sea_query::{Expr, FromValueTuple, Iden, InsertStatement, IntoColumnRef, Query, ValueTuple}; use std::{future::Future, marker::PhantomData}; @@ -139,48 +138,37 @@ where type PrimaryKey = <::Entity as EntityTrait>::PrimaryKey; type ValueTypeOf = as PrimaryKeyTrait>::ValueType; - enum QueryOrExecResult { - Query(QueryResult), - Exec(ExecResult), - } - - let insert_result = if db.support_returning() { + let last_insert_id = if db.support_returning() { let mut rows = db.query_all(statement).await?; - if rows.is_empty() { - return Err(DbErr::RecordNotInserted( - "None of the records are being inserted".to_owned(), - )); + let row = match rows.pop() { + Some(row) => row, + None => { + return Err(DbErr::RecordNotInserted( + "None of the records are being inserted".to_owned(), + )) + } + }; + match primary_key { + Some(value_tuple) => Ok(FromValueTuple::from_value_tuple(value_tuple)), + None => { + let cols = PrimaryKey::::iter() + .map(|col| col.to_string()) + .collect::>(); + row.try_get_many("", cols.as_ref()) + } } - QueryOrExecResult::Query(rows.remove(rows.len() - 1)) } else { - QueryOrExecResult::Exec(db.execute(statement).await?) - }; - - let last_insert_id = match primary_key { - Some(value_tuple) => Ok(FromValueTuple::from_value_tuple(value_tuple)), - None => { - if db.support_returning() { - match insert_result { - QueryOrExecResult::Query(row) => { - let cols = PrimaryKey::::iter() - .map(|col| col.to_string()) - .collect::>(); - row.try_get_many("", cols.as_ref()) - } - _ => unreachable!(), - } - } else { - match insert_result { - QueryOrExecResult::Exec(res) => { - let last_insert_id = res.last_insert_id(); - ValueTypeOf::::try_from_u64(last_insert_id) - } - _ => unreachable!(), - } + let res = db.execute(statement).await?; + match primary_key { + Some(value_tuple) => Ok(FromValueTuple::from_value_tuple(value_tuple)), + None => { + let last_insert_id = res.last_insert_id(); + ValueTypeOf::::try_from_u64(last_insert_id) } } } .map_err(|_| DbErr::UnpackInsertId)?; + Ok(InsertResult { last_insert_id }) } From a465d1ebac7f72008e3e4451b1991b7041032854 Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Wed, 18 Jan 2023 20:30:15 +0800 Subject: [PATCH 4/6] Refactor 2 --- src/executor/insert.rs | 53 ++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/src/executor/insert.rs b/src/executor/insert.rs index 6d7ab426..882f3df6 100644 --- a/src/executor/insert.rs +++ b/src/executor/insert.rs @@ -138,36 +138,33 @@ where type PrimaryKey = <::Entity as EntityTrait>::PrimaryKey; type ValueTypeOf = as PrimaryKeyTrait>::ValueType; - let last_insert_id = if db.support_returning() { - let mut rows = db.query_all(statement).await?; - let row = match rows.pop() { - Some(row) => row, - None => { - return Err(DbErr::RecordNotInserted( - "None of the records are being inserted".to_owned(), - )) - } - }; - match primary_key { - Some(value_tuple) => Ok(FromValueTuple::from_value_tuple(value_tuple)), - None => { - let cols = PrimaryKey::::iter() - .map(|col| col.to_string()) - .collect::>(); - row.try_get_many("", cols.as_ref()) - } + let last_insert_id = match (primary_key, db.support_returning()) { + (Some(value_tuple), _) => { + db.execute(statement).await?; + FromValueTuple::from_value_tuple(value_tuple) } - } else { - let res = db.execute(statement).await?; - match primary_key { - Some(value_tuple) => Ok(FromValueTuple::from_value_tuple(value_tuple)), - None => { - let last_insert_id = res.last_insert_id(); - ValueTypeOf::::try_from_u64(last_insert_id) - } + (None, true) => { + let mut rows = db.query_all(statement).await?; + let row = match rows.pop() { + Some(row) => row, + None => { + return Err(DbErr::RecordNotInserted( + "None of the records are being inserted".to_owned(), + )) + } + }; + let cols = PrimaryKey::::iter() + .map(|col| col.to_string()) + .collect::>(); + row.try_get_many("", cols.as_ref()) + .map_err(|_| DbErr::UnpackInsertId)? } - } - .map_err(|_| DbErr::UnpackInsertId)?; + (None, false) => { + let res = db.execute(statement).await?; + let last_insert_id = res.last_insert_id(); + ValueTypeOf::::try_from_u64(last_insert_id).map_err(|_| DbErr::UnpackInsertId)? + } + }; Ok(InsertResult { last_insert_id }) } From 03207fbda9fc9eb7239d6c558d6d031539fc7730 Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Wed, 18 Jan 2023 21:11:35 +0800 Subject: [PATCH 5/6] Use `rows_affected` when DB does not support returning --- src/error.rs | 4 ++-- src/executor/execute.rs | 2 +- src/executor/insert.rs | 14 ++++++++------ tests/upsert_tests.rs | 7 +------ 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/error.rs b/src/error.rs index 7988bd1b..2ed87ffd 100644 --- a/src/error.rs +++ b/src/error.rs @@ -58,8 +58,8 @@ pub enum DbErr { /// None of the records are being inserted into the database, /// if you insert with upsert expression that means /// all of them conflict with existing records in the database - #[error("RecordNotInserted Error: {0}")] - RecordNotInserted(String), + #[error("None of the records are being inserted")] + RecordNotInserted, } /// Runtime error diff --git a/src/executor/execute.rs b/src/executor/execute.rs index 3da4ec8a..f3a7150d 100644 --- a/src/executor/execute.rs +++ b/src/executor/execute.rs @@ -52,7 +52,7 @@ impl ExecResult { } } - /// Get the number of rows affedted by the operation + /// Get the number of rows affected by the operation pub fn rows_affected(&self) -> u64 { match &self.result { #[cfg(feature = "sqlx-mysql")] diff --git a/src/executor/insert.rs b/src/executor/insert.rs index 882f3df6..c4c988ba 100644 --- a/src/executor/insert.rs +++ b/src/executor/insert.rs @@ -140,18 +140,17 @@ where let last_insert_id = match (primary_key, db.support_returning()) { (Some(value_tuple), _) => { - db.execute(statement).await?; + let res = db.execute(statement).await?; + if res.rows_affected() == 0 { + return Err(DbErr::RecordNotInserted); + } FromValueTuple::from_value_tuple(value_tuple) } (None, true) => { let mut rows = db.query_all(statement).await?; let row = match rows.pop() { Some(row) => row, - None => { - return Err(DbErr::RecordNotInserted( - "None of the records are being inserted".to_owned(), - )) - } + None => return Err(DbErr::RecordNotInserted), }; let cols = PrimaryKey::::iter() .map(|col| col.to_string()) @@ -161,6 +160,9 @@ where } (None, false) => { let res = db.execute(statement).await?; + if res.rows_affected() == 0 { + return Err(DbErr::RecordNotInserted); + } let last_insert_id = res.last_insert_id(); ValueTypeOf::::try_from_u64(last_insert_id).map_err(|_| DbErr::UnpackInsertId)? } diff --git a/tests/upsert_tests.rs b/tests/upsert_tests.rs index ad874e79..748d5b0a 100644 --- a/tests/upsert_tests.rs +++ b/tests/upsert_tests.rs @@ -54,12 +54,7 @@ pub async fn create_insert_default(db: &DatabaseConnection) -> Result<(), DbErr> .exec(db) .await; - assert_eq!( - res.err(), - Some(DbErr::RecordNotInserted( - "None of the records are being inserted".to_owned() - )) - ); + assert_eq!(res.err(), Some(DbErr::RecordNotInserted)); Ok(()) } From 5cdcdbfc1c051b6105a6ecd50dd10c424040705c Mon Sep 17 00:00:00 2001 From: Chris Tsang Date: Thu, 19 Jan 2023 01:23:21 +0800 Subject: [PATCH 6/6] Add testcase --- tests/string_primary_key_tests.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/string_primary_key_tests.rs b/tests/string_primary_key_tests.rs index 94d1ae55..9ed4df47 100644 --- a/tests/string_primary_key_tests.rs +++ b/tests/string_primary_key_tests.rs @@ -2,7 +2,7 @@ pub mod common; pub use common::{features::*, setup::*, TestContext}; use pretty_assertions::assert_eq; -use sea_orm::{entity::prelude::*, entity::*, DatabaseConnection}; +use sea_orm::{entity::prelude::*, entity::*, sea_query::OnConflict, DatabaseConnection}; use serde_json::json; #[sea_orm_macros::test] @@ -30,7 +30,7 @@ pub async fn insert_and_delete_repository(db: &DatabaseConnection) -> Result<(), } .into_active_model(); - let result = repository.insert(db).await?; + let result = repository.clone().insert(db).await?; assert_eq!( result, @@ -42,6 +42,17 @@ pub async fn insert_and_delete_repository(db: &DatabaseConnection) -> Result<(), } ); + #[cfg(any(feature = "sqlx-sqlite", feature = "sqlx-postgres"))] + { + let err = Repository::insert(repository) + // MySQL does not support DO NOTHING, we might workaround that later + .on_conflict(OnConflict::new().do_nothing().to_owned()) + .exec(db) + .await; + + assert_eq!(err.err(), Some(DbErr::RecordNotInserted)); + } + result.delete(db).await?; assert_eq!(