tciuro/NanoStore

-[NSFNanoEngine isTransactionActive] works but misinterprets sqlite3_get_autocommit()

billgarrison opened this issue · 1 comments

The current implementation of -[NSFNanoEngine isTransactionActive] treats the return value from sqlite3_get_autocommit() as an SQLite error code. But that doesn't match the documentation for the function.

- (BOOL)isTransactionActive
{
    sqlite3* myDB = self.sqlite;

    int status = sqlite3_get_autocommit(myDB);

    // Since we're operating with extended result code support, extract the bits
    // and obtain the regular result code
    // For more info check: http://www.sqlite.org/c3ref/c_ioerr_access.html

    status = [NSFNanoEngine NSFP_stripBitsFromExtendedResultCode:status];

    return (0 == status);
}

sqlite3_get_autocommit() is documented to return either 0 or a non-zero value, to be interpreted as a boolean. Stripping bits from the returned value is unnecessary and not strictly correct in this case.

A functionally equivalent, but more accurate implementation for this method would be:

- (BOOL)isTransactionActive
{
    /* 
    sqlite3_get_autocommit(sqlite3 *db) returns 0 when the given database connection 
    is in the middle of a manually-initiated transaction.
     */
    return (sqlite3_get_autocommit(self.sqlite) == 0);
}

The current implementation does work fine and returns a proper value. It was just confusing reading the code and comparing it against the SQLite documentation.

Sure thing, looking at the documentation I don't see why this is needed. Thanks for reporting this.