search

groue / GRDB.swift

A toolkit for SQLite databases, with a focus on application development
MIT License
6.88k stars 708 forks source link

Result codes #171

Closed hartbit closed 7 years ago

hartbit commented 7 years ago

One of the pieces which I would love to see in GRDB is better support for result/error codes. One important piece of that would be to start supporting extended error codes, which can be done easily enough (I have a PR ready in case). The second would be improved errors.

Quick and dirty ideas:

enum DatabaseError : Error {
    case busy
    // ...
    case constraint(ConstraintError)
}

enum ConstraintError : Error {
    case foreignKey
    case check
    // ...
}

do {
    try db.execute("SOMETHING")
} catch .constraint(.foreignKey) {
    // do something about it
}

I this has a few issues: we've lost the error message, etc.. but perhaps it can help us find a good design.

groue commented 7 years ago

@hartbit There are two subjects in this issue:

  1. Enable extended error codes (through the sqlite3_extended_result_codes C function, currently not exposed by GRDB)
  2. Exposing SQLite error codes as Swift constants.

Enabling extended error codes is a valid feature request: thanks for the suggestion!

I'm balanced about exposing error codes through a specific GRDB API.

Those codes are already available through the C SQLite module:

import GRDB
import SQLiteMacOSX

// Without extended error codes
do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError where error.code == SQLITE_CONSTRAINT {
    // do something about it
}

// With extended error codes
do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError where (error.code & SQLITE_CONSTRAINT) != 0 {
    // do something about it
}

(However, for some reason I don't understand yet, extended error codes are not usable this way (I get an "unresolved identifier" compilation error). I'd like this oddity to be resolved.)

The reason why I'm not keen defining an error hierarchy similar to the one you provide above (enum DatabaseError, enum ConstraintError, etc.) is that I'd prefer error codes to keep their original names (SQLITE_CONSTRAINT, etc.). For me, it's part of the "don't surprise experts, don't confuse beginners" rule. Plus it fosters googleability, and supports SQLite documentation quotes:

catch let error as DatabaseError where error.code == SQLITE_CONSTRAINT {
    // https://www.sqlite.org/rescode.html#constraint says:
    // > The SQLITE_CONSTRAINT error code means that an SQL constraint
    // > violation occurred while trying to process an SQL statement. 
    ...
}

Now, I can hear that importing the C module is too much work for a use case that should be obvious. We could then modify the DatabaseError type so that its code has a ResultCode type that adopts RawRepresentable, and ships with as many static constants as SQLite defines error codes. I prefer this to an enum, especially that user can generate its own database errors: why not letting him use custom codes?

These are my initial reactions to your suggestion. Feel free to discuss!

hartbit commented 7 years ago

Enabling extended error codes is a valid feature request: thanks for the suggestion!

👍 I think it might even be worth enabling by default. It's disabled by default in SQLite for historic reasons, but more detailed error codes can't hurt!

The reason why I'm not keen defining an error hierarchy similar to the one you provide above (enum DatabaseError, enum ConstraintError, etc.) is that I'd prefer error codes to keep their original names (SQLITE_CONSTRAINT, etc.). For me, it's part of the "don't surprise experts, don't confuse beginners" rule. Plus it fosters googleability, and supports SQLite documentation quotes:

Very good argument. But I'd like to provide a counter-argument. If/when we provide extended result codes through sqlite3_extended_result_codes , matching against the non-extended codes like you show in your example requires a bitwise operation, which is much less obvious than what can be achieved through a nested enum:

do {
    try db.execute("SOMETHING")
} catch .constraint(.foreignKey) {
    // foreign key constraint error
} catch .constraint {
    // any other constraint error
} catch let error {
    // any other error
}

Now, I can hear that importing the C module is too much work for a use case that should be obvious. We could then modify the DatabaseError type so that its code has a ResultCode type that adopts RawRepresentable, and ships with as many static constants as SQLite defines error codes. I prefer this to an enum, especially that user can generate its own database errors: why not letting him use custom codes?

Sounds like a good compromise. I just hope that we can find a solution which avoids the bitwise operation when comparing against the more general errors.

groue commented 7 years ago

I just hope that we can find a solution which avoids the bitwise operation when comparing against the more general errors.

Yep, I can see how non-obvious it is (and indeed my bitwise sample code above has a bug).

What would you think of:

do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError where error.code == .SQLITE_CONSTRAINT_FOREIGNKEY {
    // foreign key constraint error
} catch let error as DatabaseError where error.primaryResultCode == .SQLITE_CONSTRAINT {
    // any other constraint error
} catch let error as DatabaseError {
    // any other database error
    error.code.rawValue    // 5
    error.code.description // "5 (database is locked)"
} catch {
    // any other error
}

I know that not using Swift enums may look like a missed opportunity. Yet I find it very valuable to preserve both the original names and raw values for googleability (sorry to repeat myself), and to allow users to introduce their own error codes (I find non extensible error enums painful to code with - Alamofire being my latest disappointment).

And I don't much mind exposing the notions of "extended result codes" and "primary result codes" (see SQLite doc) since most non-trivial uses of GRDB eventually require one to strengthen one's SQLite skills anyway.

See the Issue171 branch for the code that supports the example above.

Thoughts, suggestions, improvements, utter disapproval?

hartbit commented 7 years ago

I'm quite happy with this direction :) Could I propose one modification? If we keep the same constant names as SQLite (totally agree with the reasoning), it might be confusing to have the property named code and primaryResultCode. I would suggest naming them resultCode and extendedResultCode, to mirror their SQLite names (and improve googleability). If we need to keep code around to satisfy NSError, we could use it for pure GRDB error codes.

hartbit commented 7 years ago

@groue closed by mistake :p

groue commented 7 years ago

I would suggest naming them resultCode and extendedResultCode, to mirror their SQLite names

Good idea :-)

groue commented 7 years ago

I would suggest naming them resultCode and extendedResultCode, to mirror their SQLite names

Good idea :-)

Or is it?

First, if DatabaseError has two properties extendedResultCode and resultCode, then it would sure be a good fit for SQLite-generated errors:

do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError where error.extendedResultCode == .SQLITE_CONSTRAINT_FOREIGNKEY {
    // foreign key constraint error
} catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT {
    // any other constraint error
}

Yet is it obvious that error.resultCode returns a non-extended result code? I'm not sure.

Second, application-specific custom code wouldn't be so well supported. In the sample code below, the raw custom code is accessed with the longest property name, extendedResultCode, and the more simple resultCode property returns a random and meaningless value (extendedResultCode & 0xff)

extension ResultCode {
    static let appLogicError = ResultCode(rawValue: 1000)
}

do {
    throw DatabaseError(code: .appLogicError)
} catch let error as DatabaseError {
    error.extendedResultCode // 1000
    error.resultCode         // 232
}

Conversely, the resultCode / primaryResultCode pair does not have these issues: resultCode returns the original result code (including custom ones), and if primaryResultCode returns a transformed value that may be meaningless for custom codes, it has a low propability to be misused by mistake (it is longer, and it is repulsive for people who don't know what a "primary result code" is).

groue commented 7 years ago

Latest version:

do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT_FOREIGNKEY {
    // foreign key constraint error
} catch let error as DatabaseError where error.primaryResultCode == .SQLITE_CONSTRAINT {
    // any other constraint error
}
hartbit commented 7 years ago

The problem with those property names is that they defy the first part of the "don't surprise experts, don't confuse beginners" rule. I would have been surprised about them.

Yet is it obvious that error.resultCode returns a non-extended result code?

It is obvious to me who as I've already encountered the result_code/extended_result_code APIs. Conversely, primaryResultCode feels alien to me.

Second, application-specific custom code wouldn't be so well supported. In the sample code below, the raw custom code is accessed with the longest property name, extendedResultCode, and the more simple resultCode property returns a random and meaningless value (extendedResultCode & 0xff)

I agree with this. But I would strive for a different solution. The problem here that I see is that we are mixing the concepts of application error codes and SQLite error codes. Couldn't we solve the problem by having a code application-specific property? Or by having the result codes inside a SQLiteError struct on DatabaseError? The whole reason we have this strange behaviour of the bitwise opération on a custom-application code is that we are using resultCode for both application codes and SQLite error codes. I think we should separate them.

groue commented 7 years ago

Hello again @hartbit

You have good points for knowledgable users, and I have good points for novice users. And no we can't have a specific path for custom errors because they sometimes have to go through SQLite as Int32 (think errors returned by custom functions, for example).

I propose that we forget property pairs, and move on.

Here is a new proposal:

There is a single DatabaseError.resultCode property, which holds an extended result code for SQLite-generated errors, or a custom error code (leaving the advanced user responsible for reading the documentation in order to avoid any conflict with SQLite-reserved codes).

This allows checking for both extended result codes and custom codes:

do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT_FOREIGNKEY {
    // foreign key constraint error
} catch let error as DatabaseError where error.resultCode == .appLogicError {
    // app logic error
}

Now we also need to check for categories of extended result codes (such as all extended result codes that match SQLITE_CONSTRAINT). For that, I propose that we introduce actual Swift pattern matching, with the ~= operator:

do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT_FOREIGNKEY {
    // foreign key constraint error
} catch let error as DatabaseError where error.resultCode ~= .SQLITE_CONSTRAINT {
    // other constraint error
}

The ~= operator would also allow classic Swift switches:

do {
    try db.execute("SOMETHING")
} catch let error as DatabaseError {
    switch error.resultCode {
    case .SQLITE_CONSTRAINT_FOREIGNKEY: // foreign key constraint error
    case .SQLITE_CONSTRAINT:            // other constraint error
    default:                            // other database error
    }
}

The ~= operator could be implemented this way:

func ~= (pattern: ResultCode, value code: ResultCode) -> Bool {
    if pattern.rawValue & 0xFF == 0 {
        // pattern is not an extended result code: check if code extends it
        return (value.rawValue & 0xFF) == pattern.rawValue
    } else {
        // pattern is an extended result code
        return value.rawValue == pattern.rawValue
    }
}

A drawback of this approach is clearly the snippet below, because one has to carefully read the documentation in order to choose between == and ~=:

// correct, but not ideal
catch let error as DatabaseError where error.resultCode ~= .SQLITE_CONSTRAINT

// incorrect, unfortunately
catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT

Maybe we should drop Equatable from ResultCode in order to prevent this easy mistake: ~= would be the only available operator. That would be a litte bit tough, though.

We could also drop ~=, and implement == so that it does the bit matching:

// both correct with a fancy == operator:
catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT_FOREIGNKEY
catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT

What do you think?

hartbit commented 7 years ago

Looks like a promising direction. But I think that the dilemma we are now having with comparison is due to the fact that we are coalescing both extended result codes and result codes in the same type. For example, this might help (even though I still don't like the PimaryResultCode name):

struct PrimaryResultCode : RawRepresentable {
    private let code: Int
    init(code: Int) {
        self.code = code
    }

    static let SQLITE_CONSTRAINT = PrimaryResultCode(code: 19)
}

extension ResultCode {
    var primary: PrimaryResultCode {
        return PrimaryResultCode(code: code & 0xff)
    }
}

catch let error as DatabaseError where error.resultCode == .SQLITE_CONSTRAINT_FOREIGNKEY
catch let error as DatabaseError where error.resultCode.primary == .SQLITE_CONSTRAINT

But before we continue in this direction, could you explain to me why it's important to allow application-specific custom codes? It seems like this requirement is at the root of all the problems we've had in this thread and all solutions look less than ideal to me. If DatabaseError or a more specific SQLiteError could be specialised for sqlite errors and only those, we could have a much simpler and elegant design.

groue commented 7 years ago

If DatabaseError or a more specific SQLiteError could be specialised for sqlite errors and only those, we could have a much simpler and elegant design.

GRDB is a productivity-oriented library: elegancy is welcome, but only as long as it helps users :-)

You invoke YAGNI against custom error codes. And indeed I haven't used any custom error code in any project using GRDB yet.

Still, SQLite does not define any extended result code for "invalid argument" of custom functions, for example. I can also imagine transaction observers that want to communicate the reason why they invalidate a transaction when they throw an error from their databaseWillCommit method. I think that users like GRDB because it is smooth and doesn't resist much one's trend of thought: it welcomes many ways of thinking about problems. This is a quality I'd like to foster, and I put custom error codes there. SQLite's result codes are Int32, not a closed enum.

I think that the dilemma we are now having with comparison is due to the fact that we are coalescing both extended result codes and result codes in the same type. [...] PrimaryResultCode vs. ResultCode.

I see the point, and also had considered two different result code types. That's too much a burden in one developer's mind, I think. And which type is the extension point?

What we need is:

The most difficult point here is forgiveness: errors don't happen that much. Many programmers will write code that handle errors, hoping that they are correctly handled, but won't actually test those code paths. That's because some errors are difficult to trigger.

Considering our previous attempts:

Sorry if I nitpick a lot, @hartbit: there is a reason for this issue to exist :-)

groue commented 7 years ago

I'm sold now to your DatabaseError property pair resultCode/extendedResultCode. Thanks for your suggestion, @hartbit!

hartbit commented 7 years ago

@groue glad to hear it :) it's always worth discussing all design possibilities beforehand!

groue commented 7 years ago

Closing in favor of #180