Merge pull request #1403 from SeaQL/issues/1357

Don't call last_insert_id if not needed

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

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 {}

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(())
+}

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}")]
+    #[error("None of the records are being inserted")]
-    RecordNotInserted(String),
+    RecordNotInserted,
 }
 
 /// Runtime error

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

src/executor/insert.rs

@@ -137,29 +137,37 @@ where
 {
     type PrimaryKey<A> = <<A as ActiveModelTrait>::Entity as EntityTrait>::PrimaryKey;
     type ValueTypeOf<A> = <PrimaryKey<A> as PrimaryKeyTrait>::ValueType;
-    let last_insert_id_opt = match db.support_returning() {
+
-        true => {
+    let last_insert_id = match (primary_key, db.support_returning()) {
+        (Some(value_tuple), _) => {
+            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),
+            };
             let cols = PrimaryKey::<A>::iter()
                 .map(|col| col.to_string())
                 .collect::<Vec<_>>();
-            let rows = db.query_all(statement).await?;
+            row.try_get_many("", cols.as_ref())
-            let res = rows.last().ok_or_else(|| {
+                .map_err(|_| DbErr::UnpackInsertId)?
-                DbErr::RecordNotInserted("None of the records are being inserted".to_owned())
-            })?;
-            res.try_get_many("", cols.as_ref()).ok()
         }
-        false => {
+        (None, false) => {
-            let last_insert_id = db.execute(statement).await?.last_insert_id();
+            let res = db.execute(statement).await?;
-            ValueTypeOf::<A>::try_from_u64(last_insert_id).ok()
+            if res.rows_affected() == 0 {
+                return Err(DbErr::RecordNotInserted);
+            }
+            let last_insert_id = res.last_insert_id();
+            ValueTypeOf::<A>::try_from_u64(last_insert_id).map_err(|_| DbErr::UnpackInsertId)?
         }
     };
-    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),
-        },
-    };
     Ok(InsertResult { last_insert_id })
 }
 

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!(

tests/upsert_tests.rs

@@ -54,12 +54,7 @@ pub async fn create_insert_default(db: &DatabaseConnection) -> Result<(), DbErr>
     .exec(db)
     .await;
 
-    assert_eq!(
+    assert_eq!(res.err(), Some(DbErr::RecordNotInserted));
-        res.err(),
-        Some(DbErr::RecordNotInserted(
-            "None of the records are being inserted".to_owned()
-        ))
-    );
 
     Ok(())
 }