/diesel

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Insert is crashing while trying to insert an empty vector #384

Closed
weiznich opened this Issue Jul 29, 2016 · 5 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  0.9
3 participants
@weiznich @killercup @sgrif
@weiznich
Contributor

weiznich commented Jul 29, 2016

While trying to insert a dynamically generated vector of values diesel is crashing. It turns out the vector was empty in this special case.
I expect insert to handle the case of an empty vector and simply do nothing.

Backtrace:

thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0', /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/persistable.rs:75
stack backtrace:
   1:     0x55be81a907af - std::sys::backtrace::tracing::imp::write::h6528da8103c51ab9
   2:     0x55be81a940cb - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hbe741a5cc3c49508
   3:     0x55be81a93d4f - std::panicking::default_hook::he0146e6a74621cb4
   4:     0x55be81a84eae - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
   5:     0x55be81a94311 - std::panicking::begin_panic::he426e15a3766089a
   6:     0x55be81a8597a - std::panicking::begin_panic_fmt::hdddb415186c241e7
   7:     0x55be81a942ae - rust_begin_unwind
   8:     0x55be81aca0bf - core::panicking::panic_fmt::hf4e16cb7f0d41a25
   9:     0x55be81aca232 - core::panicking::panic_bounds_check::h14f942e6ac026712
  10:     0x55be816c5a4b - _<diesel..persistable..BatchInsertValues<'a, T, U, DB> as diesel..persistable..InsertValues<DB>>::column_names::h03daedf58da373b5
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/persistable.rs:75
  11:     0x55be816c56b9 - _<diesel..query_builder..insert_statement..InsertStatement<T, U, Op> as diesel..query_builder..QueryFragment<DB>>::to_sql::hcb041fa647432d9e
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/query_builder/insert_statement.rs:57
  12:     0x55be816c1ca5 - diesel::pg::connection::PgConnection::prepare_query::h6125a754ac0ff8c3
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/pg/connection/mod.rs:149
  13:     0x55be816c1437 - _<diesel..pg..connection..PgConnection as diesel..connection..Connection>::execute_returning_count::h1ec38088d0b52120
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/pg/connection/mod.rs:80
  14:     0x55be816c13e3 - diesel::query_dsl::load_dsl::ExecuteDsl::execute::h9f4cf5b34b6be13f
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/query_dsl/load_dsl.rs:69
...
@killercup

This comment has been minimized.

Sign in to view
Member

killercup commented Jul 29, 2016 •
edited

I can reproduce this with:

// diesel_tests/tests/insert.rs
#[test]
fn insert_records_empty_array() {
    use schema::users::table as users;
    let connection = connection();
    let new_users: &[NewUser] = &[];

    batch_insert(new_users, users, &connection);
    let actual_users: Vec<User> = users.load::<User>(&connection).unwrap();

    let expected_users: Vec<User> = vec![];
    assert_eq!(expected_users, actual_users);
}

This fails only with Postgres, as SQLite inserts multiple records one at a time in a simple for loop (that iterates zero times in this case).

Edit: @sgrif I think I have an idea how to prevent this. Let me fiddle with it a bit, I'll send a PR. (No promise that it'll be elegant, though.)

killercup added a commit to killercup/diesel that referenced this issue Jul 29, 2016

@killercup
Allow inserting empty slices 
Handle the case where an empty slice is to be inserted into a table. The
expected behaviour is to just skip the execution of the query. This
crashed previously when trying to get the column names of the fields.

Fixes diesel-rs#384
de0e5a5

@killercup killercup referenced this issue Jul 29, 2016

Closed

Allow inserting empty slices #385

killercup added a commit to killercup/diesel that referenced this issue Aug 1, 2016

@killercup
Handle inserting empty slices in a nicer way 
Fixes diesel-rs#384
090c936
@weiznich

This comment has been minimized.

Sign in to view
Contributor

weiznich commented Sep 5, 2016

The same is happening for eq_any in select.

#[test]
fn select_in_empty_array() {
    use schema::users::dsl::*;
    let connection = connection();
    let user_ids:Vec<i32> = Vec::new();

    let actual_users: Vec<User> = users
        .filter(id.eq_any(&user_ids))
        .load::<User>(&connection)
        .unwrap();

    let expected_users: Vec<User> = vec![];
    assert_eq!(expected_users, actual_users);
}

Backtrace:

thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', ../src/libcollections/vec.rs:1167
stack backtrace:
   1:     0x564915aef2bf - std::sys::backtrace::tracing::imp::write::h6528da8103c51ab9
   2:     0x564915af39fb - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hbe741a5cc3c49508
   3:     0x564915af367f - std::panicking::default_hook::he0146e6a74621cb4
   4:     0x564915ae286e - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
   5:     0x564915af3c41 - std::panicking::begin_panic::he426e15a3766089a
   6:     0x564915ae3dea - std::panicking::begin_panic_fmt::hdddb415186c241e7
   7:     0x564915af3bde - rust_begin_unwind
   8:     0x564915b297df - core::panicking::panic_fmt::hf4e16cb7f0d41a25
   9:     0x564915b29952 - core::panicking::panic_bounds_check::h14f942e6ac026712
  10:     0x5649158886af - _<collections..vec..Vec<T> as core..ops..Index<usize>>::index::h4863830f0426cb69
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libcollections/vec.rs:1167
  11:     0x564915888206 - _<diesel..expression..array_comparison..Many<T> as diesel..query_builder..QueryFragment<DB>>::to_sql::h04ea5398a50d0208
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/expression/array_comparison.rs:122
  12:     0x56491594b58c - _<diesel..expression..array_comparison..In<T, U> as diesel..query_builder..QueryFragment<DB>>::to_sql::hb077a8a5ee258ebd
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/expression/array_comparison.rs:52
  13:     0x56491594b093 - _<diesel..expression..predicates..And<T, U> as diesel..query_builder..QueryFragment<DB>>::to_sql::h92cd07251e370466
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/expression/predicates.rs:55
  14:     0x56491594aef8 - _<diesel..query_builder..where_clause..WhereClause<Expr> as diesel..query_builder..QueryFragment<DB>>::to_sql::he2873b9c28d1257d
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/query_builder/where_clause.rs:59
  15:     0x564915949223 - _<diesel..query_builder..select_statement..SelectStatement<ST, S, F, D, W, O, L, Of, G> as diesel..query_builder..QueryFragment<DB>>::to_sql::h8f51b06912f2f50b
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/query_builder/select_statement/mod.rs:160
  16:     0x564915943b25 - diesel::pg::connection::PgConnection::prepare_query::h43506e9ca901658b
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/pg/connection/mod.rs:149
  17:     0x564915943268 - _<diesel..pg..connection..PgConnection as diesel..connection..Connection>::query_all::hacdd6d89b0f35ea9
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/pg/connection/mod.rs:72
  18:     0x564915943143 - _<T as diesel..query_dsl..load_dsl..LoadDsl<Conn>>::load::h27debfc27bb7977a
                        at /home/user/.cargo/git/checkouts/diesel-6e3331fb3b9331ec/master/diesel/src/query_dsl/load_dsl.rs:53

weiznich added a commit to weiznich/diesel that referenced this issue Oct 15, 2016

@weiznich
Check if a queryresult would be empty 
* do not execute such queries, because they are invalid, instead simply
return nothing

Fix diesel-rs#384
Verified
This commit was signed with a verified signature.
@weiznich weiznich Georg Semmler
GPG key ID: A87BCEE5205CE489 Learn about signing commits
017e2ea

@weiznich weiznich referenced this issue Oct 15, 2016

Closed

Fix #384 (Invalid query on empty insert/select in) #480

@sgrif sgrif added this to the 0.9 milestone Dec 3, 2016

sgrif added a commit that referenced this issue Dec 5, 2016

@sgrif
Unify `InsertStatement` and `InsertQuery` into a single struct 
I'm working on adding support for batch insert on SQLite, and fixing the
insert empty slice bug (#384). My approach is to override `ExecuteDsl`
and `LoadDsl` at the point where `InsertStatement` currently lives.
I expect that we'll have `into` perform some polymorphic dispatch to get
back either a `SingleInsertStatement` or `BatchInsertStatement`. If we
also have `InsertQuery` on top of that, the amount of redundant structs
and impls will get quite large. This change should make that refactoring
much easier.

I also took this opportunity to make sure our compile tests were up to
snuff around this, and made sure that the tests failed in the expected
fashion when I introduced some mistakes as part of this refactor.
e774363

@sgrif sgrif referenced this issue Dec 5, 2016

Merged

Unify `InsertStatement` and `InsertQuery` into a single struct #518

sgrif added a commit that referenced this issue Dec 5, 2016

@sgrif
Unify `InsertStatement` and `InsertQuery` into a single struct 
I'm working on adding support for batch insert on SQLite, and fixing the
insert empty slice bug (#384). My approach is to override `ExecuteDsl`
and `LoadDsl` at the point where `InsertStatement` currently lives.
I expect that we'll have `into` perform some polymorphic dispatch to get
back either a `SingleInsertStatement` or `BatchInsertStatement`. If we
also have `InsertQuery` on top of that, the amount of redundant structs
and impls will get quite large. This change should make that refactoring
much easier.

I also took this opportunity to make sure our compile tests were up to
snuff around this, and made sure that the tests failed in the expected
fashion when I introduced some mistakes as part of this refactor.
ae7451e

sgrif added a commit that referenced this issue Dec 5, 2016

@sgrif
Unify `InsertStatement` and `InsertQuery` into a single struct 
I'm working on adding support for batch insert on SQLite, and fixing the
insert empty slice bug (#384). My approach is to override `ExecuteDsl`
and `LoadDsl` at the point where `InsertStatement` currently lives.
I expect that we'll have `into` perform some polymorphic dispatch to get
back either a `SingleInsertStatement` or `BatchInsertStatement`. If we
also have `InsertQuery` on top of that, the amount of redundant structs
and impls will get quite large. This change should make that refactoring
much easier.

I also took this opportunity to make sure our compile tests were up to
snuff around this, and made sure that the tests failed in the expected
fashion when I introduced some mistakes as part of this refactor.
84304fc

sgrif added a commit that referenced this issue Dec 5, 2016

@sgrif
Unify `InsertStatement` and `InsertQuery` into a single struct 
I'm working on adding support for batch insert on SQLite, and fixing the
insert empty slice bug (#384). My approach is to override `ExecuteDsl`
and `LoadDsl` at the point where `InsertStatement` currently lives.
I expect that we'll have `into` perform some polymorphic dispatch to get
back either a `SingleInsertStatement` or `BatchInsertStatement`. If we
also have `InsertQuery` on top of that, the amount of redundant structs
and impls will get quite large. This change should make that refactoring
much easier.

I also took this opportunity to make sure our compile tests were up to
snuff around this, and made sure that the tests failed in the expected
fashion when I introduced some mistakes as part of this refactor.
a7b424c

sgrif added a commit that referenced this issue Dec 5, 2016

@sgrif
Support batch insert on SQLite, fix inserting empty slices 
Those who watched my recent stream will notice that this has exactly
none of the code from that stream. Once I decided that empty errors
should be an error case, I realized that the "noop query that is
successful but should not execute" only applies to inserts, so I wanted
to take an approach that only affected inserts. I realized that the
plumbing required for batch inserts on SQLite would allow me to fix
this, since if we have a hook to execute N queries, N can be 0.

So this adds support for batch insert on SQLite. When `.into()` is
called, the return type will now either be `InsertStatement` or
`BatchInsertStatement`. The latter hooks into `ExecuteDsl` and `LoadDsl`
to do separate queries on SQLite.

While there should techincally be a separate queries implementation of
`LoadDsl` as well, we don't support any backend that supports returning
clauses but not the default keyword. I don't think such a backend
exists, so I've left out that code path as it would be untested.

We can probably partially revert this commit to leave the constraints on
`ExecuteDsl` and `LoadDsl` alone once specialization is stable, but the
addition of the second parameter on `ExecuteDsl` will still be required
unless [RFC 1672](rust-lang/rfcs#1672) is
accepted.

Fixes #384
ed84eed

sgrif added a commit that referenced this issue Dec 5, 2016

@sgrif
Support batch insert on SQLite, fix inserting empty slices 
Those who watched my recent stream will notice that this has exactly
none of the code from that stream. Once I decided that empty errors
should be an error case, I realized that the "noop query that is
successful but should not execute" only applies to inserts, so I wanted
to take an approach that only affected inserts. I realized that the
plumbing required for batch inserts on SQLite would allow me to fix
this, since if we have a hook to execute N queries, N can be 0.

So this adds support for batch insert on SQLite. When `.into()` is
called, the return type will now either be `InsertStatement` or
`BatchInsertStatement`. The latter hooks into `ExecuteDsl` and `LoadDsl`
to do separate queries on SQLite.

While there should techincally be a separate queries implementation of
`LoadDsl` as well, we don't support any backend that supports returning
clauses but not the default keyword. I don't think such a backend
exists, so I've left out that code path as it would be untested.

We can probably partially revert this commit to leave the constraints on
`ExecuteDsl` and `LoadDsl` alone once specialization is stable, but the
addition of the second parameter on `ExecuteDsl` will still be required
unless [RFC 1672](rust-lang/rfcs#1672) is
accepted.

The default type parameters on `IntoInsertStatement` are because
`NoReturningClause` is a voldemort type outside of the diesel crate, and
we're still needing to reference it in the `batch_insert` function in
our tests. That function can be removed with this feature, but it'll be
a large amount of churn that I want to commit separately. Once that's
done, the default types can be removed.

Fixes #384
8ec05c1

@sgrif sgrif referenced this issue Dec 5, 2016

Merged

Support batch insert on SQLite, fix inserting empty slices #519

@sgrif sgrif closed this in #519 Dec 5, 2016

@weiznich

This comment has been minimized.

Sign in to view
Contributor

weiznich commented Dec 5, 2016 •
edited

#519 fixes only empty insert statements.
As my second comment states, there is a similar issue with select in statements.
(Side node: #480 fixed this)
@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Dec 5, 2016

Yup, I will have a fix for those shortly
@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Dec 5, 2016

Opened #520 to track just in case

@sgrif sgrif referenced this issue Mar 13, 2017

Merged

Do less work in the inner loop of `assign_commits` #82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment