Panic in `insert_many` if `ActiveModels` have different columns set

[GeorgeHahn]

Description

insert_many panics with the message "columns mismatch" if the entities to be stored have values in a mix of columns.

Steps to Reproduce

See minimal reproduction below

Expected Behavior

This seems like it should be supported or explicitly disallowed.

Actual Behavior

Panic internal to the library

Reproduces How Often

100%

Versions

Tested with 0.10.7. Reproduces on Windows 10 and Ubuntu 20.04.

Additional Information

Minimal reproduction:

// Cargo.toml:
// [package]
// name = "sql-repr"
// version = "0.1.0"
// edition = "2021"
//
// [dependencies]
// sea-orm = { version = "0.10.7", default-features = false, features = [
//     "sqlx-sqlite",
//     "runtime-tokio-rustls",
//     "with-time",
// ] }
// sea-orm-migration = "0.10.7"
// tokio = { version = "1.23.0", features = ["full"] }
//
// [profile.dev]
// panic = "abort"

use migration::Migrator;
use sea_orm::*;
use sea_orm_migration::MigratorTrait;

mod entity {
    use super::*;
    pub mod varies {
        use super::*;

        #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, DeriveEntityModel)]
        #[sea_orm(table_name = "varies")]
        pub struct Model {
            #[sea_orm(primary_key)]
            pub id: i32,
            pub a: Option<i32>,
            pub b: Option<String>,
        }

        impl ActiveModelBehavior for ActiveModel {}

        #[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
        pub enum Relation {}
    }
}

mod migration {
    use sea_orm_migration::prelude::*;

    pub struct Migrator;
    #[async_trait::async_trait]
    impl MigratorTrait for Migrator {
        fn migrations() -> Vec<Box<dyn MigrationTrait>> {
            vec![Box::new(Create)]
        }
    }

    #[derive(DeriveMigrationName)]
    struct Create;

    #[async_trait::async_trait]
    impl MigrationTrait for Create {
        async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> {
            manager
                .create_table(
                    Table::create()
                        .table(Varies::Table)
                        .if_not_exists()
                        .col(
                            ColumnDef::new(Varies::Id)
                                .integer()
                                .not_null()
                                .auto_increment()
                                .primary_key(),
                        )
                        .col(ColumnDef::new(Varies::A).integer().null())
                        .col(ColumnDef::new(Varies::B).string().null())
                        .to_owned(),
                )
                .await?;
            Ok(())
        }

        async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> {
            todo!();
        }
    }

    #[derive(Iden)]
    enum Varies {
        Table,
        Id,
        A,
        B,
    }
}

#[tokio::main]
async fn main() {
    let db = Database::connect("sqlite::memory:").await.unwrap();
    Migrator::up(&db, None).await.unwrap();

    let multi = vec![
        entity::varies::ActiveModel {
            id: ActiveValue::NotSet,
            a: ActiveValue::Set(Some(100)),
            b: ActiveValue::NotSet,
        },
        entity::varies::ActiveModel {
            id: ActiveValue::NotSet,
            a: ActiveValue::NotSet,
            b: ActiveValue::Set(Some(String::from("word"))),
        },
    ];
    entity::varies::Entity::insert_many(multi) // <- panics internally
        .exec(&db)
        .await
        .unwrap();
}

[negezor]

I also encountered this, I think it would be better to add an explanation that you need to change the number of columns.

[tyt2y3]

I think there are two cases. If the missing columns are nullable, SeaORM might be able to stuff in NULLs. Otherwise we should raise an error instead of panicking. The panicking comes from SeaQuery (I bet), would appreciate if @GeorgeHahn can provide the stack trace.

[Diwakar-Gupta]

panicking is raised from sea-orm here https://github.com/SeaQL/sea-orm/blob/e1297850ac7fe59f28d5273c88dddb8d2e1e4e36/src/query/insert.rs#L130-L146

My understanding from the code is underlying function pub fn add<M>(mut self, m: M) -> Self is called for each model and then function iterates for all the fields in that model. For the first model columns_empty is true and it sets which column has to be provided with true and false in self.columns. When function is called for other models it checks if it has different fields set and if then panic.

If we can convert this into a compilation error that would be great, but I doubt it can be done. Stuffing in NULLs will be an easy process, will require another struct level data member, but can resolve issues in limited cases only.

here's stack trace

thread 'main' panicked at 'columns mismatch', /Users/diwakargupta/Workspace/bakery-backend/sea-orm/src/query/insert.rs:137:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/panicking.rs:64:14
   2: sea_orm::query::insert::Insert<A>::add
             at ./sea-orm/src/query/insert.rs:137:17
   3: sea_orm::query::insert::Insert<A>::add_many
             at ./sea-orm/src/query/insert.rs:159:20
   4: sea_orm::query::insert::Insert<A>::many
             at ./sea-orm/src/query/insert.rs:108:9
   5: sea_orm::entity::base_entity::EntityTrait::insert_many
             at ./sea-orm/src/entity/base_entity.rs:467:9
   6: bakery_backend::insert_my_table::{{closure}}
             at ./src/main.rs:111:25
   7: bakery_backend::run::{{closure}}
             at ./src/main.rs:50:25
   8: futures_executor::local_pool::block_on::{{closure}}
             at /Users/diwakargupta/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.25/src/local_pool.rs:317:23
   9: futures_executor::local_pool::run_executor::{{closure}}
             at /Users/diwakargupta/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.25/src/local_pool.rs:90:37
  10: std::thread::local::LocalKey<T>::try_with
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/thread/local.rs:446:16
  11: std::thread::local::LocalKey<T>::with
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/thread/local.rs:422:9
  12: futures_executor::local_pool::run_executor
             at /Users/diwakargupta/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.25/src/local_pool.rs:86:5
  13: futures_executor::local_pool::block_on
             at /Users/diwakargupta/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.25/src/local_pool.rs:317:5
  14: bakery_backend::main
             at ./src/main.rs:61:23
  15: core::ops::function::FnOnce::call_once
             at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ops/function.rs:507:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[tyt2y3]

I see. Then it is fixable to various degree; at least we should make it not panic.

[Diwakar-Gupta]

Will returning a Result work, I guess this will change return type of Entity::insert_many and may be some other function, which can break backward compatibility?

[veeti-k]

Hi! Stumbled into this. Is there a workaround to be able to do insert_many without panics?

[jryio]

Also bitten by this. I've got a simple loop which processes some data and inserts into a sqlite database.

The iterator of ActiveModel will have an assortment of ActiveValue::Set(T) and ActiveValue::NotSet.

A lack of panics would be appreciated.

[tyt2y3]

Yes, instead of panicking, we can build the query in a second pass. I think we can fix this in the next release. PR would be welcome meanwhile. The most straight-forward implementation is to fill in missing values with DEFAULT, but it depends on the database and schema, so it may require some investigation.

[belyakov-am]

I've recently bumped into this problem. While it's not clear how sea-orm should properly solve this issue, I believe it might be useful to mention this behaviour in the docs.