on_empty_do_nothing() should be enabled by default

[zevweiss]

Description

The intuitive, obvious way to do a multi-record insert (foo::Entity::insert_many(iter).exec(...)) provides highly surprising behavior if the iterator passed to it is empty (inserting a single DEFAULT record instead of inserting nothing).

https://github.com/SeaQL/sea-orm/pull/1708 added .on_empty_do_nothing() to work around that problem and enable the more predictable behavior of an empty iterator resulting in nothing being inserted, but it seems quite regrettable (and highly counterintuitive IMO) for that to be opt-in instead of the default; without it most insert_many() calls likely end up as time bombs just waiting for an empty iterator to cause havoc.

I suppose it would technically be a backwards-incompatible change to make it the default and hence unfit for a minor or patch release, but it's too bad the 0.x->1.0 release didn't take the opportunity to make that change. Could it please be added to the agenda for a 2.0 release?

Versions

$ cargo tree | grep sea-
├── sea-orm v1.1.6
│   ├── sea-orm-macros v1.1.6 (proc-macro)
│   │   ├── sea-bae v0.2.1 (proc-macro)
│   ├── sea-query v0.32.2
│   │   ├── sea-query-derive v0.4.2 (proc-macro)
│   ├── sea-query-binder v0.7.0
│   │   ├── sea-query v0.32.2 (*)
├── sea-orm-migration v1.1.6
│   ├── sea-orm v1.1.6 (*)
│   ├── sea-orm-cli v1.1.6
│   │   ├── sea-schema v0.16.1
│   │   │   ├── sea-query v0.32.2 (*)
│   │   │   └── sea-schema-derive v0.3.0 (proc-macro)
│   ├── sea-schema v0.16.1 (*)
├── sea-query v0.32.2 (*)

DB: Postgres 16.4 OS: Linux

[Expurple]

Agree, it's very annoying

[Huliiiiii]

I think, for now, adding length checks to some methods that return a Vec would not cause breaking changes (e.g. methods on LoaderTrait, query_all), and would also reduce the amount of queries that don't return any data.

Currently, Inserter doesn't have the One and Many variants like the other methods, which means insert_many shares the same struct as insert(one).

If this is refactored into InsertOne and InsertMany, TryInsertResult::Empty will no longer be needed, as an empty Vec or None can be returned.

[tyt2y3]

I suppose it would technically be a backwards-incompatible change

yes you're right. it's unfortunate that the empty vec to DEFAULT feature was added before on empty do nothing.

I do think we should remove that, so empty vec will become an error as the default behaviour. this is a breaking change, but an error is better than having a different fallback behaviour that succeeds silently. you'd get angry users either way.

If this is refactored into InsertOne and InsertMany

this is a great idea. then we can separate the cases and handle differently. we'd error for insert one, but can do nothing on insert many

[zevweiss]

I do think we should remove that, so empty vec will become an error as the default behaviour.

Wait, what? That would just be replacing one surprising behavior with another. Vec::extend_from_slice() doesn't fail if you pass it an empty slice, it just adds zero elements, exactly as you'd expect (example). Similarly, successfully inserting nothing is IMO the only reasonable behavior for .insert_many() to provide when passed an empty iterator; anything else is a gratuitous trap just waiting to inflict bugs on unwary users.

I think the way to avoid angry users is to:

1. Change the default behavior to be equivalent to .on_empty_do_nothing()
2. Add a new .on_empty_insert_default() helper to allow opting in to the old behavior if that's actually desired for some reason
3. Make a new release with the major revision number bumped and an entry the compatibility section of the release notes advising auditing all uses of .insert_many() and either:
   • removing the now-redundant .on_empty_do_nothing(),
   • adding .on_empty_insert_default() if that's actually desired for some reason,
   • or leaving it as-is to accept the free bugfix.

Though to be perfectly honest I'm having a really hard time imagining a scenario in which the current behavior is in fact actually appropriate and hence that any change in application behavior caused by this would be anything but a free bugfix -- I'm going to go out on a limb and predict that .on_empty_insert_default() will have very few users.

[tyt2y3]

Change the default behavior to be equivalent to .on_empty_do_nothing()

the blocker for this is that the InsertResult struct cannot return a valid result when nothing is inserted.

pub struct InsertResult<A>
where
    A: ActiveModelTrait,
{
    pub last_insert_id: <PrimaryKey<A> as PrimaryKeyTrait>::ValueType,
}

we might be able to use Default::default as a dummy value, but it requires the user to explicitly check the value before using it, which is not ideal.

unless, we change the method signature of EntityTrait:

trait EntityTrait {
    fn insert_many<A, I>(models: I) -> TryInsert<A>
    where
        A: ActiveModelTrait<Entity = Self>,
        I: IntoIterator<Item = A>,
    {
        Insert::many(models).on_empty_do_nothing()
    }
}

but this is a breaking change

wdyt?

[Huliiiiii]

Considering the current issues and those mentioned in #2393 , I think it would be better to make last_insert_id optional and implement it in 2.0

[Expurple]

Sounds good. Added this to https://github.com/SeaQL/sea-orm/discussions/2548

[tyt2y3]

thank you all for the input. I've decided - let's fix it and break it.

[Expurple]

https://github.com/SeaQL/sea-orm/pull/2628 has been merged into master and will be released in 2.0.0