From 404fa7b331ebe166b3a729b6f317549f2c2a1278 Mon Sep 17 00:00:00 2001 From: Remi d'Almeida Date: Tue, 20 Oct 2026 01:57:47 +0300 Subject: [PATCH 1/2] test: make the abort_for_db tests able to fail test_regular_abort_for_db_only_matching_db_key and test_regular_abort_for_db_does_not_match_colon_prefix_alias ended by calling abort_for_db for the other database. abort_for_db returns Ok(()) when nothing matches, so neither test checked that the other transaction was still tracked. Both tests now keep each task's AbortHandle. They assert that the matching task finished and the other did not, then that aborting the other database finishes it. Making abort_for_db match every transaction, or match by prefix, now fails them. --- .../tests/transaction_state_tests.rs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs b/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs index 352faa8..4307d34 100644 --- a/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs +++ b/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs @@ -364,6 +364,11 @@ async fn test_regular_abort_for_db_only_matching_db_key() { tokio::time::sleep(std::time::Duration::from_secs(60)).await; }); + // Keep abort handles to observe the tasks once the tracker owns them. + // abort_for_db awaits every task it aborts, so is_finished() is settled. + let main_task = main_handle.abort_handle(); + let other_task = other_handle.abort_handle(); + state .insert("main".into(), "main-one".into(), main_handle) .await; @@ -373,8 +378,22 @@ async fn test_regular_abort_for_db_only_matching_db_key() { state.abort_for_db("main").await.unwrap(); + assert!( + main_task.is_finished(), + "abort_for_db(\"main\") should abort and await the `main` transaction task" + ); + assert!( + !other_task.is_finished(), + "abort_for_db(\"main\") must not abort the `other` transaction task" + ); + // Other database transaction should still be tracked and abortable. state.abort_for_db("other").await.unwrap(); + + assert!( + other_task.is_finished(), + "`other` transaction should still be tracked so abort_for_db(\"other\") aborts it" + ); } #[tokio::test] @@ -388,13 +407,33 @@ async fn test_regular_abort_for_db_does_not_match_colon_prefix_alias() { tokio::time::sleep(std::time::Duration::from_secs(60)).await; }); + // Keep abort handles to observe the tasks once the tracker owns them. + // abort_for_db awaits every task it aborts, so is_finished() is settled. + let a_task = a_handle.abort_handle(); + let ab_task = ab_handle.abort_handle(); + state.insert("a".into(), "a-tx".into(), a_handle).await; state.insert("a:b".into(), "ab-tx".into(), ab_handle).await; state.abort_for_db("a").await.unwrap(); + assert!( + a_task.is_finished(), + "abort_for_db(\"a\") should abort and await the `a` transaction task" + ); + // Closing `a` must not abort the transaction belonging to database `a:b`. + assert!( + !ab_task.is_finished(), + "abort_for_db(\"a\") must not abort the `a:b` transaction task" + ); + state.abort_for_db("a:b").await.unwrap(); + + assert!( + ab_task.is_finished(), + "`a:b` transaction should still be tracked so abort_for_db(\"a:b\") aborts it" + ); } // ============================================================================ From 45ada296679bc93be52edb24088bfb57d409cd9e Mon Sep 17 00:00:00 2001 From: Remi d'Almeida Date: Tue, 20 Oct 2026 11:45:43 +0300 Subject: [PATCH 2/2] test: adopt rstest for parameterized tests A test that loops over inputs, or puts one assert per input in one function, stops at the first failing input, and cargo test cannot run one input on its own. rstest generates one test per #[case], so a run reports every failing input and each one can be run by name. The scope follows tauri-plugin-connectivity's adoption of rstest (c5d624d), with a stricter rule for merging separate tests: - Every test that checked several inputs in one function is now a table with one named case per input. - Separate tests are merged only when each case is another example of the same guarantee. A comment saying why an input is interesting is fine. A case that needs different assertions, a branch, or a comment arguing a different guarantee stays its own test. - Scenario tests stay plain: setup loops, ordered or stateful steps, races, polling, and tests that differ in which operation they run. Every input keeps its value. The storage-class round trip now reads each value through quote(), which pins the value and its storage class in one string and replaces the separate typeof probe. Some rejections now assert the exact error variant or code instead of is_err(). test_reserved_schema_names_rejected loses its closing check that nothing stayed attached: every rejection happens in validation, before any ATTACH, so that check could not fail. The workspace goes from 358 test functions to 414 generated tests, 171 of them cases in 42 tables. CHANGELOG.md names the readable-notification test by its new name. rstest 0.28.0 is a dev-dependency of all four crates with default features off. Its crate-name feature changes feature resolution for toml crates that tauri's build shares, so a warm workspace build would rebuild tauri. Cargo.lock adds rstest, rstest_macros and relative-path. It also moves some Windows-only edges from windows-sys 0.60 to 0.61, which adding any new package does here. Nothing else in the normal dependency graph changes. --- CHANGELOG.md | 2 +- Cargo.lock | 54 +- Cargo.toml | 4 + crates/sqlx-sqlite-conn-mgr/Cargo.toml | 2 + crates/sqlx-sqlite-conn-mgr/src/attached.rs | 174 +++---- crates/sqlx-sqlite-conn-mgr/src/functions.rs | 55 +- crates/sqlx-sqlite-conn-mgr/src/registry.rs | 29 +- .../tests/attached_tests.rs | 43 +- .../tests/function_tests.rs | 82 ++- crates/sqlx-sqlite-observer/Cargo.toml | 2 + crates/sqlx-sqlite-observer/src/schema.rs | 55 +- .../tests/conn_mgr_tests.rs | 51 +- .../tests/integration_tests.rs | 221 ++------ crates/sqlx-sqlite-toolkit/Cargo.toml | 2 + crates/sqlx-sqlite-toolkit/src/decode.rs | 23 +- crates/sqlx-sqlite-toolkit/src/error.rs | 271 +++++----- crates/sqlx-sqlite-toolkit/src/pagination.rs | 482 +++++++----------- .../tests/pagination_tests.rs | 233 +++------ .../tests/transaction_state_tests.rs | 72 +-- .../tests/wrapper_tests.rs | 97 ++-- package.json | 3 +- scripts/lint-rstest-attributes.cjs | 139 +++++ src/error.rs | 143 ++---- src/lib.rs | 207 ++------ src/subscriptions.rs | 70 ++- src/validate.rs | 77 ++- 26 files changed, 1062 insertions(+), 1531 deletions(-) create mode 100644 scripts/lint-rstest-attributes.cjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 89fef81..8250d72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,7 +112,7 @@ Transaction cleanup failures propagate as errors rather than being logged and ig - **Attached-database observation no longer requires the main database to be observed too.** The whole observable acquisition path was gated on the *main* database's broker, so a write through an unobserved database into an observed `readWrite` attachment registered no hooks at all and reached that attachment's own subscribers not at all — silently, with every call reporting success. This was the same class of defect as #53 itself: observation depending on the wrong object. The observable path is now taken when *any* participating database is observed — main or a `readWrite` attachment — and `main` is simply omitted from the alias → broker map when the main database has no broker of its own, so its changes are dropped rather than misattributed. Nothing had covered this configuration, because the attached-observation tests enabled observation on both databases. - **Known limitation:** a writer holds the broker it bound at acquisition for its whole lifetime, so a `disable_observation()` + `enable_observation()` cycle *during* an open transaction leaves that transaction publishing to the previous broker. Subscribers created before the cycle still receive its commit (the hook context keeps that broker alive); subscribers created after the cycle do not, while `is_observing()`, `subscribe()`, and the commit itself all report success. Reachable as: the last window `unobserve()`s and then `observe()`s again while another caller's interruptible transaction is still open. Pinned by an `#[ignore]`d test rather than only prose; the fix (reviving the slot's previous broker, gated on a lease held for the hooks' lifetime) is follow-up work. - **Known limitation:** the plugin's `observe()`/`unobserve()` reference count is keyed on webview labels, so a Rust caller that enabled observation directly on a `DatabaseWrapper` is outside it. The last window releasing tears observation down database-wide and silently ends that caller's subscription; conversely a Rust `disable_observation()` breaks every window's live subscription while leaving their registrations non-zero. A plugin-level API for Rust observers is follow-up work. -- **A change notification now means that the change is durable and readable, not only that the commit hook ran.** SQLite's commit hook runs before the commit is final. It is the hook whose non-zero return converts a `COMMIT` into a `ROLLBACK`. A subscriber that read immediately after notification previously saw pre-commit state through another connection, on most notifications. The tests `a_notification_means_the_change_is_readable_in_wal_mode` and `a_notification_means_the_change_is_readable_in_rollback_journal_mode` now guard this, one per journal mode. The commit hook now converts and holds the transaction's changes and does not publish them yet. A hook registered with `SQLITE_TRACE_PROFILE` and `SQLITE_TRACE_STMT` publishes the changes when the statement that committed finishes, or when the next statement starts. By then the commit is durable, so any connection can read it. The writer does nothing extra. +- **A change notification now means that the change is durable and readable, not only that the commit hook ran.** SQLite's commit hook runs before the commit is final. It is the hook whose non-zero return converts a `COMMIT` into a `ROLLBACK`. A subscriber that read immediately after notification previously saw pre-commit state through another connection, on most notifications. The test `a_notification_means_the_change_is_readable` now guards this, with one case per journal mode (`wal_mode` and `rollback_journal_mode`). The commit hook now converts and holds the transaction's changes and does not publish them yet. A hook registered with `SQLITE_TRACE_PROFILE` and `SQLITE_TRACE_STMT` publishes the changes when the statement that committed finishes, or when the next statement starts. By then the commit is durable, so any connection can read it. The writer does nothing extra. - **Notification order is now per connection.** Publication happens after SQLite released the write lock, so two connections that commit in one order can notify in the other. On master, the commit hook published under the lock, which kept commit order across connections. Every write through `sqlx-sqlite-conn-mgr`'s single writer, and so every write through the plugin, keeps its order. - **A commit made during a statement reset notifies when the next statement starts.** `fetch_one` stops at the first row and resets the statement, and for an autocommit `INSERT ... RETURNING` the commit happens inside that reset, after SQLite's profile event. The trace hook is therefore also registered for `SQLITE_TRACE_STMT`, which fires when a statement starts, before any of its hooks. Without it, the held change waited for the next statement's profile event, and a failed next statement's rollback hook discarded it: the row was committed and no notification ever arrived. The test `a_commit_made_during_a_statement_reset_survives_a_later_failed_write` guards this. Dropping the connection also publishes it, guarded by `dropping_a_connection_publishes_a_commit_made_during_a_statement_reset`. - The trace hook occupies the connection's one `sqlite3_trace_v2` slot. sqlx does not use that slot. If a consumer installs a trace hook on an observed connection, the new hook replaces this crate's hook, and notifications from that connection stop. diff --git a/Cargo.lock b/Cargo.lock index 9ffcefb..2be3294 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -700,7 +700,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -859,7 +859,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -2060,7 +2060,7 @@ dependencies = [ "png 0.18.1", "serde", "thiserror 2.0.18", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -2115,7 +2115,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -2742,6 +2742,12 @@ version = "0.8.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a96887878f22d7bad8a3b6dc5b7440e0ada9a245242924394987b21cf2210a4c" +[[package]] +name = "relative-path" +version = "1.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba39f3699c378cd8970968dcbff9c43159ea4cfbd88d43c00b22f2ef10a435d2" + [[package]] name = "reqwest" version = "0.13.2" @@ -2776,6 +2782,32 @@ dependencies = [ "web-sys", ] +[[package]] +name = "rstest" +version = "0.28.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b376e512f30a7686a8dbc6aeaace99b3ade1c26c1c63473473a817ade226dafc" +dependencies = [ + "rstest_macros", +] + +[[package]] +name = "rstest_macros" +version = "0.28.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "88512dbfb5b73a0dfa38aef7bc73b5657035fce477b923faf12d332dd83051ea" +dependencies = [ + "cfg-if", + "glob", + "proc-macro2", + "quote", + "regex", + "relative-path", + "rustc_version", + "syn 2.0.114", + "unicode-ident", +] + [[package]] name = "rustc-hash" version = "2.1.2" @@ -2801,7 +2833,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -3395,6 +3427,7 @@ version = "0.9.0" dependencies = [ "libsqlite3-sys", "parking_lot", + "rstest", "serde", "sqlx", "tempfile", @@ -3411,6 +3444,7 @@ dependencies = [ "libsqlite3-sys", "parking_lot", "regex", + "rstest", "sqlx", "sqlx-sqlite-conn-mgr", "tempfile", @@ -3427,6 +3461,7 @@ version = "0.9.1" dependencies = [ "base64 0.22.1", "indexmap 2.13.0", + "rstest", "serde", "serde_json", "sqlx", @@ -3745,6 +3780,7 @@ dependencies = [ "base64 0.22.1", "futures", "indexmap 2.13.0", + "rstest", "serde", "serde_json", "sqlx", @@ -3871,7 +3907,7 @@ dependencies = [ "getrandom 0.4.1", "once_cell", "rustix", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -4283,7 +4319,7 @@ dependencies = [ "png 0.18.1", "serde", "thiserror 2.0.18", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -4717,7 +4753,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -4738,7 +4774,7 @@ dependencies = [ "objc2-foundation", "objc2-quartz-core", "raw-window-handle", - "windows-sys 0.60.2", + "windows-sys 0.61.2", "windows-version", ] diff --git a/Cargo.toml b/Cargo.toml index eecd270..21556a9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,3 +50,7 @@ tauri-plugin = { version = "2.7.1", features = ["build"] } tauri = { version = "2.12.0", features = ["test"] } tempfile = "3.23.0" tokio = { version = "1.48.0", features = ["rt-multi-thread", "macros"] } +# Parameterized tests. Default features stay off: `crate-name` changes feature +# resolution for toml crates that tauri's build shares, so a warm workspace build +# would rebuild tauri, and nothing here uses `async-timeout`. +rstest = { version = "0.28.0", default-features = false } diff --git a/crates/sqlx-sqlite-conn-mgr/Cargo.toml b/crates/sqlx-sqlite-conn-mgr/Cargo.toml index 2f22296..f8c5650 100644 --- a/crates/sqlx-sqlite-conn-mgr/Cargo.toml +++ b/crates/sqlx-sqlite-conn-mgr/Cargo.toml @@ -26,3 +26,5 @@ libsqlite3-sys = "0.35.0" [dev-dependencies] tempfile = "3.24.0" +# Parameterized tests. Default features stay off: see the root Cargo.toml. +rstest = { version = "0.28.0", default-features = false } diff --git a/crates/sqlx-sqlite-conn-mgr/src/attached.rs b/crates/sqlx-sqlite-conn-mgr/src/attached.rs index cc06e3b..e9480be 100644 --- a/crates/sqlx-sqlite-conn-mgr/src/attached.rs +++ b/crates/sqlx-sqlite-conn-mgr/src/attached.rs @@ -478,6 +478,7 @@ pub async fn acquire_writer_with_attached( mod tests { use super::*; use crate::SqliteDatabase; + use rstest::rstest; use sqlx::Row; use std::sync::Arc; use tempfile::TempDir; @@ -922,39 +923,33 @@ mod tests { assert!(res1.is_ok() && res2.is_ok(), "Both tasks should succeed"); } + #[rstest] + #[case::empty("")] + #[case::starts_with_digit("123invalid")] + #[case::contains_hyphen("schema-name")] + #[case::contains_space("schema name")] + #[case::sql_injection_semicolon("schema;DROP TABLE users")] + #[case::sql_injection_quote_comment("schema'--")] + #[case::contains_special_chars("schema/*comment*/")] + #[case::longer_than_max_len(&"a".repeat(MAX_SCHEMA_NAME_LEN + 1))] #[tokio::test] - async fn test_invalid_schema_names_rejected() { + async fn test_invalid_schema_names_rejected(#[case] invalid_name: &str) { let temp_dir = TempDir::new().unwrap(); let main_db = create_test_db("main.db", &temp_dir).await; let other_db = create_test_db("other.db", &temp_dir).await; - // Test various invalid schema names - let too_long = "a".repeat(MAX_SCHEMA_NAME_LEN + 1); - let invalid_names = vec![ - "", // Empty - "123invalid", // Starts with digit - "schema-name", // Contains hyphen - "schema name", // Contains space - "schema;DROP TABLE users", // SQL injection attempt - "schema'--", // SQL injection attempt - "schema/*comment*/", // Contains special chars - &too_long, // Longer than MAX_SCHEMA_NAME_LEN - ]; - - for invalid_name in invalid_names { - let specs = vec![AttachedSpec { - database: other_db.clone(), - schema_name: invalid_name.to_string(), - mode: AttachedMode::ReadOnly, - }]; + let specs = vec![AttachedSpec { + database: other_db, + schema_name: invalid_name.to_string(), + mode: AttachedMode::ReadOnly, + }]; - let result = acquire_reader_with_attached(&main_db, specs).await; - assert!( - matches!(result, Err(Error::InvalidSchemaName(_))), - "Expected InvalidSchemaName error for '{}'", - invalid_name - ); - } + let result = acquire_reader_with_attached(&main_db, specs).await; + assert!( + matches!(result, Err(Error::InvalidSchemaName(_))), + "Expected InvalidSchemaName error for '{}'", + invalid_name + ); } /// Pins the accepting side of the length cap - the rejecting side is covered @@ -1075,136 +1070,93 @@ mod tests { ); } + /// SQLite's schema namespace is case-insensitive, so "main"/"temp" must be + /// rejected regardless of case, and on both the reader and writer paths. + #[rstest] + #[case::main("main")] + #[case::temp("temp")] + #[case::main_uppercase("MAIN")] + #[case::temp_mixed_case("Temp")] #[tokio::test] - async fn test_reserved_schema_names_rejected() { + async fn test_reserved_schema_names_rejected(#[case] reserved: &str) { let temp_dir = TempDir::new().unwrap(); let main_db = create_test_db("main.db", &temp_dir).await; let other_db = create_test_db("other.db", &temp_dir).await; - // SQLite's schema namespace is case-insensitive, so "main"/"temp" must be - // rejected regardless of case, and on both the reader and writer paths. - for reserved in ["main", "temp", "MAIN", "Temp"] { - let specs = vec![AttachedSpec { - database: other_db.clone(), - schema_name: reserved.to_string(), - mode: AttachedMode::ReadOnly, - }]; - let result = acquire_reader_with_attached(&main_db, specs).await; - assert!( - matches!(result, Err(Error::InvalidSchemaName(_))), - "reader should reject reserved alias '{}', got {:?}", - reserved, - result.err() - ); - - let specs = vec![AttachedSpec { - database: other_db.clone(), - schema_name: reserved.to_string(), - mode: AttachedMode::ReadOnly, - }]; - let result = acquire_writer_with_attached(&main_db, specs).await; - assert!( - matches!(result, Err(Error::InvalidSchemaName(_))), - "writer should reject reserved alias '{}', got {:?}", - reserved, - result.err() - ); - } - - // Load-bearing: every rejection above happens in `validate_attached_specs`, - // before any lock is acquired or any `ATTACH` is issued, so nothing should be - // left attached and a normal alias should still attach cleanly afterward. - assert_only_main_attached(&main_db).await; let specs = vec![AttachedSpec { database: other_db.clone(), - schema_name: "other".to_string(), + schema_name: reserved.to_string(), mode: AttachedMode::ReadOnly, }]; + let result = acquire_reader_with_attached(&main_db, specs).await; assert!( - acquire_writer_with_attached(&main_db, specs).await.is_ok(), - "a normal alias should still attach after the reserved-name rejections above" - ); - } - - #[tokio::test] - async fn test_duplicate_schema_name_different_paths_rejected() { - let temp_dir = TempDir::new().unwrap(); - let main_db = create_test_db("main.db", &temp_dir).await; - let db_b = create_test_db("b.db", &temp_dir).await; - let db_c = create_test_db("c.db", &temp_dir).await; - - // Two different files sharing one alias pass the (path-keyed) duplicate-path - // check but must still be rejected before any `ATTACH` runs. - let specs = vec![ - AttachedSpec { - database: db_b.clone(), - schema_name: "x".to_string(), - mode: AttachedMode::ReadOnly, - }, - AttachedSpec { - database: db_c.clone(), - schema_name: "x".to_string(), - mode: AttachedMode::ReadOnly, - }, - ]; - let result = acquire_writer_with_attached(&main_db, specs).await; - assert!( - matches!(result, Err(Error::DuplicateSchemaName(_))), - "two different databases sharing alias 'x' should be rejected, got {:?}", + matches!(result, Err(Error::InvalidSchemaName(_))), + "reader should reject reserved alias '{}', got {:?}", + reserved, result.err() ); - // Load-bearing: the rejection above must not have attached either database, so - // the alias 'x' is still free to use afterward. - assert_only_main_attached(&main_db).await; let specs = vec![AttachedSpec { - database: db_b.clone(), - schema_name: "x".to_string(), + database: other_db, + schema_name: reserved.to_string(), mode: AttachedMode::ReadOnly, }]; + let result = acquire_writer_with_attached(&main_db, specs).await; assert!( - acquire_writer_with_attached(&main_db, specs).await.is_ok(), - "alias 'x' should attach cleanly after the rejected duplicate-alias pair" + matches!(result, Err(Error::InvalidSchemaName(_))), + "writer should reject reserved alias '{}', got {:?}", + reserved, + result.err() ); } + /// Two different files sharing one alias pass the (path-keyed) duplicate-path + /// check but must still be rejected before any `ATTACH` runs. + #[rstest] + #[case::same_alias("x")] + // "x" and "X" compare unequal as plain strings but collide in SQLite's + // case-insensitive schema namespace - a plain `HashSet` would miss this. + #[case::case_insensitive("X")] #[tokio::test] - async fn test_duplicate_schema_name_case_insensitive_rejected() { + async fn test_duplicate_schema_name_rejected(#[case] second_alias: &str) { let temp_dir = TempDir::new().unwrap(); let main_db = create_test_db("main.db", &temp_dir).await; - let db_x = create_test_db("x.db", &temp_dir).await; - let db_upper_x = create_test_db("upper_x.db", &temp_dir).await; + let db_b = create_test_db("b.db", &temp_dir).await; + let db_c = create_test_db("c.db", &temp_dir).await; - // "x" and "X" compare unequal as plain strings but collide in SQLite's - // case-insensitive schema namespace - a plain `HashSet` would miss this. let specs = vec![ AttachedSpec { - database: db_x.clone(), + database: db_b.clone(), schema_name: "x".to_string(), mode: AttachedMode::ReadOnly, }, AttachedSpec { - database: db_upper_x.clone(), - schema_name: "X".to_string(), + database: db_c.clone(), + schema_name: second_alias.to_string(), mode: AttachedMode::ReadOnly, }, ]; let result = acquire_writer_with_attached(&main_db, specs).await; assert!( matches!(result, Err(Error::DuplicateSchemaName(_))), - "the 'x'/'X' pair should be rejected as a case-insensitive duplicate, got {:?}", + "two different databases with aliases 'x' and '{}' should be rejected as \ + duplicates, got {:?}", + second_alias, result.err() ); + // Load-bearing: the rejection above must not have attached either database, so + // the alias 'x' is still free to use afterward. assert_only_main_attached(&main_db).await; let specs = vec![AttachedSpec { - database: db_x.clone(), + database: db_b.clone(), schema_name: "x".to_string(), mode: AttachedMode::ReadOnly, }]; assert!( acquire_writer_with_attached(&main_db, specs).await.is_ok(), - "alias 'x' should attach cleanly after the rejected 'x'/'X' pair" + "alias 'x' should attach cleanly after the rejected 'x'/'{}' pair", + second_alias ); } diff --git a/crates/sqlx-sqlite-conn-mgr/src/functions.rs b/crates/sqlx-sqlite-conn-mgr/src/functions.rs index 9ec7a8a..0c136ec 100644 --- a/crates/sqlx-sqlite-conn-mgr/src/functions.rs +++ b/crates/sqlx-sqlite-conn-mgr/src/functions.rs @@ -915,6 +915,7 @@ impl SqlValue { #[cfg(test)] mod tests { use super::*; + use rstest::rstest; /// Every test in this binary shares the process-global registry, so each one uses a /// name of its own. @@ -928,26 +929,13 @@ mod tests { } } - #[test] - fn rejects_an_empty_name() { - assert!(matches!( - register_function(function("", 1)), - Err(Error::InvalidFunctionName(_)) - )); - } - - #[test] - fn rejects_a_name_over_255_bytes() { - assert!(matches!( - register_function(function(&"x".repeat(256), 1)), - Err(Error::InvalidFunctionName(_)) - )); - } - - #[test] - fn rejects_a_name_holding_a_nul_byte() { + #[rstest] + #[case::empty("")] + #[case::over_255_bytes(&"x".repeat(256))] + #[case::holding_a_nul_byte("unit_n\0ul")] + fn rejects_an_invalid_name(#[case] name: &str) { assert!(matches!( - register_function(function("unit_n\0ul", 1)), + register_function(function(name, 1)), Err(Error::InvalidFunctionName(_)) )); } @@ -971,24 +959,19 @@ mod tests { )); } - #[test] - fn refuses_a_name_that_replaces_a_builtin() { - // Exact-arity match against a built-in: lower() takes one argument. - assert!(matches!( - register_function(function("lower", 1)), - Err(Error::ShadowsBuiltinFunction { arity: 1, .. }) - )); - - // Case-insensitive, matching how SQLite resolves names. - assert!(matches!( - register_function(function("LOWER", 1)), - Err(Error::ShadowsBuiltinFunction { arity: 1, .. }) - )); - - // A variadic built-in matches every arity: max() accepts any argument count. + /// The "lower" and "LOWER" cases share a registry key. Both are refused before + /// anything is stored, so they can run in parallel without colliding. + #[rstest] + // Exact-arity match against a built-in: lower() takes one argument. + #[case::exact_arity("lower", 1)] + // Case-insensitive, matching how SQLite resolves names. + #[case::case_insensitive("LOWER", 1)] + // A variadic built-in matches every arity: max() accepts any argument count. + #[case::variadic_builtin("max", 3)] + fn refuses_a_name_that_replaces_a_builtin(#[case] name: &str, #[case] arity: i32) { assert!(matches!( - register_function(function("max", 3)), - Err(Error::ShadowsBuiltinFunction { arity: 3, .. }) + register_function(function(name, arity)), + Err(Error::ShadowsBuiltinFunction { arity: refused, .. }) if refused == arity )); } diff --git a/crates/sqlx-sqlite-conn-mgr/src/registry.rs b/crates/sqlx-sqlite-conn-mgr/src/registry.rs index f3915ff..7e567eb 100644 --- a/crates/sqlx-sqlite-conn-mgr/src/registry.rs +++ b/crates/sqlx-sqlite-conn-mgr/src/registry.rs @@ -150,20 +150,14 @@ pub async fn uncache_database(path: &Path) -> std::io::Result<()> { #[cfg(test)] mod tests { use super::*; + use rstest::rstest; - #[test] - fn test_canonicalize_path() { - let temp_dir = std::env::temp_dir(); - let test_path = temp_dir.join("test.db"); - - // Test that path is canonicalized to absolute path - let canonical = canonicalize_database_path(&test_path).unwrap(); + #[rstest] + #[case::absolute_path(std::env::temp_dir().join("test.db"))] + #[case::relative_path(PathBuf::from("./test_relative.db"))] + fn test_canonicalize_path_returns_absolute_path(#[case] path: PathBuf) { + let canonical = canonicalize_database_path(&path).unwrap(); assert!(canonical.is_absolute()); - - // Test relative path - let relative_path = Path::new("./test_relative.db"); - let canonical_relative = canonicalize_database_path(relative_path).unwrap(); - assert!(canonical_relative.is_absolute()); } #[test] @@ -176,12 +170,11 @@ mod tests { assert!(result.is_err()); } - #[test] - fn test_mode_memory_query_param() { - assert!(is_memory_database(Path::new("file:test?mode=memory"))); - assert!(is_memory_database(Path::new( - "file:/data/db?cache=shared&mode=memory" - ))); + #[rstest] + #[case::only_param("file:test?mode=memory")] + #[case::after_another_param("file:/data/db?cache=shared&mode=memory")] + fn test_mode_memory_query_param(#[case] path: &str) { + assert!(is_memory_database(Path::new(path))); } #[test] diff --git a/crates/sqlx-sqlite-conn-mgr/tests/attached_tests.rs b/crates/sqlx-sqlite-conn-mgr/tests/attached_tests.rs index f276b91..17b0335 100644 --- a/crates/sqlx-sqlite-conn-mgr/tests/attached_tests.rs +++ b/crates/sqlx-sqlite-conn-mgr/tests/attached_tests.rs @@ -1,5 +1,6 @@ //! Integration tests for attached database functionality +use rstest::rstest; use sqlx_sqlite_conn_mgr::{ AttachedMode, AttachedSpec, Error, SqliteDatabase, acquire_reader_with_attached, acquire_writer_with_attached, @@ -236,8 +237,14 @@ async fn test_attach_multiple_databases() { conn.detach_all().await.unwrap(); } +#[rstest] +#[case::starts_with_digit("123invalid")] +#[case::contains_hyphen("invalid-name")] +#[case::contains_space("invalid name")] +#[case::contains_semicolon("invalid;drop")] +#[case::empty_string("")] #[tokio::test] -async fn test_attach_invalid_schema_name() { +async fn test_attach_rejects_invalid_schema_name(#[case] invalid_name: &str) { let temp_dir = TempDir::new().unwrap(); let main_path = temp_dir.path().join("test_attach_invalid_schema.db"); let other_path = temp_dir.path().join("test_attach_invalid_other.db"); @@ -245,30 +252,18 @@ async fn test_attach_invalid_schema_name() { let main_db = SqliteDatabase::connect(&main_path, None).await.unwrap(); let other_db = SqliteDatabase::connect(&other_path, None).await.unwrap(); - // Try invalid schema names - let invalid_names = vec![ - "123invalid", // starts with digit - "invalid-name", // contains hyphen - "invalid name", // contains space - "invalid;drop", // contains semicolon - "", // empty string - ]; + let specs = vec![AttachedSpec { + database: Arc::clone(&other_db), + schema_name: invalid_name.to_string(), + mode: AttachedMode::ReadOnly, + }]; - for invalid_name in invalid_names { - let specs = vec![AttachedSpec { - database: Arc::clone(&other_db), - schema_name: invalid_name.to_string(), - mode: AttachedMode::ReadOnly, - }]; - - let result = acquire_reader_with_attached(&main_db, specs).await; - assert!( - result.is_err(), - "Schema name '{}' should be rejected", - invalid_name - ); - assert!(matches!(result.unwrap_err(), Error::InvalidSchemaName(_))); - } + let result = acquire_reader_with_attached(&main_db, specs).await; + assert!( + matches!(result, Err(Error::InvalidSchemaName(_))), + "Schema name '{invalid_name}' should be rejected with InvalidSchemaName, got {:?}", + result.err() + ); } #[tokio::test] diff --git a/crates/sqlx-sqlite-conn-mgr/tests/function_tests.rs b/crates/sqlx-sqlite-conn-mgr/tests/function_tests.rs index c21da71..64ddc8e 100644 --- a/crates/sqlx-sqlite-conn-mgr/tests/function_tests.rs +++ b/crates/sqlx-sqlite-conn-mgr/tests/function_tests.rs @@ -8,6 +8,7 @@ use std::sync::{Arc, LazyLock}; use std::time::{Duration, Instant}; +use rstest::rstest; use sqlx::sqlite::{Sqlite, SqliteRow}; use sqlx::{Executor, FromRow}; use sqlx_sqlite_conn_mgr::{ @@ -238,39 +239,25 @@ async fn a_panicking_handler_produces_an_error_and_leaves_the_pool_usable() { assert_eq!(survivor, 1); } +/// SQLite's `quote()` renders the echoed value as an SQL literal, so one string pins both +/// the value and its storage class: text is quoted, a blob is `X'..'`, and `42` and `42.0` +/// differ. +#[rstest] +#[case::null("SELECT quote(fn_round_trip(NULL))", "NULL")] +#[case::integer("SELECT quote(fn_round_trip(42))", "42")] +#[case::real("SELECT quote(fn_round_trip(1.5))", "1.5")] +#[case::text("SELECT quote(fn_round_trip('abc'))", "'abc'")] +#[case::blob("SELECT quote(fn_round_trip(x'0102ff'))", "X'0102FF'")] #[tokio::test] -async fn every_storage_class_round_trips_and_null_passes_through() { +async fn every_storage_class_round_trips_and_null_passes_through( + #[case] sql: &'static str, + #[case] expected: &str, +) { let dir = TempDir::new().unwrap(); let db = open(&dir, None).await; - let pool = db.read_pool().unwrap(); - let null: Option = scalar(pool, "SELECT fn_round_trip(NULL)").await; - assert_eq!(null, None); - - let integer: i64 = scalar(pool, "SELECT fn_round_trip(42)").await; - assert_eq!(integer, 42); - - let real: f64 = scalar(pool, "SELECT fn_round_trip(1.5)").await; - assert_eq!(real, 1.5); - - let text: String = scalar(pool, "SELECT fn_round_trip('abc')").await; - assert_eq!(text, "abc"); - - let blob: Vec = scalar(pool, "SELECT fn_round_trip(x'0102ff')").await; - assert_eq!(blob, vec![1, 2, 255]); - - // The storage class survives the round trip, rather than everything arriving as text. - // sqlx coerces a text value into an i64, so the typed reads above pass either way. - let classes: Vec = sqlx::query_scalar( - "SELECT typeof(fn_round_trip(42)) \ - UNION ALL SELECT typeof(fn_round_trip(1.5)) \ - UNION ALL SELECT typeof(fn_round_trip('abc')) \ - UNION ALL SELECT typeof(fn_round_trip(x'01'))", - ) - .fetch_all(pool) - .await - .unwrap(); - assert_eq!(classes, vec!["integer", "real", "text", "blob"]); + let literal: String = scalar(db.read_pool().unwrap(), sql).await; + assert_eq!(literal, expected); } /// A zero-argument function drives `dispatch` with an empty argument vector, which no @@ -284,34 +271,39 @@ async fn a_zero_argument_function_resolves() { assert_eq!(value, "no arguments"); } -/// Two calls with the arguments swapped. One call cannot tell a correct argument order -/// from a reversed one. +/// Two calls with the arguments swapped. Either call alone catches a reversed order, +/// since its two arguments differ. The `swapped` case also catches a dispatch that orders +/// the arguments by value rather than by position. +#[rstest] +#[case::in_order("SELECT fn_pair('a', 'b')", "a|b")] +#[case::swapped("SELECT fn_pair('b', 'a')", "b|a")] #[tokio::test] -async fn a_two_argument_function_receives_its_arguments_in_order() { +async fn a_two_argument_function_receives_its_arguments_in_order( + #[case] sql: &'static str, + #[case] expected: &str, +) { let dir = TempDir::new().unwrap(); let db = open(&dir, None).await; - let pool = db.read_pool().unwrap(); - - let forward: String = scalar(pool, "SELECT fn_pair('a', 'b')").await; - assert_eq!(forward, "a|b"); - let reversed: String = scalar(pool, "SELECT fn_pair('b', 'a')").await; - assert_eq!(reversed, "b|a"); + let value: String = scalar(db.read_pool().unwrap(), sql).await; + assert_eq!(value, expected); } /// SQLite overloads a name by argument count, and the registry holds one entry per name /// and arity pair. Each call reaches the handler registered for its own arity. +#[rstest] +#[case::one_argument("SELECT fn_over('x')", "one argument")] +#[case::two_arguments("SELECT fn_over('x', 'y')", "two arguments")] #[tokio::test] -async fn one_name_registered_at_two_arities_resolves_at_both() { +async fn one_name_registered_at_two_arities_resolves_at_both( + #[case] sql: &'static str, + #[case] expected: &str, +) { let dir = TempDir::new().unwrap(); let db = open(&dir, None).await; - let pool = db.read_pool().unwrap(); - - let one: String = scalar(pool, "SELECT fn_over('x')").await; - assert_eq!(one, "one argument"); - let two: String = scalar(pool, "SELECT fn_over('x', 'y')").await; - assert_eq!(two, "two arguments"); + let value: String = scalar(db.read_pool().unwrap(), sql).await; + assert_eq!(value, expected); } #[tokio::test] diff --git a/crates/sqlx-sqlite-observer/Cargo.toml b/crates/sqlx-sqlite-observer/Cargo.toml index 9f53460..6c2919a 100644 --- a/crates/sqlx-sqlite-observer/Cargo.toml +++ b/crates/sqlx-sqlite-observer/Cargo.toml @@ -36,3 +36,5 @@ tokio = { version = "1.49.0", features = ["full", "macros"] } futures = "0.3.31" tempfile = "3.24.0" tracing-subscriber = "0.3.22" +# Parameterized tests. Default features stay off: see the root Cargo.toml. +rstest = { version = "0.28.0", default-features = false } diff --git a/crates/sqlx-sqlite-observer/src/schema.rs b/crates/sqlx-sqlite-observer/src/schema.rs index 1ec59e2..ad16d21 100644 --- a/crates/sqlx-sqlite-observer/src/schema.rs +++ b/crates/sqlx-sqlite-observer/src/schema.rs @@ -124,37 +124,30 @@ async fn query_pk_columns( #[cfg(test)] mod tests { use super::*; + use rstest::rstest; + + #[rstest] + #[case::trailing_clause("CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID")] + #[case::trailing_space("CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID ")] + #[case::extra_whitespace("CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID")] + #[case::lowercase("CREATE TABLE t (id TEXT PRIMARY KEY) without rowid")] + #[case::newline_before_clause("CREATE TABLE t (id TEXT PRIMARY KEY)\nWITHOUT ROWID")] + fn test_has_without_rowid_clause_detects(#[case] sql: &str) { + assert!( + has_without_rowid_clause(sql), + "{sql:?} should be detected as WITHOUT ROWID" + ); + } - #[test] - fn test_has_without_rowid_clause() { - // Positive cases - assert!(has_without_rowid_clause( - "CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID" - )); - assert!(has_without_rowid_clause( - "CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID " - )); - assert!(has_without_rowid_clause( - "CREATE TABLE t (id TEXT PRIMARY KEY) WITHOUT ROWID" - )); - assert!(has_without_rowid_clause( - "CREATE TABLE t (id TEXT PRIMARY KEY) without rowid" - )); - assert!(has_without_rowid_clause( - "CREATE TABLE t (id TEXT PRIMARY KEY)\nWITHOUT ROWID" - )); - - // Negative cases - normal tables - assert!(!has_without_rowid_clause( - "CREATE TABLE t (id INTEGER PRIMARY KEY)" - )); - - // Negative cases - false positive prevention - assert!(!has_without_rowid_clause( - "CREATE TABLE t (note TEXT DEFAULT 'see WITHOUT ROWID docs')" - )); - assert!(!has_without_rowid_clause( - "CREATE TABLE t (id INT, note TEXT) -- WITHOUT ROWID comment" - )); + #[rstest] + #[case::normal_table("CREATE TABLE t (id INTEGER PRIMARY KEY)")] + // False positive prevention + #[case::in_string_literal("CREATE TABLE t (note TEXT DEFAULT 'see WITHOUT ROWID docs')")] + #[case::in_sql_comment("CREATE TABLE t (id INT, note TEXT) -- WITHOUT ROWID comment")] + fn test_has_without_rowid_clause_ignores(#[case] sql: &str) { + assert!( + !has_without_rowid_clause(sql), + "{sql:?} should not be detected as WITHOUT ROWID" + ); } } diff --git a/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs b/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs index 1aab99a..23dd288 100644 --- a/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs +++ b/crates/sqlx-sqlite-observer/tests/conn_mgr_tests.rs @@ -14,6 +14,7 @@ use std::sync::Arc; use std::time::Duration; use futures::StreamExt; +use rstest::rstest; use sqlx_sqlite_conn_mgr::{AttachedMode, AttachedSpec, SqliteDatabase, SqliteDatabaseConfig}; use sqlx_sqlite_observer::{ChangeOperation, ObservableSqliteDatabase, ObserverConfig}; use tokio::time::timeout; @@ -160,8 +161,14 @@ async fn test_rollback_discards_changes() { // CRUD Operations // ============================================================================ +#[rstest] +#[case::update("UPDATE users SET name = 'Bob' WHERE id = 1", ChangeOperation::Update)] +#[case::delete("DELETE FROM users WHERE id = 1", ChangeOperation::Delete)] #[tokio::test] -async fn test_update_notification() { +async fn test_committed_change_notification( + #[case] sql: &'static str, + #[case] expected_operation: ChangeOperation, +) { let test_db = setup_test_db().await; let config = ObserverConfig::new().with_tables(["users"]); let observable = ObservableSqliteDatabase::new(test_db.db.clone(), config); @@ -179,45 +186,7 @@ async fn test_update_notification() { let mut writer = observable.acquire_writer().await.unwrap(); sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); - sqlx::query("UPDATE users SET name = 'Bob' WHERE id = 1") - .execute(&mut *writer) - .await - .unwrap(); - - sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); - - let change = timeout(Duration::from_millis(100), rx.recv()) - .await - .unwrap() - .unwrap(); - - assert_eq!(change.table, "users"); - assert_eq!(change.operation, Some(ChangeOperation::Update)); -} - -#[tokio::test] -async fn test_delete_notification() { - let test_db = setup_test_db().await; - let config = ObserverConfig::new().with_tables(["users"]); - let observable = ObservableSqliteDatabase::new(test_db.db.clone(), config); - - // Seed data - let mut writer = observable.acquire_writer().await.unwrap(); - sqlx::query("INSERT INTO users (name) VALUES ('Alice')") - .execute(&mut *writer) - .await - .unwrap(); - - drop(writer); - - let mut rx = observable.subscribe(["users"]); - let mut writer = observable.acquire_writer().await.unwrap(); - - sqlx::query("BEGIN").execute(&mut *writer).await.unwrap(); - sqlx::query("DELETE FROM users WHERE id = 1") - .execute(&mut *writer) - .await - .unwrap(); + sqlx::query(sql).execute(&mut *writer).await.unwrap(); sqlx::query("COMMIT").execute(&mut *writer).await.unwrap(); @@ -227,7 +196,7 @@ async fn test_delete_notification() { .unwrap(); assert_eq!(change.table, "users"); - assert_eq!(change.operation, Some(ChangeOperation::Delete)); + assert_eq!(change.operation, Some(expected_operation)); } // ============================================================================ diff --git a/crates/sqlx-sqlite-observer/tests/integration_tests.rs b/crates/sqlx-sqlite-observer/tests/integration_tests.rs index af6920b..eb61169 100644 --- a/crates/sqlx-sqlite-observer/tests/integration_tests.rs +++ b/crates/sqlx-sqlite-observer/tests/integration_tests.rs @@ -8,6 +8,7 @@ //! - Multi-subscriber: all subscribers receive notifications use futures::StreamExt; +use rstest::rstest; use sqlx::SqlitePool; use sqlx::sqlite::SqliteConnectOptions; use sqlx_sqlite_observer::{ChangeOperation, ColumnValue, ObserverConfig, SqliteObserver}; @@ -557,179 +558,71 @@ async fn test_capture_values_disabled() { // Primary Key Extraction // ============================================================================ +#[rstest] +// The PK is the auto-incremented id (1). +#[case::single_column( + "CREATE TABLE users (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL)", + "INSERT INTO users (name) VALUES ('Alice')", + "users", + vec![ColumnValue::Integer(1)], + Some(1) +)] +// PK columns are in declaration order: (user_id, role_id). +#[case::composite( + "CREATE TABLE user_roles (user_id INTEGER NOT NULL, role_id INTEGER NOT NULL, \ + granted_at TEXT, PRIMARY KEY (user_id, role_id))", + "INSERT INTO user_roles (user_id, role_id, granted_at) VALUES (42, 7, '2024-01-01')", + "user_roles", + vec![ColumnValue::Integer(42), ColumnValue::Integer(7)], + Some(1) +)] +#[case::text( + "CREATE TABLE settings (key TEXT PRIMARY KEY, value TEXT)", + "INSERT INTO settings (key, value) VALUES ('theme', 'dark')", + "settings", + vec![ColumnValue::Text("theme".to_string())], + Some(1) +)] +// A WITHOUT ROWID table has rowid=None, but its primary_key is still populated. +#[case::without_rowid_table( + "CREATE TABLE kv_store (key TEXT PRIMARY KEY, value BLOB) WITHOUT ROWID", + "INSERT INTO kv_store (key, value) VALUES ('mykey', X'DEADBEEF')", + "kv_store", + vec![ColumnValue::Text("mykey".to_string())], + None +)] #[tokio::test] -async fn test_single_column_primary_key() { - let pool = setup_test_db().await; - let config = ObserverConfig::new().with_tables(["users"]); - let observer = SqliteObserver::new(pool, config); - - let mut rx = observer.subscribe(["users"]); - let mut conn = observer.acquire().await.unwrap(); - - sqlx::query("INSERT INTO users (name) VALUES ('Alice')") - .execute(&mut **conn) - .await - .unwrap(); - - let change = timeout(Duration::from_millis(100), rx.recv()) - .await - .unwrap() - .unwrap(); - - assert_eq!(change.table, "users"); - assert!(!change.primary_key.is_empty(), "Should have primary key"); - assert_eq!(change.primary_key.len(), 1, "Single-column PK"); - - // The PK should be the auto-incremented id (1) - assert_eq!( - change.primary_key[0], - ColumnValue::Integer(1), - "PK should be id=1" - ); -} - -#[tokio::test] -async fn test_composite_primary_key() { +async fn test_insert_reports_primary_key( + #[case] create_table_sql: &'static str, + #[case] insert_sql: &'static str, + #[case] table: &str, + #[case] expected_primary_key: Vec, + #[case] expected_rowid: Option, +) { let pool = SqlitePool::connect("sqlite::memory:").await.unwrap(); + sqlx::query(create_table_sql).execute(&pool).await.unwrap(); - // Create a table with a composite primary key - sqlx::query( - r#" - CREATE TABLE user_roles ( - user_id INTEGER NOT NULL, - role_id INTEGER NOT NULL, - granted_at TEXT, - PRIMARY KEY (user_id, role_id) - ) - "#, - ) - .execute(&pool) - .await - .unwrap(); - - let config = ObserverConfig::new().with_tables(["user_roles"]); + let config = ObserverConfig::new().with_tables([table]); let observer = SqliteObserver::new(pool, config); - let mut rx = observer.subscribe(["user_roles"]); + let mut rx = observer.subscribe([table]); let mut conn = observer.acquire().await.unwrap(); - sqlx::query( - "INSERT INTO user_roles (user_id, role_id, granted_at) VALUES (42, 7, '2024-01-01')", - ) - .execute(&mut **conn) - .await - .unwrap(); + sqlx::query(insert_sql).execute(&mut **conn).await.unwrap(); let change = timeout(Duration::from_millis(100), rx.recv()) .await .unwrap() .unwrap(); - assert_eq!(change.table, "user_roles"); - assert_eq!(change.primary_key.len(), 2, "Composite PK has 2 columns"); - - // PK columns should be in declaration order: (user_id, role_id) + assert_eq!(change.table, table); assert_eq!( - change.primary_key[0], - ColumnValue::Integer(42), - "First PK column is user_id=42" + change.rowid, expected_rowid, + "rowid should be the inserted row's rowid, or None for a WITHOUT ROWID table" ); assert_eq!( - change.primary_key[1], - ColumnValue::Integer(7), - "Second PK column is role_id=7" - ); -} - -#[tokio::test] -async fn test_text_primary_key() { - let pool = SqlitePool::connect("sqlite::memory:").await.unwrap(); - - // Create a table with a TEXT primary key - sqlx::query( - r#" - CREATE TABLE settings ( - key TEXT PRIMARY KEY, - value TEXT - ) - "#, - ) - .execute(&pool) - .await - .unwrap(); - - let config = ObserverConfig::new().with_tables(["settings"]); - let observer = SqliteObserver::new(pool, config); - - let mut rx = observer.subscribe(["settings"]); - let mut conn = observer.acquire().await.unwrap(); - - sqlx::query("INSERT INTO settings (key, value) VALUES ('theme', 'dark')") - .execute(&mut **conn) - .await - .unwrap(); - - let change = timeout(Duration::from_millis(100), rx.recv()) - .await - .unwrap() - .unwrap(); - - assert_eq!(change.table, "settings"); - assert_eq!(change.primary_key.len(), 1, "Single TEXT PK"); - assert_eq!( - change.primary_key[0], - ColumnValue::Text("theme".to_string()), - "PK should be key='theme'" - ); -} - -#[tokio::test] -async fn test_without_rowid_table() { - let pool = SqlitePool::connect("sqlite::memory:").await.unwrap(); - - // Create a WITHOUT ROWID table - sqlx::query( - r#" - CREATE TABLE kv_store ( - key TEXT PRIMARY KEY, - value BLOB - ) WITHOUT ROWID - "#, - ) - .execute(&pool) - .await - .unwrap(); - - let config = ObserverConfig::new().with_tables(["kv_store"]); - let observer = SqliteObserver::new(pool, config); - - let mut rx = observer.subscribe(["kv_store"]); - let mut conn = observer.acquire().await.unwrap(); - - sqlx::query("INSERT INTO kv_store (key, value) VALUES ('mykey', X'DEADBEEF')") - .execute(&mut **conn) - .await - .unwrap(); - - let change = timeout(Duration::from_millis(100), rx.recv()) - .await - .unwrap() - .unwrap(); - - assert_eq!(change.table, "kv_store"); - - // For WITHOUT ROWID tables, rowid should be None - assert!( - change.rowid.is_none(), - "WITHOUT ROWID table should have rowid=None" - ); - - // But primary_key should still be populated - assert_eq!(change.primary_key.len(), 1); - assert_eq!( - change.primary_key[0], - ColumnValue::Text("mykey".to_string()), - "PK should be key='mykey'" + change.primary_key, expected_primary_key, + "PK should hold the inserted key columns in declaration order" ); } @@ -788,17 +681,11 @@ async fn test_delete_returns_old_primary_key() { /// assumption went untested. In WAL mode an early notification shows as a /// stale count. In rollback-journal mode it shows as a locked read, which is /// why the reader below does not wait on the lock. +#[rstest] +#[case::wal_mode("WAL")] +#[case::rollback_journal_mode("DELETE")] #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn a_notification_means_the_change_is_readable_in_wal_mode() { - a_notification_means_the_change_is_readable("WAL").await; -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn a_notification_means_the_change_is_readable_in_rollback_journal_mode() { - a_notification_means_the_change_is_readable("DELETE").await; -} - -async fn a_notification_means_the_change_is_readable(journal_mode: &str) { +async fn a_notification_means_the_change_is_readable(#[case] journal_mode: &str) { const WRITES: i64 = 50; let dir = tempfile::tempdir().unwrap(); diff --git a/crates/sqlx-sqlite-toolkit/Cargo.toml b/crates/sqlx-sqlite-toolkit/Cargo.toml index 81b5712..1298bf4 100644 --- a/crates/sqlx-sqlite-toolkit/Cargo.toml +++ b/crates/sqlx-sqlite-toolkit/Cargo.toml @@ -33,3 +33,5 @@ tracing = { version = "0.1", default-features = false, features = ["std", "relea [dev-dependencies] tempfile = "3.23.0" tokio = { version = "1.48.0", features = ["rt-multi-thread", "macros"] } +# Parameterized tests. Default features stay off: see the root Cargo.toml. +rstest = { version = "0.28.0", default-features = false } diff --git a/crates/sqlx-sqlite-toolkit/src/decode.rs b/crates/sqlx-sqlite-toolkit/src/decode.rs index 2d13a61..19977ec 100644 --- a/crates/sqlx-sqlite-toolkit/src/decode.rs +++ b/crates/sqlx-sqlite-toolkit/src/decode.rs @@ -122,19 +122,16 @@ fn base64_encode(data: &[u8]) -> String { #[cfg(test)] mod tests { use super::*; - - #[test] - fn test_base64_encode() { - assert_eq!(base64_encode(b"hello"), "aGVsbG8="); - assert_eq!(base64_encode(&[1, 2, 3, 4, 5]), "AQIDBAU="); - assert_eq!(base64_encode(&[]), ""); - } - - #[test] - fn test_base64_encode_binary() { - // Test with binary data including null bytes - assert_eq!(base64_encode(&[0, 0, 0]), "AAAA"); - assert_eq!(base64_encode(&[255, 255, 255]), "////"); + use rstest::rstest; + + #[rstest] + #[case::text(b"hello", "aGVsbG8=")] + #[case::small_bytes(&[1, 2, 3, 4, 5], "AQIDBAU=")] + #[case::empty(&[], "")] + #[case::binary_null_bytes(&[0, 0, 0], "AAAA")] + #[case::binary_high_bytes(&[255, 255, 255], "////")] + fn test_base64_encode(#[case] data: &[u8], #[case] expected: &str) { + assert_eq!(base64_encode(data), expected); } #[test] diff --git a/crates/sqlx-sqlite-toolkit/src/error.rs b/crates/sqlx-sqlite-toolkit/src/error.rs index d6a878c..6da0258 100644 --- a/crates/sqlx-sqlite-toolkit/src/error.rs +++ b/crates/sqlx-sqlite-toolkit/src/error.rs @@ -166,163 +166,146 @@ impl Error { #[cfg(test)] mod tests { use super::*; + use rstest::rstest; - #[test] - fn test_error_code_unsupported_datatype() { - let err = Error::UnsupportedDatatype("WEIRD".into()); - assert_eq!(err.error_code(), "UNSUPPORTED_DATATYPE"); - } - - #[test] - fn test_error_code_multiple_rows_returned() { - let err = Error::MultipleRowsReturned(5); - assert_eq!(err.error_code(), "MULTIPLE_ROWS_RETURNED"); - assert!(err.to_string().contains("5 rows")); - } - - #[test] - fn test_error_code_transaction_rollback_failed() { - let err = Error::TransactionRollbackFailed { + #[rstest] + #[case::unsupported_datatype( + Error::UnsupportedDatatype("WEIRD".into()), + "UNSUPPORTED_DATATYPE", + &[] + )] + #[case::multiple_rows_returned( + Error::MultipleRowsReturned(5), + "MULTIPLE_ROWS_RETURNED", + &["5 rows"] + )] + #[case::transaction_rollback_failed( + Error::TransactionRollbackFailed { transaction_error: "constraint".into(), rollback_error: "busy".into(), - }; - assert_eq!(err.error_code(), "TRANSACTION_ROLLBACK_FAILED"); - assert!(err.to_string().contains("constraint")); - assert!(err.to_string().contains("busy")); - } - - #[test] - fn test_error_code_transaction_already_finalized() { - assert_eq!( - Error::TransactionAlreadyFinalized.error_code(), - "TRANSACTION_ALREADY_FINALIZED" - ); - } - - #[test] - fn test_error_code_transaction_already_active() { - let err = Error::TransactionAlreadyActive("main.db".into()); - assert_eq!(err.error_code(), "TRANSACTION_ALREADY_ACTIVE"); - assert!(err.to_string().contains("main.db")); - } - - #[test] - fn test_error_code_no_active_transaction() { - let err = Error::NoActiveTransaction("test.db".into()); - assert_eq!(err.error_code(), "NO_ACTIVE_TRANSACTION"); - assert!(err.to_string().contains("test.db")); - } - - #[test] - fn test_error_code_invalid_transaction_token() { - assert_eq!( - Error::InvalidTransactionToken.error_code(), - "INVALID_TRANSACTION_TOKEN" - ); - } - - #[test] - fn test_error_code_io() { - let err = Error::Io(std::io::Error::new(std::io::ErrorKind::NotFound, "missing")); - assert_eq!(err.error_code(), "IO_ERROR"); - } - - #[test] - fn test_error_code_transaction_timed_out() { - let err = Error::TransactionTimedOut("test.db".into()); - assert_eq!(err.error_code(), "TRANSACTION_TIMED_OUT"); - assert!(err.to_string().contains("test.db")); - } - - #[test] - fn test_error_code_transaction_cancelled() { - let err = Error::TransactionCancelled("MAIN".into()); - assert_eq!(err.error_code(), "TRANSACTION_CANCELLED"); - assert!(err.to_string().contains("MAIN")); - } - - #[test] - fn test_error_code_transaction_cleanup_failed() { - let err = Error::TransactionCleanupFailed(vec![ + }, + "TRANSACTION_ROLLBACK_FAILED", + &["constraint", "busy"] + )] + #[case::transaction_already_finalized( + Error::TransactionAlreadyFinalized, + "TRANSACTION_ALREADY_FINALIZED", + &[] + )] + #[case::transaction_already_active( + Error::TransactionAlreadyActive("main.db".into()), + "TRANSACTION_ALREADY_ACTIVE", + &["main.db"] + )] + #[case::no_active_transaction( + Error::NoActiveTransaction("test.db".into()), + "NO_ACTIVE_TRANSACTION", + &["test.db"] + )] + #[case::invalid_transaction_token( + Error::InvalidTransactionToken, + "INVALID_TRANSACTION_TOKEN", + &[] + )] + #[case::io( + Error::Io(std::io::Error::new(std::io::ErrorKind::NotFound, "missing")), + "IO_ERROR", + &[] + )] + #[case::transaction_timed_out( + Error::TransactionTimedOut("test.db".into()), + "TRANSACTION_TIMED_OUT", + &["test.db"] + )] + #[case::transaction_cancelled( + Error::TransactionCancelled("MAIN".into()), + "TRANSACTION_CANCELLED", + &["MAIN"] + )] + #[case::transaction_cleanup_failed( + Error::TransactionCleanupFailed(vec![ Error::Other("first failure".into()), Error::Other("second failure".into()), - ]); - assert_eq!(err.error_code(), "TRANSACTION_CLEANUP_FAILED"); - assert!(err.to_string().contains("2 error(s)")); - assert!(err.to_string().contains("first failure")); - assert!(err.to_string().contains("second failure")); - assert_eq!(err.cleanup_errors().unwrap().len(), 2); - } - - #[test] - fn test_error_code_other() { - let err = Error::Other("something went wrong".into()); - assert_eq!(err.error_code(), "ERROR"); - assert_eq!(err.to_string(), "something went wrong"); - } - - #[test] - fn test_error_code_sqlx_non_database() { - // RowNotFound is not a database error, so no SQLite code - let err = Error::Sqlx(sqlx::Error::RowNotFound); - assert_eq!(err.error_code(), "SQLX_ERROR"); - } - - #[test] - fn test_error_code_empty_keyset_columns() { - let err = Error::EmptyKeysetColumns; - assert_eq!(err.error_code(), "EMPTY_KEYSET_COLUMNS"); - assert!(err.to_string().contains("at least one column")); - } - - #[test] - fn test_error_code_invalid_page_size() { - let err = Error::InvalidPageSize; - assert_eq!(err.error_code(), "INVALID_PAGE_SIZE"); - assert!(err.to_string().contains("greater than zero")); - } - - #[test] - fn test_error_code_cursor_length_mismatch() { - let err = Error::CursorLengthMismatch { + ]), + "TRANSACTION_CLEANUP_FAILED", + &["2 error(s)", "first failure", "second failure"] + )] + #[case::other(Error::Other("something went wrong".into()), "ERROR", &[])] + // RowNotFound is not a database error, so no SQLite code + #[case::sqlx_non_database(Error::Sqlx(sqlx::Error::RowNotFound), "SQLX_ERROR", &[])] + #[case::empty_keyset_columns( + Error::EmptyKeysetColumns, + "EMPTY_KEYSET_COLUMNS", + &["at least one column"] + )] + #[case::invalid_page_size(Error::InvalidPageSize, "INVALID_PAGE_SIZE", &["greater than zero"])] + #[case::cursor_length_mismatch( + Error::CursorLengthMismatch { cursor_len: 2, keyset_len: 3, - }; - assert_eq!(err.error_code(), "CURSOR_LENGTH_MISMATCH"); - assert!(err.to_string().contains("2")); - assert!(err.to_string().contains("3")); - } - - #[test] - fn test_error_code_invalid_pagination_query() { - let err = Error::InvalidPaginationQuery; - assert_eq!(err.error_code(), "INVALID_PAGINATION_QUERY"); - assert!(err.to_string().contains("top-level ORDER BY or LIMIT")); - } - - #[test] - fn test_error_code_cursor_column_not_found() { - let err = Error::CursorColumnNotFound { + }, + "CURSOR_LENGTH_MISMATCH", + &["2", "3"] + )] + #[case::invalid_pagination_query( + Error::InvalidPaginationQuery, + "INVALID_PAGINATION_QUERY", + &["top-level ORDER BY or LIMIT"] + )] + #[case::cursor_column_not_found( + Error::CursorColumnNotFound { column: "score".into(), - }; - assert_eq!(err.error_code(), "CURSOR_COLUMN_NOT_FOUND"); - assert!(err.to_string().contains("score")); + }, + "CURSOR_COLUMN_NOT_FOUND", + &["score"] + )] + #[case::invalid_column_name( + Error::InvalidColumnName { + name: "bad;name".into(), + }, + "INVALID_COLUMN_NAME", + &["bad;name"] + )] + #[case::conflicting_cursors( + Error::ConflictingCursors, + "CONFLICTING_CURSORS", + &["after", "before"] + )] + fn test_error_code_and_message( + #[case] err: Error, + #[case] expected_code: &str, + #[case] message_parts: &[&str], + ) { + assert_eq!(err.error_code(), expected_code); + + let message = err.to_string(); + for part in message_parts { + assert!( + message.contains(part), + "{message:?} should contain {part:?}" + ); + } } #[test] - fn test_error_code_invalid_column_name() { - let err = Error::InvalidColumnName { - name: "bad;name".into(), - }; - assert_eq!(err.error_code(), "INVALID_COLUMN_NAME"); - assert!(err.to_string().contains("bad;name")); + fn test_other_displays_message_verbatim() { + let err = Error::Other("something went wrong".into()); + assert_eq!(err.to_string(), "something went wrong"); } #[test] - fn test_error_code_conflicting_cursors() { - let err = Error::ConflictingCursors; - assert_eq!(err.error_code(), "CONFLICTING_CURSORS"); - assert!(err.to_string().contains("after")); - assert!(err.to_string().contains("before")); + fn test_cleanup_errors_returns_each_error() { + let err = Error::TransactionCleanupFailed(vec![ + Error::Other("first failure".into()), + Error::Other("second failure".into()), + ]); + + let messages: Vec = err + .cleanup_errors() + .unwrap() + .iter() + .map(ToString::to_string) + .collect(); + assert_eq!(messages, ["first failure", "second failure"]); } } diff --git a/crates/sqlx-sqlite-toolkit/src/pagination.rs b/crates/sqlx-sqlite-toolkit/src/pagination.rs index 8bae064..060821f 100644 --- a/crates/sqlx-sqlite-toolkit/src/pagination.rs +++ b/crates/sqlx-sqlite-toolkit/src/pagination.rs @@ -472,284 +472,195 @@ pub(crate) fn build_paginated_query( #[cfg(test)] mod tests { use super::*; + use rstest::rstest; use serde_json::json; // ─── validate_base_query ─── - #[test] - fn validate_rejects_top_level_order_by() { - let result = validate_base_query("SELECT * FROM posts ORDER BY id"); - assert!(result.is_err()); - } - - #[test] - fn validate_rejects_top_level_limit() { - let result = validate_base_query("SELECT * FROM posts LIMIT 10"); - assert!(result.is_err()); - } - - #[test] - fn validate_accepts_clean_query() { - let result = validate_base_query("SELECT * FROM posts WHERE category = ?"); - assert!(result.is_ok()); - } - - #[test] - fn validate_allows_order_by_inside_subquery() { - let result = validate_base_query("SELECT * FROM (SELECT * FROM posts ORDER BY id LIMIT 5)"); - assert!(result.is_ok()); - } - - #[test] - fn validate_allows_limit_inside_subquery() { - let result = validate_base_query("SELECT * FROM (SELECT * FROM posts LIMIT 5)"); - assert!(result.is_ok()); + #[rstest] + #[case::top_level_order_by("SELECT * FROM posts ORDER BY id")] + #[case::top_level_limit("SELECT * FROM posts LIMIT 10")] + #[case::order_by_after_subquery("SELECT * FROM (SELECT * FROM posts LIMIT 5) ORDER BY id")] + #[case::order_by_with_double_space("SELECT * FROM posts ORDER BY id")] + #[case::order_by_with_tab("SELECT * FROM posts ORDER\tBY id")] + #[case::order_by_with_newline("SELECT * FROM posts ORDER\nBY id")] + #[case::order_by_after_block_comment("SELECT * FROM posts /* comment */ ORDER BY id")] + #[case::limit_after_line_comment("SELECT * FROM posts -- comment\nLIMIT 10")] + fn validate_base_query_rejects(#[case] query: &str) { + let result = validate_base_query(query); + assert!( + matches!(result, Err(Error::InvalidPaginationQuery)), + "{query:?} should be rejected, got {result:?}" + ); } - #[test] - fn validate_rejects_order_by_after_subquery() { - let result = validate_base_query("SELECT * FROM (SELECT * FROM posts LIMIT 5) ORDER BY id"); - assert!(result.is_err()); + #[rstest] + #[case::clean_query("SELECT * FROM posts WHERE category = ?")] + #[case::order_by_inside_subquery("SELECT * FROM (SELECT * FROM posts ORDER BY id LIMIT 5)")] + #[case::limit_inside_subquery("SELECT * FROM (SELECT * FROM posts LIMIT 5)")] + #[case::order_by_in_line_comment("SELECT * FROM posts -- ORDER BY id")] + #[case::limit_in_block_comment("SELECT * FROM posts /* LIMIT 10 */")] + #[case::order_by_in_string_literal("SELECT * FROM posts WHERE name = 'ORDER BY clause'")] + // SQLite escapes single quotes by doubling: 'order''s ORDER BY clause' + #[case::keywords_in_escaped_single_quotes( + "SELECT * FROM t WHERE name = 'order''s ORDER BY clause'" + )] + #[case::keywords_in_double_quoted_identifier(r#"SELECT "ORDER BY" FROM posts"#)] + fn validate_base_query_accepts(#[case] query: &str) { + let result = validate_base_query(query); + assert!( + result.is_ok(), + "{query:?} should be accepted, got {result:?}" + ); } // ─── has_top_level_where ─── - #[test] - fn detects_top_level_where() { - assert!(has_top_level_where("SELECT * FROM posts WHERE id > 5")); - } - - #[test] - fn no_where_clause() { - assert!(!has_top_level_where("SELECT * FROM posts")); - } - - #[test] - fn where_inside_subquery_only() { - assert!(!has_top_level_where( - "SELECT * FROM (SELECT * FROM posts WHERE id > 5)" - )); - } - - // ─── scanner: comments and quoted strings ─── - - #[test] - fn validate_ignores_order_by_in_line_comment() { - let result = validate_base_query("SELECT * FROM posts -- ORDER BY id"); - assert!(result.is_ok()); - } - - #[test] - fn validate_ignores_limit_in_block_comment() { - let result = validate_base_query("SELECT * FROM posts /* LIMIT 10 */"); - assert!(result.is_ok()); - } - - #[test] - fn validate_ignores_order_by_in_string_literal() { - let result = validate_base_query("SELECT * FROM posts WHERE name = 'ORDER BY clause'"); - assert!(result.is_ok()); - } - - #[test] - fn validate_ignores_keywords_in_escaped_single_quotes() { - // SQLite escapes single quotes by doubling: 'order''s ORDER BY clause' - let result = validate_base_query("SELECT * FROM t WHERE name = 'order''s ORDER BY clause'"); - assert!(result.is_ok()); - } - - #[test] - fn validate_ignores_keywords_in_double_quoted_identifier() { - let result = validate_base_query(r#"SELECT "ORDER BY" FROM posts"#); - assert!(result.is_ok()); - } - - #[test] - fn validate_detects_order_by_after_block_comment() { - let result = validate_base_query("SELECT * FROM posts /* comment */ ORDER BY id"); - assert!(result.is_err()); - } - - #[test] - fn validate_rejects_order_by_with_extra_whitespace() { - // Double space - assert!(validate_base_query("SELECT * FROM posts ORDER BY id").is_err()); - // Tab - assert!(validate_base_query("SELECT * FROM posts ORDER\tBY id").is_err()); - // Newline - assert!(validate_base_query("SELECT * FROM posts ORDER\nBY id").is_err()); - } - - #[test] - fn validate_detects_limit_after_line_comment() { - let result = validate_base_query("SELECT * FROM posts -- comment\nLIMIT 10"); - assert!(result.is_err()); - } - - #[test] - fn where_ignores_where_in_line_comment() { - assert!(!has_top_level_where("SELECT * FROM posts -- WHERE id > 5")); + #[rstest] + #[case::top_level_where("SELECT * FROM posts WHERE id > 5")] + // The real WHERE is found; the WHERE inside the escaped-quote string is ignored + #[case::where_beside_escaped_quote_string("SELECT * FROM t WHERE name = 'it''s WHERE we go'")] + fn has_top_level_where_detects(#[case] query: &str) { + assert!( + has_top_level_where(query), + "{query:?} should have a top-level WHERE" + ); } - #[test] - fn where_ignores_where_in_string_with_escaped_quotes() { - // The real WHERE is found; the WHERE inside the escaped-quote string is ignored - assert!(has_top_level_where( - "SELECT * FROM t WHERE name = 'it''s WHERE we go'" - )); - // No real WHERE — the word WHERE is entirely inside a string with escaped quotes - assert!(!has_top_level_where("SELECT 'it''s WHERE we go' FROM t")); + #[rstest] + #[case::no_where_clause("SELECT * FROM posts")] + #[case::where_inside_subquery_only("SELECT * FROM (SELECT * FROM posts WHERE id > 5)")] + #[case::where_in_line_comment("SELECT * FROM posts -- WHERE id > 5")] + // No real WHERE — the word WHERE is entirely inside a string with escaped quotes + #[case::where_only_in_escaped_quote_string("SELECT 'it''s WHERE we go' FROM t")] + fn has_top_level_where_ignores(#[case] query: &str) { + assert!( + !has_top_level_where(query), + "{query:?} should not have a top-level WHERE" + ); } // ─── validate_column_name ─── - #[test] - fn column_name_valid_simple() { - assert!(validate_column_name("id").is_ok()); - assert!(validate_column_name("category").is_ok()); - assert!(validate_column_name("_private").is_ok()); - assert!(validate_column_name("col_123").is_ok()); - } - - #[test] - fn column_name_valid_qualified() { - assert!(validate_column_name("posts.id").is_ok()); - assert!(validate_column_name("schema.table.column").is_ok()); - } - - #[test] - fn column_name_rejects_empty() { - assert!(validate_column_name("").is_err()); - } - - #[test] - fn column_name_rejects_injection() { - assert!(validate_column_name("id; DROP TABLE posts --").is_err()); - assert!(validate_column_name("id)--").is_err()); - assert!(validate_column_name("1bad").is_err()); - assert!(validate_column_name("col name").is_err()); + #[rstest] + #[case::simple_id("id")] + #[case::simple_word("category")] + #[case::leading_underscore("_private")] + #[case::underscore_and_digits("col_123")] + #[case::qualified_table_column("posts.id")] + #[case::qualified_schema_table_column("schema.table.column")] + fn validate_column_name_accepts(#[case] name: &str) { + let result = validate_column_name(name); + assert!( + result.is_ok(), + "{name:?} should be accepted, got {result:?}" + ); } - #[test] - fn column_name_rejects_invalid_dot_structure() { - assert!(validate_column_name("table.").is_err()); // trailing dot - assert!(validate_column_name("table..column").is_err()); // consecutive dots - assert!(validate_column_name(".column").is_err()); // leading dot + #[rstest] + #[case::empty("")] + #[case::semicolon_injection("id; DROP TABLE posts --")] + #[case::paren_comment_injection("id)--")] + #[case::leading_digit("1bad")] + #[case::contains_space("col name")] + #[case::trailing_dot("table.")] + #[case::consecutive_dots("table..column")] + #[case::leading_dot(".column")] + fn validate_column_name_rejects(#[case] name: &str) { + let result = validate_column_name(name); + assert!( + matches!(&result, Err(Error::InvalidColumnName { name: rejected }) if rejected == name), + "{name:?} should be rejected as InvalidColumnName naming it, got {result:?}" + ); } // ─── build_cursor_condition ─── - #[test] - fn cursor_condition_uniform_asc() { - let keyset = vec![KeysetColumn::asc("a"), KeysetColumn::asc("b")]; - let cursor = vec![json!(1), json!(2)]; - - let (sql, values) = build_cursor_condition(&keyset, &cursor, 0); - - assert_eq!(sql, r#"("a", "b") > ($1, $2)"#); - assert_eq!(values, vec![json!(1), json!(2)]); - } - - #[test] - fn cursor_condition_uniform_asc_with_offset() { - let keyset = vec![KeysetColumn::asc("a"), KeysetColumn::asc("b")]; - let cursor = vec![json!(1), json!(2)]; - - // Simulate 2 user parameters ($1, $2) preceding the cursor - let (sql, values) = build_cursor_condition(&keyset, &cursor, 2); - - assert_eq!(sql, r#"("a", "b") > ($3, $4)"#); - assert_eq!(values, vec![json!(1), json!(2)]); - } - - #[test] - fn cursor_condition_uniform_desc() { - let keyset = vec![KeysetColumn::desc("a"), KeysetColumn::desc("b")]; - let cursor = vec![json!(10), json!(20)]; - - let (sql, values) = build_cursor_condition(&keyset, &cursor, 0); - - assert_eq!(sql, r#"("a", "b") < ($1, $2)"#); - assert_eq!(values, vec![json!(10), json!(20)]); - } - - #[test] - fn cursor_condition_mixed_directions() { - let keyset = vec![ + #[rstest] + #[case::uniform_asc( + vec![KeysetColumn::asc("a"), KeysetColumn::asc("b")], + vec![json!(1), json!(2)], + 0, + r#"("a", "b") > ($1, $2)"#, + vec![json!(1), json!(2)] + )] + // Simulate 2 user parameters ($1, $2) preceding the cursor + #[case::uniform_asc_with_offset( + vec![KeysetColumn::asc("a"), KeysetColumn::asc("b")], + vec![json!(1), json!(2)], + 2, + r#"("a", "b") > ($3, $4)"#, + vec![json!(1), json!(2)] + )] + #[case::uniform_desc( + vec![KeysetColumn::desc("a"), KeysetColumn::desc("b")], + vec![json!(10), json!(20)], + 0, + r#"("a", "b") < ($1, $2)"#, + vec![json!(10), json!(20)] + )] + #[case::mixed_directions( + vec![ KeysetColumn::asc("a"), KeysetColumn::desc("b"), KeysetColumn::asc("c"), - ]; - let cursor = vec![json!("va"), json!("vb"), json!("vc")]; - - let (sql, values) = build_cursor_condition(&keyset, &cursor, 0); - - assert_eq!( - sql, - r#"("a" > $1) OR ("a" = $2 AND "b" < $3) OR ("a" = $4 AND "b" = $5 AND "c" > $6)"# - ); - assert_eq!( - values, - vec![ - json!("va"), - json!("va"), - json!("vb"), - json!("va"), - json!("vb"), - json!("vc"), - ] - ); - } - - #[test] - fn cursor_condition_mixed_directions_with_offset() { - let keyset = vec![ + ], + vec![json!("va"), json!("vb"), json!("vc")], + 0, + r#"("a" > $1) OR ("a" = $2 AND "b" < $3) OR ("a" = $4 AND "b" = $5 AND "c" > $6)"#, + vec![ + json!("va"), + json!("va"), + json!("vb"), + json!("va"), + json!("vb"), + json!("vc"), + ] + )] + // Simulate 1 user parameter ($1) preceding the cursor + #[case::mixed_directions_with_offset( + vec![ KeysetColumn::asc("a"), KeysetColumn::desc("b"), KeysetColumn::asc("c"), - ]; - let cursor = vec![json!("va"), json!("vb"), json!("vc")]; - - // Simulate 1 user parameter ($1) preceding the cursor - let (sql, values) = build_cursor_condition(&keyset, &cursor, 1); - - assert_eq!( - sql, - r#"("a" > $2) OR ("a" = $3 AND "b" < $4) OR ("a" = $5 AND "b" = $6 AND "c" > $7)"# - ); - assert_eq!( - values, - vec![ - json!("va"), - json!("va"), - json!("vb"), - json!("va"), - json!("vb"), - json!("vc"), - ] - ); - } - - #[test] - fn cursor_condition_single_column_asc() { - let keyset = vec![KeysetColumn::asc("id")]; - let cursor = vec![json!(42)]; - - let (sql, values) = build_cursor_condition(&keyset, &cursor, 0); - - assert_eq!(sql, r#"("id") > ($1)"#); - assert_eq!(values, vec![json!(42)]); - } - - #[test] - fn cursor_condition_single_column_desc() { - let keyset = vec![KeysetColumn::desc("id")]; - let cursor = vec![json!(42)]; - - let (sql, values) = build_cursor_condition(&keyset, &cursor, 0); - - assert_eq!(sql, r#"("id") < ($1)"#); - assert_eq!(values, vec![json!(42)]); + ], + vec![json!("va"), json!("vb"), json!("vc")], + 1, + r#"("a" > $2) OR ("a" = $3 AND "b" < $4) OR ("a" = $5 AND "b" = $6 AND "c" > $7)"#, + vec![ + json!("va"), + json!("va"), + json!("vb"), + json!("va"), + json!("vb"), + json!("vc"), + ] + )] + #[case::single_column_asc( + vec![KeysetColumn::asc("id")], + vec![json!(42)], + 0, + r#"("id") > ($1)"#, + vec![json!(42)] + )] + #[case::single_column_desc( + vec![KeysetColumn::desc("id")], + vec![json!(42)], + 0, + r#"("id") < ($1)"#, + vec![json!(42)] + )] + fn cursor_condition_builds_sql_and_values( + #[case] keyset: Vec, + #[case] cursor: Vec, + #[case] param_offset: usize, + #[case] expected_sql: &str, + #[case] expected_values: Vec, + ) { + let (sql, values) = build_cursor_condition(&keyset, &cursor, param_offset); + + assert_eq!(sql, expected_sql); + assert_eq!(values, expected_values); } // ─── build_order_by ─── @@ -823,10 +734,11 @@ mod tests { fn paginated_query_strips_trailing_semicolon() { let keyset = vec![KeysetColumn::asc("id")]; - let (sql, _) = + let (sql, values) = build_paginated_query("SELECT * FROM posts;", &keyset, None, 10, false, 0).unwrap(); assert_eq!(sql, r#"SELECT * FROM posts ORDER BY "id" ASC LIMIT 11"#); + assert!(values.is_empty()); } #[test] @@ -876,10 +788,11 @@ mod tests { // ─── SortDirection::reversed ─── - #[test] - fn sort_direction_reversed() { - assert_eq!(SortDirection::Asc.reversed(), SortDirection::Desc); - assert_eq!(SortDirection::Desc.reversed(), SortDirection::Asc); + #[rstest] + #[case::asc(SortDirection::Asc, SortDirection::Desc)] + #[case::desc(SortDirection::Desc, SortDirection::Asc)] + fn sort_direction_reversed(#[case] direction: SortDirection, #[case] expected: SortDirection) { + assert_eq!(direction.reversed(), expected); } // ─── build_paginated_query backward ─── @@ -994,43 +907,34 @@ mod tests { // ─── quote_identifier ─── - #[test] - fn quote_identifier_simple() { - assert_eq!(quote_identifier("id"), r#""id""#); - } - - #[test] - fn quote_identifier_with_dot() { - assert_eq!(quote_identifier("t.id"), r#""t"."id""#); - } - - #[test] - fn quote_identifier_schema_table_column() { - assert_eq!( - quote_identifier("schema.table.col"), - r#""schema"."table"."col""# - ); + #[rstest] + #[case::simple("id", r#""id""#)] + #[case::with_dot("t.id", r#""t"."id""#)] + #[case::schema_table_column("schema.table.col", r#""schema"."table"."col""#)] + fn quote_identifier_quotes_each_part(#[case] name: &str, #[case] expected: &str) { + assert_eq!(quote_identifier(name), expected); } // ─── SortDirection serde ─── - #[test] - fn sort_direction_serializes_to_camel_case() { - assert_eq!( - serde_json::to_string(&SortDirection::Asc).unwrap(), - "\"asc\"" - ); - assert_eq!( - serde_json::to_string(&SortDirection::Desc).unwrap(), - "\"desc\"" - ); - } - - #[test] - fn sort_direction_deserializes_from_camel_case() { - let asc: SortDirection = serde_json::from_str("\"asc\"").unwrap(); - let desc: SortDirection = serde_json::from_str("\"desc\"").unwrap(); - assert_eq!(asc, SortDirection::Asc); - assert_eq!(desc, SortDirection::Desc); + #[rstest] + #[case::asc(SortDirection::Asc, "\"asc\"")] + #[case::desc(SortDirection::Desc, "\"desc\"")] + fn sort_direction_serializes_to_camel_case( + #[case] direction: SortDirection, + #[case] expected: &str, + ) { + assert_eq!(serde_json::to_string(&direction).unwrap(), expected); + } + + #[rstest] + #[case::asc("\"asc\"", SortDirection::Asc)] + #[case::desc("\"desc\"", SortDirection::Desc)] + fn sort_direction_deserializes_from_camel_case( + #[case] json: &str, + #[case] expected: SortDirection, + ) { + let direction: SortDirection = serde_json::from_str(json).unwrap(); + assert_eq!(direction, expected); } } diff --git a/crates/sqlx-sqlite-toolkit/tests/pagination_tests.rs b/crates/sqlx-sqlite-toolkit/tests/pagination_tests.rs index 1479a08..282b6a5 100644 --- a/crates/sqlx-sqlite-toolkit/tests/pagination_tests.rs +++ b/crates/sqlx-sqlite-toolkit/tests/pagination_tests.rs @@ -1,4 +1,5 @@ -use serde_json::json; +use rstest::rstest; +use serde_json::{Value as JsonValue, json}; use sqlx_sqlite_toolkit::{DatabaseWrapper, Error, KeysetColumn, KeysetPage}; use tempfile::TempDir; @@ -64,27 +65,37 @@ fn row_ids(page: &KeysetPage) -> Vec { // ─── Core Forward Pagination ─── +#[rstest] +// ── Page 1 (no cursor) ── +// Generated SQL: +// SELECT id, title FROM posts ORDER BY id ASC LIMIT 4 +// +// The builder appends ORDER BY from the keyset and LIMIT = page_size + 1. +// The extra row is a "sentinel" — if it comes back, there are more pages. +#[case::asc_single_column(vec![KeysetColumn::asc("id")], &[1, 2, 3], vec![json!(3)])] +// ── Page 1 (descending, no cursor) ── +// Generated SQL: +// SELECT id, title FROM posts ORDER BY id DESC LIMIT 4 +// +// Descending keyset means the highest IDs come first. +#[case::desc_single_column(vec![KeysetColumn::desc("id")], &[7, 6, 5], vec![json!(5)])] #[tokio::test] -async fn first_page_no_cursor() { +async fn first_page_no_cursor( + #[case] keyset: Vec, + #[case] expected_ids: &[i64], + #[case] next_cursor: Vec, +) { let (db, _temp) = create_test_db().await; seed_posts_table(&db).await; - let keyset = vec![KeysetColumn::asc("id")]; - - // ── Page 1 (no cursor) ── - // Generated SQL: - // SELECT id, title FROM posts ORDER BY id ASC LIMIT 4 - // - // The builder appends ORDER BY from the keyset and LIMIT = page_size + 1. - // The extra row is a "sentinel" — if it comes back, there are more pages. let page = db .fetch_page("SELECT id, title FROM posts".into(), vec![], keyset, 3) .await .unwrap(); - assert_eq!(row_ids(&page), vec![1, 2, 3]); + assert_eq!(row_ids(&page), expected_ids); assert!(page.has_more); - assert_eq!(page.next_cursor, Some(vec![json!(3)])); + assert_eq!(page.next_cursor, Some(next_cursor)); db.remove().await.unwrap(); } @@ -152,30 +163,6 @@ async fn forward_pagination_all_pages() { db.remove().await.unwrap(); } -#[tokio::test] -async fn desc_keyset_single_column() { - let (db, _temp) = create_test_db().await; - seed_posts_table(&db).await; - - let keyset = vec![KeysetColumn::desc("id")]; - - // ── Page 1 (descending, no cursor) ── - // Generated SQL: - // SELECT id, title FROM posts ORDER BY id DESC LIMIT 4 - // - // Descending keyset means the highest IDs come first. - let page = db - .fetch_page("SELECT id, title FROM posts".into(), vec![], keyset, 3) - .await - .unwrap(); - - assert_eq!(row_ids(&page), vec![7, 6, 5]); - assert!(page.has_more); - assert_eq!(page.next_cursor, Some(vec![json!(5)])); - - db.remove().await.unwrap(); -} - // ─── Backward Pagination ─── #[tokio::test] @@ -398,48 +385,29 @@ async fn empty_table() { db.remove().await.unwrap(); } +// fetch_page asks for page_size + 1 rows; the extra row is the sentinel that sets has_more. +#[rstest] +// 3 rows = page_size: no sentinel row, so has_more is false. +#[case::exactly_page_size_rows(3, 3, &[1, 2, 3], false, None)] +// 4 rows = page_size + 1: the sentinel sets has_more and is truncated away. +#[case::page_size_plus_one_rows(4, 3, &[1, 2, 3], true, Some(vec![json!(3)]))] +// 1 row, well under page_size (10): no sentinel, so has_more is false. +#[case::single_row(1, 10, &[1], false, None)] #[tokio::test] -async fn exactly_page_size_rows() { - let (db, _temp) = create_test_db().await; - - db.execute("CREATE TABLE t (id INTEGER PRIMARY KEY)".into(), vec![]) - .await - .unwrap(); - - for id in 1..=3 { - db.execute("INSERT INTO t (id) VALUES ($1)".into(), vec![json!(id)]) - .await - .unwrap(); - } - - let keyset = vec![KeysetColumn::asc("id")]; - - // ── Exactly page_size rows ── - // Generated SQL: - // SELECT id FROM t ORDER BY id ASC LIMIT 4 - // - // 3 rows returned = page_size. No sentinel row → has_more=false. - let page = db - .fetch_page("SELECT id FROM t".into(), vec![], keyset, 3) - .await - .unwrap(); - - assert_eq!(row_ids(&page), vec![1, 2, 3]); - assert!(!page.has_more); - assert_eq!(page.next_cursor, None); - - db.remove().await.unwrap(); -} - -#[tokio::test] -async fn page_size_plus_one_rows() { +async fn has_more_only_when_rows_exceed_page_size( + #[case] row_count: i64, + #[case] page_size: usize, + #[case] expected_ids: &[i64], + #[case] has_more: bool, + #[case] next_cursor: Option>, +) { let (db, _temp) = create_test_db().await; db.execute("CREATE TABLE t (id INTEGER PRIMARY KEY)".into(), vec![]) .await .unwrap(); - for id in 1..=4 { + for id in 1..=row_count { db.execute("INSERT INTO t (id) VALUES ($1)".into(), vec![json!(id)]) .await .unwrap(); @@ -447,51 +415,14 @@ async fn page_size_plus_one_rows() { let keyset = vec![KeysetColumn::asc("id")]; - // ── page_size + 1 rows ── - // Generated SQL: - // SELECT id FROM t ORDER BY id ASC LIMIT 4 - // - // 4 rows returned > page_size (3). Sentinel present → has_more=true. - // Truncated to 3 rows. let page = db - .fetch_page("SELECT id FROM t".into(), vec![], keyset, 3) + .fetch_page("SELECT id FROM t".into(), vec![], keyset, page_size) .await .unwrap(); - assert_eq!(row_ids(&page), vec![1, 2, 3]); - assert!(page.has_more); - assert_eq!(page.next_cursor, Some(vec![json!(3)])); - - db.remove().await.unwrap(); -} - -#[tokio::test] -async fn single_row() { - let (db, _temp) = create_test_db().await; - - db.execute("CREATE TABLE t (id INTEGER PRIMARY KEY)".into(), vec![]) - .await - .unwrap(); - - db.execute("INSERT INTO t (id) VALUES ($1)".into(), vec![json!(1)]) - .await - .unwrap(); - - let keyset = vec![KeysetColumn::asc("id")]; - - // ── Single row, generous page_size ── - // Generated SQL: - // SELECT id FROM t ORDER BY id ASC LIMIT 11 - // - // 1 row << page_size (10). No sentinel → has_more=false. - let page = db - .fetch_page("SELECT id FROM t".into(), vec![], keyset, 10) - .await - .unwrap(); - - assert_eq!(row_ids(&page), vec![1]); - assert!(!page.has_more); - assert_eq!(page.next_cursor, None); + assert_eq!(row_ids(&page), expected_ids); + assert_eq!(page.has_more, has_more); + assert_eq!(page.next_cursor, next_cursor); db.remove().await.unwrap(); } @@ -591,30 +522,36 @@ async fn where_clause_multiple_params_combined_with_cursor() { // ─── Error Cases ─── +#[rstest] +#[case::empty_keyset("SELECT 1", vec![], 10, "EMPTY_KEYSET_COLUMNS")] +#[case::zero_page_size("SELECT 1", vec![KeysetColumn::asc("id")], 0, "INVALID_PAGE_SIZE")] +#[case::query_contains_order_by( + "SELECT id FROM posts ORDER BY id", + vec![KeysetColumn::asc("id")], + 10, + "INVALID_PAGINATION_QUERY" +)] +#[case::query_contains_limit( + "SELECT id FROM posts LIMIT 10", + vec![KeysetColumn::asc("id")], + 10, + "INVALID_PAGINATION_QUERY" +)] #[tokio::test] -async fn error_empty_keyset() { +async fn error_invalid_page_request( + #[case] query: &str, + #[case] keyset: Vec, + #[case] page_size: usize, + #[case] expected_code: &str, +) { let (db, _temp) = create_test_db().await; let err = db - .fetch_page("SELECT 1".into(), vec![], vec![], 10) + .fetch_page(query.into(), vec![], keyset, page_size) .await .unwrap_err(); - assert!(matches!(err, Error::EmptyKeysetColumns)); - - db.remove().await.unwrap(); -} - -#[tokio::test] -async fn error_zero_page_size() { - let (db, _temp) = create_test_db().await; - - let err = db - .fetch_page("SELECT 1".into(), vec![], vec![KeysetColumn::asc("id")], 0) - .await - .unwrap_err(); - - assert!(matches!(err, Error::InvalidPageSize)); + assert_eq!(err.error_code(), expected_code, "got {err:?}"); db.remove().await.unwrap(); } @@ -640,41 +577,3 @@ async fn error_cursor_length_mismatch() { db.remove().await.unwrap(); } - -#[tokio::test] -async fn error_query_contains_order_by() { - let (db, _temp) = create_test_db().await; - - let err = db - .fetch_page( - "SELECT id FROM posts ORDER BY id".into(), - vec![], - vec![KeysetColumn::asc("id")], - 10, - ) - .await - .unwrap_err(); - - assert!(matches!(err, Error::InvalidPaginationQuery)); - - db.remove().await.unwrap(); -} - -#[tokio::test] -async fn error_query_contains_limit() { - let (db, _temp) = create_test_db().await; - - let err = db - .fetch_page( - "SELECT id FROM posts LIMIT 10".into(), - vec![], - vec![KeysetColumn::asc("id")], - 10, - ) - .await - .unwrap_err(); - - assert!(matches!(err, Error::InvalidPaginationQuery)); - - db.remove().await.unwrap(); -} diff --git a/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs b/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs index 4307d34..330dd19 100644 --- a/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs +++ b/crates/sqlx-sqlite-toolkit/tests/transaction_state_tests.rs @@ -1,5 +1,6 @@ //! Tests for transaction state management types. +use rstest::rstest; use serde_json::json; use sqlx_sqlite_toolkit::{ ActiveInterruptibleTransactions, ActiveRegularTransactions, DatabaseWrapper, Error, @@ -353,11 +354,18 @@ async fn test_regular_abort_all_clears_state() { state.insert("main".into(), "a".into(), h3).await; } +#[rstest] +#[case::only_matching_db_key("main", "other")] +// Closing `a` must not abort the transaction belonging to database `a:b`. +#[case::colon_prefix_alias("a", "a:b")] #[tokio::test] -async fn test_regular_abort_for_db_only_matching_db_key() { +async fn test_regular_abort_for_db_aborts_only_matching_db( + #[case] target_db: &str, + #[case] other_db: &str, +) { let state = ActiveRegularTransactions::default(); - let main_handle = tokio::spawn(async { + let target_handle = tokio::spawn(async { tokio::time::sleep(std::time::Duration::from_secs(60)).await; }); let other_handle = tokio::spawn(async { @@ -366,73 +374,33 @@ async fn test_regular_abort_for_db_only_matching_db_key() { // Keep abort handles to observe the tasks once the tracker owns them. // abort_for_db awaits every task it aborts, so is_finished() is settled. - let main_task = main_handle.abort_handle(); + let target_task = target_handle.abort_handle(); let other_task = other_handle.abort_handle(); state - .insert("main".into(), "main-one".into(), main_handle) + .insert(target_db.into(), "target-tx".into(), target_handle) .await; state - .insert("other".into(), "other-two".into(), other_handle) + .insert(other_db.into(), "other-tx".into(), other_handle) .await; - state.abort_for_db("main").await.unwrap(); + state.abort_for_db(target_db).await.unwrap(); assert!( - main_task.is_finished(), - "abort_for_db(\"main\") should abort and await the `main` transaction task" + target_task.is_finished(), + "abort_for_db({target_db:?}) should abort and await the {target_db:?} transaction task" ); assert!( !other_task.is_finished(), - "abort_for_db(\"main\") must not abort the `other` transaction task" + "abort_for_db({target_db:?}) must not abort the {other_db:?} transaction task" ); - // Other database transaction should still be tracked and abortable. - state.abort_for_db("other").await.unwrap(); + // The other database's transaction should still be tracked and abortable. + state.abort_for_db(other_db).await.unwrap(); assert!( other_task.is_finished(), - "`other` transaction should still be tracked so abort_for_db(\"other\") aborts it" - ); -} - -#[tokio::test] -async fn test_regular_abort_for_db_does_not_match_colon_prefix_alias() { - let state = ActiveRegularTransactions::default(); - - let a_handle = tokio::spawn(async { - tokio::time::sleep(std::time::Duration::from_secs(60)).await; - }); - let ab_handle = tokio::spawn(async { - tokio::time::sleep(std::time::Duration::from_secs(60)).await; - }); - - // Keep abort handles to observe the tasks once the tracker owns them. - // abort_for_db awaits every task it aborts, so is_finished() is settled. - let a_task = a_handle.abort_handle(); - let ab_task = ab_handle.abort_handle(); - - state.insert("a".into(), "a-tx".into(), a_handle).await; - state.insert("a:b".into(), "ab-tx".into(), ab_handle).await; - - state.abort_for_db("a").await.unwrap(); - - assert!( - a_task.is_finished(), - "abort_for_db(\"a\") should abort and await the `a` transaction task" - ); - - // Closing `a` must not abort the transaction belonging to database `a:b`. - assert!( - !ab_task.is_finished(), - "abort_for_db(\"a\") must not abort the `a:b` transaction task" - ); - - state.abort_for_db("a:b").await.unwrap(); - - assert!( - ab_task.is_finished(), - "`a:b` transaction should still be tracked so abort_for_db(\"a:b\") aborts it" + "{other_db:?} transaction should still be tracked so abort_for_db({other_db:?}) aborts it" ); } diff --git a/crates/sqlx-sqlite-toolkit/tests/wrapper_tests.rs b/crates/sqlx-sqlite-toolkit/tests/wrapper_tests.rs index 4a0bb7d..0f5e971 100644 --- a/crates/sqlx-sqlite-toolkit/tests/wrapper_tests.rs +++ b/crates/sqlx-sqlite-toolkit/tests/wrapper_tests.rs @@ -1,3 +1,4 @@ +use rstest::rstest; use serde_json::{Value as JsonValue, json}; use sqlx_sqlite_toolkit::DatabaseWrapper; use tempfile::TempDir; @@ -218,25 +219,60 @@ async fn test_transactions() { db.remove().await.unwrap(); } +const TYPES_TABLE_DDL: &str = "CREATE TABLE t (id INTEGER PRIMARY KEY, txt TEXT, num REAL, \ + big INTEGER, flag BOOLEAN, data BLOB)"; + +const LARGE_INT: i64 = 9_007_199_254_740_992; // 2^53 + +#[rstest] +#[case::null_decoding( + "INSERT INTO t (txt) VALUES ($1)", + vec![JsonValue::Null], + "txt", + JsonValue::Null +)] +#[case::large_integer_precision( + "INSERT INTO t (big) VALUES ($1)", + vec![json!(LARGE_INT)], + "big", + json!(LARGE_INT) +)] +#[case::boolean_stored_as_integer("INSERT INTO t (flag) VALUES (TRUE)", vec![], "flag", json!(1))] +// BLOB ("Hello" in hex) +#[case::blob_as_base64( + "INSERT INTO t (data) VALUES (X'48656C6C6F')", + vec![], + "data", + json!("SGVsbG8=") +)] #[tokio::test] -async fn test_type_binding_and_decoding() { +async fn test_type_binding_and_decoding( + #[case] insert_sql: &str, + #[case] values: Vec, + #[case] column: &str, + #[case] expected: JsonValue, +) { let (db, _temp) = create_test_db().await; - db.execute( - "CREATE TABLE t (id INTEGER PRIMARY KEY, txt TEXT, num REAL, big INTEGER, flag BOOLEAN, data BLOB)".into(), - vec![], - ) - .await - .unwrap(); + db.execute(TYPES_TABLE_DDL.into(), vec![]).await.unwrap(); - let large_int: i64 = 9_007_199_254_740_992; // 2^53 + db.execute(insert_sql.into(), values).await.unwrap(); - // Insert with various types including NULL - db.execute( - "INSERT INTO t (txt) VALUES ($1)".into(), - vec![JsonValue::Null], - ) - .await - .unwrap(); + let rows = db + .fetch_all("SELECT * FROM t ORDER BY id".into(), vec![]) + .await + .unwrap(); + + assert_eq!(rows[0].get(column), Some(&expected)); + + db.remove().await.unwrap(); +} + +// A float is compared with a tolerance rather than by equality, so it stays out +// of the `test_type_binding_and_decoding` table. +#[tokio::test] +async fn test_float_binding_and_decoding() { + let (db, _temp) = create_test_db().await; + db.execute(TYPES_TABLE_DDL.into(), vec![]).await.unwrap(); db.execute( "INSERT INTO t (txt, num) VALUES ($1, $2)".into(), @@ -245,44 +281,15 @@ async fn test_type_binding_and_decoding() { .await .unwrap(); - db.execute( - "INSERT INTO t (big) VALUES ($1)".into(), - vec![json!(large_int)], - ) - .await - .unwrap(); - - // Boolean - db.execute("INSERT INTO t (flag) VALUES (TRUE)".into(), vec![]) - .await - .unwrap(); - - // BLOB ("Hello" in hex) - db.execute("INSERT INTO t (data) VALUES (X'48656C6C6F')".into(), vec![]) - .await - .unwrap(); - let rows = db .fetch_all("SELECT * FROM t ORDER BY id".into(), vec![]) .await .unwrap(); - // NULL decoding - assert_eq!(rows[0].get("txt"), Some(&JsonValue::Null)); - // Float decoding (with tolerance) - let num = rows[1].get("num").unwrap().as_f64().unwrap(); + let num = rows[0].get("num").unwrap().as_f64().unwrap(); assert!((num - 1.23456).abs() < 0.0001); - // Large integer precision - assert_eq!(rows[2].get("big"), Some(&json!(large_int))); - - // Boolean stored as integer - assert_eq!(rows[3].get("flag"), Some(&json!(1))); - - // BLOB as base64 - assert_eq!(rows[4].get("data").unwrap().as_str(), Some("SGVsbG8=")); - db.remove().await.unwrap(); } diff --git a/package.json b/package.json index 46eebde..46e796f 100644 --- a/package.json +++ b/package.json @@ -29,8 +29,9 @@ "markdownlint": "markdownlint-cli2", "prepare": "rollup -c", "standards": "npm run eslint && npm run type-check && npm run markdownlint && npm run check:iife && npm run rust:lint && npm run commitlint && npm run check-node-version", - "rust:lint": "cargo lint-clippy && cargo lint-fmt", + "rust:lint": "cargo lint-clippy && cargo lint-fmt && npm run rust:lint:rstest", "rust:lint:fix": "cargo fix-clippy && cargo fix-fmt", + "rust:lint:rstest": "node scripts/lint-rstest-attributes.cjs", "test": "vitest run && cargo test --workspace --lib --test '*'", "test:rust": "cargo test --workspace --lib --test '*'", "test:rust:unit": "cargo test --workspace --lib", diff --git a/scripts/lint-rstest-attributes.cjs b/scripts/lint-rstest-attributes.cjs new file mode 100644 index 0000000..3219926 --- /dev/null +++ b/scripts/lint-rstest-attributes.cjs @@ -0,0 +1,139 @@ +/** + * Checks the attribute order of #[rstest] functions in the Rust sources. + * + * On rstest 0.28, a test attribute placed before a #[case] silently drops cases, and + * clippy does not warn: `#[rstest] #[test] #[case] #[case]` generates only the first + * case. This script flags any test attribute (#[test], #[tokio::test] or another + * #[::test]) above the last #[case]. The forms this repo uses are: + * + * sync: #[rstest], the #[case] lines, then the fn (rstest adds #[test] itself) + * async: #[rstest], the #[case] lines, #[tokio::test], then the fn + * + * Usage: node scripts/lint-rstest-attributes.cjs [dir ...] (default: crates src) + */ + +'use strict'; + +const fs = require('fs/promises'), + path = require('path'); + +const REPO_ROOT = path.resolve(__dirname, '..'), + DEFAULT_ROOTS = [ 'crates', 'src' ], + SKIP_DIRS = new Set([ 'target', 'node_modules' ]), + RSTEST_ATTR = /^#\[(?:(?:::)?rstest::)?rstest\s*[\](]/, + CASE_ATTR = /^#\[case\b/, + PLAIN_TEST_ATTR = /^#\[test\s*\]/, + // #[tokio::test], #[tokio::test(flavor = "multi_thread")] or any #[::test] + PATH_TEST_ATTR = /^#\[((?:::)?(?:\w+::)+test)\s*[\](]/, + FN_LINE = /^(?:(?:pub(?:\([^)]*\))?|const|async|unsafe)\s+)*fn\s/; + +async function findRustFiles(dir) { + const entries = await fs.readdir(dir, { withFileTypes: true }); + + const nested = await Promise.all(entries.map((entry) => { + const entryPath = path.join(dir, entry.name); + + if (entry.isDirectory()) { + return SKIP_DIRS.has(entry.name) ? [] : findRustFiles(entryPath); + } + return entry.isFile() && entry.name.endsWith('.rs') ? [ entryPath ] : []; + })); + + return nested.flat(); +} + +function checkAttributes(file, attrs) { + const lastCase = attrs.findLastIndex((attr) => { return CASE_ATTR.test(attr.text); }); + + return attrs.flatMap((attr, index) => { + const pathTest = PATH_TEST_ATTR.exec(attr.text), + at = `${file}:${attr.line}:`; + + if (PLAIN_TEST_ATTR.test(attr.text) && index < lastCase) { + return [ `${at} #[test] before the last #[case] silently drops cases; remove it (rstest adds the test attribute itself)` ]; + } + if (pathTest && index < lastCase) { + return [ `${at} #[${pathTest[1]}] before the last #[case] silently drops cases; move it after the last #[case]` ]; + } + return []; + }); +} + +/** + * A block starts at an #[rstest] attribute and runs until its fn line. Inside it, only a + * line starting with #[ begins an attribute: other lines are comments, blank lines or + * the rest of a multi-line attribute. + */ +function checkSource(file, source) { + const problems = []; + + let attrs = null, + rstestLine = 0, + functionCount = 0; + + source.split(/\r?\n/).forEach((rawLine, index) => { + const line = rawLine.trim(); + + if (attrs === null) { + if (RSTEST_ATTR.test(line)) { + attrs = []; + rstestLine = index + 1; + } + return; + } + + if (FN_LINE.test(line)) { + problems.push(...checkAttributes(file, attrs)); + functionCount += 1; + attrs = null; + } else if (line.startsWith('#[')) { + attrs.push({ line: index + 1, text: line }); + } + }); + + if (attrs !== null) { + problems.push(`${file}:${rstestLine}: #[rstest] has no fn line after it; check its attributes`); + } + + return { problems, functionCount }; +} + +function defaultRoots() { + return DEFAULT_ROOTS.map((dir) => { + return path.relative(process.cwd(), path.join(REPO_ROOT, dir)); + }); +} + +async function main() { + const args = process.argv.slice(2), + roots = args.length > 0 ? args : defaultRoots(), + files = (await Promise.all(roots.map(findRustFiles))).flat().sort(); + + const results = await Promise.all(files.map(async (file) => { + return checkSource(file, await fs.readFile(file, 'utf8')); + })); + + const problems = results.flatMap((result) => { return result.problems; }), + functionCount = results.reduce((sum, result) => { return sum + result.functionCount; }, 0); + + if (problems.length > 0) { + process.stderr.write(`${problems.join('\n')}\n${problems.length} rstest attribute problem(s) found\n`); + process.exitCode = 1; + return; + } + + // Finding nothing means the roots or RSTEST_ATTR are wrong, not that every + // function is fine: passing here would turn this check off without a trace. + if (functionCount === 0) { + process.stderr.write(`no #[rstest] functions found in ${files.length} files; check the roots and RSTEST_ATTR\n`); + process.exitCode = 1; + return; + } + + process.stdout.write(`rstest attributes OK: checked ${functionCount} #[rstest] functions in ${files.length} files\n`); +} + +main().catch((err) => { + process.stderr.write(`${err.stack || err}\n`); + process.exitCode = 1; +}); diff --git a/src/error.rs b/src/error.rs index ec8e3f7..a3180b5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -126,23 +126,30 @@ impl Serialize for Error { #[cfg(test)] mod tests { use super::*; - - #[test] - fn test_error_code_database_not_loaded() { - let err = Error::DatabaseNotLoaded("test.db".into()); - assert_eq!(err.error_code(), "DATABASE_NOT_LOADED"); - } - - #[test] - fn test_error_code_invalid_path() { - let err = Error::InvalidPath("/bad/path".into()); - assert_eq!(err.error_code(), "INVALID_PATH"); - } - - #[test] - fn test_error_code_path_not_registered() { - let err = Error::PathNotRegistered("MAIN".into()); - assert_eq!(err.error_code(), "PATH_NOT_REGISTERED"); + use rstest::rstest; + + #[rstest] + #[case::database_not_loaded(Error::DatabaseNotLoaded("test.db".into()), "DATABASE_NOT_LOADED")] + #[case::invalid_path(Error::InvalidPath("/bad/path".into()), "INVALID_PATH")] + #[case::path_not_registered(Error::PathNotRegistered("MAIN".into()), "PATH_NOT_REGISTERED")] + #[case::missing_state(Error::MissingState("DbInstances".into()), "MISSING_STATE")] + #[case::unsupported_datatype( + Error::Toolkit(sqlx_sqlite_toolkit::Error::UnsupportedDatatype("WEIRD_TYPE".into())), + "UNSUPPORTED_DATATYPE" + )] + #[case::multiple_rows( + Error::Toolkit(sqlx_sqlite_toolkit::Error::MultipleRowsReturned(5)), + "MULTIPLE_ROWS_RETURNED" + )] + #[case::transaction_rollback_failed( + Error::Toolkit(sqlx_sqlite_toolkit::Error::TransactionRollbackFailed { + transaction_error: "constraint violation".to_string(), + rollback_error: "connection lost".to_string(), + }), + "TRANSACTION_ROLLBACK_FAILED" + )] + fn test_error_code_matches_variant(#[case] err: Error, #[case] expected: &str) { + assert_eq!(err.error_code(), expected); } #[test] @@ -157,26 +164,6 @@ mod tests { ); } - #[test] - fn test_error_code_missing_state() { - let err = Error::MissingState("DbInstances".into()); - assert_eq!(err.error_code(), "MISSING_STATE"); - } - - #[test] - fn test_error_code_unsupported_datatype() { - let err = Error::Toolkit(sqlx_sqlite_toolkit::Error::UnsupportedDatatype( - "WEIRD_TYPE".into(), - )); - assert_eq!(err.error_code(), "UNSUPPORTED_DATATYPE"); - } - - #[test] - fn test_error_code_multiple_rows() { - let err = Error::Toolkit(sqlx_sqlite_toolkit::Error::MultipleRowsReturned(5)); - assert_eq!(err.error_code(), "MULTIPLE_ROWS_RETURNED"); - } - #[test] fn test_error_serialization_structure() { let err = Error::DatabaseNotLoaded("mydb.db".into()); @@ -188,34 +175,41 @@ mod tests { assert!(json.get("message").is_some()); } - #[test] - fn test_error_serialization_database_not_loaded() { - let err = Error::DatabaseNotLoaded("mydb.db".into()); - let json = serde_json::to_value(&err).unwrap(); - - assert_eq!(json["code"], "DATABASE_NOT_LOADED"); - assert!(json["message"].as_str().unwrap().contains("mydb.db")); - assert!(json["message"].as_str().unwrap().contains("not loaded")); - } - - #[test] - fn test_error_serialization_invalid_path() { - let err = Error::InvalidPath("/bad/path".into()); - let json = serde_json::to_value(&err).unwrap(); - - assert_eq!(json["code"], "INVALID_PATH"); - assert!(json["message"].as_str().unwrap().contains("/bad/path")); - } - - #[test] - fn test_error_serialization_multiple_rows() { - let err = Error::Toolkit(sqlx_sqlite_toolkit::Error::MultipleRowsReturned(3)); + #[rstest] + #[case::database_not_loaded( + Error::DatabaseNotLoaded("mydb.db".into()), + "DATABASE_NOT_LOADED", + &["mydb.db", "not loaded"] + )] + #[case::invalid_path(Error::InvalidPath("/bad/path".into()), "INVALID_PATH", &["/bad/path"])] + #[case::multiple_rows( + Error::Toolkit(sqlx_sqlite_toolkit::Error::MultipleRowsReturned(3)), + "MULTIPLE_ROWS_RETURNED", + &["3 rows", "0 or 1"] + )] + #[case::transaction_rollback_failed( + Error::Toolkit(sqlx_sqlite_toolkit::Error::TransactionRollbackFailed { + transaction_error: "constraint violation".to_string(), + rollback_error: "connection lost".to_string(), + }), + "TRANSACTION_ROLLBACK_FAILED", + &["constraint violation", "connection lost", "transaction failed", "rollback also failed"] + )] + fn test_error_serialization_includes_code_and_message( + #[case] err: Error, + #[case] expected_code: &str, + #[case] message_parts: &[&str], + ) { let json = serde_json::to_value(&err).unwrap(); - assert_eq!(json["code"], "MULTIPLE_ROWS_RETURNED"); + assert_eq!(json["code"], expected_code); let message = json["message"].as_str().unwrap(); - assert!(message.contains("3 rows")); - assert!(message.contains("0 or 1")); + for &part in message_parts { + assert!( + message.contains(part), + "{message:?} should contain {part:?}" + ); + } } #[test] @@ -227,29 +221,4 @@ mod tests { assert!(message.contains("5 rows")); assert!(message.contains("expected 0 or 1")); } - - #[test] - fn test_error_code_transaction_rollback_failed() { - let err = Error::Toolkit(sqlx_sqlite_toolkit::Error::TransactionRollbackFailed { - transaction_error: "constraint violation".to_string(), - rollback_error: "connection lost".to_string(), - }); - assert_eq!(err.error_code(), "TRANSACTION_ROLLBACK_FAILED"); - } - - #[test] - fn test_error_serialization_transaction_rollback_failed() { - let err = Error::Toolkit(sqlx_sqlite_toolkit::Error::TransactionRollbackFailed { - transaction_error: "constraint violation".to_string(), - rollback_error: "connection lost".to_string(), - }); - let json = serde_json::to_value(&err).unwrap(); - - assert_eq!(json["code"], "TRANSACTION_ROLLBACK_FAILED"); - let message = json["message"].as_str().unwrap(); - assert!(message.contains("constraint violation")); - assert!(message.contains("connection lost")); - assert!(message.contains("transaction failed")); - assert!(message.contains("rollback also failed")); - } } diff --git a/src/lib.rs b/src/lib.rs index a11da95..c976f55 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1408,6 +1408,7 @@ mod tests { use super::*; use crate::commands; use crate::subscriptions::ObserverConfigParams; + use rstest::rstest; use std::collections::HashMap; use tauri::plugin::Plugin; use tauri::test::{MockRuntime, mock_app, mock_builder, mock_context, noop_assets}; @@ -2768,13 +2769,19 @@ mod tests { }); } - /// A second window's explicit `captureValues` request that conflicts with - /// the value already active for a database must be rejected with - /// `OBSERVATION_CONFIG_CONFLICT`, without mutating the live broker or - /// recording that window's registration - the check must return before - /// `register()` runs. - #[test] - fn test_observe_conflicting_capture_values_rejected() { + /// A second window's explicit `captureValues` or `channelCapacity` request + /// that conflicts with the value already active for a database must be + /// rejected with `OBSERVATION_CONFIG_CONFLICT`, without mutating the live + /// broker or recording that window's registration - the check must return + /// before `register()` runs. + #[rstest] + #[case::capture_values( + ObserverConfigParams { channel_capacity: None, capture_values: Some(false) } + )] + #[case::channel_capacity( + ObserverConfigParams { channel_capacity: Some(512), capture_values: None } + )] + fn test_observe_conflicting_config_rejected(#[case] config_b: ObserverConfigParams) { let temp_dir = tempfile::tempdir().unwrap(); let db_path = validate::validate_database_path(temp_dir.path().join("main.db")).unwrap(); let key = "MAIN".to_string(); @@ -2811,13 +2818,10 @@ mod tests { webview_b.as_ref().clone(), "MAIN".to_string(), vec!["test".to_string()], - Some(ObserverConfigParams { - channel_capacity: None, - capture_values: Some(false), - }), + Some(config_b), ) .await - .expect_err("conflicting captureValues should be rejected"); + .expect_err("window B's conflicting config should be rejected"); assert!(matches!(err, Error::ObservationConfigConflict(_))); @@ -2852,151 +2856,29 @@ mod tests { }); } - /// Same as `test_observe_conflicting_capture_values_rejected`, but for a - /// conflicting `channelCapacity` request. - #[test] - fn test_observe_conflicting_channel_capacity_rejected() { - let temp_dir = tempfile::tempdir().unwrap(); - let db_path = validate::validate_database_path(temp_dir.path().join("main.db")).unwrap(); - let key = "MAIN".to_string(); - - tauri::async_runtime::block_on(async { - let (app, _) = - tokio::task::spawn_blocking(move || init_app_with_registered_db_at_path(&key, db_path)) - .await - .expect("plugin init task should succeed"); - - load_and_create_test_table(&app, "MAIN").await; - - let webview_a = tauri::WebviewWindowBuilder::new(&app, "window-a", Default::default()) - .build() - .expect("webview window A should build"); - let webview_b = tauri::WebviewWindowBuilder::new(&app, "window-b", Default::default()) - .build() - .expect("webview window B should build"); - - commands::observe( - app.state::(), - app.state::(), - webview_a.as_ref().clone(), - "MAIN".to_string(), - vec!["test".to_string()], - None, - ) - .await - .expect("window A observe with default config should succeed"); - - let err = commands::observe( - app.state::(), - app.state::(), - webview_b.as_ref().clone(), - "MAIN".to_string(), - vec!["test".to_string()], - Some(ObserverConfigParams { - channel_capacity: Some(512), - capture_values: None, - }), - ) - .await - .expect_err("conflicting channelCapacity should be rejected"); - - assert!(matches!(err, Error::ObservationConfigConflict(_))); - - let (capacity, capture) = { - let instances = app.state::().inner().inner.read().await; - let observable = instances - .get("MAIN") - .expect("MAIN should be loaded") - .observable() - .expect("observation should be enabled"); - ( - observable.broker().channel_capacity(), - observable.broker().capture_values(), - ) - }; - assert_eq!( - capacity, 256, - "the live broker's channelCapacity must be unchanged by a rejected observe()" - ); - assert!( - capture, - "the live broker's captureValues must be unchanged by a rejected observe()" - ); - - assert!( - !app - .state::() - .is_registered("MAIN", webview_b.label()) - .await, - "window B's registration must not be recorded when its observe() is rejected" - ); - }); - } - - /// An explicit config that matches the live broker's already-active values - /// exactly is not a conflict and must succeed. - #[test] - fn test_observe_identical_explicit_config_succeeds() { - let temp_dir = tempfile::tempdir().unwrap(); - let db_path = validate::validate_database_path(temp_dir.path().join("main.db")).unwrap(); - let key = "MAIN".to_string(); - - tauri::async_runtime::block_on(async { - let (app, _) = - tokio::task::spawn_blocking(move || init_app_with_registered_db_at_path(&key, db_path)) - .await - .expect("plugin init task should succeed"); - - load_and_create_test_table(&app, "MAIN").await; - - let webview_a = tauri::WebviewWindowBuilder::new(&app, "window-a", Default::default()) - .build() - .expect("webview window A should build"); - let webview_b = tauri::WebviewWindowBuilder::new(&app, "window-b", Default::default()) - .build() - .expect("webview window B should build"); - - commands::observe( - app.state::(), - app.state::(), - webview_a.as_ref().clone(), - "MAIN".to_string(), - vec!["test".to_string()], - None, - ) - .await - .expect("window A observe with default config should succeed"); - - commands::observe( - app.state::(), - app.state::(), - webview_b.as_ref().clone(), - "MAIN".to_string(), - vec!["test".to_string()], - Some(ObserverConfigParams { - channel_capacity: Some(256), - capture_values: Some(true), - }), - ) - .await - .expect("explicit config identical to the live broker's values should succeed"); - - assert!( - app.state::() - .is_registered("MAIN", webview_b.label()) - .await, - "window B's registration must be recorded once its observe() succeeds" - ); - }); - } - - /// Guards the `seeded` block in `observe()`: once window A has enabled - /// observation with a non-default explicit config, a second window calling - /// `observe()` with `config: None` must succeed - its defaults must never be - /// compared against the live broker's (already non-default) values as if - /// they were an explicit, conflicting request. - #[test] - fn test_observe_omitted_config_after_explicit_config_succeeds() { + /// A second window's `observe()` whose config does not conflict with the + /// live broker's active values must succeed and record that window's + /// registration. + #[rstest] + // An explicit config that matches the live broker's already-active values + // exactly is not a conflict and must succeed. + #[case::identical_explicit_config( + None, + Some(ObserverConfigParams { channel_capacity: Some(256), capture_values: Some(true) }) + )] + // Guards the `seeded` block in `observe()`: once window A has enabled + // observation with a non-default explicit config, a second window calling + // `observe()` with `config: None` must succeed - its defaults must never be + // compared against the live broker's (already non-default) values as if + // they were an explicit, conflicting request. + #[case::omitted_config_after_explicit_config( + Some(ObserverConfigParams { channel_capacity: Some(512), capture_values: Some(false) }), + None + )] + fn test_observe_compatible_config_succeeds( + #[case] config_a: Option, + #[case] config_b: Option, + ) { let temp_dir = tempfile::tempdir().unwrap(); let db_path = validate::validate_database_path(temp_dir.path().join("main.db")).unwrap(); let key = "MAIN".to_string(); @@ -3022,13 +2904,10 @@ mod tests { webview_a.as_ref().clone(), "MAIN".to_string(), vec!["test".to_string()], - Some(ObserverConfigParams { - channel_capacity: Some(512), - capture_values: Some(false), - }), + config_a, ) .await - .expect("window A observe with explicit non-default config should succeed"); + .expect("window A observe should succeed"); commands::observe( app.state::(), @@ -3036,10 +2915,10 @@ mod tests { webview_b.as_ref().clone(), "MAIN".to_string(), vec!["test".to_string()], - None, + config_b, ) .await - .expect("window B observe with config: None should succeed"); + .expect("window B observe with a compatible config should succeed"); assert!( app.state::() diff --git a/src/subscriptions.rs b/src/subscriptions.rs index d09a6fc..a1b4120 100644 --- a/src/subscriptions.rs +++ b/src/subscriptions.rs @@ -489,6 +489,7 @@ impl ObserverRegistrations { #[cfg(test)] mod tests { use super::*; + use rstest::rstest; /// Test-only: an empty, never-populated `DbInstances`, used purely to /// obtain real [`DbInstancesGuard`] witnesses (via its own `write()`) for @@ -501,8 +502,16 @@ mod tests { crate::DbInstances::default() } + #[rstest] + #[case::distinct_label_is_additive("window-b", 2)] + // Same window calling observe() again (e.g. to add more tables) must not + // inflate the refcount. + #[case::same_label_twice_is_idempotent("window-a", 1)] #[tokio::test] - async fn test_register_is_additive_across_distinct_labels() { + async fn test_register_counts_distinct_labels( + #[case] second_label: &str, + #[case] expected: usize, + ) { let regs = ObserverRegistrations::default(); let db_lock = dummy_db_lock(); @@ -514,30 +523,9 @@ mod tests { ); assert_eq!( regs - .register(&mut db_lock.write().await, "MAIN", "window-b") + .register(&mut db_lock.write().await, "MAIN", second_label) .await, - 2 - ); - } - - #[tokio::test] - async fn test_register_same_label_twice_is_idempotent() { - let regs = ObserverRegistrations::default(); - let db_lock = dummy_db_lock(); - - assert_eq!( - regs - .register(&mut db_lock.write().await, "MAIN", "window-a") - .await, - 1 - ); - // Same window calling observe() again (e.g. to add more tables) must not - // inflate the refcount. - assert_eq!( - regs - .register(&mut db_lock.write().await, "MAIN", "window-a") - .await, - 1 + expected ); } @@ -570,33 +558,35 @@ mod tests { ); } + #[rstest] + // Releasing from a db_key with no registrations at all is "never + // registered", not "last observer released" - `None`, not `Some(0)`. + #[case::db_without_registrations(&[], "window-a")] + // Releasing a label that was never registered, for a db_key that DOES + // have other registrations, is also a no-op: window-a's registration is + // left untouched. + #[case::unknown_label_on_registered_db(&["window-a"], "window-unknown")] #[tokio::test] - async fn test_release_unknown_label_or_db_is_a_noop() { + async fn test_release_unknown_label_or_db_is_a_noop( + #[case] registered: &[&str], + #[case] released: &str, + ) { let regs = ObserverRegistrations::default(); let db_lock = dummy_db_lock(); - // Releasing from a db_key with no registrations at all is "never - // registered", not "last observer released" - `None`, not `Some(0)`. - assert_eq!( + for &label in registered { regs - .release(&mut db_lock.write().await, "MAIN", "window-a") - .await, - None - ); + .register(&mut db_lock.write().await, "MAIN", label) + .await; + } - regs - .register(&mut db_lock.write().await, "MAIN", "window-a") - .await; - // Releasing a label that was never registered, for a db_key that DOES - // have other registrations, is also a no-op: window-a's registration is - // left untouched. assert_eq!( regs - .release(&mut db_lock.write().await, "MAIN", "window-unknown") + .release(&mut db_lock.write().await, "MAIN", released) .await, None ); - assert_eq!(regs.count_for_db("MAIN").await, 1); + assert_eq!(regs.count_for_db("MAIN").await, registered.len()); } #[tokio::test] diff --git a/src/validate.rs b/src/validate.rs index ee24103..7707c84 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -66,6 +66,7 @@ pub fn validate_database_path(path: impl AsRef) -> Result { #[cfg(test)] mod tests { use super::*; + use rstest::rstest; use std::fs; use std::sync::atomic::{AtomicU64, Ordering}; @@ -78,34 +79,21 @@ mod tests { dir } - #[test] - fn test_memory_passthrough() { - assert_eq!( - validate_database_path(":memory:").unwrap(), - PathBuf::from(":memory:"), - ); - } - - #[test] - fn test_file_memory_uri_passthrough() { - assert_eq!( - validate_database_path("file::memory:?cache=shared").unwrap(), - PathBuf::from("file::memory:?cache=shared"), - ); - } - - #[test] - fn test_mode_memory_passthrough() { - assert_eq!( - validate_database_path("file:test?mode=memory").unwrap(), - PathBuf::from("file:test?mode=memory"), - ); + #[rstest] + #[case::memory(":memory:")] + #[case::file_memory_uri("file::memory:?cache=shared")] + #[case::mode_memory("file:test?mode=memory")] + fn test_memory_path_passes_through(#[case] path: &str) { + assert_eq!(validate_database_path(path).unwrap(), PathBuf::from(path)); } #[test] fn test_mode_memory_substring_in_value_is_not_treated_as_memory() { let err = validate_database_path("file:/home/user/real.db?x=mode=memory").unwrap_err(); - assert!(matches!(err, Error::InvalidPath(_))); + assert!( + matches!(err, Error::InvalidPath(_)), + "expected InvalidPath, got {err:?}" + ); } #[test] @@ -142,30 +130,27 @@ mod tests { #[test] fn test_rejects_relative_path() { let err = validate_database_path("relative.db").unwrap_err(); - assert!(matches!(err, Error::InvalidPath(_))); - } - - #[test] - fn test_rejects_absolute_path_with_parent_traversal() { - let dir = make_temp_dir(); - let abs_str = format!("{}/../escape.db", dir.to_str().unwrap()); - - let err = validate_database_path(&abs_str).unwrap_err(); - assert!(matches!(err, Error::PathTraversal(_))); - } - - #[test] - fn test_rejects_absolute_path_with_embedded_traversal() { - let dir = make_temp_dir(); - let abs_str = format!("{}/sub/../../escape.db", dir.to_str().unwrap()); - - let err = validate_database_path(&abs_str).unwrap_err(); - assert!(matches!(err, Error::PathTraversal(_))); + assert!( + matches!(err, Error::InvalidPath(_)), + "expected InvalidPath, got {err:?}" + ); } - #[test] - fn test_rejects_null_byte() { - let err = validate_database_path("path\0evil").unwrap_err(); - assert!(matches!(err, Error::PathTraversal(_))); + #[rstest] + #[case::absolute_path_with_parent_traversal(format!( + "{}/../escape.db", + make_temp_dir().to_str().unwrap() + ))] + #[case::absolute_path_with_embedded_traversal(format!( + "{}/sub/../../escape.db", + make_temp_dir().to_str().unwrap() + ))] + #[case::null_byte("path\0evil".to_string())] + fn test_rejects_path_traversal(#[case] path: String) { + let err = validate_database_path(&path).unwrap_err(); + assert!( + matches!(err, Error::PathTraversal(_)), + "expected PathTraversal, got {err:?}" + ); } }