From 5c48baf55f567f9a46cdb354d2f2f71e0e55d755 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Joly?= Date: Mon, 6 Nov 2023 23:46:15 +0000 Subject: [PATCH] feat: return full foreign_key_check error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `foreign_key_check` pragma returns a list of all the violated foreign key constraints[^1]: > The foreign_key_check pragma returns one row output for each foreign > key violation Each ForeignKeyCheckError store one of those rows. However, only the first row is stored. Store all row returned in a vector in the returned error variant, `Error::ForeignKeyCheck(…)`. [^1]: https://www.sqlite.org/pragma.html#pragma_foreign_key_check --- rusqlite_migration/src/errors.rs | 12 ++-- rusqlite_migration/src/lib.rs | 34 +++++----- rusqlite_migration/src/tests/helpers.rs | 1 + ...ration__tests__synch__error_display-2.snap | 5 ++ ...ration__tests__synch__error_display-3.snap | 5 ++ ...ration__tests__synch__error_display-4.snap | 5 ++ ...ration__tests__synch__error_display-5.snap | 5 ++ ...igration__tests__synch__error_display.snap | 5 ++ ...__tests__synch__invalid_fk_check_test.snap | 22 +++++++ rusqlite_migration/src/tests/synch.rs | 63 +++++++++---------- 10 files changed, 103 insertions(+), 54 deletions(-) create mode 100644 rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-2.snap create mode 100644 rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-3.snap create mode 100644 rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-4.snap create mode 100644 rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-5.snap create mode 100644 rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display.snap create mode 100644 rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__invalid_fk_check_test.snap diff --git a/rusqlite_migration/src/errors.rs b/rusqlite_migration/src/errors.rs index ad8d995..9db33ab 100644 --- a/rusqlite_migration/src/errors.rs +++ b/rusqlite_migration/src/errors.rs @@ -27,7 +27,7 @@ pub enum Error { /// Something wrong with migration definitions MigrationDefinition(MigrationDefinitionError), /// The foreign key check failed - ForeignKeyCheck(ForeignKeyCheckError), + ForeignKeyCheck(Vec), /// Error returned by the migration hook Hook(String), /// Error returned when loading migrations from directory @@ -80,7 +80,7 @@ impl std::error::Error for Error { Error::RusqliteError { query: _, err } => Some(err), Error::SpecifiedSchemaVersion(e) => Some(e), Error::MigrationDefinition(e) => Some(e), - Error::ForeignKeyCheck(e) => Some(e), + Error::ForeignKeyCheck(vec) => Some(vec.get(0)?), Error::Hook(_) | Error::FileLoad(_) => None, #[cfg(feature = "async-tokio-rusqlite")] Error::ConnectionClosed => None, @@ -338,18 +338,18 @@ mod tests { #[test] fn test_rusqlite_error_fkc() { assert_ne!( - Error::ForeignKeyCheck(ForeignKeyCheckError { + Error::ForeignKeyCheck(vec![ForeignKeyCheckError { table: "t1".to_owned(), rowid: 1, parent: "t2".to_owned(), fkid: 3 - }), - Error::ForeignKeyCheck(ForeignKeyCheckError { + }]), + Error::ForeignKeyCheck(vec![ForeignKeyCheckError { table: "t1".to_owned(), rowid: 3, parent: "t2".to_owned(), fkid: 3 - },), + }]), ) } diff --git a/rusqlite_migration/src/lib.rs b/rusqlite_migration/src/lib.rs index e55efd0..33b8008 100644 --- a/rusqlite_migration/src/lib.rs +++ b/rusqlite_migration/src/lib.rs @@ -20,7 +20,7 @@ limitations under the License. #![doc = include_str!(concat!(env!("OUT_DIR"), "/readme_for_rustdoc.md"))] use log::{debug, info, trace, warn}; -use rusqlite::{Connection, OptionalExtension, Transaction}; +use rusqlite::{Connection, Transaction}; #[cfg(feature = "from-directory")] use include_dir::Dir; @@ -773,20 +773,26 @@ fn set_user_version(conn: &Connection, v: usize) -> Result<()> { // Validate that no foreign keys are violated fn validate_foreign_keys(conn: &Connection) -> Result<()> { let pragma_fk_check = "PRAGMA foreign_key_check"; - conn.query_row(pragma_fk_check, [], |row| { - Ok(ForeignKeyCheckError { - table: row.get(0)?, - rowid: row.get(1)?, - parent: row.get(2)?, - fkid: row.get(3)?, + let mut stmt = conn + .prepare_cached(pragma_fk_check) + .map_err(|e| Error::with_sql(e, pragma_fk_check))?; + + let fk_errors = stmt + .query_map([], |row| { + Ok(ForeignKeyCheckError { + table: row.get(0)?, + rowid: row.get(1)?, + parent: row.get(2)?, + fkid: row.get(3)?, + }) }) - }) - .optional() - .map_err(|e| Error::with_sql(e, pragma_fk_check)) - .and_then(|o| match o { - Some(e) => Err(Error::ForeignKeyCheck(e)), - None => Ok(()), - }) + .map_err(|e| Error::with_sql(e, pragma_fk_check))? + .collect::, _>>()?; + if !fk_errors.is_empty() { + Err(crate::Error::ForeignKeyCheck(fk_errors)) + } else { + Ok(()) + } } impl<'u> FromIterator> for Migrations<'u> { diff --git a/rusqlite_migration/src/tests/helpers.rs b/rusqlite_migration/src/tests/helpers.rs index 0904d11..a5973fb 100644 --- a/rusqlite_migration/src/tests/helpers.rs +++ b/rusqlite_migration/src/tests/helpers.rs @@ -72,6 +72,7 @@ pub fn m_invalid_fk() -> M<'static> { FOREIGN KEY(a) REFERENCES fk1(a) \ ); \ INSERT INTO fk2 (a) VALUES ('foo'); \ + INSERT INTO fk2 (a) VALUES ('bar'); \ ", ) .foreign_key_check() diff --git a/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-2.snap b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-2.snap new file mode 100644 index 0000000..9cfca22 --- /dev/null +++ b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-2.snap @@ -0,0 +1,5 @@ +--- +source: rusqlite_migration/src/tests/synch.rs +expression: "Error::Hook(String::new())" +--- +rusqlite_migrate error: Hook("") diff --git a/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-3.snap b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-3.snap new file mode 100644 index 0000000..b919092 --- /dev/null +++ b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-3.snap @@ -0,0 +1,5 @@ +--- +source: rusqlite_migration/src/tests/synch.rs +expression: "Error::ForeignKeyCheck(vec![ForeignKeyCheckError\n {\n table : String :: new(), rowid : 1, parent : String :: new(), fkid\n : 2,\n }, ForeignKeyCheckError\n {\n table : String :: new(), rowid : 2, parent : String :: new(), fkid\n : 3,\n },])" +--- +rusqlite_migrate error: ForeignKeyCheck([ForeignKeyCheckError { table: "", rowid: 1, parent: "", fkid: 2 }, ForeignKeyCheckError { table: "", rowid: 2, parent: "", fkid: 3 }]) diff --git a/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-4.snap b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-4.snap new file mode 100644 index 0000000..ab2a2bd --- /dev/null +++ b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-4.snap @@ -0,0 +1,5 @@ +--- +source: rusqlite_migration/src/tests/synch.rs +expression: "Error::MigrationDefinition(MigrationDefinitionError::NoMigrationsDefined)" +--- +rusqlite_migrate error: MigrationDefinition(NoMigrationsDefined) diff --git a/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-5.snap b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-5.snap new file mode 100644 index 0000000..37797d3 --- /dev/null +++ b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display-5.snap @@ -0,0 +1,5 @@ +--- +source: rusqlite_migration/src/tests/synch.rs +expression: "Error::RusqliteError {\n query: String::new(),\n err: rusqlite::Error::InvalidQuery,\n}" +--- +rusqlite_migrate error: RusqliteError { query: "", err: InvalidQuery } diff --git a/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display.snap b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display.snap new file mode 100644 index 0000000..ca50b0e --- /dev/null +++ b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__error_display.snap @@ -0,0 +1,5 @@ +--- +source: rusqlite_migration/src/tests/synch.rs +expression: "Error::SpecifiedSchemaVersion(SchemaVersionError::TargetVersionOutOfRange {\n specified: SchemaVersion::NoneSet,\n highest: SchemaVersion::NoneSet,\n })" +--- +rusqlite_migrate error: SpecifiedSchemaVersion(TargetVersionOutOfRange { specified: NoneSet, highest: NoneSet }) diff --git a/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__invalid_fk_check_test.snap b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__invalid_fk_check_test.snap new file mode 100644 index 0000000..b8bf77a --- /dev/null +++ b/rusqlite_migration/src/tests/snapshots/rusqlite_migration__tests__synch__invalid_fk_check_test.snap @@ -0,0 +1,22 @@ +--- +source: rusqlite_migration/src/tests/synch.rs +expression: migrations.validate() +--- +Err( + ForeignKeyCheck( + [ + ForeignKeyCheckError { + table: "fk2", + rowid: 1, + parent: "fk1", + fkid: 0, + }, + ForeignKeyCheckError { + table: "fk2", + rowid: 2, + parent: "fk1", + fkid: 0, + }, + ], + ), +) diff --git a/rusqlite_migration/src/tests/synch.rs b/rusqlite_migration/src/tests/synch.rs index c101fdf..99bcc09 100644 --- a/rusqlite_migration/src/tests/synch.rs +++ b/rusqlite_migration/src/tests/synch.rs @@ -148,40 +148,38 @@ fn test_migration_definition_error_display() { #[test] fn test_error_display() { - let err = Error::SpecifiedSchemaVersion(SchemaVersionError::TargetVersionOutOfRange { - specified: SchemaVersion::NoneSet, - highest: SchemaVersion::NoneSet, - }); - assert_eq!( - "rusqlite_migrate error: SpecifiedSchemaVersion(TargetVersionOutOfRange { specified: NoneSet, highest: NoneSet })", - format!("{err}") - ); + insta::assert_display_snapshot!(Error::SpecifiedSchemaVersion( + SchemaVersionError::TargetVersionOutOfRange { + specified: SchemaVersion::NoneSet, + highest: SchemaVersion::NoneSet, + } + )); - let err = Error::Hook(String::new()); - assert_eq!("rusqlite_migrate error: Hook(\"\")", format!("{err}")); + insta::assert_display_snapshot!(Error::Hook(String::new())); - let err = Error::ForeignKeyCheck(ForeignKeyCheckError { - table: String::new(), - rowid: 1, - parent: String::new(), - fkid: 2, - }); - assert_eq!("rusqlite_migrate error: ForeignKeyCheck(ForeignKeyCheckError { table: \"\", rowid: 1, parent: \"\", fkid: 2 })", format!("{err}")); + insta::assert_display_snapshot!(Error::ForeignKeyCheck(vec![ + ForeignKeyCheckError { + table: String::new(), + rowid: 1, + parent: String::new(), + fkid: 2, + }, + ForeignKeyCheckError { + table: String::new(), + rowid: 2, + parent: String::new(), + fkid: 3, + }, + ])); - let err = Error::MigrationDefinition(MigrationDefinitionError::NoMigrationsDefined); - assert_eq!( - "rusqlite_migrate error: MigrationDefinition(NoMigrationsDefined)", - format!("{err}") - ); + insta::assert_display_snapshot!(Error::MigrationDefinition( + MigrationDefinitionError::NoMigrationsDefined + )); - let err = Error::RusqliteError { + insta::assert_display_snapshot!(Error::RusqliteError { query: String::new(), err: rusqlite::Error::InvalidQuery, - }; - assert_eq!( - "rusqlite_migrate error: RusqliteError { query: \"\", err: InvalidQuery }", - format!("{err}") - ); + }); } #[test] @@ -240,12 +238,12 @@ fn error_test_source() { &MigrationDefinitionError::NoMigrationsDefined ); - let err = Error::ForeignKeyCheck(ForeignKeyCheckError { + let err = Error::ForeignKeyCheck(vec![ForeignKeyCheckError { table: String::new(), rowid: 1i64, parent: String::new(), fkid: 1i64, - }); + }]); assert_eq!( std::error::Error::source(&err) .and_then(|e| e.downcast_ref::()) @@ -350,10 +348,7 @@ fn valid_fk_check_test() { #[test] fn invalid_fk_check_test() { let migrations = Migrations::new(vec![m_invalid_fk()]); - assert!(matches!( - dbg!(migrations.validate()), - Err(Error::ForeignKeyCheck(_)) - )); + insta::assert_debug_snapshot!(migrations.validate()); } #[test]