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

sea-orm/src/query/insert.rs

Lines 130 to 146 in e129785

	let columns_empty = self.columns.is_empty();
	for (idx, col) in <A::Entity as EntityTrait>::Column::iter().enumerate() {
	let av = am.take(col);
	let av_has_val = av.is_set() || av.is_unchanged();
	if columns_empty {
	self.columns.push(av_has_val);
	} else if self.columns[idx] != av_has_val {
	panic!("columns mismatch");
	}
	match av {
	ActiveValue::Set(value) | ActiveValue::Unchanged(value) => {
	columns.push(col);
	values.push(col.save_as(Expr::val(value)));
	}
	ActiveValue::NotSet => {}
	}
	}

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?

[kveeti]

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.

[j-mendez]

@belyakov-am do you know how to trace the issue?

[CandleCandle]

Encountered this when using loco-rs/loco and the seeding of data feature.

I have a table with several nullable columns. Loco takes a yaml file as input and converts it to an insert_many call.

Sample failing yaml:

- id: 1
  something: some value
  nullable1: value
- id: 2
  something: some value
  nullable2: value

Observe that the entry at ID 2 does not set a value for nullable1, nor does the first entry set a value for nullable2.

When this is converted to an insert_many, the same error is generated.

There are two possible work-arounds:

1. Each yaml file to include empty values for unused fields:

- id: 1
  something: some value
  nullable1: value
  nullable2:
- id: 2
  something: some value
  nullable1:
  nullable2: value

2. group your inserts into separate files, one file per distinct set of nullable fields. This separates it into many calls to insert_many

- id: 1
  something: some value
  nullable1: value

- id: 2
  something: some value
  nullable2: value

[tyt2y3]

Dear all who are interested in this issue, I've made a fix in the PR 2433. It is slightly tricky, so I'd appreciate if someone can help reading / testing it!

@CandleCandle @belyakov-am @jryio

[j-mendez]

Dear all who are interested in this issue, I've made a fix in the PR 2433. It is slightly tricky, so I'd appreciate if someone can help reading / testing it!

@CandleCandle @belyakov-am @jryio

issue still occurs sea-orm-1.1.2

[tyt2y3]

we haven't released it yet, so to try out one has to point the dependency in cargo to something like:

sea-orm = { git = "https://github.com/SeaQL/sea-orm/", branch = "insert-no-panic"}

[CandleCandle]

- id: 1
  something: some value
  nullable1: value
- id: 2
  something: some value
  nullable2: value

It works as I expect using my sample data via loco-rs 0.9.0.

• Data is inserted to my table with null values
• No panic occurs

---

A work-around was added to loco-rs as loco-rs/loco#845 that is marked as part of milestone https://github.com/loco-rs/loco/milestone/21

Procedure

1. update cargo.toml to include the override
2. run cargo update
3. modify sample data to remove fields that are nullable and should have null values
4. run tests.

cargo.toml override

[patch.crates-io]
sea-orm = { git = "https://github.com/SeaQL/sea-orm/", branch = "insert-no-panic"  }

package.lock entries

[[package]]
name = "sea-orm"
version = "1.1.2"
source = "git+https://github.com/SeaQL/sea-orm/?branch=insert-no-panic#cd345512031ddf813361a0bd98501553f914b0f8"

...

[[package]]
name = "loco-rs"
version = "0.9.0"
source = "registry+https://github.com/rust-lang/crates.io-index"

[tyt2y3]

Thank you @CandleCandle for helping! it's been released in 1.1.3