From 1793c387106c6df93daad3be307dacc0a7a49a32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Thu, 30 Jul 2026 18:20:20 -0400 Subject: [PATCH 1/3] fix(sqlite): delete resource_fts on every purge path (#386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resource_fts` is an FTS5 *virtual* table, so it can carry no foreign key and the `ON DELETE CASCADE` from `resources` never reached it — and no SQLite purge path deleted from it explicitly. Purged resources therefore left their narrative and their entire serialized body in the database after the API reported them removed, and a resource reusing the purged logical id resurrected that text as a match oracle through `_text` / `_content`. PostgreSQL was unaffected: its `resource_fts` is an ordinary table with `ON DELETE CASCADE`. Adds a probe-guarded, strict `DELETE FROM resource_fts` to `purge`, `purge_all` and `purge_tenant_data`. Strict because a swallowed failure on a purge path means the API attests to an erasure it did not perform; probe-guarded because FTS5 is an optional compile-time feature and a database built without it legitimately has no table. Ungated by `is_search_offloaded()`: that flag is a write-path optimisation, while this is an erasure guarantee, and a deployment that indexed locally before moving search to Elasticsearch still has rows to remove. `purge` and `purge_all` were four independent autocommit statements, so a failure partway through produced the very orphan state this prevents, indistinguishable on retry from a purge that never ran. Both are now transactional. All three use `TransactionBehavior::Immediate`: they read before they write, and under WAL a deferred transaction fails that upgrade with `SQLITE_BUSY_SNAPSHOT`, for which the busy handler — and so the configured `busy_timeout` — is never consulted. Also fixes a latent data-loss bug on BOTH backends that blocked the fourth purge path. `run_reindex` deletes every resource's search entries via `delete_search_entries`, which drops the `resource_fts` row, while `write_search_entries` never rebuilt it — so `$reindex` silently disabled `_text`/`_content` until each resource was next written, with or without `clear_existing`. `write_search_entries` now repopulates FTS on SQLite and PostgreSQL, which is what makes adding the FTS delete to SQLite's `clear_search_index` safe rather than destructive. Migration v14 -> v15 sweeps orphans already on disk: the code fix alone stops new ones but leaves every database that has ever served a purge still holding the content. Soft-deleted resources keep their `resources` row and are correctly preserved. `/ui/tenants` discarded `purge_tenant_data`'s error after already deregistering the tenant, rendering success while the data remained; it now surfaces the failure. Tests: a backend-agnostic suite shared by SQLite and PostgreSQL. It asserts on raw `resource_fts` rows rather than search results, because purge clears `search_index` and the orphaned row is therefore unreachable through search — the obvious "search finds nothing after purge" test passes on the unfixed code. Each scenario opens with a positive control so it cannot pass vacuously. Verified against unfixed source: 10 of 13 fail, including the id-reuse oracle. Two existing assertions that accepted zero results ("FTS unavailable") are tightened, since they would stay green through a total full-text outage. --- .../src/backends/postgres/storage.rs | 13 + .../persistence/src/backends/sqlite/schema.rs | 67 +- .../src/backends/sqlite/storage.rs | 186 +++++- crates/persistence/tests/postgres_tests.rs | 159 +++++ .../tests/search/fts_purge_suite.rs | 599 ++++++++++++++++++ .../tests/search/fts_purge_tests.rs | 417 ++++++++++++ crates/persistence/tests/search/mod.rs | 4 + crates/persistence/tests/sqlite_tests.rs | 32 +- crates/ui/src/tenants.rs | 22 +- 9 files changed, 1454 insertions(+), 45 deletions(-) create mode 100644 crates/persistence/tests/search/fts_purge_suite.rs create mode 100644 crates/persistence/tests/search/fts_purge_tests.rs diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index fd6ef1cce..a93353c8a 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -3286,6 +3286,19 @@ impl ReindexTarget for PostgresBackend { .index_contained_resources(&client, tenant_id, resource_type, resource_id, content) .await?; + // Rebuild the full-text row as well. `run_reindex` deletes each + // resource's search entries first (`delete_search_entries` -> + // `delete_search_index`), and that drops the `resource_fts` row; without + // this call nothing put it back, so `$reindex` silently disabled + // `_text`/`_content` on every reindex, with or without `clear_existing`. + // Same defect as the SQLite side — this is not PostgreSQL-specific. + // + // Not counted in `count`, which reports `search_index` entries only. + // `index_fts_content` is DELETE-then-INSERT here, so it is idempotent + // regardless of what ran before it. + self.index_fts_content(&client, tenant_id, resource_type, resource_id, content) + .await?; + Ok(count) } diff --git a/crates/persistence/src/backends/sqlite/schema.rs b/crates/persistence/src/backends/sqlite/schema.rs index 86fcaba60..ae3d0dbf1 100644 --- a/crates/persistence/src/backends/sqlite/schema.rs +++ b/crates/persistence/src/backends/sqlite/schema.rs @@ -5,7 +5,7 @@ use rusqlite::Connection; use crate::error::StorageResult; /// Current schema version. -pub const SCHEMA_VERSION: i32 = 14; +pub const SCHEMA_VERSION: i32 = 15; /// Initialize the database schema. pub fn initialize_schema(conn: &Connection) -> StorageResult<()> { @@ -295,6 +295,7 @@ fn migrate_schema(conn: &Connection, from_version: i32) -> StorageResult<()> { 11 => migrate_v11_to_v12(conn)?, 12 => migrate_v12_to_v13(conn)?, 13 => migrate_v13_to_v14(conn)?, + 14 => migrate_v14_to_v15(conn)?, _ => { return Err(crate::error::StorageError::Backend( crate::error::BackendError::Internal { @@ -375,8 +376,13 @@ fn migrate_v2_to_v3(conn: &Connection) -> StorageResult<()> { return Ok(()); } - // Create the FTS5 virtual table for full-text search - // Uses external content mode for smaller index size + // Create the FTS5 virtual table for full-text search. + // + // NOTE: this is a plain fts5 table, NOT external-content mode — there is no + // `content=` clause. It therefore keeps a full second copy of every + // resource's narrative and serialized body in its `resource_fts_content` + // shadow table. That matters when reasoning about where purged PHI lives: + // deleting the `resources` row does not reach this copy (see issue #386). conn.execute( "CREATE VIRTUAL TABLE IF NOT EXISTS resource_fts USING fts5( resource_id UNINDEXED, @@ -1258,6 +1264,61 @@ fn migrate_v13_to_v14(conn: &Connection) -> StorageResult<()> { ensure_tenants_table(conn) } +/// Migrate from schema version 14 to version 15. +/// +/// Sweeps `resource_fts` rows orphaned by the pre-v15 purge paths (issue #386). +/// +/// `resource_fts` is an FTS5 *virtual* table, so it can carry no foreign key and +/// the `ON DELETE CASCADE` from `resources` never reached it — and no purge path +/// deleted from it explicitly. Every database that has ever served `$purge`, +/// type-level `$purge`, or a tenant purge is therefore still holding the +/// narrative text and the complete serialized body of resources an operator was +/// told had been removed. The code fix stops new orphans; this removes the ones +/// already on disk, which is the half that matters to anyone running today. +/// +/// Soft-deleted resources keep their `resources` row, so `NOT EXISTS` correctly +/// preserves their entries — matching current behaviour, where a soft delete +/// does not drop the FTS row. +/// +/// The single statement runs in its own implicit transaction. It scans +/// `resource_fts` once (unavoidable: its key columns are `UNINDEXED`, and an +/// FTS5 virtual table admits no auxiliary index) and probes `resources` by +/// primary key. +fn migrate_v14_to_v15(conn: &Connection) -> StorageResult<()> { + // FTS5 is optional at compile time; a database built without it has no + // table to sweep. + let fts_exists: bool = conn + .query_row( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='resource_fts'", + [], + |_| Ok(true), + ) + .unwrap_or(false); + if !fts_exists { + return Ok(()); + } + + let swept = conn + .execute( + "DELETE FROM resource_fts + WHERE NOT EXISTS ( + SELECT 1 FROM resources r + WHERE r.tenant_id = resource_fts.tenant_id + AND r.resource_type = resource_fts.resource_type + AND r.id = resource_fts.resource_id)", + [], + ) + .map_err(|e| migration_err(format!("v15 resource_fts orphan sweep: {e}")))?; + + if swept > 0 { + tracing::info!( + orphaned_fts_rows = swept, + "Swept full-text rows left behind by pre-v15 purges (issue #386)" + ); + } + Ok(()) +} + fn migration_err(message: String) -> crate::error::StorageError { crate::error::StorageError::Backend(crate::error::BackendError::Internal { backend_name: "sqlite".to_string(), diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index e0fbfb3ef..b431d5d15 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -42,6 +42,58 @@ fn serialization_error(message: String) -> StorageError { StorageError::Backend(BackendError::SerializationError { message }) } +/// Whether the optional `resource_fts` FTS5 virtual table exists on this +/// database. +/// +/// FTS5 is an optional SQLite compile-time feature. `create_fts_table` +/// (`schema.rs`) succeeds silently when it is absent, so `resource_fts` may +/// legitimately not exist — and a database created by an FTS5-less build keeps +/// no table even once reopened by an FTS5-capable one, because the `v2 -> v3` +/// migration is already recorded as done. +/// +/// Callers therefore probe before touching the table, and treat "table absent" +/// (a determinate fact: there is nothing indexed, so nothing to erase) very +/// differently from "the statement failed" (an unknown, which must never be +/// swallowed on a purge path — see the `DELETE FROM resource_fts` call sites). +/// +/// Cheap: `sqlite_master` is answered from the connection's in-memory schema +/// cache and does not touch disk. +pub(crate) fn fts_table_exists(conn: &rusqlite::Connection) -> StorageResult { + use rusqlite::OptionalExtension; + + conn.query_row( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='resource_fts'", + [], + |_| Ok(()), + ) + .optional() + .map(|found| found.is_some()) + .map_err(|e| internal_error(format!("Failed to probe for resource_fts: {e}"))) +} + +/// Runs a `DELETE FROM resource_fts …` on a purge path, skipping it when FTS5 +/// is unavailable and propagating any other failure. +/// +/// The error handling is the point. Elsewhere in this backend an FTS delete is +/// best-effort (`let _ = …`), which is tolerable on an index-maintenance path +/// where the worst case is a stale entry. On a *purge* path it is not: a +/// swallowed failure means the API answers `200` — "the record is gone" — while +/// the resource's full text is still on disk. That is a false erasure +/// attestation, and it is the same class of defect issue #386 reports, just +/// reached by a different route. So: probe, then be strict. +fn purge_fts_rows( + conn: &rusqlite::Connection, + sql: &str, + params: impl rusqlite::Params, +) -> StorageResult<()> { + if !fts_table_exists(conn)? { + return Ok(()); + } + conn.execute(sql, params) + .map_err(|e| internal_error(format!("purge fts delete: {e}")))?; + Ok(()) +} + /// Extracts the `value[x]` payload from a FHIRPath Patch `Parameters.part` /// entry whose `name` is `"value"`. Returns the value of the first key /// matching `value[A-Z]…` (e.g. `valueString`, `valueQuantity`, @@ -867,8 +919,14 @@ impl ResourceStorage for SqliteBackend { async fn purge_tenant_data(&self, id: &str) -> StorageResult { let mut conn = self.get_connection()?; + // IMMEDIATE, not the DEFERRED default: this transaction reads (the count + // below) before it writes. Under WAL a deferred transaction takes a read + // snapshot on that first read and then fails the read-to-write upgrade + // with SQLITE_BUSY_SNAPSHOT if another connection committed in between — + // and the busy handler is *not* invoked for that code, so the configured + // busy_timeout does not cover it. Taking the write lock up front does. let tx = conn - .transaction() + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) .map_err(|e| internal_error(format!("purge begin: {e}")))?; // Count current-version rows first so we can report what was removed. let removed: i64 = tx @@ -880,6 +938,8 @@ impl ResourceStorage for SqliteBackend { .map_err(|e| internal_error(format!("purge count: {e}")))?; // search_index has ON DELETE CASCADE from resources, but delete it // explicitly too in case foreign keys are not enforced on this handle. + // (That explicit delete is also what fires the `search_index_fts` + // triggers, though a cascade fires them too.) for sql in [ "DELETE FROM search_index WHERE tenant_id = ?1", "DELETE FROM resource_history WHERE tenant_id = ?1", @@ -888,6 +948,22 @@ impl ResourceStorage for SqliteBackend { tx.execute(sql, params![id]) .map_err(|e| internal_error(format!("purge delete: {e}")))?; } + // `resource_fts` is an FTS5 *virtual* table: it can carry no foreign key, + // so the cascade above never reaches it and it must be deleted explicitly + // (issue #386). Left behind, the purged resource's narrative and its + // entire serialized body stay in the database, and are resurrected as a + // match oracle the moment a resource reuses the same logical id. + // + // Deliberately NOT gated on `is_search_offloaded()`: that flag is a + // write-path optimisation ("don't maintain an index nobody reads"), + // whereas this is an erasure guarantee. A deployment that indexed + // locally and later moved search to Elasticsearch still has rows here, + // and a purge is precisely when they must go. + purge_fts_rows( + &tx, + "DELETE FROM resource_fts WHERE tenant_id = ?1", + params![id], + )?; tx.commit() .map_err(|e| internal_error(format!("purge commit: {e}")))?; Ok(removed.max(0) as u64) @@ -1071,16 +1147,7 @@ impl SqliteBackend { ) -> StorageResult<()> { use super::search::fts::extract_searchable_content; - // Check if FTS table exists (created in schema v3) - let fts_exists: bool = conn - .query_row( - "SELECT 1 FROM sqlite_master WHERE type='table' AND name='resource_fts'", - [], - |_| Ok(true), - ) - .unwrap_or(false); - - if !fts_exists { + if !fts_table_exists(conn)? { // FTS5 not available - skip silently return Ok(()); } @@ -2311,11 +2378,23 @@ impl PurgableStorage for SqliteBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - let conn = self.get_connection()?; + let mut conn = self.get_connection()?; let tenant_id = tenant.tenant_id().as_str(); + // One transaction for the whole purge. Previously these were four + // independent autocommit statements, so a failure (or a crash) partway + // through left the resource deleted but its full text still in + // `resource_fts` — the exact orphan state this method now exists to + // prevent — and the caller could not tell a partial purge from a failed + // one, because a retry hits the not-found guard below and reports + // `NotFound` while the residue remains. IMMEDIATE for the same + // read-then-write / WAL reason as `purge_tenant_data`. + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("purge begin: {e}")))?; + // Check if resource exists (in any state) - let exists: bool = conn + let exists: bool = tx .query_row( "SELECT 1 FROM resources WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], @@ -2325,7 +2404,7 @@ impl PurgableStorage for SqliteBackend { if !exists { // Also check history in case it was already purged from main table - let history_exists: bool = conn + let history_exists: bool = tx .query_row( "SELECT 1 FROM resource_history WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], @@ -2342,35 +2421,51 @@ impl PurgableStorage for SqliteBackend { } // Delete from resources table - conn.execute( + tx.execute( "DELETE FROM resources WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], ) .map_err(|e| internal_error(format!("Failed to purge resource: {}", e)))?; // Delete from history table - conn.execute( + tx.execute( "DELETE FROM resource_history WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3", params![tenant_id, resource_type, id], ) .map_err(|e| internal_error(format!("Failed to purge resource history: {}", e)))?; // Delete from search index - conn.execute( + tx.execute( "DELETE FROM search_index WHERE tenant_id = ?1 AND resource_type = ?2 AND resource_id = ?3", params![tenant_id, resource_type, id], ) .map_err(|e| internal_error(format!("Failed to purge search index: {}", e)))?; + // Delete the full-text rows: no cascade reaches an FTS5 virtual table. + // See `purge_tenant_data` for why this is strict and ungated. + purge_fts_rows( + &tx, + "DELETE FROM resource_fts WHERE tenant_id = ?1 AND resource_type = ?2 AND resource_id = ?3", + params![tenant_id, resource_type, id], + )?; + + tx.commit() + .map_err(|e| internal_error(format!("purge commit: {e}")))?; + Ok(()) } async fn purge_all(&self, tenant: &TenantContext, resource_type: &str) -> StorageResult { - let conn = self.get_connection()?; + let mut conn = self.get_connection()?; let tenant_id = tenant.tenant_id().as_str(); + // Single transaction — see `purge` for the rationale. + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("purge_all begin: {e}")))?; + // Count how many we're about to delete - let count: i64 = conn + let count: i64 = tx .query_row( "SELECT COUNT(DISTINCT id) FROM resources WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], @@ -2379,26 +2474,37 @@ impl PurgableStorage for SqliteBackend { .unwrap_or(0); // Delete from resources table - conn.execute( + tx.execute( "DELETE FROM resources WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], ) .map_err(|e| internal_error(format!("Failed to purge resources: {}", e)))?; // Delete from history table - conn.execute( + tx.execute( "DELETE FROM resource_history WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], ) .map_err(|e| internal_error(format!("Failed to purge resource history: {}", e)))?; // Delete from search index - conn.execute( + tx.execute( "DELETE FROM search_index WHERE tenant_id = ?1 AND resource_type = ?2", params![tenant_id, resource_type], ) .map_err(|e| internal_error(format!("Failed to purge search index: {}", e)))?; + // Delete the full-text rows: no cascade reaches an FTS5 virtual table. + // See `purge_tenant_data` for why this is strict and ungated. + purge_fts_rows( + &tx, + "DELETE FROM resource_fts WHERE tenant_id = ?1 AND resource_type = ?2", + params![tenant_id, resource_type], + )?; + + tx.commit() + .map_err(|e| internal_error(format!("purge_all commit: {e}")))?; + Ok(count as u64) } } @@ -3653,6 +3759,29 @@ impl ReindexTarget for SqliteBackend { content, )?; + // Rebuild the full-text row as well. Without this, `$reindex` was a + // *destructive* operation for `_text`/`_content`: `run_reindex` deletes + // each resource's search entries via `delete_search_entries` -> + // `delete_search_index`, which does drop the FTS row, and nothing here + // put it back. That happened on every reindex, with or without + // `clear_existing`, so the documented recovery operation silently + // disabled full-text search until each resource was next written. + // + // Not counted in `count`: that value is the number of `search_index` + // entries, which `$reindex-status` reports, and an FTS row is not one. + // + // Safe against duplicates because `run_reindex` always calls + // `delete_search_entries` for the resource immediately before this, and + // SQLite's `index_fts_content` is a bare INSERT with no delete-first. + // If that ordering ever changes, this must become delete-then-insert. + self.index_fts_content( + &conn, + tenant.tenant_id().as_str(), + resource_type, + resource_id, + content, + )?; + Ok(count) } @@ -3667,6 +3796,19 @@ impl ReindexTarget for SqliteBackend { ) .map_err(|e| internal_error(format!("Failed to clear search index: {}", e)))?; + // Clear the full-text rows too, matching PostgreSQL. This is only sound + // because `write_search_entries` above now repopulates them; adding this + // delete on its own would have made `$reindex --clear-existing` wipe + // `_text`/`_content` permanently. + // + // The returned count deliberately stays the `search_index` total — + // callers and tests treat it as the number of index entries cleared. + purge_fts_rows( + &conn, + "DELETE FROM resource_fts WHERE tenant_id = ?1", + params![tenant_id], + )?; + Ok(deleted as u64) } } diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index e8844857a..e9fd5d244 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -21,6 +21,20 @@ use helios_persistence::core::BackendKind; #[path = "transactions/if_match_suite.rs"] mod if_match_suite; +/// The backend-agnostic full-text purge-completeness scenarios (issue #386), +/// shared verbatim with the SQLite suite that owns the file. +/// +/// PostgreSQL already deleted `resource_fts` in its purge paths — the defect was +/// SQLite-only — so these lock the *reference* backend's behaviour in place so a +/// future change cannot silently regress it. The `$reindex` scenarios are a +/// different matter: those failed on PostgreSQL too, because +/// `write_search_entries` never rebuilt the full-text row. +/// +/// Declared at the top level for the same `#[path]` resolution reason as +/// `if_match_suite` above. +#[path = "search/fts_purge_suite.rs"] +mod fts_purge_suite; + // ============================================================================ // Backend Configuration Tests (no PostgreSQL instance required) // ============================================================================ @@ -5116,4 +5130,149 @@ mod postgres_integration { postgres_integration_transaction_delete_accepts_matching_if_match, transaction_delete_accepts_matching_if_match ); + + // ======================================================================== + // Full-text purge completeness (issue #386) + // ======================================================================== + + use crate::fts_purge_suite::{self as fts_suite, FtsProbe}; + + /// Reads `resource_fts` directly over the backend's own pool. + /// + /// `PostgresBackend::get_client` is `#[doc(hidden)] pub` precisely so + /// out-of-crate tests can run raw SQL; other tests in this module already + /// do the same. + struct PgFtsProbe(PostgresBackend); + + #[async_trait::async_trait] + impl FtsProbe for PgFtsProbe { + async fn fts_row_count(&self, tenant_id: &str) -> u64 { + let client = self.0.get_client().await.expect("get_client"); + let row = client + .query_one( + "SELECT COUNT(*)::bigint FROM resource_fts WHERE tenant_id = $1", + &[&tenant_id], + ) + .await + .expect("count resource_fts"); + row.get::<_, i64>(0) as u64 + } + + async fn fts_rows_containing(&self, needle: &str) -> u64 { + let client = self.0.get_client().await.expect("get_client"); + let pattern = format!("%{needle}%"); + let row = client + .query_one( + "SELECT COUNT(*)::bigint FROM resource_fts \ + WHERE full_content LIKE $1 OR narrative_text LIKE $1", + &[&pattern], + ) + .await + .expect("count resource_fts by content"); + row.get::<_, i64>(0) as u64 + } + } + + /// One `#[tokio::test]` per shared scenario, each on its own UUID-suffixed + /// tenant so they cannot collide on the shared container. + macro_rules! pg_fts_test { + ($name:ident, $scenario:ident) => { + #[tokio::test] + async fn $name() { + let backend = create_backend().await; + let probe = PgFtsProbe(create_backend().await); + let tenant = create_tenant(stringify!($scenario)); + fts_suite::$scenario(&backend, &probe, &tenant).await; + } + }; + } + + pg_fts_test!( + postgres_integration_purge_removes_fts_rows, + purge_removes_fts_rows + ); + pg_fts_test!( + postgres_integration_purge_all_removes_fts_rows, + purge_all_removes_fts_rows + ); + pg_fts_test!( + postgres_integration_purge_tenant_data_removes_fts_rows, + purge_tenant_data_removes_fts_rows + ); + pg_fts_test!( + postgres_integration_reuse_after_purge_does_not_resurrect_narrative, + reuse_after_purge_does_not_resurrect_narrative + ); + pg_fts_test!( + postgres_integration_tenant_reuse_does_not_resurrect_narrative, + tenant_reuse_does_not_resurrect_narrative + ); + pg_fts_test!( + postgres_integration_repeated_purge_and_recreate_does_not_grow_fts, + repeated_purge_and_recreate_does_not_grow_fts + ); + + #[tokio::test] + async fn postgres_integration_purge_tenant_data_leaves_other_tenants_intact() { + let backend = create_backend().await; + let probe = PgFtsProbe(create_backend().await); + fts_suite::purge_tenant_data_leaves_other_tenants_intact( + &backend, + &probe, + &create_tenant("fts_victim"), + &create_tenant("fts_bystander"), + ) + .await; + } + + /// `$reindex` must rebuild full-text search, not destroy it. + /// + /// This failed on PostgreSQL before the fix, in both modes: `run_reindex` + /// drops each resource's `resource_fts` row via `delete_search_entries`, and + /// `write_search_entries` never put it back. + #[tokio::test] + async fn postgres_integration_reindex_preserves_full_text_search_without_clear() { + pg_reindex_case(false, "fts_reindex_noclear").await; + } + + #[tokio::test] + async fn postgres_integration_reindex_preserves_full_text_search_with_clear() { + pg_reindex_case(true, "fts_reindex_clear").await; + } + + async fn pg_reindex_case(clear_existing: bool, tenant_label: &str) { + use helios_persistence::search::ReindexOperation; + use std::sync::Arc; + + let backend = Arc::new(create_backend().await); + let probe = PgFtsProbe(create_backend().await); + let tenant = create_tenant(tenant_label); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + fts_suite::reindex_preserves_full_text_search( + backend.as_ref(), + &probe, + &tenant, + &reindex, + clear_existing, + ) + .await; + } + + #[tokio::test] + async fn postgres_integration_repeated_reindex_does_not_duplicate_fts_rows() { + use helios_persistence::search::ReindexOperation; + use std::sync::Arc; + + let backend = Arc::new(create_backend().await); + let probe = PgFtsProbe(create_backend().await); + let tenant = create_tenant("fts_reindex_repeat"); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + fts_suite::repeated_reindex_does_not_duplicate_fts_rows( + backend.as_ref(), + &probe, + &tenant, + &reindex, + ) + .await; + } } diff --git a/crates/persistence/tests/search/fts_purge_suite.rs b/crates/persistence/tests/search/fts_purge_suite.rs new file mode 100644 index 000000000..c455234e7 --- /dev/null +++ b/crates/persistence/tests/search/fts_purge_suite.rs @@ -0,0 +1,599 @@ +//! Backend-agnostic full-text purge-completeness suite (issue #386). +//! +//! Every scenario is generic over the storage traits, so the *same* assertions +//! run on SQLite and PostgreSQL rather than being retyped per engine. Like +//! `transactions/if_match_suite.rs`, this file is `#[path]`-included by each +//! backend's test binary rather than living in `tests/common/`, which no test +//! target declares and cargo therefore never compiles (issue #306). +//! +//! ## Why these tests assert on raw rows, not on search results +//! +//! The obvious regression test — "purge a resource, then check `_text` finds +//! nothing" — **passes on the unfixed code**, and is therefore worthless. +//! +//! SQLite compiles `_text`/`_content` into a sub-filter: +//! `resource_id IN (SELECT resource_id FROM resource_fts WHERE tenant_id = ?1 +//! AND … MATCH ?n)`, AND-ed onto an outer query that selects from +//! `search_index`. Purge *does* clear `search_index`, so an orphaned +//! `resource_fts` row is unreachable through search while the resource's +//! narrative and complete serialized body are still on disk. +//! +//! So each scenario asserts two independent things: +//! +//! 1. **Residency** — a direct `SELECT COUNT(*) FROM resource_fts` via +//! [`FtsProbe`], which bypasses every query path and answers "is the PHI +//! still there", not "can a search reach it". +//! 2. **Exploitability** — the id-reuse oracle. Purge a resource, create a new +//! one reusing the same logical id, and the *old* resource's text becomes +//! matchable again and is attributed to the new resource. This is the +//! user-visible harm in the issue, and unlike (1) it fails through the +//! public search API. +//! +//! Every scenario also opens with a **positive control** asserting the row was +//! indexed in the first place. Without it, a broken indexer, a typo'd tenant, a +//! missing feature, or a renamed parameter would sail straight through the +//! post-conditions and the test would pass while proving nothing. +//! +//! ## Shared-database backends +//! +//! The PostgreSQL suite runs every scenario against one long-lived container +//! database, so scenarios must not collide. Each takes its own +//! [`TenantContext`]; callers must pass a **distinct tenant per scenario**. + +#![allow(dead_code)] + +use serde_json::{Value, json}; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{PurgableStorage, ResourceStorage, SearchProvider}; +use helios_persistence::tenant::TenantContext; +use helios_persistence::types::{SearchParamType, SearchParameter, SearchQuery, SearchValue}; + +/// A rare token planted in narrative text. Long and unambiguous so a match can +/// only come from the resource this suite created, never from seeded +/// conformance data or another test's fixtures. +pub const PLANTED_TERM: &str = "Zebracrossingdiagnosis"; + +/// A second planted token, for the resource that must survive. +pub const BYSTANDER_TERM: &str = "Quokkaflavoured"; + +/// Reads physical `resource_fts` rows, bypassing every query path. +/// +/// This is what distinguishes "the PHI is gone" from "a search cannot currently +/// reach the PHI". Both backends can implement it from an integration-test +/// target without any production API change: PostgreSQL exposes +/// `#[doc(hidden)] pub get_client`, and SQLite can be pointed at a file-backed +/// database that the test opens a second `rusqlite` connection onto. +#[async_trait::async_trait] +pub trait FtsProbe { + /// Number of `resource_fts` rows held for `tenant_id`. + async fn fts_row_count(&self, tenant_id: &str) -> u64; + + /// Number of `resource_fts` rows anywhere whose stored text contains + /// `needle`, **with no tenant predicate**. + /// + /// The unqualified form is what catches a `DELETE` whose `WHERE` clause is + /// wrong in a way a tenant-scoped count would hide. + async fn fts_rows_containing(&self, needle: &str) -> u64; +} + +/// A Patient carrying `term` in its generated narrative, which is what +/// `_text` indexes. +pub fn patient_with_narrative(id: &str, term: &str) -> Value { + json!({ + "resourceType": "Patient", + "id": id, + "text": { + "status": "generated", + "div": format!( + "

Assessment: {term}.

" + ) + }, + "name": [{"family": "Purgetest"}] + }) +} + +/// A Patient with no trace of the planted term, for id-reuse scenarios. +pub fn unrelated_patient(id: &str) -> Value { + json!({ + "resourceType": "Patient", + "id": id, + "text": { + "status": "generated", + "div": "

Routine review.

" + }, + "name": [{"family": "Replacement"}] + }) +} + +fn text_query(term: &str) -> SearchQuery { + SearchQuery::new("Patient").with_parameter(SearchParameter { + name: "_text".to_string(), + param_type: SearchParamType::Special, + modifier: None, + values: vec![SearchValue::eq(term)], + chain: vec![], + components: vec![], + }) +} + +fn content_query(term: &str) -> SearchQuery { + SearchQuery::new("Patient").with_parameter(SearchParameter { + name: "_content".to_string(), + param_type: SearchParamType::Special, + modifier: None, + values: vec![SearchValue::eq(term)], + chain: vec![], + components: vec![], + }) +} + +async fn text_hits(backend: &B, tenant: &TenantContext, term: &str) -> Vec +where + B: SearchProvider, +{ + backend + .search(tenant, &text_query(term)) + .await + .expect("_text search should succeed") + .resources + .items + .iter() + .map(|r| r.id().to_string()) + .collect() +} + +async fn content_hits(backend: &B, tenant: &TenantContext, term: &str) -> Vec +where + B: SearchProvider, +{ + backend + .search(tenant, &content_query(term)) + .await + .expect("_content search should succeed") + .resources + .items + .iter() + .map(|r| r.id().to_string()) + .collect() +} + +/// Seeds one Patient carrying `PLANTED_TERM` and asserts the full-text pipeline +/// actually indexed it. Every scenario starts here; if this fails, nothing +/// after it would have meant anything. +async fn seed_and_control(backend: &B, probe: &P, tenant: &TenantContext, id: &str) +where + B: ResourceStorage + SearchProvider, + P: FtsProbe, +{ + backend + .create( + tenant, + "Patient", + patient_with_narrative(id, PLANTED_TERM), + FhirVersion::default(), + ) + .await + .expect("seed create should succeed"); + + let rows = probe.fts_row_count(tenant.tenant_id().as_str()).await; + assert_eq!( + rows, 1, + "POSITIVE CONTROL: the seeded resource must be present in resource_fts \ + before the purge, or every assertion below is vacuous" + ); + assert_eq!( + text_hits(backend, tenant, PLANTED_TERM).await, + vec![id.to_string()], + "POSITIVE CONTROL: _text must find the seeded narrative before the purge" + ); +} + +// =========================================================================== +// Residency: the purge paths must remove the physical rows +// =========================================================================== + +/// Single-resource `$purge` removes the resource's full-text row. +pub async fn purge_removes_fts_rows(backend: &B, probe: &P, tenant: &TenantContext) +where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge(tenant, "Patient", "p1") + .await + .expect("purge should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "purge must delete the resource's resource_fts row; a surviving row keeps \ + the narrative and the full serialized body on disk after the API \ + reported the resource was removed" + ); + assert_eq!( + probe.fts_rows_containing(PLANTED_TERM).await, + 0, + "no resource_fts row anywhere may still contain the purged text" + ); +} + +/// Type-level `$purge` removes every matching resource's full-text row. +pub async fn purge_all_removes_fts_rows(backend: &B, probe: &P, tenant: &TenantContext) +where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge_all(tenant, "Patient") + .await + .expect("purge_all should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "purge_all must delete the type's resource_fts rows" + ); + assert_eq!(probe.fts_rows_containing(PLANTED_TERM).await, 0); +} + +/// Tenant purge removes every one of the tenant's full-text rows, across types. +pub async fn purge_tenant_data_removes_fts_rows( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .create( + tenant, + "Observation", + json!({ + "resourceType": "Observation", + "id": "o1", + "status": "final", + "code": {"text": "vitals"}, + "text": { + "status": "generated", + "div": format!( + "

{PLANTED_TERM}

" + ) + } + }), + FhirVersion::default(), + ) + .await + .expect("second seed should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 2, + "POSITIVE CONTROL: both seeded resources must be indexed" + ); + + backend + .purge_tenant_data(tenant.tenant_id().as_str()) + .await + .expect("purge_tenant_data should succeed"); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "purge_tenant_data must delete every one of the tenant's resource_fts \ + rows, across all resource types" + ); + assert_eq!(probe.fts_rows_containing(PLANTED_TERM).await, 0); +} + +/// The purge must not be over-broad: another tenant's rows survive untouched. +/// +/// A `DELETE FROM resource_fts` that lost its tenant predicate would satisfy +/// every other assertion in this suite. This is the only scenario that catches +/// it. +pub async fn purge_tenant_data_leaves_other_tenants_intact( + backend: &B, + probe: &P, + victim: &TenantContext, + bystander: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, victim, "p1").await; + + backend + .create( + bystander, + "Patient", + patient_with_narrative("p1", BYSTANDER_TERM), + FhirVersion::default(), + ) + .await + .expect("bystander seed should succeed"); + assert_eq!( + probe.fts_row_count(bystander.tenant_id().as_str()).await, + 1, + "POSITIVE CONTROL: the bystander tenant must be indexed too" + ); + + backend + .purge_tenant_data(victim.tenant_id().as_str()) + .await + .expect("purge_tenant_data should succeed"); + + assert_eq!( + probe.fts_row_count(victim.tenant_id().as_str()).await, + 0, + "the purged tenant's rows must be gone" + ); + assert_eq!( + probe.fts_row_count(bystander.tenant_id().as_str()).await, + 1, + "purging one tenant must not touch another tenant's resource_fts rows" + ); + assert_eq!( + text_hits(backend, bystander, BYSTANDER_TERM).await, + vec!["p1".to_string()], + "the bystander tenant's full-text search must still work after the purge" + ); +} + +// =========================================================================== +// Exploitability: the id-reuse oracle +// =========================================================================== + +/// Purging a resource and then reusing its logical id must not resurrect the +/// purged narrative. +/// +/// This is the harm the issue describes, and it fails through the public search +/// API on unfixed code: the orphaned `resource_fts` row is matched by +/// `_text`, and because the sub-select intersects on `resource_id` alone, the +/// hit is attributed to the *new* resource. +pub async fn reuse_after_purge_does_not_resurrect_narrative( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge(tenant, "Patient", "p1") + .await + .expect("purge should succeed"); + + // Same logical id — the ordinary case for client-assigned ids, fixtures and + // reload workflows. + backend + .create( + tenant, + "Patient", + unrelated_patient("p1"), + FhirVersion::default(), + ) + .await + .expect("re-create should succeed"); + + assert_eq!( + text_hits(backend, tenant, PLANTED_TERM).await, + Vec::::new(), + "the purged resource's narrative must not be matchable through a \ + re-created resource that merely reuses its id" + ); + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "only the replacement resource may be indexed; a count of 2 means the \ + purged generation's row is still on disk" + ); +} + +/// Same oracle, reached through a tenant purge and tenant-id reuse — the +/// scenario named in the issue. +pub async fn tenant_reuse_does_not_resurrect_narrative( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + seed_and_control(backend, probe, tenant, "p1").await; + + backend + .purge_tenant_data(tenant.tenant_id().as_str()) + .await + .expect("purge_tenant_data should succeed"); + + // A tenant of the same id is provisioned again and stores its own data. + backend + .create( + tenant, + "Patient", + unrelated_patient("p1"), + FhirVersion::default(), + ) + .await + .expect("re-create under the reused tenant id should succeed"); + + assert_eq!( + text_hits(backend, tenant, PLANTED_TERM).await, + Vec::::new(), + "a re-created tenant must not be able to surface the previous tenant's \ + purged content" + ); + assert_eq!( + content_hits(backend, tenant, PLANTED_TERM).await, + Vec::::new(), + "_content must be clean too, not only _text" + ); + assert_eq!(probe.fts_row_count(tenant.tenant_id().as_str()).await, 1); +} + +/// Repeated create/purge cycles must not accumulate orphaned rows. +/// +/// Guards against unbounded growth of the FTS table — every purged generation +/// leaving a row behind is both a leak and a slow, permanent index bloat. +pub async fn repeated_purge_and_recreate_does_not_grow_fts( + backend: &B, + probe: &P, + tenant: &TenantContext, +) where + B: ResourceStorage + SearchProvider + PurgableStorage, + P: FtsProbe, +{ + for cycle in 0..5 { + backend + .create( + tenant, + "Patient", + patient_with_narrative("p1", PLANTED_TERM), + FhirVersion::default(), + ) + .await + .unwrap_or_else(|e| panic!("create on cycle {cycle} should succeed: {e}")); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "POSITIVE CONTROL on cycle {cycle}: exactly one row should be indexed" + ); + + backend + .purge(tenant, "Patient", "p1") + .await + .unwrap_or_else(|e| panic!("purge on cycle {cycle} should succeed: {e}")); + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 0, + "cycle {cycle} left an orphaned resource_fts row behind" + ); + } +} + +// =========================================================================== +// $reindex must REBUILD full-text search, not destroy it +// =========================================================================== + +/// Drives a reindex job to completion, panicking on failure or timeout. +async fn run_reindex_to_completion( + reindex: &helios_persistence::search::ReindexOperation, + tenant: &TenantContext, + request: helios_persistence::search::ReindexRequest, +) { + use helios_persistence::search::ReindexStatus; + + let job_id = reindex + .start(tenant.clone(), request, None) + .await + .expect("reindex should start"); + + for _ in 0..200 { + tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; + let progress = reindex + .get_progress(&job_id) + .await + .expect("progress should be readable"); + match progress.status { + ReindexStatus::Completed => return, + ReindexStatus::Failed => { + panic!("reindex failed: {:?}", progress.error_message) + } + _ => {} + } + } + panic!("reindex timed out"); +} + +/// `$reindex` must leave `_text`/`_content` working. +/// +/// On unfixed code this fails on **both** backends and in **both** modes: +/// `run_reindex` calls `delete_search_entries` for every resource +/// unconditionally — which does drop the `resource_fts` row — while +/// `write_search_entries` never put it back. So the documented recovery +/// operation silently disabled full-text search until each resource happened to +/// be written again. `clear_existing` was never the discriminator. +pub async fn reindex_preserves_full_text_search( + backend: &B, + probe: &P, + tenant: &TenantContext, + reindex: &helios_persistence::search::ReindexOperation, + clear_existing: bool, +) where + B: ResourceStorage + SearchProvider, + P: FtsProbe, +{ + use helios_persistence::search::ReindexRequest; + + seed_and_control(backend, probe, tenant, "p1").await; + + let mut request = ReindexRequest::for_types(vec!["Patient"]); + if clear_existing { + request = request.clear_existing(); + } + run_reindex_to_completion(reindex, tenant, request).await; + + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "$reindex (clear_existing={clear_existing}) must rebuild the resource's \ + resource_fts row, not drop it" + ); + assert_eq!( + text_hits(backend, tenant, PLANTED_TERM).await, + vec!["p1".to_string()], + "_text must still work after $reindex (clear_existing={clear_existing})" + ); + assert_eq!( + content_hits(backend, tenant, PLANTED_TERM).await, + vec!["p1".to_string()], + "_content must still work after $reindex (clear_existing={clear_existing})" + ); +} + +/// A reindex must not duplicate full-text rows. +/// +/// SQLite's `index_fts_content` is a bare INSERT with no delete-first; it is +/// only safe because `run_reindex` deletes the resource's entries immediately +/// beforehand. This pins that ordering — if it is ever changed, the row count +/// doubles here rather than silently bloating production indexes. +pub async fn repeated_reindex_does_not_duplicate_fts_rows( + backend: &B, + probe: &P, + tenant: &TenantContext, + reindex: &helios_persistence::search::ReindexOperation, +) where + B: ResourceStorage + SearchProvider, + P: FtsProbe, +{ + use helios_persistence::search::ReindexRequest; + + seed_and_control(backend, probe, tenant, "p1").await; + + for pass in 0..3 { + run_reindex_to_completion(reindex, tenant, ReindexRequest::for_types(vec!["Patient"])) + .await; + assert_eq!( + probe.fts_row_count(tenant.tenant_id().as_str()).await, + 1, + "reindex pass {pass} must leave exactly one resource_fts row" + ); + } + + assert_eq!( + text_hits(backend, tenant, PLANTED_TERM).await, + vec!["p1".to_string()], + "_text must still return exactly one hit after repeated reindexing" + ); +} diff --git a/crates/persistence/tests/search/fts_purge_tests.rs b/crates/persistence/tests/search/fts_purge_tests.rs new file mode 100644 index 000000000..211db6767 --- /dev/null +++ b/crates/persistence/tests/search/fts_purge_tests.rs @@ -0,0 +1,417 @@ +//! SQLite bindings for the backend-agnostic full-text purge suite (issue #386). +//! +//! The scenarios live in `fts_purge_suite.rs` and are shared verbatim with the +//! PostgreSQL test binary; this file only supplies the SQLite backend, the +//! [`FtsProbe`] implementation, and one `#[tokio::test]` per scenario. +//! +//! ## Why these tests use a file-backed database +//! +//! Proving the fix requires reading `resource_fts` directly — a search-level +//! assertion is vacuous here (see the suite's module docs). `SqliteBackend`'s +//! connection pool is `pub(crate)` and unreachable from an integration test, and +//! the usual `:memory:` harness uses a private per-instance shared-cache name +//! that nothing else can attach to. A temporary file solves both without adding +//! a `#[doc(hidden)] pub` accessor to production code purely for tests. + +#![cfg(feature = "sqlite")] + +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use helios_persistence::backends::sqlite::{SqliteBackend, SqliteBackendConfig}; +use helios_persistence::search::ReindexOperation; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; + +use super::fts_purge_suite::{self as suite, FtsProbe}; + +/// A file-backed SQLite backend plus the path its data lives at. +/// +/// The `TempDir` is returned so the caller keeps it alive: dropping it deletes +/// the database out from under the backend. +fn file_backend() -> (SqliteBackend, tempfile::TempDir, PathBuf) { + let data_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(|p| p.parent()) + .map(|p| p.join("data")) + .unwrap_or_else(|| PathBuf::from("data")); + + let dir = tempfile::tempdir().expect("temp dir"); + let db_path = dir.path().join("fts_purge.db"); + + let config = SqliteBackendConfig { + data_dir: Some(data_dir), + ..Default::default() + }; + let backend = SqliteBackend::with_config(&db_path, config).expect("file-backed SQLite backend"); + backend.init_schema().expect("init schema"); + (backend, dir, db_path) +} + +fn tenant(id: &str) -> TenantContext { + TenantContext::new(TenantId::new(id), TenantPermissions::full_access()) +} + +/// Reads `resource_fts` over a second connection to the same database file. +struct SqliteFtsProbe(PathBuf); + +impl SqliteFtsProbe { + fn open(&self) -> rusqlite::Connection { + rusqlite::Connection::open(&self.0).expect("probe connection") + } +} + +#[async_trait::async_trait] +impl FtsProbe for SqliteFtsProbe { + async fn fts_row_count(&self, tenant_id: &str) -> u64 { + let conn = self.open(); + conn.query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = ?1", + [tenant_id], + |row| row.get::<_, i64>(0), + ) + .expect("resource_fts must exist in a bundled-rusqlite build") as u64 + } + + async fn fts_rows_containing(&self, needle: &str) -> u64 { + let conn = self.open(); + let pattern = format!("%{needle}%"); + conn.query_row( + "SELECT COUNT(*) FROM resource_fts \ + WHERE full_content LIKE ?1 OR narrative_text LIKE ?1", + [&pattern], + |row| row.get::<_, i64>(0), + ) + .expect("resource_fts must exist in a bundled-rusqlite build") as u64 + } +} + +/// FTS5 must be compiled into the test build. +/// +/// Without it `create_fts_table` silently creates nothing and every full-text +/// assertion in this suite would be testing an absent feature. `rusqlite` is +/// pinned with the `bundled` feature, whose amalgamation is built with +/// `-DSQLITE_ENABLE_FTS5`; this fails loudly if that ever changes. +#[test] +fn fts5_is_compiled_into_the_test_build() { + let conn = rusqlite::Connection::open_in_memory().expect("in-memory connection"); + let modules: i64 = conn + .query_row( + "SELECT COUNT(*) FROM pragma_module_list WHERE name = 'fts5'", + [], + |row| row.get(0), + ) + .expect("pragma_module_list should be queryable"); + assert_eq!( + modules, 1, + "FTS5 is absent from this SQLite build, so every _text/_content \ + assertion in this suite would be vacuous. rusqlite must keep its \ + `bundled` feature (crates/persistence/Cargo.toml)." + ); +} + +/// The FTS5 table really is created by `init_schema` on a fresh database. +#[test] +fn resource_fts_table_exists_after_init_schema() { + let (_backend, _dir, path) = file_backend(); + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + let found: i64 = conn + .query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='resource_fts'", + [], + |row| row.get(0), + ) + .expect("sqlite_master query"); + assert_eq!(found, 1, "init_schema must create resource_fts"); +} + +macro_rules! sqlite_fts_test { + ($name:ident, $scenario:ident) => { + #[tokio::test] + async fn $name() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + suite::$scenario(&backend, &probe, &tenant("acme")).await; + } + }; +} + +sqlite_fts_test!(sqlite_purge_removes_fts_rows, purge_removes_fts_rows); +sqlite_fts_test!( + sqlite_purge_all_removes_fts_rows, + purge_all_removes_fts_rows +); +sqlite_fts_test!( + sqlite_purge_tenant_data_removes_fts_rows, + purge_tenant_data_removes_fts_rows +); +sqlite_fts_test!( + sqlite_reuse_after_purge_does_not_resurrect_narrative, + reuse_after_purge_does_not_resurrect_narrative +); +sqlite_fts_test!( + sqlite_tenant_reuse_does_not_resurrect_narrative, + tenant_reuse_does_not_resurrect_narrative +); +sqlite_fts_test!( + sqlite_repeated_purge_and_recreate_does_not_grow_fts, + repeated_purge_and_recreate_does_not_grow_fts +); + +#[tokio::test] +async fn sqlite_purge_tenant_data_leaves_other_tenants_intact() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + suite::purge_tenant_data_leaves_other_tenants_intact( + &backend, + &probe, + &tenant("acme"), + &tenant("globex"), + ) + .await; +} + +#[tokio::test] +async fn sqlite_reindex_preserves_full_text_search_without_clear() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + let backend = Arc::new(backend); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + suite::reindex_preserves_full_text_search( + backend.as_ref(), + &probe, + &tenant("acme"), + &reindex, + false, + ) + .await; +} + +#[tokio::test] +async fn sqlite_reindex_preserves_full_text_search_with_clear() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + let backend = Arc::new(backend); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + suite::reindex_preserves_full_text_search( + backend.as_ref(), + &probe, + &tenant("acme"), + &reindex, + true, + ) + .await; +} + +#[tokio::test] +async fn sqlite_repeated_reindex_does_not_duplicate_fts_rows() { + let (backend, _dir, path) = file_backend(); + let probe = SqliteFtsProbe(path); + let backend = Arc::new(backend); + let reindex = ReindexOperation::new(backend.clone(), backend.tenant_registries().clone()); + suite::repeated_reindex_does_not_duplicate_fts_rows( + backend.as_ref(), + &probe, + &tenant("acme"), + &reindex, + ) + .await; +} + +/// A purge must succeed on a database with no FTS5 table. +/// +/// FTS5 is an optional SQLite compile-time feature and `create_fts_table` +/// tolerates its absence, so the purge paths probe before deleting. Dropping the +/// table reproduces that state: purge must return `Ok`, not fail on +/// `no such table: resource_fts`. +#[tokio::test] +async fn sqlite_purge_succeeds_when_fts_table_is_absent() { + let (backend, _dir, path) = file_backend(); + let tenant = tenant("acme"); + + helios_persistence::core::ResourceStorage::create( + &backend, + &tenant, + "Patient", + suite::patient_with_narrative("p1", suite::PLANTED_TERM), + helios_fhir::FhirVersion::default(), + ) + .await + .expect("seed create"); + + drop_fts_table(&path); + + use helios_persistence::core::PurgableStorage; + backend + .purge(&tenant, "Patient", "p1") + .await + .expect("purge must tolerate a database without FTS5"); + backend + .purge_all(&tenant, "Patient") + .await + .expect("purge_all must tolerate a database without FTS5"); + helios_persistence::core::ResourceStorage::purge_tenant_data(&backend, "acme") + .await + .expect("purge_tenant_data must tolerate a database without FTS5"); +} + +fn drop_fts_table(path: &Path) { + let conn = rusqlite::Connection::open(path).expect("probe connection"); + conn.execute("DROP TABLE resource_fts", []) + .expect("drop resource_fts"); +} + +/// The `v14 -> v15` migration sweeps orphans left by the pre-fix purge paths. +/// +/// The code fix only stops *new* orphans. Every database that has already served +/// a purge is still holding the narrative and full body of resources its +/// operator was told were removed, so the fix is only half delivered without +/// this sweep. +/// +/// Reproduces that state directly — an FTS row whose `resources` row is gone, +/// which is exactly what the old `purge` left behind — then reopens the database +/// at the pre-migration version and asserts the orphan is swept while a live +/// resource's row is untouched. +#[tokio::test] +async fn migration_sweeps_orphaned_fts_rows_from_existing_databases() { + let (backend, _dir, path) = file_backend(); + let tenant = tenant("acme"); + + // One resource that stays, one that will be orphaned. + for id in ["keeper", "orphan"] { + helios_persistence::core::ResourceStorage::create( + &backend, + &tenant, + "Patient", + suite::patient_with_narrative(id, suite::PLANTED_TERM), + helios_fhir::FhirVersion::default(), + ) + .await + .expect("seed create"); + } + drop(backend); + + { + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + // Delete the resource WITHOUT touching resource_fts — precisely what the + // pre-fix purge paths did. + conn.execute( + "DELETE FROM resources WHERE tenant_id = 'acme' AND id = 'orphan'", + [], + ) + .expect("simulate a pre-fix purge"); + + let rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = 'acme'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + rows, 2, + "POSITIVE CONTROL: the simulated pre-fix purge must leave the orphan behind" + ); + + // Wind the recorded schema version back so the migration ladder re-runs. + conn.execute("DELETE FROM schema_version", []) + .expect("clear version"); + conn.execute("INSERT INTO schema_version (version) VALUES (14)", []) + .expect("set version 14"); + } + + // Reopening runs migrate_v14_to_v15. + let config = SqliteBackendConfig { + data_dir: None, + ..Default::default() + }; + let reopened = SqliteBackend::with_config(&path, config).expect("reopen backend"); + reopened.init_schema().expect("migrate to v15"); + + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + let remaining: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = 'acme'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + remaining, 1, + "the v15 migration must sweep the orphaned resource_fts row and keep the live one" + ); + + let orphan_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE resource_id = 'orphan'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!(orphan_rows, 0, "the orphan's row specifically must be gone"); + + let keeper_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE resource_id = 'keeper'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + keeper_rows, 1, + "the migration must not delete rows for resources that still exist" + ); +} + +/// A soft-deleted resource keeps its `resources` row, so the sweep must not +/// touch its full-text entry — matching current behaviour, where a soft delete +/// does not drop the FTS row. +#[tokio::test] +async fn migration_preserves_fts_rows_for_soft_deleted_resources() { + let (backend, _dir, path) = file_backend(); + let tenant = tenant("acme"); + + helios_persistence::core::ResourceStorage::create( + &backend, + &tenant, + "Patient", + suite::patient_with_narrative("p1", suite::PLANTED_TERM), + helios_fhir::FhirVersion::default(), + ) + .await + .expect("seed create"); + helios_persistence::core::ResourceStorage::delete(&backend, &tenant, "Patient", "p1") + .await + .expect("soft delete"); + drop(backend); + + { + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + conn.execute("DELETE FROM schema_version", []).unwrap(); + conn.execute("INSERT INTO schema_version (version) VALUES (14)", []) + .unwrap(); + } + + let reopened = SqliteBackend::with_config( + &path, + SqliteBackendConfig { + data_dir: None, + ..Default::default() + }, + ) + .expect("reopen backend"); + reopened.init_schema().expect("migrate to v15"); + + let conn = rusqlite::Connection::open(&path).expect("probe connection"); + let rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM resource_fts WHERE tenant_id = 'acme'", + [], + |row| row.get(0), + ) + .expect("count"); + assert_eq!( + rows, 1, + "a soft-deleted resource still has a `resources` row, so the sweep must \ + leave its full-text entry alone" + ); +} diff --git a/crates/persistence/tests/search/mod.rs b/crates/persistence/tests/search/mod.rs index 1dbf97dc7..01ed30722 100644 --- a/crates/persistence/tests/search/mod.rs +++ b/crates/persistence/tests/search/mod.rs @@ -40,6 +40,10 @@ pub fn make_sqlite_backend() -> SqliteBackend { pub mod chained_tests; pub mod date_tests; +/// Backend-agnostic scenarios, shared with the PostgreSQL test binary via +/// `#[path]` (issue #386). +pub mod fts_purge_suite; +pub mod fts_purge_tests; pub mod include_tests; pub mod modifier_tests; pub mod number_tests; diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 83251749c..1a9edc8d2 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -3260,13 +3260,15 @@ async fn test_content_search_basic() { let result = backend.search(&tenant, &query).await.unwrap(); - // Should find patients from Illinois (FTS5 must be available) - // If FTS5 is not available, no results will be returned - let count = result.resources.items.len(); - assert!( - count == 0 || count == 2, - "Should find 0 (FTS unavailable) or 2 (FTS available) patients, found {}", - count + // `rusqlite` is pinned with the `bundled` feature, whose amalgamation is + // built with `-DSQLITE_ENABLE_FTS5`, so FTS5 is always available here. + // This used to accept 0 as well ("FTS unavailable"), which meant it stayed + // green through a *total* full-text outage — exactly what happened while + // `$reindex` was silently dropping every `resource_fts` row (issue #386). + assert_eq!( + result.resources.items.len(), + 2, + "Should find both Illinois patients" ); } @@ -3312,16 +3314,16 @@ async fn test_text_search_narrative() { let result = backend.search(&tenant, &query).await.unwrap(); - // FTS5 dependent - either finds 1 or 0 - let count = result.resources.items.len(); - assert!( - count <= 1, - "Should find at most 1 patient with diabetes in narrative" + // Was `count <= 1`, which is satisfied by zero and so could not detect + // full-text search being broken at all (issue #386). FTS5 is always + // compiled in via rusqlite's `bundled` feature, so assert the real result. + assert_eq!( + result.resources.items.len(), + 1, + "Should find the patient with diabetes in the narrative" ); - if count == 1 { - assert_eq!(result.resources.items[0].id(), "text-1"); - } + assert_eq!(result.resources.items[0].id(), "text-1"); } // ============================================================================ diff --git a/crates/ui/src/tenants.rs b/crates/ui/src/tenants.rs index 42fb66b7c..4d79d279a 100644 --- a/crates/ui/src/tenants.rs +++ b/crates/ui/src/tenants.rs @@ -357,13 +357,25 @@ pub async fn delete( }; let _ = storage.deregister_tenant(&id).await; - if query.purge { - let _ = storage.purge_tenant_data(&id).await; - } + // A failed purge must be surfaced, not swallowed. The tenant has already + // been deregistered by the line above, so discarding this error leaves the + // data on disk with nothing in the registry pointing at it, while the page + // renders an ordinary success — an operator told "purged" who still holds + // every resource. `purge_tenant_data` is transactional on the SQL backends, + // so on failure nothing was removed and a retry is safe. + let purge_error = if query.purge { + storage + .purge_tenant_data(&id) + .await + .err() + .map(|e| format!("Tenant '{id}' was deregistered but its data was NOT purged: {e}")) + } else { + None + }; match load_rows(storage, "").await { - Ok(rows) => rows_response(i18n, rows, None), - Err(e) => rows_response(i18n, Vec::new(), Some(e)), + Ok(rows) => rows_response(i18n, rows, purge_error), + Err(e) => rows_response(i18n, Vec::new(), purge_error.or(Some(e))), } } From 95113a3662b197bf35675923c7ec09cf755c2f4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Thu, 30 Jul 2026 19:42:21 -0400 Subject: [PATCH 2/3] fix(postgres): apply _text/_content instead of dropping the filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PostgreSQL query builder had no arm for `_text`/`_content`: both are `SearchParamType::Special`, which fell through to the `None` case, so the parameter contributed no condition at all. A search whose only parameter was `_text` therefore built an empty filter and `search`/`search_count` answered a full-text query with **every** non-deleted resource of that type. Not a false negative — an unfiltered result set presented as a match. SQLite, Elasticsearch and MongoDB have all handled both parameters for as long as they have existed; PostgreSQL alone silently ignored them, while `resource_fts` was populated on every write and its tsvector columns and GIN indexes sat unused by the ordinary search path. `TextSearchProvider::search_text` does implement it correctly, but nothing calls it: the REST layer goes through `SearchProvider::search`. Both parameters now resolve through the same `resource_fts` columns the write path fills, with `plainto_tsquery('english', …)` to match `search_text`/`search_content`, and the term bound rather than spliced so tsquery operators in user input stay data. The sub-select is scoped by `tenant_id = $1` **and** `resource_type = $2`: it yields a bare `resource_id` set that the outer query intersects with this tenant's resources, so without the tenant predicate tenant B's Patient/123 would select tenant A's Patient/123 — a cross-tenant match oracle — and without the type predicate an Observation's narrative would select a Patient of the same id. Both binds are already tenant and resource type in every caller of `build_search_query`, the invariant `build_missing_condition` and `build_compartment_condition` already rely on. This is what the two failing PostgreSQL cases in the #386 suite were reporting. The suite asserts through the public search API precisely so that a backend which cannot filter cannot pass, and PostgreSQL could not: the id-reuse oracle re-created a resource under the purged id and `_text` returned it, not because the purged narrative survived (PostgreSQL's purge does delete the row) but because the search matched everything. Its positive controls passed for the same reason they were meant to catch — one resource in the tenant is indistinguishable from a filter that matched it. Both backends' FTS conditions also numbered placeholders off the loop index while skipping unmatchable terms, so a skipped value left a gap: on SQLite `_text=*,fracture` emitted `?4` while binding one parameter, which SQLite rejects when preparing the statement. Numbering now follows the accepted terms. And a term that survives neither escaping (SQLite) nor is present at all (blank) no longer drops the parameter, which would return the whole resource type — the same fail-open this commit exists to close. It now matches nothing, which is what a stopword-only term already does through `plainto_tsquery`. --- .../backends/postgres/search/query_builder.rs | 178 ++++++++++++++++++ .../backends/sqlite/search/query_builder.rs | 50 ++++- 2 files changed, 225 insertions(+), 3 deletions(-) diff --git a/crates/persistence/src/backends/postgres/search/query_builder.rs b/crates/persistence/src/backends/postgres/search/query_builder.rs index 00a6838f4..748ff999e 100644 --- a/crates/persistence/src/backends/postgres/search/query_builder.rs +++ b/crates/persistence/src/backends/postgres/search/query_builder.rs @@ -527,6 +527,19 @@ impl PostgresQueryBuilder { "_lastUpdated" => { return Self::build_last_updated_condition(¶m.values, param_offset); } + // Full text over the generated narrative (`_text`) and over the whole + // serialized resource (`_content`), against the same `resource_fts` + // tsvector columns the write path populates. + "_text" => { + return Self::build_fts_condition( + ¶m.values, + "narrative_tsvector", + param_offset, + ); + } + "_content" => { + return Self::build_fts_condition(¶m.values, "content_tsvector", param_offset); + } _ => {} } @@ -563,6 +576,72 @@ impl PostgresQueryBuilder { Some(combined) } + /// Builds the `_text` / `_content` full-text condition against + /// `resource_fts`. + /// + /// Without this, `SearchParamType::Special` fell through to the `None` arm + /// below and the parameter contributed **no** condition. A search whose only + /// parameter was `_text` therefore produced an empty filter and returned + /// every resource of the type — a text query answered with the whole + /// compartment, not a narrower or empty result. SQLite has handled both + /// parameters (via FTS5 `MATCH`) all along; this is the PostgreSQL half. + /// + /// `plainto_tsquery('english', …)` matches `search_text`/`search_content` in + /// `search_impl.rs`, and it parameterises the user's term rather than + /// splicing it, so a term containing tsquery operators is data, not syntax. + /// + /// `tenant_id = $1` is not optional: the sub-select yields a bare + /// `resource_id` set that the outer query intersects with *this* tenant's + /// resources, so omitting it would let tenant B's Patient/123 select tenant + /// A's Patient/123 — a cross-tenant match oracle. `resource_type = $2` + /// likewise keeps an Observation's narrative from selecting a Patient of the + /// same id. Both are already bound as tenant and resource type by every + /// caller of `build_search_query` (`search`, `search_count`), the same + /// invariant `build_missing_condition` and `build_compartment_condition` + /// rely on. + fn build_fts_condition( + values: &[SearchValue], + column: &str, + offset: usize, + ) -> Option { + let mut conditions = Vec::new(); + // Numbered off the running count of *accepted* values, not the loop + // index: a skipped value must not leave a gap, because the caller binds + // this fragment's params consecutively from `offset`. + let mut param_num = offset; + for value in values { + let term = value.value.trim(); + if term.is_empty() { + continue; + } + param_num += 1; + conditions.push(SqlFragment::with_params( + format!( + "id IN (SELECT resource_id FROM resource_fts \ + WHERE tenant_id = $1 AND resource_type = $2 \ + AND {} @@ plainto_tsquery('english', ${}))", + column, param_num + ), + vec![SqlParam::text(term)], + )); + } + if conditions.is_empty() { + // Every value was blank. Returning `None` here would drop the + // parameter and hand back the entire resource type — the defect this + // function exists to fix — so fail closed instead. A term of nothing + // matches nothing, which is also what a stopword-only term already + // does through `plainto_tsquery`. + return Some(SqlFragment::new("FALSE".to_string())); + } + // Repeated values of one parameter are a logical OR, as elsewhere in + // this builder. + let mut combined = conditions.remove(0); + for cond in conditions { + combined = combined.or(cond); + } + Some(combined) + } + fn build_last_updated_condition(values: &[SearchValue], offset: usize) -> Option { let mut conditions = Vec::new(); for (i, value) in values.iter().enumerate() { @@ -1625,6 +1704,105 @@ mod tests { assert_eq!(frag.params.len(), 3); } + fn special_param(name: &str, values: Vec) -> SearchParameter { + SearchParameter { + name: name.to_string(), + param_type: SearchParamType::Special, + modifier: None, + values, + chain: vec![], + components: vec![], + } + } + + #[test] + fn text_search_filters_instead_of_returning_everything() { + // Regression: `SearchParamType::Special` fell through to `None`, so a + // `_text`-only query built no filter at all and `search` answered a + // full-text query with every resource of the type. + let query = SearchQuery::new("Patient").with_parameter(special_param( + "_text", + vec![SearchValue::eq("Zebracrossingdiagnosis")], + )); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("_text must produce a condition, not an empty filter"); + + assert!( + frag.sql.contains("FROM resource_fts"), + "must resolve through the full-text table: {}", + frag.sql + ); + assert!( + frag.sql + .contains("narrative_tsvector @@ plainto_tsquery('english', $3)"), + "_text matches the narrative column: {}", + frag.sql + ); + // Tenant and type scoping keep the sub-select from selecting another + // tenant's — or another resource type's — row of the same id. + assert!(frag.sql.contains("tenant_id = $1"), "{}", frag.sql); + assert!(frag.sql.contains("resource_type = $2"), "{}", frag.sql); + assert_eq!(frag.params.len(), 1); + } + + #[test] + fn content_search_uses_the_content_column() { + let query = SearchQuery::new("Patient").with_parameter(special_param( + "_content", + vec![SearchValue::eq("Quokkaflavoured")], + )); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("_content must produce a condition"); + + assert!( + frag.sql + .contains("content_tsvector @@ plainto_tsquery('english', $3)"), + "{}", + frag.sql + ); + assert_eq!(frag.params.len(), 1); + } + + #[test] + fn text_or_list_placeholders_are_gap_free() { + // Two terms OR together, and a blank one must not consume a placeholder + // number it never binds — the caller binds this fragment's params + // consecutively. + let query = SearchQuery::new("Patient").with_parameter(special_param( + "_text", + vec![ + SearchValue::eq(" "), + SearchValue::eq("fracture"), + SearchValue::eq("sprain"), + ], + )); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("_text OR-list should produce a condition"); + + assert_eq!(frag.params.len(), 2, "the blank term binds nothing"); + assert!(frag.sql.contains("$3"), "{}", frag.sql); + assert!(frag.sql.contains("$4"), "{}", frag.sql); + assert!( + !frag.sql.contains("$5"), + "placeholder numbering must be gap-free: {}", + frag.sql + ); + assert!(frag.sql.contains(" OR "), "{}", frag.sql); + } + + #[test] + fn blank_text_term_fails_closed() { + // Dropping the parameter would return the whole resource type, which is + // the exact failure mode being fixed. + let query = SearchQuery::new("Patient") + .with_parameter(special_param("_text", vec![SearchValue::eq("")])); + let frag = PostgresQueryBuilder::build_search_query(&query, 2) + .expect("a blank _text must still constrain the query"); + + assert_eq!(frag.sql, "FALSE"); + assert!(frag.params.is_empty()); + } + #[test] fn token_or_list_placeholders_are_gap_free() { // Regression: `status=finished,in-progress` — two code-only token values, diff --git a/crates/persistence/src/backends/sqlite/search/query_builder.rs b/crates/persistence/src/backends/sqlite/search/query_builder.rs index c64b89036..aa35b4244 100644 --- a/crates/persistence/src/backends/sqlite/search/query_builder.rs +++ b/crates/persistence/src/backends/sqlite/search/query_builder.rs @@ -542,7 +542,15 @@ impl QueryBuilder { let mut conditions = Vec::new(); - for (i, value) in values.iter().enumerate() { + // Numbered off the running count of *accepted* terms rather than the + // loop index: escaping can empty a term (`_text=*` escapes to nothing), + // and the caller binds this fragment's params consecutively from + // `param_offset`, so a skipped value must not leave a placeholder gap. + // `_text=*,fracture` used to emit `?4` while binding only one param, + // which SQLite rejects at prepare time. + let mut param_num = param_offset; + + for value in values { // Escape and prepare the search term let search_term = Fts5Search::escape_fts_query(&value.value); if search_term.is_empty() { @@ -551,7 +559,7 @@ impl QueryBuilder { // Build the FTS match query // Use the column prefix to search only the specified column - let param_num = param_offset + i + 1; + param_num += 1; // `tenant_id = ?1` is not optional. Without it this sub-select // matches *every* tenant's `resource_fts` rows and yields a bare // `resource_id` set, which the outer query then intersects with this @@ -571,7 +579,10 @@ impl QueryBuilder { } if conditions.is_empty() { - return None; + // Every term escaped to nothing. Returning `None` would drop the + // parameter and answer a full-text query with every resource of the + // type; a term that cannot match must match nothing instead. + return Some(SqlFragment::new("0")); } // OR together multiple search terms @@ -901,6 +912,39 @@ mod tests { assert!(combined.sql.contains("OR")); } + #[test] + fn fts_placeholders_are_gap_free() { + // `*` escapes to nothing and binds no parameter, so the term after it + // must still take the next consecutive placeholder. The old index-based + // numbering emitted `?4` while binding a single param, and SQLite + // rejects the prepared statement. + let builder = QueryBuilder::new("tenant1", "Patient"); + let values = vec![SearchValue::eq("*"), SearchValue::eq("fracture")]; + + let frag = builder + .build_fts_condition(&values, "narrative_text", 2) + .expect("a matchable term must produce a condition"); + + assert_eq!(frag.params.len(), 1); + assert!(frag.sql.contains("?3"), "{}", frag.sql); + assert!(!frag.sql.contains("?4"), "{}", frag.sql); + } + + #[test] + fn fts_unmatchable_term_fails_closed() { + // Nothing survives escaping. Dropping the parameter would answer a + // full-text query with every resource of the type. + let builder = QueryBuilder::new("tenant1", "Patient"); + let values = vec![SearchValue::eq("*")]; + + let frag = builder + .build_fts_condition(&values, "narrative_text", 2) + .expect("an unmatchable term must still constrain the query"); + + assert_eq!(frag.sql, "0"); + assert!(frag.params.is_empty()); + } + #[test] fn test_query_builder_basic() { let builder = QueryBuilder::new("tenant1", "Patient"); From 55bafbb86b3d82db8b440171278b131d17e1282d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Fri, 31 Jul 2026 09:16:57 -0400 Subject: [PATCH 3/3] fix(postgres): index the narrative into _content; scope planted FTS terms Two CI failures on this branch, with two unrelated causes. 1. `_content` never saw the narrative on PostgreSQL. `collect_strings` skips the `div` key to keep raw XHTML markup out of the index, which also dropped the narrative text from `full_content` entirely -- so a term appearing only in `text.div` was findable through `_text` and invisible to `_content`. FHIR defines `_content` as a search over the entire content of the resource, which makes it a superset of `_text`; SQLite has always had the narrative in `_content`. Append the already-stripped narrative to `full_content`, so the markup still stays out (as does base64 `data`), and pin the superset relationship with unit tests. This is what `reindex_preserves_full_text_search` was failing on, not the reindex fix. 2. The residency probe is deliberately not tenant-scoped, so it counted rows planted by *other* scenarios: PostgreSQL runs every scenario concurrently against one shared container database, and several end with a live indexed row on purpose. A distinct tenant per scenario was not enough because the planted token was a shared constant. Derive the token from the tenant id instead -- the probe stays global, and still catches a surviving row under any tenant, type or id, but only counts rows this scenario planted. --- .../src/backends/postgres/storage.rs | 105 +++++++++++++++++- .../tests/search/fts_purge_suite.rs | 90 +++++++++++---- .../tests/search/fts_purge_tests.rs | 6 +- 3 files changed, 173 insertions(+), 28 deletions(-) diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index 66f9f1a50..317593a27 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -3410,10 +3410,29 @@ impl SearchableContent { } /// Extracts searchable text content from a FHIR resource. +/// +/// `full_content` backs `_content`, which FHIR defines as a search over *the +/// entire content of the resource* — so it must be a superset of `_text`, the +/// narrative-only search. `collect_strings` skips the `div` key deliberately, to +/// keep raw XHTML markup (`div`, `p`, `xmlns`, attribute values) out of the +/// index; that left the narrative missing from `_content` altogether, so a term +/// that appeared only in `text.div` was findable through `_text` and invisible +/// to `_content`. Appending the already-stripped narrative restores the +/// superset relationship without indexing the markup, and matches SQLite, which +/// has always had the narrative in `_content`. `data` stays excluded — base64 +/// attachment blobs are not text. fn extract_searchable_content(resource: &Value) -> SearchableContent { + let narrative = extract_narrative(resource); + let mut full_content = extract_all_strings(resource); + if !narrative.is_empty() { + if !full_content.is_empty() { + full_content.push(' '); + } + full_content.push_str(&narrative); + } SearchableContent { - narrative: extract_narrative(resource), - full_content: extract_all_strings(resource), + narrative, + full_content, } } @@ -3477,3 +3496,85 @@ fn collect_strings(value: &Value, parts: &mut Vec) { _ => {} } } + +#[cfg(test)] +mod fts_extraction_tests { + use super::*; + use serde_json::json; + + fn patient_with_narrative() -> Value { + json!({ + "resourceType": "Patient", + "name": [{"family": "Purgetest"}], + "text": { + "status": "generated", + "div": "

Assessment: \ + Zebracrossingdiagnosis.

" + }, + "photo": [{"contentType": "image/png", "data": "iVBORw0KGgoAAAANSUhEUg=="}] + }) + } + + #[test] + fn content_includes_the_narrative() { + // Regression: `_content` is "the entire content of the resource", so a + // term that only appears in the narrative must be reachable through it. + // `collect_strings` skips the `div` key, which used to drop the + // narrative from `full_content` entirely — `_text` found the term, + // `_content` did not. + let content = extract_searchable_content(&patient_with_narrative()); + + assert!( + content.narrative.contains("Zebracrossingdiagnosis"), + "narrative: {}", + content.narrative + ); + assert!( + content.full_content.contains("Zebracrossingdiagnosis"), + "_content must be a superset of _text: {}", + content.full_content + ); + assert!( + content.full_content.contains("Purgetest"), + "the non-narrative fields must still be there: {}", + content.full_content + ); + } + + #[test] + fn content_excludes_markup_and_binary_payloads() { + // The narrative goes in stripped, not raw: tag and attribute noise would + // make every resource match `xmlns` or `div`. Base64 attachment data + // stays out for the same reason plus index size. + let content = extract_searchable_content(&patient_with_narrative()); + + assert!( + !content.full_content.contains("xmlns"), + "{}", + content.full_content + ); + assert!( + !content.full_content.contains("

"), + "{}", + content.full_content + ); + assert!( + !content.full_content.contains("iVBORw0KGgo"), + "{}", + content.full_content + ); + } + + #[test] + fn narrative_only_resource_is_not_empty() { + // `index_fts_content` returns early on `is_empty()`; a resource whose + // only text is its narrative must still be indexed. + let content = extract_searchable_content(&json!({ + "resourceType": "Binary", + "text": {"div": "

Solitary

"} + })); + + assert!(!content.is_empty()); + assert!(content.full_content.contains("Solitary")); + } +} diff --git a/crates/persistence/tests/search/fts_purge_suite.rs b/crates/persistence/tests/search/fts_purge_suite.rs index c455234e7..e6577e1a6 100644 --- a/crates/persistence/tests/search/fts_purge_suite.rs +++ b/crates/persistence/tests/search/fts_purge_suite.rs @@ -39,6 +39,16 @@ //! The PostgreSQL suite runs every scenario against one long-lived container //! database, so scenarios must not collide. Each takes its own //! [`TenantContext`]; callers must pass a **distinct tenant per scenario**. +//! +//! A distinct tenant is not enough on its own, because [`FtsProbe:: +//! fts_rows_containing`] is deliberately *not* tenant-scoped — that is the whole +//! point of it. Scenarios run concurrently under `cargo test`, and several end +//! with a live indexed row on purpose (the reindex scenarios, the bystander +//! tenant), so a term shared across scenarios makes the unqualified count +//! non-zero for reasons that have nothing to do with the purge under test. +//! [`planted_term`] therefore derives the token from the tenant id: the probe +//! stays global — it still catches a surviving row under *any* tenant, type or +//! id — while only ever counting rows this scenario planted. #![allow(dead_code)] @@ -49,13 +59,45 @@ use helios_persistence::core::{PurgableStorage, ResourceStorage, SearchProvider} use helios_persistence::tenant::TenantContext; use helios_persistence::types::{SearchParamType, SearchParameter, SearchQuery, SearchValue}; -/// A rare token planted in narrative text. Long and unambiguous so a match can -/// only come from the resource this suite created, never from seeded +/// Stem of the token planted in narrative text. Rare and unambiguous, so a +/// match can only come from a resource this suite created, never from seeded /// conformance data or another test's fixtures. -pub const PLANTED_TERM: &str = "Zebracrossingdiagnosis"; +const PLANTED_STEM: &str = "Zebracrossingdiagnosis"; + +/// Stem of the second planted token, for the resource that must survive. +const BYSTANDER_STEM: &str = "Quokkaflavoured"; + +/// The token this scenario plants, unique to `tenant`. +/// +/// See the module docs: the residency probe counts rows across *all* tenants, so +/// the token — not the tenant predicate — is what keeps concurrent scenarios on +/// a shared database from reading as each other's leaks. +pub fn planted_term(tenant: &TenantContext) -> String { + scoped_term(PLANTED_STEM, tenant) +} -/// A second planted token, for the resource that must survive. -pub const BYSTANDER_TERM: &str = "Quokkaflavoured"; +/// The bystander token for `tenant`, unique on the same basis as +/// [`planted_term`]. +pub fn bystander_term(tenant: &TenantContext) -> String { + scoped_term(BYSTANDER_STEM, tenant) +} + +/// Appends the tenant id to `stem` as a single indexable word. +/// +/// Non-alphanumerics are dropped rather than kept: both engines' tokenizers +/// split on `-` and `_`, which would turn one rare token into several common +/// ones and break the exact-match assertions. What remains is one long word that +/// FTS5 and `to_tsvector('english', …)` both keep intact, and that a `LIKE +/// '%…%'` residency probe matches literally. +fn scoped_term(stem: &str, tenant: &TenantContext) -> String { + let suffix: String = tenant + .tenant_id() + .as_str() + .chars() + .filter(|c| c.is_ascii_alphanumeric()) + .collect(); + format!("{stem}{suffix}") +} /// Reads physical `resource_fts` rows, bypassing every query path. /// @@ -158,19 +200,20 @@ where .collect() } -/// Seeds one Patient carrying `PLANTED_TERM` and asserts the full-text pipeline -/// actually indexed it. Every scenario starts here; if this fails, nothing -/// after it would have meant anything. +/// Seeds one Patient carrying this tenant's [`planted_term`] and asserts the +/// full-text pipeline actually indexed it. Every scenario starts here; if this +/// fails, nothing after it would have meant anything. async fn seed_and_control(backend: &B, probe: &P, tenant: &TenantContext, id: &str) where B: ResourceStorage + SearchProvider, P: FtsProbe, { + let term = planted_term(tenant); backend .create( tenant, "Patient", - patient_with_narrative(id, PLANTED_TERM), + patient_with_narrative(id, &term), FhirVersion::default(), ) .await @@ -183,7 +226,7 @@ where before the purge, or every assertion below is vacuous" ); assert_eq!( - text_hits(backend, tenant, PLANTED_TERM).await, + text_hits(backend, tenant, &term).await, vec![id.to_string()], "POSITIVE CONTROL: _text must find the seeded narrative before the purge" ); @@ -214,7 +257,7 @@ where reported the resource was removed" ); assert_eq!( - probe.fts_rows_containing(PLANTED_TERM).await, + probe.fts_rows_containing(&planted_term(tenant)).await, 0, "no resource_fts row anywhere may still contain the purged text" ); @@ -238,7 +281,7 @@ where 0, "purge_all must delete the type's resource_fts rows" ); - assert_eq!(probe.fts_rows_containing(PLANTED_TERM).await, 0); + assert_eq!(probe.fts_rows_containing(&planted_term(tenant)).await, 0); } /// Tenant purge removes every one of the tenant's full-text rows, across types. @@ -251,6 +294,7 @@ pub async fn purge_tenant_data_removes_fts_rows( P: FtsProbe, { seed_and_control(backend, probe, tenant, "p1").await; + let term = planted_term(tenant); backend .create( @@ -264,7 +308,7 @@ pub async fn purge_tenant_data_removes_fts_rows( "text": { "status": "generated", "div": format!( - "

{PLANTED_TERM}

" + "

{term}

" ) } }), @@ -290,7 +334,7 @@ pub async fn purge_tenant_data_removes_fts_rows( "purge_tenant_data must delete every one of the tenant's resource_fts \ rows, across all resource types" ); - assert_eq!(probe.fts_rows_containing(PLANTED_TERM).await, 0); + assert_eq!(probe.fts_rows_containing(&term).await, 0); } /// The purge must not be over-broad: another tenant's rows survive untouched. @@ -313,7 +357,7 @@ pub async fn purge_tenant_data_leaves_other_tenants_intact( .create( bystander, "Patient", - patient_with_narrative("p1", BYSTANDER_TERM), + patient_with_narrative("p1", &bystander_term(bystander)), FhirVersion::default(), ) .await @@ -340,7 +384,7 @@ pub async fn purge_tenant_data_leaves_other_tenants_intact( "purging one tenant must not touch another tenant's resource_fts rows" ); assert_eq!( - text_hits(backend, bystander, BYSTANDER_TERM).await, + text_hits(backend, bystander, &bystander_term(bystander)).await, vec!["p1".to_string()], "the bystander tenant's full-text search must still work after the purge" ); @@ -385,7 +429,7 @@ pub async fn reuse_after_purge_does_not_resurrect_narrative( .expect("re-create should succeed"); assert_eq!( - text_hits(backend, tenant, PLANTED_TERM).await, + text_hits(backend, tenant, &planted_term(tenant)).await, Vec::::new(), "the purged resource's narrative must not be matchable through a \ re-created resource that merely reuses its id" @@ -427,13 +471,13 @@ pub async fn tenant_reuse_does_not_resurrect_narrative( .expect("re-create under the reused tenant id should succeed"); assert_eq!( - text_hits(backend, tenant, PLANTED_TERM).await, + text_hits(backend, tenant, &planted_term(tenant)).await, Vec::::new(), "a re-created tenant must not be able to surface the previous tenant's \ purged content" ); assert_eq!( - content_hits(backend, tenant, PLANTED_TERM).await, + content_hits(backend, tenant, &planted_term(tenant)).await, Vec::::new(), "_content must be clean too, not only _text" ); @@ -457,7 +501,7 @@ pub async fn repeated_purge_and_recreate_does_not_grow_fts( .create( tenant, "Patient", - patient_with_narrative("p1", PLANTED_TERM), + patient_with_narrative("p1", &planted_term(tenant)), FhirVersion::default(), ) .await @@ -551,12 +595,12 @@ pub async fn reindex_preserves_full_text_search( resource_fts row, not drop it" ); assert_eq!( - text_hits(backend, tenant, PLANTED_TERM).await, + text_hits(backend, tenant, &planted_term(tenant)).await, vec!["p1".to_string()], "_text must still work after $reindex (clear_existing={clear_existing})" ); assert_eq!( - content_hits(backend, tenant, PLANTED_TERM).await, + content_hits(backend, tenant, &planted_term(tenant)).await, vec!["p1".to_string()], "_content must still work after $reindex (clear_existing={clear_existing})" ); @@ -592,7 +636,7 @@ pub async fn repeated_reindex_does_not_duplicate_fts_rows( } assert_eq!( - text_hits(backend, tenant, PLANTED_TERM).await, + text_hits(backend, tenant, &planted_term(tenant)).await, vec!["p1".to_string()], "_text must still return exactly one hit after repeated reindexing" ); diff --git a/crates/persistence/tests/search/fts_purge_tests.rs b/crates/persistence/tests/search/fts_purge_tests.rs index 211db6767..a945d63ec 100644 --- a/crates/persistence/tests/search/fts_purge_tests.rs +++ b/crates/persistence/tests/search/fts_purge_tests.rs @@ -232,7 +232,7 @@ async fn sqlite_purge_succeeds_when_fts_table_is_absent() { &backend, &tenant, "Patient", - suite::patient_with_narrative("p1", suite::PLANTED_TERM), + suite::patient_with_narrative("p1", &suite::planted_term(&tenant)), helios_fhir::FhirVersion::default(), ) .await @@ -282,7 +282,7 @@ async fn migration_sweeps_orphaned_fts_rows_from_existing_databases() { &backend, &tenant, "Patient", - suite::patient_with_narrative(id, suite::PLANTED_TERM), + suite::patient_with_narrative(id, &suite::planted_term(&tenant)), helios_fhir::FhirVersion::default(), ) .await @@ -374,7 +374,7 @@ async fn migration_preserves_fts_rows_for_soft_deleted_resources() { &backend, &tenant, "Patient", - suite::patient_with_narrative("p1", suite::PLANTED_TERM), + suite::patient_with_narrative("p1", &suite::planted_term(&tenant)), helios_fhir::FhirVersion::default(), ) .await