/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

`sqlite::Statement::step` can panic #1357

Closed
sgrif opened this Issue Dec 6, 2017 · 0 comments

Comments

Assignees

@sgrif sgrif

Labels
bug
Projects
None yet
Milestone
  1.0
1 participant
@sgrif
@sgrif
Member

sgrif commented Dec 6, 2017

We need to adjust this method to return an error. SQLITE_BUSY is a reasonably common, expected return value here. This is a release blocker.

@sgrif sgrif added the bug label Dec 6, 2017

@sgrif sgrif added this to the 1.0 milestone Dec 6, 2017

@sgrif sgrif self-assigned this Dec 6, 2017

sgrif added a commit that referenced this issue Dec 12, 2017

@sgrif
Don't panic in `SqliteStatement::step` 
I'm not sure how this ever got in there. Diesel ever panicking is
unacceptable, and we need to just return the error here.

The reason we've never run into this is that this function is only
called from queries with a return value. Previously `.execute` went
through a different code path, which did return an error.

Since SQLite doesn't support a RETURNING keyword, and select statements
should never be able to produce an error in Diesel (I'm not even sure
how we would make a select statement that errors when executed, but
prepares fine), this went unnoticed.

However, there's an additional return value that we *can* get from
select statements: `SQLITE_BUSY`. This is not really an error, but we
have no choice other than to treat it as such. I'm not sure if SQLite
actually gives us an error code here, and I'm not sure how to test it.
Either way we should give this a variant in `DatabaseErrorKind` in 1.1.

Fixes #1357.
a86ce2f

@sgrif sgrif referenced this issue Dec 12, 2017

Merged

Don't panic in `SqliteStatement::step` #1376

@sgrif sgrif closed this in #1376 Dec 12, 2017

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