Cannot detect unique constraint violation in a backend-agnostic way

[jooert]

When trying to insert a new row with an already existing primary key, Diesel returns the generic DatabaseError(String). Therefore it is impossible to tell this case apart from any other database error that might occur without parsing the (backend-specific) error message.

(This came up while implementing ruma/ruma#77.)

[sgrif]

Agreed that we should include more information about the error when possible. I need to think through the API though, as I'm not sure I want to make QueryResult generic over the backend.

[sgrif]

I guess rather than try to implement an exhaustive enum that is backend specific, a more useful API would be to identify the "common" errors that are likely to be handled programmatically, and abstract over them.

[jooert]

Sounds great!

[sgrif]

Anyone have opinions on

enum Error {
    DatabaseError(Box<DatabaseErrorInformation>),
    UniqueConstraintViolation(Box<DatabaseErrorInformation>),
    ForeignKeyViolation(Box<DatabaseErrorInformation>),
    InvalidTransactionState(Box<DatabaseErrorInformation>),
    // Rest of error variants, which is not related to the database specifically
}

vs.

enum Error {
    DatabaseError(DatabaseErrorKind, Box<DatabaseErrorInformation>),
    // Rest of error variants, which is not related to the database specifically
}

enum DatabaseErrorKind {
    UniqueConstraintViolation,
    ForeignKeyViolation,
    InvalidTransactionState,
    #[doc(hidden)]
    __Unknown, // Match against _ instead, more variants may be added in the future
}

vs.

enum Error {
    DatabaseError(DatabaseErrorKind, Box<DatabaseErrorInformation>),
    // Rest of error variants, which is not related to the database specifically
}

enum DatabaseErrorKind {
    ConstraintViolation(ConstraintKind),
    InvalidTransactionState,
    #[doc(hidden)]
    __Unknown, // Match against _ instead, more variants may be added in the future
}

enum ConstraintKind {
    Integrity,
    Restrict,
    NotNull,
    ForeignKey,
    Unique,
    Check,
    Exclusion,
}

So the difference between matching: UniqueConstraintViolation(error), DatabaseError(UniqueConstraintViolation, error), or DatabaseError(ConstraintViolation(Unique), error). The first one is definitely more concise, but the second one feels better structured internally. The third one is quite verbose (especially when you include the additional imports), but it is the most flexible in that it would allow users to for example handle any constraint violation without caring what kind.

/cc @diesel-rs/contributors

[killercup]

I would probably go with the third as it's the most specific representation: You can easily match against a very specific error as well as an error class if you know it's a common one that you want to deal with explicitly.

Let me fiddle a bit:

fn save_user(&self, &new_user) {
    let saving = insert(&new_user).into(users::table).execute(self.connection);

    if let Err(Error::DatabaseError(DatabaseErrorKind::ConstraintViolation(ConstraintKind::Unique))) = saving {
        //? Is there a way to easily know which field(s) the constraint failed for?
        let suggestion = format!("{}1", new_user.name);
        return self.abort_with(MyError::NameTaken(suggestion));
    } else if let Err(Error::DeserializationError(_)) = saving {
        return self.abort_with(MyError::ShitsOnFire);
    }

    let result = try(saving); // No custom message for the rest
    ...
}

This looks a bit verbose, but is really specific. Maybe you could implement some convenience methods on Error?

(Back in the old days, I once parsed the string representation of a MongoDB error because the JS driver only gave me access to that. Not that great of an afternoon.)

[sgrif]

I'm starting to lean more towards 2 actually. This isn't meant to be an exhaustive list of all the possible errors, but to aid the common cases that are recovered from programatically. If we need exhaustiveness, I would probably want to expose a method with the raw error code instead. I do like separating the kind out into its own enum though to make it easy to handle "the query failed"

[sgrif]

    //? Is there a way to easily know which field(s) the constraint failed for?

You can get the table name and the constraint name on PG, but it won't tell you the column separately or anything like that (makes sense, it could be a composite index). On SQLite there is no way to get the specific constraint that failed.

EDIT: Also you can't pattern match on those specifics (other than with a guard). I really wanted to try to make that happen, but there's no way without adding a lifetime to the error enum, since it'd need to be an &'a str, and it'd be ugly to pattern match on anyway since the variant would have to contain the result, etc. Only way around either one is to use a String, and you can't pattern match that.