Skip to content
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

Crash unwrapping lastInsertRowid after insert when primary key may contain 0 #409

Closed
ewanmellor opened this issue Apr 15, 2016 · 1 comment

Comments

@ewanmellor
Copy link

This bug is similar to #235 and #383, but is not fixed by the changes in #383.

The crash is the same: Query.run(query: Insert) successfully runs the insert, but sqlite3_last_insert_rowid returns 0 afterwards, which means that Connection.lastInsertRowid returns nil, and Query.run crashes when it force-unwraps that nil.

    public func run(query: Insert) throws -> Int64 {
        let expression = query.expression
        return try sync {
            try self.run(expression.template, expression.bindings)     // This succeeds
            return self.lastInsertRowid!                               // This crashes
        }
    }

This can happen in two situations according to the SQLite last_insert_row_id documentation. The first is the one that I hit:

  1. The rowid is an alias for a declared integer primary key column in the table, and the insert sets the primary key to 0. As documented: "If the table has a column of type INTEGER PRIMARY KEY then that column is another alias for the rowid."
  2. The table has been created with WITHOUT ROWID. As documented: "Inserts into WITHOUT ROWID tables are not recorded."

To reproduce the first crash, you can do something like this:

    let tblTest = Table("test")
    let colIndex = Expression<Int>("index")

    db.run(tblTest.create() { t in
                t.column(colIndex, primaryKey: true)
            })

    db.run(tblTest.insert(colIndex <- 0))

In #383, @stephencelis said "My take: the main insert() method shouldn't return an optional." I think that is OK, but it's at odds with the assumption inside Connection.lastInsertRowid that sqlite3_last_insert_rowid() == 0 always means "not set". I think that, since rowid can be an alias to a user-defined column, it's lastInsertRowid that is wrong, and it should always return the integer that it gets, even if that is 0.

Technically that's a breaking API change; I don't know if you have a policy about that.

@jberkel
Copy link
Collaborator

jberkel commented Dec 7, 2016

Fixed in 0.11.1 / #532

@jberkel jberkel closed this as completed Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants