Crash when lastInsertRowId is negative

[zmeyc]

Consider, for example, the following statement:

Insert(template: "INSERT INTO \"sessions\" (\"owner_id\", \"started\", \"victim_name\", \"topic\", \"line\", \"chat_id\") VALUES (?, ?, ?, ?, ?, ?)", bindings: [Optional(230594948), Optional(1), Optional(""), Optional(""), Optional(0), Optional(-147636081)])

chat_id is INTEGER PRIMARY KEY and negative value is inserted.

sqlite3_last_insert_rowid() returns -147636081 (as expected)

Lib's lastInsertRowId turns it into nil:

    public var lastInsertRowid: Int64? {
        let rowid = sqlite3_last_insert_rowid(handle)
        return rowid > 0 ? rowid : nil
    }

The app crashes because Connection.run() dereferences nil.

[zmeyc]

I'm not sure if any return value of sqlite3_last_inserted_rowid can be treated as an error condition. Lib's wrapper probably shouldn't return an optional value:
https://www.sqlite.org/c3ref/last_insert_rowid.html

[iNoles]

1 ) "If no successful INSERTs into rowid tables have ever occurred on the database connection D, then sqlite3_last_insert_rowid(D) returns zero."
2) constraint violation = also return zero.

[zmeyc]

An INSERT that fails due to a constraint violation is not a successful INSERT and does not change the value returned by this routine.

I think it's zero initially, but changed only on successful inserts.

[stephencelis]

Would you like to submit a PR that changes that line to the following?

return rowid != 0 ? rowid : nil

[zmeyc]

@stephencelis
It appears zero rowid is a valid value, I've done some testing:

sqlite> CREATE TABLE foo (id INTEGER PRIMARY KEY, bar TEXT); 
sqlite> SELECT last_insert_rowid();
0
sqlite> INSERT INTO foo (id, bar) VALUES (0, 'test');
sqlite> SELECT last_insert_rowid();
0
sqlite> INSERT INTO foo (bar) VALUES ('more'); 
sqlite> SELECT last_insert_rowid();
1
sqlite> SELECT * FROM foo; 
0|test
1|more
sqlite> SELECT rowid, * FROM foo; 
0|0|test
1|1|more

So, I believe the correct fix would be simply returning rowid as non-optional Int64. This doesn't seem to break anything in the lib except this place:

            try self.run(expression.template, expression.bindings)
            return self.lastInsertRowid!

where rowid was unconditionally dereferenced anyway. If it's ok, I'll submit a PR with these changes.

[stephencelis]

While it's possible to insert a 0 id, it's unlikely and 0 reflects failed inserts for the last insert rowid. I'd like to somehow account for the fact that the last insert was successful. If we return 0 from lastInsertRowid, we're giving developers more of an opportunity to make a mistake in naîve insert logic. Is there a surefire way of detecting inserts and holding onto this state in the connection?

[zmeyc]

This function can't be used for error checking because it doesn't change last_insert_rowid on failed insert. It's return value is meaningful only after a successful insert. Returning nil from it can give a false impression that it can be used for error checking.

Probably run() itself should fail, and it does by throwing an exception.

sqlite> SELECT last_insert_rowid();
1

Trying to insert a duplicate value:

sqlite> INSERT INTO foo (id, bar) VALUES (1, 'more');
Error: UNIQUE constraint failed: foo.id
sqlite> SELECT last_insert_rowid();
1

[zmeyc]

A possible way of reporting errors would be storing the result of sqlite3_step somewhere and returning nil if the last insert failed, or rowid (including 0) otherwise, but it will change the behavior of lastInsertRowId compared to the original sqlite function which could be confusing.

[stephencelis]

One of my goals for the Swift wrapper was to reduce error, so the fact that a 0 last insert rowid could mean either

• the last insert failed, or
• the last insert succeeded with rowid 0

is a confusion I'd like to solve. Could we use sqlite3_errcode in the lastInsertRowId property to detect whether or not we return nil?

[zmeyc]

Probably yes, but it's very hard to implement properly. For example:

    public func lastInsertRowid() throws -> Int64 {
        try check(sqlite3_errcode(handle))
        return sqlite3_last_insert_rowid(handle)
    }

There are two problems with this implementation:

1. "INSERT then failed SELECT": lastInsertRowId will now fail while it would simply return a rowId in plain sqlite.
2. "INSERT then failed INSERT then successful SELECT": lastInsertRowId will now return rowid of FIRST insert which is really confusing.

Another option is renaming the function to avoid confusion, something along the:

  public var lastSuccessfulInsertRowId -> Int64 {
        return sqlite3_last_insert_rowid(handle)
  }

[iNoles]

According to FMDB, they didn't check rowid at all.
https://github.com/ccgus/fmdb/blob/master/src/fmdb/FMDatabase.m#L514

[zmeyc]

@jberkel @stephencelis Yes, FMDB handles this correctly, SQLite.swift doesn't.

SQLite.swift implementation:

public var lastInsertRowid: Int64? {
  let rowid = sqlite3_last_insert_rowid(handle)
  return rowid != 0 ? rowid : nil
}

I'm trying to explain that there's NO reason to compare rowid with zero!

1. sqlite3_last_insert_rowid doesn't get reset to 0 on errors, so it's unsuitable for error handling!

2. 0 and negative values are valid rowids.

I believe the correct implementation is simply returning sqlite3_last_insert_rowid() as FMDB does (and all other SQLite3 wrappers).

[jberkel]

@zmeyc thanks, sounds simple enough. I created a new issue (#532)