sqlite3_prepare errors are fatal

[zmeyc]

In Statement's constructor try! statement makes impossible handling schema mismatch errors gracefully:

public final class Statement {
    init(_ connection: Connection, _ SQL: String) {
        self.connection = connection
        try! check(sqlite3_prepare_v2(connection.handle, SQL, -1, &handle, nil))
    }

This leads to crashes in some unexpected places such as:

do {
        let sql = table
            .filter(keyColumn == value)
            .update(setters)
        let rowsUpdated = try db.run(sql) // crash
} catch {
}

I propose considering making Statement's constructor throwable and replacing try! with try. This will lead to API changes in other places (pluck and some other functions will become throw-able), but will allow recovering from errors.

[stephencelis]

@zmeyc Could you elaborate on the schema mismatch issue you're coming across? Do you not have control over the database file you're working with?

[zmeyc]

@stephencelis I'm using the library to implement a backend. One of the tables didn't have a required column due to migration problems. When users invoke endpoints which try to update that table the server crashes. Ideally, it should handle this in a controllable manner, for example log the error and return "Internal server error". I'll probably try to fix this in a cloned repo to see how it impacts the rest of the library API.

[stephencelis]

I've tried to make read-only queries throw fatal errors since they usually indicate a programming issue that can be solved without error handling. Is this something that could be addressed at the migration layer?

[zmeyc]

I don’t think this can be solved at the migration layer. I understand that in most cases sqlite3_prepare failing indicates a programming error or hdd failing or db admin mistake etc. But I’m trying to avoid shutting down the entire app if the error is recoverable (in my case affects only one endpoint).

Another problem is that some functions are declared as throwable and user would expect to handle errors by declaring a do{}catch{} yet they shut down the app in some cases.

Imho, it’s better to propagate errors from underlying lib where possible, possibly adding helper methods with try!.
I.e. non-throwing “scalar()” and “checkedScalar() throws”. In this case the API can be revisited so that functions with “throws” actually return all errors. It can probably even remain compatible with the current API.

I’ve replaced “try!” with “try” in Statement’s constructor and it went smoother than I thought:
db.prepare and db.scalar became throwable, other changes were internal to the lib.

Looks like try! is used only in a few places:
Statement’s constructor
createFunction()
createCollation()
scalar()
next()
I can try to implement error propagation if you like the idea in general.

[stephencelis]

Let me think about what the API would look like a bit more. Exposing an underlying path that throws sounds like a reasonable compromise, but I'd like it to be a secondary option to avoid less boilerplate and unnecessary error handling for the average use case.

[stephencelis]

After sleeping on it I'm still wondering if prepare should ever throw. Is there a reason you can't use PRAGMA user_version to handle migrations? Or, if all else fails, PRAGMA table_info(table)? Can you give more context, perhaps with actual code, as to what causes the error and why?

[stephencelis]

@zmeyc Any chance you have time to address my questions above?

[zmeyc]

Sorry for the late response. My migration code looks like this, I'm using PRAGMA user_version:

    var userVersion: Int {
        get { return try! Int(db.scalar("PRAGMA user_version") as! Int64) }
        set {
            do {
                try db.run("PRAGMA user_version = \(transcode(Int64(newValue)))")
            } catch {
                fatalError("Unable to update user_version")
            }
        }
    }

    func migrate() throws {
        while case let u = userVersion where u != currentVersion {
            print("Upgrading DB: v.\(u) -> v.\(u + 1)")
            try db.transaction {
                try self.migrationStep(u)
                self.userVersion = (u > 0) ? (u + 1) : self.currentVersion
            }
        }
    }

    func migrationStep(userVersion: Int) throws {
        switch self.userVersion {
        case XXXXX:
            /// Create tables etc
        default:
            throw Error.NoMigrationRulesForVersion(version: userVersion)
        }
    }

The column became invalid during migration, but the app crashed later when referencing the column while serving a request. By design it should have returned "Internal server error" to user and log the error to system log. Theoretically it could also crash in other circumstances, for example if database file became unavailable. There's currently no way to handle this kind of errors gracefully. I understand that this is not the average use case and makes API more verbose.

[mikemee]

Closing as this appears to have been fixed by the accepted PR #290.