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

[dav-wolff]

Setup

Versions

• Rust: rustc 1.81.0 (eeb90cda1 2024-09-04) (rustc 1.83.0-nightly (fb4aebddd 2024-09-30) originally tested )
• Diesel: diesel v2.2.4
• Database: 3.46.0 2024-05-23 13:25:27 96c92aba00c8375bc32fafcdf12429c58bd8aabfcadab6683e35bbb9cdebf19e (64-bit)
• Operating System NixOS 24.11.20241006.c31898a (Vicuna)

Feature Flags

• diesel: "sqlite", "returning_clauses_for_sqlite_3_35", "r2d2"

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

• I have already looked over the issue tracker and the discussion forum for similar possible closed issues.

• This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)
• This issue can be reproduced without requiring a third party crate

[weiznich]

Thanks for reporting this issue. It seems like the error type matching is incomplete in this case. The relevant code is here:

diesel/diesel/src/sqlite/connection/stmt.rs

Lines 194 to 208 in 9ed7de2

	fn last_error(raw_connection: *mut ffi::sqlite3) -> Error {
	let error_message = last_error_message(raw_connection);
	let error_information = Box::new(error_message);
	let error_kind = match last_error_code(raw_connection) {
	ffi::SQLITE_CONSTRAINT_UNIQUE | ffi::SQLITE_CONSTRAINT_PRIMARYKEY => {
	DatabaseErrorKind::UniqueViolation
	}
	ffi::SQLITE_CONSTRAINT_FOREIGNKEY => DatabaseErrorKind::ForeignKeyViolation,
	ffi::SQLITE_CONSTRAINT_NOTNULL => DatabaseErrorKind::NotNullViolation,
	ffi::SQLITE_CONSTRAINT_CHECK => DatabaseErrorKind::CheckViolation,
	_ => DatabaseErrorKind::Unknown,
	};
	DatabaseError(error_kind, error_information)
	}

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]

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?