diesel-rs / diesel

A safe, extensible ORM and Query Builder for Rust
https://diesel.rs
Apache License 2.0
12.79k stars 1.08k forks source link

DatabaseError caused by foreign key `ON DELETE RESTRICT` is not classified as `ForeignKeyViolation` using Sqlite #4299

Open dav-wolff opened 1 month ago

dav-wolff commented 1 month ago

Setup

Versions

Feature Flags

Problem Description

Diesel using sqlite doesn't recognize foreign key violations caused by deleting a row that is referenced by a row in a different table which is a foreign key with ON DELETE RESTRICT. It returns a DatabaseErrorKind::Unknown instead of a DatabaseErrorKind::ForeignKeyViolation.

I believe a DatabaseErrorKind::ForeignKeyViolation would be appropriate because the error arises from the foreign key constraint and the error message says "FOREIGN KEY constraint failed".

What are you trying to accomplish?

Check if the error return from diesel::delete(...).execute(...) is caused by a foreign key violation, or some general database error, in order to properly handle the error.

For now I'm thinking of just checking for the error message, but I'm not sure if that is stable or might change at some point.

What is the expected output?

DatabaseError(
    ForeignKeyViolation,
    "FOREIGN KEY constraint failed",
)

What is the actual output?

DatabaseError(
    Unknown,
    "FOREIGN KEY constraint failed",
)

Are you seeing any additional errors?

No

Steps to reproduce

  1. Create a table with a foreign key with ON DELETE RESTRICT
  2. Violate that restriction using diesel::delete.

Reproduction: https://github.com/dav-wolff/diesel-foreign-key-violation

Checklist

weiznich commented 1 month ago

Thanks for reporting this issue. It seems like the error type matching is incomplete in this case. The relevant code is here: https://github.com/diesel-rs/diesel/blob/9ed7de24c0b9a0afce8dbae845316d74fb795258/diesel/src/sqlite/connection/stmt.rs#L194-L208

For fixing this someone would need to:

  1. Put together a test case based on the provided example
  2. Put a debug in the linked function
  3. Execute the test case and see which error code comes back
  4. Hopefully it's a meaningful error code, in that case we can just map it to the relevant error variant. If it's not meaningful we are out of luck here, as the best we could do then is matching on the error string.

PR's are welcome.

dav-wolff commented 1 month ago

Thanks for the pointers. I've tested it and the error code generated is SQLITE_CONSTRAINT_TRIGGER which does seem too generic to be interpreted as a foreign key violation.

Matching like this is able to detect the error:

ffi::SQLITE_CONSTRAINT_TRIGGER if *error_information == "FOREIGN KEY constraint failed" => DatabaseErrorKind::ForeignKeyViolation,

however I'm not sure if the error message is stable enough to be relied on. I couldn't find any details about that in the relevant SQLite documentation.

I've also realized that simply removing the ON DELETE RESTRICT results in the error code SQLITE_CONSTRAINT_FOREIGNKEY which is correctly interpreted as a DatabaseErrorKind::ForeignKeyViolation and doesn't meaningfully impact the functionality for my use case. I'm not very experiences with SQL so when I read about ON DELETE RESTRICT in the documentation that seemed like the most appropriate action but seeing that NO ACTION still restricts the deletion I'm gonna go with that for now.

Should I still open a PR or is it not worth it if it means relying on the error message?