Inserting active models by `insert_many` with `on_conflict` and `do_nothing` panics if no rows are inserted.

[Karahik]

Description

insert_many with on_conflit and do_nothing panics if every given key conflicts with existing rows.

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/sea-orm-0.9.0/src/executor/insert.rs:121:54
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It seems that the code the error message indicates is trying to retrieve the last inserted row, but it could get nothing because insertion did not happen.

I'd expect this to do nothing (without panics).

Steps to Reproduce

This is what my example project looks like.

.
├── Cargo.lock
├── Cargo.toml
├── sql
│   └── 00000000000000_schema.sql
├── src
│   ├── main.rs
│   └── orm
│       ├── fruits.rs
│       ├── mod.rs
│       └── prelude.rs
└── target

Cargo.toml.

[package]
name = "sea-orm-issue"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
anyhow = "1.0.58"
chrono = { version = "0.4.19", features = ["serde"] }
sea-orm = { version = "0.9.0", features = ["sqlx-postgres", "runtime-tokio-rustls", "macros"] }
serde = { version = "1.0.139", features = ["derive"] }
serde_json = "1.0.82"
sqlx = { version = "0.6.0", features = ["runtime-tokio-rustls", "postgres", "chrono"] }
tokio = { version = "1.20.0", features = ["full"] }

This is the example code(main.rs).

mod orm;

use crate::orm::fruits;
use sea_orm::{sea_query::OnConflict, ActiveValue, ConnectOptions, Database, EntityTrait};

#[tokio::main]
async fn main() -> anyhow::Result<()> {
    let opt =
        ConnectOptions::new("postgres://postgres:postgres@localhost:5432/postgres".to_owned());

    let db = Database::connect(opt).await?;

    let fruits = vec![
        fruits::ActiveModel {
            name: ActiveValue::set("Orange".to_owned()),
            ..Default::default()
        },
        fruits::ActiveModel {
            name: ActiveValue::set("Apple".to_owned()),
            ..Default::default()
        },
    ];

    fruits::Entity::insert_many(fruits)
        .on_conflict(
            OnConflict::column(fruits::Column::Name)
                .do_nothing()
                .to_owned(),
        )
        .exec(&db)
        .await?;

    Ok(())
}

#[cfg(test)]
mod test {
    use super::*;
    #[test]
    fn test_fruit() -> anyhow::Result<()> {
        main()?;
        main()
    }
}

Here is the definition of the fruits table(./sql/00000000000000_schema.sql).

CREATE TABLE "fruits" (
  "name" TEXT NOT NULL PRIMARY KEY,
  "created_at" timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP,
  "updated_at" timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP
);

2. Run these commands.

sqlx migrate run --source ./sql
sea-orm-cli generate entity -o ./src/orm

3. Run insert_many in conjunction with on_conflict and do_nothing several times. In the examle above, the main function is called twice in the test function. So, you can test this by running cargo test.

Expected Behavior

When 'do_nothing' is used, insert_many ends successfully even if no rows are inserted.

Actual Behavior

It panics with the following error.

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/sea-orm-0.9.0/src/executor/insert.rs:121:54
note: run with `RUST_BACKTRACE=1` e

Reproduces How Often

Always

Versions

│   ├── sea-orm v0.9.0
│   │   ├── sea-orm-macros v0.9.0 (proc-macro)
│   │   ├── sea-query v0.26.0
│   │   │   ├── sea-query-derive v0.2.0 (proc-macro)
│   │   │   ├── sea-query-driver v0.2.0 (proc-macro)
│   │   ├── sea-strum v0.23.0
│   │   │   └── sea-strum_macros v0.23.0 (proc-macro)

[billy1624]

Hey @Karahik, welcome! This was the behaviour of PostgreSQL. If you perform the same insert twice with on conflict do nothing and returning. The second insert will do nothing and return nothing. In fact that it return nothing trigger the panic!

https://dbfiddle.uk/?rdbms=postgres_14&fiddle=cb7cf10c577779d92dc626869a79ea26

I think we should throw an error at the line below instead of panicking:

https://github.com/SeaQL/sea-orm/blob/1385b749cbf6425c62bf2c7cf060d655f1a649dc/src/executor/insert.rs#L121

[tyt2y3]

I agree.

[saintazunya]

I second this issue, also observed this behavior. And a temporary I found is that you can use .update_column(Column::ColumnName) instead of .do_nothing() to resolve.

[yulidai]

same problem.

It is better if it does not use unwrap and returns rows_effective instead of last_inserted_id

[billy1624]

Hey everyone, please check https://github.com/SeaQL/sea-orm/pull/1021, on PostgreSQL, inserting many records with upsert will throw DbErr::RecordNotInserted if none of the records are being inserted.

[Vaider7]

But why we should return an error if record wasn't inserted? I think if someone uses DO NOTHING statement, they understand it can return nothing. And I think it's better to return None variant and handle it. A quick example.

When a new user want to sign up, obviously he needs to create a username, and obviously the username should be unique

Usually the code looks like this:

let user = User::find()
        .filter(users::Column::Username.contains(payload.username))
        .one(&db)
        .await?;

if user.is_some() {
    return ServiceError::Conflit("User already exist");
}

users::ActiveModel {
    username: Set(payload.username),
    ..Default::default()
}
.save(&db)
.await?;

It takes two queries, but we can rewrite it with one via .do_nothing() if it returns Option<Model>:

let new_user = users::ActiveModel {
        username: Set(payload.username),
        ..Default::default()
    };

let user: Option<Model> = Insert::one(new_user)
        .on_conflict(OnConflict::column(users::Column::Username).do_nothing())
        .exec(&db)
        .await?;

if user.is_none() {
    return ServiceError::Conflit("Username already exist");
}

Ok(())

I think it's useful functionality. Ofc we can just handle DbErr::RecordNotInserted and get the same. But I think it doesn't look like an error (:

[PoOnesNerfect]

I ran into the same problem as well.

When RETURNING is inserted into all queries (when the db supports it), it's almost certain that this query will encounter panic when used with on_conflict( do nothing); and this behavior does not seem obvious to the users at first glance.

I also agree with @Vaider7 in that the expected return should be more of None or affected_rows = 0 than to return an Error, as the user does expect nothing to happen and is not exceptional case.

However, I do understand that it may be hard to make such behavioral exemptions to the codebase that is so generically crafted.

Thank you for creating a wonderful ORM library, I'm having a blast using it at work and with personal projects.

p.s. the PR looks like a clean and simple fix for the issue.

[0xpr03]

I'm also hitting this issue, without any insert-many. Simply by inserting an existing row with "do-nothing".

[baszalmstra]

Does https://github.com/SeaQL/sea-orm/pull/1208 provide a usable workaround for this issue?

[lucat1]

Hi, I've also been getting this in the lastest release candidate (0.11.0-rc.2). Has the fix been merged there yet?

[tyt2y3]

Yes, please double check your dependency tree