From 16431781db365aa05423f8e36e290bcbaefe8cbd Mon Sep 17 00:00:00 2001 From: Thierry Date: Mon, 11 May 2026 20:54:42 +0200 Subject: [PATCH] refactor(db): extract db_ahead payload helper (Sourcery PR #65) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #65 follow-up. Sourcery flagged the find-and-slice for the DB_AHEAD payload as duplicated logic between lib.rs and the regression test. One implementation is better. - New `pub fn extract_db_ahead_payload(err: &str) -> Option<&str>` in `db/mod.rs`. Returns `Some(payload)` when the marker is in the string, `None` otherwise. lib.rs and the test now both consume this helper. - Negative-case test added: unrelated errors (sidecar timeout, out-of-disk-space, window builder errors, empty string) must return None — otherwise STARTUP_BLOCKED.txt would get written for unrelated failures and dilute its signal. Tests: Rust 207/207 (was 205), clippy + fmt clean. Five db_ahead* tests now cover the helper end-to-end: - passes on fresh install - passes when DB matches binary - blocks when DB has future migration - extracts even when wrapped - returns None when marker absent Co-Authored-By: Claude Opus 4.7 --- src-tauri/src/db/mod.rs | 64 ++++++++++++++++++++++++++++++----------- src-tauri/src/lib.rs | 16 +++++------ 2 files changed, 56 insertions(+), 24 deletions(-) diff --git a/src-tauri/src/db/mod.rs b/src-tauri/src/db/mod.rs index 69231f2..d4de708 100644 --- a/src-tauri/src/db/mod.rs +++ b/src-tauri/src/db/mod.rs @@ -81,16 +81,30 @@ pub async fn init_pool() -> Result { } /// Token prepended to the error message when `check_db_is_not_ahead_of_binary` -/// detects the failure mode. The setup hook in `lib.rs` matches this exact -/// token via `find()` (not `strip_prefix`) so the consumer is robust to any -/// future wrapping the producer adds. Single source of truth on both sides: -/// changing the token here must compile-error if the consumer drifts. +/// detects the failure mode. The setup hook in `lib.rs` extracts the payload +/// via `extract_db_ahead_payload` (which uses `find()` not `strip_prefix`), +/// so the consumer stays robust to any future wrapping the producer adds. +/// Single source of truth on both sides: changing the token here must +/// compile-error if the consumer drifts. /// /// Boundary string `::` chosen because it's syntactically unusual in plain /// French error messages, so a false positive on `find()` is effectively /// impossible. pub const DB_AHEAD_MARKER: &str = "DB_AHEAD::"; +/// Locate the `DB_AHEAD_MARKER` inside an arbitrary error string and return +/// the payload following it, or `None` when the marker is absent. +/// +/// Centralising the find-and-slice in one helper means `lib.rs` (the runtime +/// consumer that writes `STARTUP_BLOCKED.txt`) and the regression test share +/// the same extraction implementation. A future tweak — say trimming a +/// trailing newline or stripping a leading colon — only has to land here +/// (Sourcery review on PR #65). +pub fn extract_db_ahead_payload(err: &str) -> Option<&str> { + err.find(DB_AHEAD_MARKER) + .map(|idx| &err[idx + DB_AHEAD_MARKER.len()..]) +} + /// Detect the "DB has been migrated by a newer version than this binary /// knows about" failure mode BEFORE `sqlx::migrate!().run()` does. The /// stock sqlx error message — `migration 18 was previously applied but @@ -691,14 +705,14 @@ mod migration_tests { } #[test] - fn db_ahead_marker_is_findable_even_when_wrapped() { - // Regression guard against the Sourcery review on PR #64: the - // consumer in lib.rs uses `find(db::DB_AHEAD_MARKER)` rather - // than `strip_prefix("Failed to init SQLite: DB_AHEAD::")`. - // This test simulates the marker wrapped under arbitrary - // prefixes and confirms the detection still works — so a - // future refactor of the error wrapping in lib.rs cannot - // silently break the STARTUP_BLOCKED.txt path. + fn db_ahead_payload_extracts_even_when_wrapped() { + // Regression guard against the Sourcery review on PR #64 / #65: the + // consumer in lib.rs uses `extract_db_ahead_payload(&err)` (shared + // helper) rather than `strip_prefix("Failed to init SQLite: DB_AHEAD::")` + // hardcoded inline. This test simulates the marker wrapped under + // arbitrary prefixes and confirms the helper still extracts the + // payload — so a future refactor of the error wrapping in lib.rs + // cannot silently break the STARTUP_BLOCKED.txt path. let marker = super::DB_AHEAD_MARKER; let wrappings = [ format!("Failed to init SQLite: {marker}payload"), @@ -707,10 +721,8 @@ mod migration_tests { format!("Layer A → Layer B: {marker}payload"), ]; for wrapped in &wrappings { - let idx = wrapped - .find(marker) - .unwrap_or_else(|| panic!("marker must be findable in wrapping: {wrapped}")); - let extracted = &wrapped[idx + marker.len()..]; + let extracted = super::extract_db_ahead_payload(wrapped) + .unwrap_or_else(|| panic!("marker must be extractable from: {wrapped}")); assert_eq!( extracted, "payload", "payload must be cleanly extractable after the marker" @@ -718,6 +730,26 @@ mod migration_tests { } } + #[test] + fn db_ahead_payload_returns_none_when_marker_absent() { + // Negative case: a plain Tauri setup failure that doesn't carry + // the DB_AHEAD marker must not match. Otherwise lib.rs would + // write STARTUP_BLOCKED.txt for unrelated errors and dilute the + // signal — the file is meant to mean exactly one thing. + let unrelated_errors = [ + "Failed to init SQLite: out of disk space", + "Sidecar timeout", + "Tauri setup failed: window builder error", + "", // empty input edge case + ]; + for err in unrelated_errors { + assert!( + super::extract_db_ahead_payload(err).is_none(), + "must not match an unrelated error: {err:?}" + ); + } + } + #[tokio::test] async fn db_ahead_check_blocks_when_db_has_future_migration() { // Reproduces the v0.3.8 ← v0.3.9 dev workflow crash: the user diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 4fc175a..1d466a3 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -122,14 +122,14 @@ pub fn run() { // Pure best-effort — if the write fails too we just // fall through to the normal log-and-exit. // - // We `find()` the marker rather than `strip_prefix()` - // so the detection survives any future change to the - // upstream wrapping in `setup_result` (e.g. a different - // prefix than "Failed to init SQLite: "). The marker - // itself is the single source of truth, defined in - // `db::DB_AHEAD_MARKER` and prepended in `init_pool`. - if let Some(idx) = e.find(db::DB_AHEAD_MARKER) { - let payload = &e[idx + db::DB_AHEAD_MARKER.len()..]; + // `extract_db_ahead_payload` does the find-and-slice in + // a single helper shared with the regression test, so + // the detection survives any future change to the + // upstream wrapping in `setup_result` and any tweak to + // the extraction (trim, strip, normalise) lands in one + // place. Marker itself is the single source of truth + // (`db::DB_AHEAD_MARKER`) prepended in `init_pool`. + if let Some(payload) = db::extract_db_ahead_payload(&e) { write_startup_blocked_notice(payload); } return Err(e.into());