From 4ef55d8b98a3ea476d2151f0fb1bbef75d7cda47 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 08:53:51 -0400 Subject: [PATCH 01/10] fix(sqlite): make update and delete atomic compare-and-swaps `SqliteBackend::update` was a `SELECT version_id`, a comparison in Rust and an `UPDATE` with no version in its `WHERE`, each statement auto-committed on a pooled connection. With the default pool (10 connections, WAL) on a multi-threaded runtime, two writers holding the same version both pass the comparison. The second `UPDATE` overwrites the first and commits; only its history `INSERT` then fails on `PRIMARY KEY (.., version_id)`. So the loser gets a 500 while its content is already the current row, under a version whose history entry holds the winner's content: a lost update plus a current/history divergence, not just a missed 409. The #1399 race test did not see this because it drives the writers with `join_all` on a current-thread runtime, where synchronous rusqlite calls never interleave. `update` now carries the expected version in the `UPDATE` predicate (zero rows -> `VersionConflict`, or `NotFound` when nothing is live) and runs the history row and search index in the same IMMEDIATE transaction, so a loser leaves nothing behind. Index extraction moves ahead of the write lock. Adds `ResourceStorage::delete_versioned`, the delete half of optimistic locking. The default implementation is read-compare-delete and documented as not atomic; SQLite implements it (and plain `delete`) in one IMMEDIATE transaction, and `delete_with_match` now deletes exactly the version its `If-Match` list was evaluated against instead of calling plain `delete`. New cross-backend suite `versioned_write_race_suite.rs`: 8 tasks on a multi-threaded runtime, released by a barrier, over many resources; asserts one winner, every loser a `ConcurrencyError`, stored state and history are the winner's with no gap or duplicate. Refs #1404 --- .../src/backends/sqlite/storage.rs | 400 +++++++++++------- crates/persistence/src/core/storage.rs | 64 ++- .../search/versioned_write_race_suite.rs | 344 +++++++++++++++ crates/persistence/tests/sqlite_tests.rs | 51 +++ 4 files changed, 694 insertions(+), 165 deletions(-) create mode 100644 crates/persistence/tests/search/versioned_write_race_suite.rs diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index d589926be..2b7d41f69 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -461,48 +461,15 @@ impl ResourceStorage for SqliteBackend { let resource_type = current.resource_type(); tenant.check_permission(Operation::Update, resource_type)?; - let conn = self.get_connection()?; + let mut conn = self.get_connection()?; let tenant_id = tenant.tenant_id().as_str(); let id = current.id(); - // Check that the resource still exists with the expected version - let actual_version: Result = conn.query_row( - "SELECT version_id FROM resources - WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", - params![tenant_id, resource_type, id], - |row| row.get(0), - ); - - let actual_version = match actual_version { - Ok(v) => v, - Err(rusqlite::Error::QueryReturnedNoRows) => { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - Err(e) => { - return Err(internal_error(format!( - "Failed to get current version: {}", - e - ))); - } - }; - - // Check version match - if actual_version != current.version_id() { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_version: current.version_id().to_string(), - actual_version, - }, - )); - } - - // Calculate new version - let new_version: u64 = actual_version.parse().unwrap_or(0) + 1; + // The expected version is `current`'s, and the UPDATE below only matches + // a row that still carries it — so the new version follows from what the + // caller already read. + let expected_version = current.version_id(); + let new_version: u64 = expected_version.parse().unwrap_or(0) + 1; let new_version_str = new_version.to_string(); // Ensure the resource has correct type and id @@ -519,27 +486,85 @@ impl ResourceStorage for SqliteBackend { let data = serde_json::to_vec(&resource) .map_err(|e| serialization_error(format!("Failed to serialize resource: {}", e)))?; + // Extract the search values before taking the write lock: it is pure + // CPU, and SQLite has one writer. + let prepared = (!self.is_search_offloaded()) + .then(|| self.prepare_index(tenant_id, resource_type, id, &resource)); + let now = Utc::now(); let last_updated = now.to_rfc3339(); + let fhir_version_str = current.fhir_version().as_mime_param(); - // Update the resource - conn.execute( - "UPDATE resources SET version_id = ?1, data = ?2, last_updated = ?3 - WHERE tenant_id = ?4 AND resource_type = ?5 AND id = ?6", - params![ - new_version_str, - data, - last_updated, - tenant_id, - resource_type, - id - ], - ) - .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))?; + // Compare-and-swap, history row and search index in ONE transaction. + // + // This used to be a `SELECT version_id`, a comparison in Rust, and then + // an `UPDATE` with no version in its `WHERE`, each statement + // auto-committed on a pooled connection. Two writers holding the same + // version on two connections both passed the comparison; the second + // `UPDATE` then overwrote the first and committed, and only its history + // `INSERT` failed — on `PRIMARY KEY (…, version_id)` — so that writer got + // a 500 while its content was already the current row, under a version + // whose history entry holds the *winner's* content (#1404). + // + // The version now rides in the `UPDATE`'s predicate, so the comparison + // and the write are one statement; and everything that follows shares + // its transaction, so a writer that loses leaves nothing behind. + // IMMEDIATE takes the write lock up front, where the busy handler + // applies (see `purge_tenant_data`). + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("Failed to begin update: {}", e)))?; + + let updated = tx + .execute( + "UPDATE resources SET version_id = ?1, data = ?2, last_updated = ?3 + WHERE tenant_id = ?4 AND resource_type = ?5 AND id = ?6 + AND version_id = ?7 AND is_deleted = 0", + params![ + new_version_str, + data, + last_updated, + tenant_id, + resource_type, + id, + expected_version + ], + ) + .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))?; + + if updated == 0 { + // Matched nothing; which of the two reasons it was costs a query, + // but only on the path that is already failing. + let actual: Result = tx.query_row( + "SELECT version_id FROM resources + WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", + params![tenant_id, resource_type, id], + |row| row.get(0), + ); + return match actual { + Ok(actual_version) => Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version, + }, + )), + Err(rusqlite::Error::QueryReturnedNoRows) => { + Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })) + } + Err(e) => Err(internal_error(format!( + "Failed to get current version: {}", + e + ))), + }; + } // Insert into history (preserve the original FHIR version) - let fhir_version_str = current.fhir_version().as_mime_param(); - conn.execute( + tx.execute( "INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) VALUES (?1, ?2, ?3, ?4, ?5, ?6, 0, ?7)", params![tenant_id, resource_type, id, new_version_str, data, last_updated, fhir_version_str], @@ -547,8 +572,13 @@ impl ResourceStorage for SqliteBackend { .map_err(|e| internal_error(format!("Failed to insert history: {}", e)))?; // Re-index the resource (delete old entries, add new) - self.delete_search_index(&conn, tenant_id, resource_type, id)?; - self.index_resource(&conn, tenant_id, resource_type, id, &resource)?; + if let Some(prepared) = prepared { + self.delete_search_index(&tx, tenant_id, resource_type, id)?; + self.write_prepared_index(&tx, tenant_id, resource_type, id, &resource, prepared)?; + } + + tx.commit() + .map_err(|e| internal_error(format!("Failed to commit update: {}", e)))?; // A SearchParameter write invalidates this tenant's cached registry. if resource_type == "SearchParameter" { @@ -574,112 +604,17 @@ impl ResourceStorage for SqliteBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let conn = self.get_connection()?; - let tenant_id = tenant.tenant_id().as_str(); - - // Check if resource exists and get its fhir_version - let result: Result<(String, Vec, String), _> = conn.query_row( - "SELECT version_id, data, fhir_version FROM resources - WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", - params![tenant_id, resource_type, id], - |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), - ); - - let (current_version, data, fhir_version_str) = match result { - Ok(v) => v, - Err(rusqlite::Error::QueryReturnedNoRows) => { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - Err(e) => { - return Err(internal_error(format!("Failed to check resource: {}", e))); - } - }; - - let now = Utc::now(); - let deleted_at = now.to_rfc3339(); - - // Calculate new version for the deletion record - let new_version: u64 = current_version.parse().unwrap_or(0) + 1; - let new_version_str = new_version.to_string(); - - // Soft delete the resource, guarded by the version we just read. - // - // The `version_id`/`is_deleted` predicates make this a compare-and-swap - // rather than a blind overwrite. Without them a concurrent writer that - // lands between the SELECT above and this UPDATE is silently clobbered, - // and worse: `new_version` was computed from the stale read, so the - // history INSERT below then collides with the row that writer already - // wrote and trips `PRIMARY KEY (tenant_id, resource_type, id, - // version_id)`. Because neither statement runs in a transaction, the - // UPDATE is already committed at that point — the caller gets a 500 and - // the current row now points at a version whose history entry holds - // someone else's content. - // - // MongoDB and S3 already guarded their equivalent writes (a - // `version_id` term in the update filter, and a conditional PUT - // respectively); this brings SQLite to parity. Losing the race is - // reported as `NotFound`, which is what a caller racing a concurrent - // delete would have seen anyway. - let updated = conn - .execute( - "UPDATE resources SET is_deleted = 1, deleted_at = ?1, version_id = ?2, last_updated = ?1 - WHERE tenant_id = ?3 AND resource_type = ?4 AND id = ?5 - AND version_id = ?6 AND is_deleted = 0", - params![ - deleted_at, - new_version_str, - tenant_id, - resource_type, - id, - current_version - ], - ) - .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; - - if updated == 0 { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - - // Insert deletion record into history (preserve fhir_version) - conn.execute( - "INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, 1, ?7)", - params![tenant_id, resource_type, id, new_version_str, data, deleted_at, fhir_version_str], - ) - .map_err(|e| internal_error(format!("Failed to insert deletion history: {}", e)))?; - - // Delete search index entries (skip when search is offloaded). Keyed on - // resource_key. The tenant_id/resource_type prefix is required for the - // delete to seek idx_search_composite instead of full-scanning - // search_index (see delete_search_index, #1197); the soft-delete keeps - // the resources row, so the subquery resolves. - if !self.is_search_offloaded() { - conn.execute( - "DELETE FROM search_index - WHERE tenant_id = ?1 AND resource_type = ?2 - AND resource_key = ( - SELECT rowid 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 delete search index: {}", e)))?; - } - - // A SearchParameter delete invalidates this tenant's cached registry. - if resource_type == "SearchParameter" { - self.tenant_registries().invalidate(tenant_id); - } + self.soft_delete(tenant, resource_type, id, None) + } - Ok(()) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) } async fn count( @@ -1279,6 +1214,138 @@ impl ResourceStorage for SqliteBackend { // Search Index Helpers impl SqliteBackend { + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + /// + /// The read of the current row, the tombstone `UPDATE`, the deletion history + /// row and the search-index cleanup share one `IMMEDIATE` transaction. They + /// used to be auto-committed statements: a failure after the `UPDATE` left a + /// tombstone with no history entry, and a writer landing between the read + /// and the `UPDATE` turned a plain delete into a spurious `NotFound`. Inside + /// the write lock neither can happen, and `expected_version` is compared + /// against the very row the `UPDATE` then tombstones — the comparison and + /// the delete cannot be separated (#1404). + fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let mut conn = self.get_connection()?; + let tenant_id = tenant.tenant_id().as_str(); + + let tx = conn + .transaction_with_behavior(rusqlite::TransactionBehavior::Immediate) + .map_err(|e| internal_error(format!("Failed to begin delete: {}", e)))?; + + // Check if resource exists and get its fhir_version + let result: Result<(String, Vec, String), _> = tx.query_row( + "SELECT version_id, data, fhir_version FROM resources + WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 0", + params![tenant_id, resource_type, id], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + ); + + let (current_version, data, fhir_version_str) = match result { + Ok(v) => v, + Err(rusqlite::Error::QueryReturnedNoRows) => { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + Err(e) => { + return Err(internal_error(format!("Failed to check resource: {}", e))); + } + }; + + if let Some(expected) = expected_version + && expected != current_version + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: current_version, + }, + )); + } + + let now = Utc::now(); + let deleted_at = now.to_rfc3339(); + + // Calculate new version for the deletion record + let new_version: u64 = current_version.parse().unwrap_or(0) + 1; + let new_version_str = new_version.to_string(); + + // Soft delete the resource. The `version_id`/`is_deleted` predicates + // keep the statement a compare-and-swap in its own right: the write + // lock already guarantees the row is the one read above, and the + // predicate is what would say so if that ever stopped being true. + let updated = tx + .execute( + "UPDATE resources SET is_deleted = 1, deleted_at = ?1, version_id = ?2, last_updated = ?1 + WHERE tenant_id = ?3 AND resource_type = ?4 AND id = ?5 + AND version_id = ?6 AND is_deleted = 0", + params![ + deleted_at, + new_version_str, + tenant_id, + resource_type, + id, + current_version + ], + ) + .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; + + if updated == 0 { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + + // Insert deletion record into history (preserve fhir_version) + tx.execute( + "INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, 1, ?7)", + params![tenant_id, resource_type, id, new_version_str, data, deleted_at, fhir_version_str], + ) + .map_err(|e| internal_error(format!("Failed to insert deletion history: {}", e)))?; + + // Delete search index entries (skip when search is offloaded). Keyed on + // resource_key. The tenant_id/resource_type prefix is required for the + // delete to seek idx_search_composite instead of full-scanning + // search_index (see delete_search_index, #1197); the soft-delete keeps + // the resources row, so the subquery resolves. + if !self.is_search_offloaded() { + tx.execute( + "DELETE FROM search_index + WHERE tenant_id = ?1 AND resource_type = ?2 + AND resource_key = ( + SELECT rowid 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 delete search index: {}", e)))?; + } + + tx.commit() + .map_err(|e| internal_error(format!("Failed to commit delete: {}", e)))?; + + // A SearchParameter delete invalidates this tenant's cached registry. + if resource_type == "SearchParameter" { + self.tenant_registries().invalidate(tenant_id); + } + + Ok(()) + } + /// Brings a soft-deleted resource back to life with new content. /// /// FHIR permits a deleted resource to be restored by a subsequent update @@ -2118,9 +2185,14 @@ impl VersionedStorage for SqliteBackend { }, )); } + drop(conn); - // Perform delete - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against. + // A plain `delete` here was check-then-act: a writer landing after the + // read above was deleted along with the version the client named + // (#1404). + self.delete_versioned(tenant, resource_type, id, ¤t_version) + .await } async fn list_versions( diff --git a/crates/persistence/src/core/storage.rs b/crates/persistence/src/core/storage.rs index fbea93526..1d4a49566 100644 --- a/crates/persistence/src/core/storage.rs +++ b/crates/persistence/src/core/storage.rs @@ -13,7 +13,7 @@ use serde_json::Value; use crate::core::preconditions::EntityTagPrecondition; use crate::core::sof_runner::SofRunner; -use crate::error::{BackendError, ResourceError, StorageError, StorageResult}; +use crate::error::{BackendError, ConcurrencyError, ResourceError, StorageError, StorageResult}; use crate::tenant::TenantContext; /// A cheap per-tenant change detector for committed writes (#1078). @@ -546,6 +546,68 @@ pub trait ResourceStorage: Send + Sync { id: &str, ) -> StorageResult<()>; + /// Deletes a resource (soft delete) only if `expected_version` is still its + /// current version — the delete half of optimistic locking, as + /// [`update`](Self::update) is the update half. + /// + /// This is what a `DELETE` carrying `If-Match` must go through. Evaluating + /// the precondition against one read and then calling + /// [`delete`](Self::delete) is check-then-act: a writer landing in between + /// is deleted along with the version the client named, a version the + /// client never saw (#1404). + /// + /// `expected_version` is a bare version id (`3`), not an `If-Match` field + /// value; [`VersionedStorage::delete_with_match`] takes the latter. + /// + /// # Atomicity + /// + /// SQLite, PostgreSQL and MongoDB implement this as ONE conditional write + /// carrying the version in its predicate, so the comparison and the delete + /// cannot be separated. S3 makes the tombstone write conditional on the + /// object it compared. The **default implementation is not atomic**: it + /// reads, compares and calls [`delete`](Self::delete), which narrows the + /// window to this call but does not close it. It exists so that stores + /// which are never the system of record for a version (search secondaries, + /// test doubles) need not invent a guarantee they cannot give; a wrapper + /// around a real backend MUST delegate rather than inherit it. + /// + /// # Errors + /// + /// * `StorageError::Resource(NotFound)` - If no live resource exists + /// (never created, or already deleted) + /// * `StorageError::Concurrency(VersionConflict)` - If the current version + /// is not `expected_version`; nothing is deleted + /// * `StorageError::Tenant` - If the tenant doesn't have delete permission + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + let current = match self.read(tenant, resource_type, id).await { + Ok(Some(current)) => current, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) => { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + Err(e) => return Err(e), + }; + if current.version_id() != expected_version { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version: current.version_id().to_string(), + }, + )); + } + self.delete(tenant, resource_type, id).await + } + /// Checks if a resource exists. /// /// This is more efficient than `read` when you only need to check existence. diff --git a/crates/persistence/tests/search/versioned_write_race_suite.rs b/crates/persistence/tests/search/versioned_write_race_suite.rs new file mode 100644 index 000000000..956db52a6 --- /dev/null +++ b/crates/persistence/tests/search/versioned_write_race_suite.rs @@ -0,0 +1,344 @@ +//! Backend-agnostic race suite for version-aware writes (issues #1404, #1405). +//! +//! `ResourceStorage::update` and `ResourceStorage::delete_versioned` are +//! compare-and-swaps on the version the caller holds: of any number of writers +//! holding the same version, exactly one writes, and every other one is told +//! it lost with a `ConcurrencyError` — not with a backend `Internal` error, and +//! never with a success that silently overwrote the winner. +//! +//! The writers are real tasks on a multi-threaded runtime, released together +//! by a barrier and repeated over many resources, so the interleaving these +//! tests need happens by volume rather than by timing. Each test's caller must +//! therefore run it with `#[tokio::test(flavor = "multi_thread")]`; on a +//! current-thread runtime a synchronous backend (SQLite) never interleaves and +//! the assertions hold vacuously. +//! +//! Included by `#[path]` into each backend's test binary, the same arrangement +//! as `conditional_if_match_suite.rs`. + +#![allow(dead_code)] + +use std::sync::Arc; + +use serde_json::{Value, json}; +use tokio::sync::Barrier; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{ResourceStorage, VersionedStorage}; +use helios_persistence::error::{ResourceError, StorageError}; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; +use helios_persistence::types::StoredResource; + +const WRITERS: usize = 8; + +fn tenant(base: &str, label: &str) -> TenantContext { + TenantContext::new( + TenantId::new(format!("{base}-{label}")), + TenantPermissions::full_access(), + ) +} + +fn patient(id: &str, family: &str) -> Value { + json!({ + "resourceType": "Patient", + "id": id, + "name": [{"family": family}] + }) +} + +fn family(stored: &StoredResource) -> String { + stored.content()["name"][0]["family"] + .as_str() + .unwrap_or("?") + .to_string() +} + +/// Creates `Patient/{id}` and proves it reads back at version 1 — the positive +/// control: every writer below holds exactly this `StoredResource`. +async fn seed(backend: &S, tenant: &TenantContext, id: &str) -> StoredResource { + backend + .create( + tenant, + "Patient", + patient(id, "Seed"), + FhirVersion::default(), + ) + .await + .expect("seed patient"); + let current = backend + .read(tenant, "Patient", id) + .await + .expect("read seeded patient") + .expect("seeded patient exists"); + assert_eq!(current.version_id(), "1"); + assert_eq!(family(¤t), "Seed"); + current +} + +/// What one racing writer did. +#[derive(Debug)] +enum Outcome { + Updated(StoredResource), + Deleted, + Failed(StorageError), +} + +fn assert_losers_are_concurrency_errors(outcomes: &[Outcome], deleted: bool, context: &str) { + for outcome in outcomes { + match outcome { + Outcome::Updated(_) | Outcome::Deleted => {} + Outcome::Failed(StorageError::Concurrency(_)) => {} + // A writer that lost to a *delete* finds no live resource at all. + Outcome::Failed(StorageError::Resource(ResourceError::NotFound { .. })) if deleted => {} + Outcome::Failed(StorageError::Resource(ResourceError::Gone { .. })) if deleted => {} + other => panic!("{context}: a loser is a concurrency refusal, got {other:?}"), + } + } +} + +/// `WRITERS` tasks all hold version 1 of the same resource and `update` it at +/// once, `rounds` times over. Exactly one writes version 2; the rest are +/// concurrency errors; the stored resource and the history are the winner's. +pub async fn concurrent_updates_from_the_same_version_admit_one( + backend: Arc, + base: &str, + rounds: usize, +) where + S: ResourceStorage + VersionedStorage + Send + Sync + 'static, +{ + let t = tenant(base, "update-race"); + + for round in 0..rounds { + let id = format!("race-{round}"); + let current = seed(backend.as_ref(), &t, &id).await; + let barrier = Arc::new(Barrier::new(WRITERS)); + + let mut tasks = Vec::new(); + for n in 0..WRITERS { + let (backend, t, current, barrier, id) = ( + backend.clone(), + t.clone(), + current.clone(), + barrier.clone(), + id.clone(), + ); + tasks.push(tokio::spawn(async move { + barrier.wait().await; + match backend + .update(&t, ¤t, patient(&id, &format!("Writer{n}"))) + .await + { + Ok(stored) => Outcome::Updated(stored), + Err(e) => Outcome::Failed(e), + } + })); + } + let mut outcomes = Vec::new(); + for task in tasks { + outcomes.push(task.await.expect("writer task")); + } + let context = format!("round {round}"); + + let winners: Vec<&StoredResource> = outcomes + .iter() + .filter_map(|o| match o { + Outcome::Updated(stored) => Some(stored), + _ => None, + }) + .collect(); + assert_eq!( + winners.len(), + 1, + "{context}: exactly one of {WRITERS} writers holding version 1 may write: {outcomes:?}" + ); + assert_losers_are_concurrency_errors(&outcomes, false, &context); + assert_eq!(winners[0].version_id(), "2", "{context}"); + + let stored = backend + .read(&t, "Patient", &id) + .await + .expect("read after race") + .expect("still live"); + assert_eq!(stored.version_id(), "2", "{context}"); + assert_eq!(family(&stored), family(winners[0]), "{context}"); + + assert_eq!( + backend + .list_versions(&t, "Patient", &id) + .await + .expect("list versions"), + vec!["1".to_string(), "2".to_string()], + "{context}: history has no gap and no duplicate" + ); + let v2 = backend + .vread(&t, "Patient", &id, "2") + .await + .expect("vread 2") + .expect("version 2 exists"); + assert_eq!( + family(&v2), + family(winners[0]), + "{context}: history's version 2 is the winner's content" + ); + } +} + +/// Half the tasks `update` from version 1 and half `delete_versioned` version +/// 1, all at once. Exactly one of them wins: either version 2 is an update and +/// the resource is live, or version 2 is the tombstone — never an update *and* +/// a delete of "version 1", which is the delete removing a version its caller +/// never saw. +pub async fn concurrent_update_and_versioned_delete_admit_one( + backend: Arc, + base: &str, + rounds: usize, +) where + S: ResourceStorage + VersionedStorage + Send + Sync + 'static, +{ + let t = tenant(base, "delete-race"); + + for round in 0..rounds { + let id = format!("race-{round}"); + let current = seed(backend.as_ref(), &t, &id).await; + let barrier = Arc::new(Barrier::new(WRITERS)); + + let mut tasks = Vec::new(); + for n in 0..WRITERS { + let (backend, t, current, barrier, id) = ( + backend.clone(), + t.clone(), + current.clone(), + barrier.clone(), + id.clone(), + ); + tasks.push(tokio::spawn(async move { + barrier.wait().await; + if n % 2 == 0 { + match backend + .update(&t, ¤t, patient(&id, &format!("Writer{n}"))) + .await + { + Ok(stored) => Outcome::Updated(stored), + Err(e) => Outcome::Failed(e), + } + } else { + match backend.delete_versioned(&t, "Patient", &id, "1").await { + Ok(()) => Outcome::Deleted, + Err(e) => Outcome::Failed(e), + } + } + })); + } + let mut outcomes = Vec::new(); + for task in tasks { + outcomes.push(task.await.expect("writer task")); + } + let context = format!("round {round}"); + + let winners: Vec<&Outcome> = outcomes + .iter() + .filter(|o| matches!(o, Outcome::Updated(_) | Outcome::Deleted)) + .collect(); + assert_eq!( + winners.len(), + 1, + "{context}: exactly one of {WRITERS} writers holding version 1 may write: {outcomes:?}" + ); + let deleted = matches!(winners[0], Outcome::Deleted); + assert_losers_are_concurrency_errors(&outcomes, deleted, &context); + + let after = backend.read(&t, "Patient", &id).await; + match winners[0] { + Outcome::Updated(winner) => { + let stored = after.expect("read after race").expect("still live"); + assert_eq!(stored.version_id(), "2", "{context}"); + assert_eq!(family(&stored), family(winner), "{context}"); + } + _ => assert!( + matches!( + after, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) + ), + "{context}: the delete won, so nothing is live: {after:?}" + ), + } + + assert_eq!( + backend + .list_versions(&t, "Patient", &id) + .await + .expect("list versions"), + vec!["1".to_string(), "2".to_string()], + "{context}: one write landed, as version 2" + ); + } +} + +/// `delete_versioned` is a compare-and-swap on the *current* version: a stale +/// version is refused and deletes nothing, the current one deletes, and a +/// second delete of the same version finds nothing live. +pub async fn versioned_delete_is_a_compare_and_swap(backend: &S, base: &str) +where + S: ResourceStorage + VersionedStorage, +{ + let t = tenant(base, "delete-cas"); + let v1 = seed(backend, &t, "cas").await; + let v2 = backend + .update(&t, &v1, patient("cas", "Updated")) + .await + .expect("update to v2"); + assert_eq!(v2.version_id(), "2"); + + // The version the client saw is gone: refuse, and leave v2 live. + let stale = backend.delete_versioned(&t, "Patient", "cas", "1").await; + assert!( + matches!(stale, Err(StorageError::Concurrency(_))), + "a stale versioned delete is a concurrency error, got {stale:?}" + ); + let live = backend + .read(&t, "Patient", "cas") + .await + .expect("read") + .expect("a refused delete deletes nothing"); + assert_eq!(live.version_id(), "2"); + assert_eq!(family(&live), "Updated"); + + // `delete_with_match` takes an `If-Match` field value and goes the same way. + let stale = backend + .delete_with_match(&t, "Patient", "cas", "W/\"1\"") + .await; + assert!( + matches!(stale, Err(StorageError::Concurrency(_))), + "{stale:?}" + ); + + backend + .delete_versioned(&t, "Patient", "cas", "2") + .await + .expect("the current version deletes"); + let after = backend.read(&t, "Patient", "cas").await; + assert!( + matches!( + after, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) + ), + "{after:?}" + ); + assert_eq!( + backend + .list_versions(&t, "Patient", "cas") + .await + .expect("list versions"), + vec!["1".to_string(), "2".to_string(), "3".to_string()], + "the tombstone is version 3" + ); + + let again = backend.delete_versioned(&t, "Patient", "cas", "2").await; + assert!( + matches!( + again, + Err(StorageError::Resource(ResourceError::NotFound { .. })) + ), + "nothing live is left to delete: {again:?}" + ); +} diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 33ddd74c7..6168f3709 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -98,6 +98,57 @@ async fn sqlite_conditional_writers_with_the_same_if_match_admit_one() { .await; } +/// The backend-agnostic race suite for version-aware writes (#1404, #1405). +/// Same `#[path]` arrangement. +#[path = "search/versioned_write_race_suite.rs"] +mod versioned_write_race_suite; + +/// A file-backed (WAL) backend with the default pool — what `hfs` runs in +/// production. Several pooled connections on several runtime threads is the +/// configuration in which SQLite writers really interleave; the shared-cache +/// `:memory:` database serialises them at the table lock instead. +fn create_file_backend(dir: &tempfile::TempDir) -> SqliteBackend { + let backend = + SqliteBackend::with_config(dir.path().join("race.db"), SqliteBackendConfig::default()) + .expect("Failed to create SQLite backend"); + backend.init_schema().expect("Failed to initialize schema"); + backend +} + +/// #1404: of several writers holding the same version, one `update` writes. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn sqlite_concurrent_updates_from_the_same_version_admit_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let backend = std::sync::Arc::new(create_file_backend(&dir)); + versioned_write_race_suite::concurrent_updates_from_the_same_version_admit_one( + backend, + "update-race-1404", + 40, + ) + .await; +} + +/// #1404: an update and a versioned delete of the same version: one wins. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn sqlite_concurrent_update_and_versioned_delete_admit_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let backend = std::sync::Arc::new(create_file_backend(&dir)); + versioned_write_race_suite::concurrent_update_and_versioned_delete_admit_one( + backend, + "delete-race-1404", + 40, + ) + .await; +} + +/// #1404: `delete_versioned` compares and deletes in one step. +#[tokio::test] +async fn sqlite_versioned_delete_is_a_compare_and_swap() { + let backend = create_backend(); + versioned_write_race_suite::versioned_delete_is_a_compare_and_swap(&backend, "delete-cas-1404") + .await; +} + fn create_backend() -> SqliteBackend { // Configure with data directory to load spec SearchParameters // CARGO_MANIFEST_DIR for tests is crates/persistence From 5eb1ae1209a168375ed517046aedebc34dcdf5b1 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 08:59:13 -0400 Subject: [PATCH 02/10] feat(persistence): one patch applier and one conditional_patch for every backend Conditional PATCH was unavailable on MongoDB (unimplemented) and on every composite with a dedicated search backend (refused since #1384; a silent no-match before). Root cause: patch application was not available at the persistence level. REST, SQLite and PostgreSQL each had a private applier, so the composite - which resolves criteria through its search backend and writes through its primary - had nothing to apply a patch with, and MongoDB had none at all. - core/patch.rs: `apply_patch` (JSON Patch, JSON Merge Patch) with typed `PatchError`s, carried as `ValidationError::Patch`. It refuses a patch that changes or removes `resourceType` / `id`, and refuses FHIRPath Patch. - `ConditionalStorage::conditional_patch` is now a provided implementation: `resolve_conditional_matches` -> exactly one -> `read` (the primary's content on a composite; a search copy of another version is a VersionConflict) -> `conditional_if_match_gate` -> `apply_patch` -> `update` (compare-and-swap). - SQLite, PostgreSQL: private appliers and `conditional_patch` deleted; they provide only the resolver. The `apply_fhirpath_patch` stubs, which changed nothing on most paths and still wrote a new version, are gone. - MongoDB and CompositeStorage provide the resolver and declare `ConditionalPatch`; the composite's #1384 refusal is removed. - REST applies instance PATCH with the same applier and maps `PatchError` in one place (a failed `test` op has its own arm, for #1393). Fixes #1406 --- Cargo.lock | 1 - .../src/backends/mongodb/backend.rs | 3 +- .../src/backends/mongodb/search_impl.rs | 21 +- .../src/backends/postgres/storage.rs | 221 +------------ .../src/backends/sqlite/storage.rs | 263 +-------------- crates/persistence/src/composite/storage.rs | 107 +++--- crates/persistence/src/core/backend.rs | 3 +- crates/persistence/src/core/mod.rs | 2 + crates/persistence/src/core/patch.rs | 290 +++++++++++++++++ crates/persistence/src/core/storage.rs | 112 ++++++- crates/persistence/src/error.rs | 5 + .../tests/backend_capability_contract.rs | 9 +- .../composite_conditional_capabilities.rs | 206 +++++++++--- crates/persistence/tests/mongodb_tests.rs | 27 +- crates/persistence/tests/postgres_tests.rs | 17 + .../tests/search/conditional_patch_suite.rs | 304 ++++++++++++++++++ crates/persistence/tests/sqlite_tests.rs | 17 + crates/rest/Cargo.toml | 1 - crates/rest/src/error.rs | 26 ++ crates/rest/src/handlers/patch.rs | 97 +----- 20 files changed, 1032 insertions(+), 700 deletions(-) create mode 100644 crates/persistence/src/core/patch.rs create mode 100644 crates/persistence/tests/search/conditional_patch_suite.rs diff --git a/Cargo.lock b/Cargo.lock index a0c2a618f..d9cfc061c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3721,7 +3721,6 @@ dependencies = [ "helios-subscriptions", "hmac 0.12.1", "http 1.4.0", - "json-patch", "jsonpath-rust", "jsonwebtoken", "mime", diff --git a/crates/persistence/src/backends/mongodb/backend.rs b/crates/persistence/src/backends/mongodb/backend.rs index 947cc2d9e..15d5b2541 100644 --- a/crates/persistence/src/backends/mongodb/backend.rs +++ b/crates/persistence/src/backends/mongodb/backend.rs @@ -278,8 +278,7 @@ impl MongoBackend { BackendCapability::ConditionalCreate, BackendCapability::ConditionalUpdate, BackendCapability::ConditionalDelete, - // No `ConditionalPatch`: `conditional_patch` answers - // `UnsupportedCapability` on this backend. + BackendCapability::ConditionalPatch, BackendCapability::SharedSchema, ] } diff --git a/crates/persistence/src/backends/mongodb/search_impl.rs b/crates/persistence/src/backends/mongodb/search_impl.rs index ee815c0ac..e65e173cf 100644 --- a/crates/persistence/src/backends/mongodb/search_impl.rs +++ b/crates/persistence/src/backends/mongodb/search_impl.rs @@ -13,9 +13,8 @@ use regex::escape as regex_escape; use serde_json::Value; use crate::core::{ - ConditionalCreateResult, ConditionalDeleteResult, ConditionalPatchResult, ConditionalStorage, - ConditionalUpdateResult, IncludeProvider, PatchFormat, ResourceStorage, RevincludeProvider, - SearchProvider, SearchResult, + ConditionalCreateResult, ConditionalDeleteResult, ConditionalStorage, ConditionalUpdateResult, + IncludeProvider, ResourceStorage, RevincludeProvider, SearchProvider, SearchResult, }; use crate::error::{BackendError, QueryErrorExt, SearchError, StorageError, StorageResult}; use crate::search::{DatePredicate, FhirDateValue, StorageResolution}; @@ -885,19 +884,17 @@ impl ConditionalStorage for MongoBackend { } } - async fn conditional_patch( + /// The criteria resolver the provided + /// [`ConditionalStorage::conditional_patch`] is written in terms of + /// (#1406). + async fn resolve_conditional_matches( &self, tenant: &TenantContext, resource_type: &str, search_params: &str, - patch: &PatchFormat, - if_match: &crate::core::EntityTagPrecondition, - ) -> StorageResult { - let _ = (tenant, resource_type, search_params, patch, if_match); - Err(StorageError::Backend(BackendError::UnsupportedCapability { - backend_name: "mongodb".to_string(), - capability: "conditional_patch".to_string(), - })) + ) -> StorageResult> { + self.find_matching_resources(tenant, resource_type, search_params) + .await } } diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index 715665d05..9bf48837f 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -224,22 +224,6 @@ fn serialization_error(message: String) -> StorageError { StorageError::Backend(BackendError::SerializationError { message }) } -/// 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`, -/// `valueReference`), so every FHIR polymorphic variant is accepted rather -/// than only the handful the patch handler used to special-case. -fn extract_part_value(part: &Value) -> Option { - part.as_object()?.iter().find_map(|(k, v)| { - let suffix = k.strip_prefix("value")?; - suffix - .chars() - .next()? - .is_ascii_uppercase() - .then(|| v.clone()) - }) -} - #[async_trait] impl ResourceStorage for PostgresBackend { @@ -3178,48 +3162,17 @@ impl ConditionalStorage for PostgresBackend { } } - async fn conditional_patch( + /// The criteria resolver the provided + /// [`ConditionalStorage::conditional_patch`] is written in terms of: this + /// backend has no patch code of its own (#1406). + async fn resolve_conditional_matches( &self, tenant: &TenantContext, resource_type: &str, search_params: &str, - patch: &crate::core::PatchFormat, - if_match: &crate::core::EntityTagPrecondition, - ) -> StorageResult { - use crate::core::{ConditionalPatchResult, PatchFormat}; - - // Find matching resources based on search parameters - let matches = self - .find_matching_resources(tenant, resource_type, search_params) - .await?; - - match matches.len() { - 0 => Ok(ConditionalPatchResult::NoMatch), - 1 => { - // Exactly one match - apply the patch - let existing = matches.into_iter().next().unwrap(); - crate::core::conditional_if_match_gate(if_match, resource_type, Some(&existing))?; - let current_content = existing.content().clone(); - - // Apply the patch based on format - let patched_content = match patch { - PatchFormat::JsonPatch(patch_doc) => { - self.apply_json_patch(¤t_content, patch_doc)? - } - PatchFormat::FhirPathPatch(patch_params) => { - self.apply_fhirpath_patch(¤t_content, patch_params)? - } - PatchFormat::MergePatch(merge_doc) => { - self.apply_merge_patch(¤t_content, merge_doc) - } - }; - - // Update the resource with the patched content - let updated = self.update(tenant, &existing, patched_content).await?; - Ok(ConditionalPatchResult::Patched(updated)) - } - n => Ok(ConditionalPatchResult::MultipleMatches(n)), - } + ) -> StorageResult> { + self.find_matching_resources(tenant, resource_type, search_params) + .await } } @@ -3290,166 +3243,6 @@ impl PostgresBackend { crate::search::ResourceTypeScope::version(self.config().fhir_version), ) } - - // ======================================================================== - // Patch Helper Methods - // ======================================================================== - - /// Applies a JSON Patch (RFC 6902) to a resource. - fn apply_json_patch(&self, resource: &Value, patch_doc: &Value) -> StorageResult { - use crate::error::ValidationError; - - let patch: json_patch::Patch = serde_json::from_value(patch_doc.clone()).map_err(|e| { - StorageError::Validation(ValidationError::InvalidResource { - message: format!("Invalid JSON Patch document: {}", e), - details: vec![], - }) - })?; - - let mut patched = resource.clone(); - json_patch::patch(&mut patched, &patch).map_err(|e| { - StorageError::Validation(ValidationError::InvalidResource { - message: format!("Failed to apply JSON Patch: {}", e), - details: vec![], - }) - })?; - - Ok(patched) - } - - /// Applies a FHIRPath Patch to a resource. - fn apply_fhirpath_patch(&self, resource: &Value, patch_params: &Value) -> StorageResult { - use crate::error::ValidationError; - - let parameter = patch_params.get("parameter").and_then(|p| p.as_array()); - if parameter.is_none() { - return Err(StorageError::Validation(ValidationError::InvalidResource { - message: "FHIRPath Patch must have a 'parameter' array".to_string(), - details: vec![], - })); - } - - let mut patched = resource.clone(); - - for operation in parameter.unwrap() { - let parts = operation.get("part").and_then(|p| p.as_array()); - if parts.is_none() { - continue; - } - - let mut op_type = None; - let mut op_path = None; - let mut op_name = None; - let mut op_value = None; - - for part in parts.unwrap() { - match part.get("name").and_then(|n| n.as_str()) { - Some("type") => { - op_type = part - .get("valueCode") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()); - } - Some("path") => { - op_path = part - .get("valueString") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()); - } - Some("name") => { - op_name = part - .get("valueString") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()); - } - Some("value") => { - op_value = extract_part_value(part); - } - _ => {} - } - } - - match op_type.as_deref() { - Some("replace") => { - if let (Some(path), Some(value)) = (&op_path, &op_value) { - self.fhirpath_replace(&mut patched, path, value)?; - } - } - Some("add") => { - if let (Some(path), Some(name), Some(value)) = (&op_path, &op_name, &op_value) { - self.fhirpath_add(&mut patched, path, name, value)?; - } - } - Some("delete") => { - if let Some(path) = &op_path { - self.fhirpath_delete(&mut patched, path)?; - } - } - _ => { - // Unsupported operation type - skip - } - } - } - - Ok(patched) - } - - /// Helper for FHIRPath replace operation. - fn fhirpath_replace( - &self, - resource: &mut Value, - path: &str, - value: &Value, - ) -> StorageResult<()> { - let parts: Vec<&str> = path.split('.').collect(); - if parts.len() == 2 { - if let Some(obj) = resource.as_object_mut() { - obj.insert(parts[1].to_string(), value.clone()); - } - } - Ok(()) - } - - /// Helper for FHIRPath add operation. - fn fhirpath_add( - &self, - resource: &mut Value, - path: &str, - name: &str, - value: &Value, - ) -> StorageResult<()> { - let parts: Vec<&str> = path.split('.').collect(); - if parts.len() == 1 - && parts[0] - == resource - .get("resourceType") - .and_then(|r| r.as_str()) - .unwrap_or("") - { - if let Some(obj) = resource.as_object_mut() { - obj.insert(name.to_string(), value.clone()); - } - } - Ok(()) - } - - /// Helper for FHIRPath delete operation. - fn fhirpath_delete(&self, resource: &mut Value, path: &str) -> StorageResult<()> { - let parts: Vec<&str> = path.split('.').collect(); - if parts.len() == 2 { - if let Some(obj) = resource.as_object_mut() { - obj.remove(parts[1]); - } - } - Ok(()) - } - - /// Applies a JSON Merge Patch (RFC 7386) to a resource. - fn apply_merge_patch(&self, resource: &Value, merge_doc: &Value) -> Value { - let mut patched = resource.clone(); - json_patch::merge(&mut patched, merge_doc); - patched - } } // ============================================================================ diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index 7e37c1494..4d2e930ad 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -205,22 +205,6 @@ fn purge_fts_rows( 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`, -/// `valueReference`), so every FHIR polymorphic variant is accepted rather -/// than only the handful the patch handler used to special-case. -fn extract_part_value(part: &Value) -> Option { - part.as_object()?.iter().find_map(|(k, v)| { - let suffix = k.strip_prefix("value")?; - suffix - .chars() - .next()? - .is_ascii_uppercase() - .then(|| v.clone()) - }) -} - #[async_trait] impl ResourceStorage for SqliteBackend { fn backend_name(&self) -> &'static str { @@ -3244,57 +3228,17 @@ impl ConditionalStorage for SqliteBackend { } } - /// Patches a resource based on search criteria. - /// - /// This implements conditional patch as defined in FHIR: - /// `PATCH [base]/[type]?[search-params]` - /// - /// Supports three patch formats: - /// - JSON Patch (RFC 6902) - /// - FHIRPath Patch (FHIR-specific) - /// - JSON Merge Patch (RFC 7386) - async fn conditional_patch( + /// The criteria resolver the provided + /// [`ConditionalStorage::conditional_patch`] is written in terms of: this + /// backend has no patch code of its own (#1406). + async fn resolve_conditional_matches( &self, tenant: &TenantContext, resource_type: &str, search_params: &str, - patch: &crate::core::PatchFormat, - if_match: &crate::core::EntityTagPrecondition, - ) -> StorageResult { - use crate::core::{ConditionalPatchResult, PatchFormat}; - - // Find matching resources based on search parameters - let matches = self - .find_matching_resources(tenant, resource_type, search_params) - .await?; - - match matches.len() { - 0 => Ok(ConditionalPatchResult::NoMatch), - 1 => { - // Exactly one match - apply the patch - let existing = matches.into_iter().next().unwrap(); - crate::core::conditional_if_match_gate(if_match, resource_type, Some(&existing))?; - let current_content = existing.content().clone(); - - // Apply the patch based on format - let patched_content = match patch { - PatchFormat::JsonPatch(patch_doc) => { - self.apply_json_patch(¤t_content, patch_doc)? - } - PatchFormat::FhirPathPatch(patch_params) => { - self.apply_fhirpath_patch(¤t_content, patch_params)? - } - PatchFormat::MergePatch(merge_doc) => { - self.apply_merge_patch(¤t_content, merge_doc) - } - }; - - // Update the resource with the patched content - let updated = self.update(tenant, &existing, patched_content).await?; - Ok(ConditionalPatchResult::Patched(updated)) - } - n => Ok(ConditionalPatchResult::MultipleMatches(n)), - } + ) -> StorageResult> { + self.find_matching_resources(tenant, resource_type, search_params) + .await } } @@ -3360,199 +3304,6 @@ impl SqliteBackend { crate::search::ResourceTypeScope::version(self.config().fhir_version), ) } - - // ======================================================================== - // Patch Helper Methods - // ======================================================================== - - /// Applies a JSON Patch (RFC 6902) to a resource. - /// - /// JSON Patch operations: - /// - `add`: Add a value at the specified path - /// - `remove`: Remove the value at the specified path - /// - `replace`: Replace the value at the specified path - /// - `move`: Move a value from one path to another - /// - `copy`: Copy a value from one path to another - /// - `test`: Test that a value equals the expected value - fn apply_json_patch(&self, resource: &Value, patch_doc: &Value) -> StorageResult { - use crate::error::ValidationError; - - // Parse the patch document as an array of operations - let patch: json_patch::Patch = serde_json::from_value(patch_doc.clone()).map_err(|e| { - StorageError::Validation(ValidationError::InvalidResource { - message: format!("Invalid JSON Patch document: {}", e), - details: vec![], - }) - })?; - - // Apply the patch to a mutable copy - let mut patched = resource.clone(); - json_patch::patch(&mut patched, &patch).map_err(|e| { - StorageError::Validation(ValidationError::InvalidResource { - message: format!("Failed to apply JSON Patch: {}", e), - details: vec![], - }) - })?; - - Ok(patched) - } - - /// Applies a FHIRPath Patch to a resource. - /// - /// FHIRPath Patch uses a Parameters resource with operation parts: - /// - `type`: add, insert, delete, replace, move - /// - `path`: FHIRPath expression - /// - `name`: element name (for add) - /// - `value`: new value - /// - /// Note: Full FHIRPath Patch support requires the helios-fhirpath evaluator. - /// This implementation handles common cases. - fn apply_fhirpath_patch(&self, resource: &Value, patch_params: &Value) -> StorageResult { - use crate::error::ValidationError; - - // The patch_params should be a Parameters resource with operation parts - let parameter = patch_params.get("parameter").and_then(|p| p.as_array()); - if parameter.is_none() { - return Err(StorageError::Validation(ValidationError::InvalidResource { - message: "FHIRPath Patch must have a 'parameter' array".to_string(), - details: vec![], - })); - } - - let mut patched = resource.clone(); - - for operation in parameter.unwrap() { - // Each operation has parts with name "type", "path", "name", "value" - let parts = operation.get("part").and_then(|p| p.as_array()); - if parts.is_none() { - continue; - } - - let mut op_type = None; - let mut op_path = None; - let mut op_name = None; - let mut op_value = None; - - for part in parts.unwrap() { - match part.get("name").and_then(|n| n.as_str()) { - Some("type") => { - op_type = part - .get("valueCode") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()); - } - Some("path") => { - op_path = part - .get("valueString") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()); - } - Some("name") => { - op_name = part - .get("valueString") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()); - } - Some("value") => { - op_value = extract_part_value(part); - } - _ => {} - } - } - - // Apply the operation based on type - match op_type.as_deref() { - Some("replace") => { - if let (Some(path), Some(value)) = (&op_path, &op_value) { - self.fhirpath_replace(&mut patched, path, value)?; - } - } - Some("add") => { - if let (Some(path), Some(name), Some(value)) = (&op_path, &op_name, &op_value) { - self.fhirpath_add(&mut patched, path, name, value)?; - } - } - Some("delete") => { - if let Some(path) = &op_path { - self.fhirpath_delete(&mut patched, path)?; - } - } - _ => { - // Unsupported operation type - skip - } - } - } - - Ok(patched) - } - - /// Helper for FHIRPath replace operation. - fn fhirpath_replace( - &self, - resource: &mut Value, - path: &str, - value: &Value, - ) -> StorageResult<()> { - // Simple implementation for common paths like "Resource.field" - // Full implementation would use helios-fhirpath for path evaluation - let parts: Vec<&str> = path.split('.').collect(); - if parts.len() == 2 { - // Simple path like "Patient.active" - if let Some(obj) = resource.as_object_mut() { - obj.insert(parts[1].to_string(), value.clone()); - } - } - Ok(()) - } - - /// Helper for FHIRPath add operation. - fn fhirpath_add( - &self, - resource: &mut Value, - path: &str, - name: &str, - value: &Value, - ) -> StorageResult<()> { - // Simple implementation for adding to root or nested object - let parts: Vec<&str> = path.split('.').collect(); - if parts.len() == 1 - && parts[0] - == resource - .get("resourceType") - .and_then(|r| r.as_str()) - .unwrap_or("") - { - // Adding to root level - if let Some(obj) = resource.as_object_mut() { - obj.insert(name.to_string(), value.clone()); - } - } - Ok(()) - } - - /// Helper for FHIRPath delete operation. - fn fhirpath_delete(&self, resource: &mut Value, path: &str) -> StorageResult<()> { - // Simple implementation for deleting fields - let parts: Vec<&str> = path.split('.').collect(); - if parts.len() == 2 { - if let Some(obj) = resource.as_object_mut() { - obj.remove(parts[1]); - } - } - Ok(()) - } - - /// Applies a JSON Merge Patch (RFC 7386) to a resource. - /// - /// Merge Patch is simpler than JSON Patch: - /// - Fields in the patch replace those in the target - /// - null values remove fields from the target - /// - Nested objects are merged recursively - fn apply_merge_patch(&self, resource: &Value, merge_doc: &Value) -> Value { - let mut patched = resource.clone(); - json_patch::merge(&mut patched, merge_doc); - patched - } } #[async_trait] diff --git a/crates/persistence/src/composite/storage.rs b/crates/persistence/src/composite/storage.rs index 529388156..8422d9f12 100644 --- a/crates/persistence/src/composite/storage.rs +++ b/crates/persistence/src/composite/storage.rs @@ -45,12 +45,12 @@ use tracing::{debug, instrument, warn}; use crate::core::history::HistoryParams; use crate::core::{ BundleEntry, BundleProvider, BundleResult, CapabilityProvider, ChainedSearchProvider, - ConditionalCreateResult, ConditionalDeleteResult, ConditionalPatchResult, ConditionalStorage, - ConditionalUpdateResult, ExportDataProvider, ExportRequest, GroupExportProvider, - IncludeProvider, InstanceHistoryProvider, NdjsonBatch, PatchFormat, PatientExportProvider, - PurgableStorage, ResourceStorage, RevincludeProvider, SearchProvider, SearchResult, SofRunner, - StorageCapabilities, SystemHistoryProvider, TerminologySearchProvider, TextSearchProvider, - TypeHistoryProvider, VersionedStorage, + ConditionalCreateResult, ConditionalDeleteResult, ConditionalStorage, ConditionalUpdateResult, + ExportDataProvider, ExportRequest, GroupExportProvider, IncludeProvider, + InstanceHistoryProvider, NdjsonBatch, PatientExportProvider, PurgableStorage, ResourceStorage, + RevincludeProvider, SearchProvider, SearchResult, SofRunner, StorageCapabilities, + SystemHistoryProvider, TerminologySearchProvider, TextSearchProvider, TypeHistoryProvider, + VersionedStorage, }; use crate::error::{BackendError, ResourceError, StorageError, StorageResult, TransactionError}; use crate::search::ChainResolveOptions; @@ -1380,20 +1380,51 @@ impl SearchProvider for CompositeStorage { impl ConditionalStorage for CompositeStorage { /// Composed, not copied from the primary (#1384). With a dedicated search /// backend the composite resolves the criteria itself and needs only plain - /// CRUD from the primary for create / update / delete — which is how - /// `s3-elasticsearch` serves them over a primary that declares none. Patch - /// is the reverse: only the primary can apply one, and it resolves the - /// criteria against its own index, which a dedicated search backend leaves - /// offloaded and empty — so no such composite supports it. + /// CRUD from the primary — which is how `s3-elasticsearch` serves all four + /// over a primary that declares none. Patch included (#1406): the patch is + /// applied by the shared [`apply_patch`](crate::core::apply_patch), not by + /// the primary. fn supports_conditional(&self, interaction: crate::core::ConditionalInteraction) -> bool { if self.has_dedicated_search_backend() { - return interaction != crate::core::ConditionalInteraction::Patch; + return true; } self.conditional_storage .as_ref() .is_some_and(|primary| primary.supports_conditional(interaction)) } + /// Criteria go to whichever backend holds the search index: the dedicated + /// search backend when there is one — the primary's own index is then + /// offloaded and empty — and the primary otherwise. + /// + /// `conditional_patch` is the trait's provided implementation on top of + /// this. Its `read` and `update` are the composite's: the current content + /// comes from the primary, not from the search backend's copy, and the + /// write compares-and-swaps there and is synced to the secondaries like + /// any other update. + async fn resolve_conditional_matches( + &self, + tenant: &TenantContext, + resource_type: &str, + search_params: &str, + ) -> StorageResult> { + if self.has_dedicated_search_backend() { + return self + .find_conditional_matches(tenant, resource_type, search_params) + .await; + } + + let storage = self.conditional_storage.as_ref().ok_or_else(|| { + StorageError::Backend(BackendError::UnsupportedCapability { + backend_name: "composite".to_string(), + capability: "ConditionalStorage".to_string(), + }) + })?; + storage + .resolve_conditional_matches(tenant, resource_type, search_params) + .await + } + async fn conditional_create( &self, tenant: &TenantContext, @@ -1675,58 +1706,6 @@ impl ConditionalStorage for CompositeStorage { Ok(result) } - - async fn conditional_patch( - &self, - tenant: &TenantContext, - resource_type: &str, - search_params: &str, - patch: &PatchFormat, - if_match: &crate::core::EntityTagPrecondition, - ) -> StorageResult { - let storage = self.conditional_storage.as_ref().ok_or_else(|| { - StorageError::Backend(BackendError::UnsupportedCapability { - backend_name: "composite".to_string(), - capability: "ConditionalStorage".to_string(), - }) - })?; - - // Patch application lives in the primary, and the primary resolves the - // criteria against its own index — which, with a dedicated search - // backend, is offloaded and empty (even `_id` enumerates index rows on - // SQLite). Handed the criteria it matched nothing, so every conditional - // patch was a silent no-match; refuse instead, in step with - // `supports_conditional` (#1384). - if !self.supports_conditional(crate::core::ConditionalInteraction::Patch) { - return Err(StorageError::Backend(BackendError::UnsupportedCapability { - backend_name: "composite".to_string(), - capability: "conditional_patch".to_string(), - })); - } - - let result = storage - .conditional_patch(tenant, resource_type, search_params, patch, if_match) - .await?; - - // Sync patched resource to secondaries - if let ConditionalPatchResult::Patched(ref stored) = result { - if let Err(e) = self - .sync_to_secondaries(SyncEvent::Update { - resource_type: resource_type.to_string(), - resource_id: stored.id().to_string(), - content: stored.content().clone(), - tenant_id: tenant.tenant_id().clone(), - version: stored.version_id().to_string(), - fhir_version: stored.fhir_version(), - }) - .await - { - warn!(error = %e, "Failed to sync conditional_patch to secondaries"); - } - } - - Ok(result) - } } #[async_trait] diff --git a/crates/persistence/src/core/backend.rs b/crates/persistence/src/core/backend.rs index 7fd532010..4c05d80dd 100644 --- a/crates/persistence/src/core/backend.rs +++ b/crates/persistence/src/core/backend.rs @@ -208,8 +208,7 @@ pub enum BackendCapability { /// Conditional delete (`DELETE [type]?criteria`). Single-match only: more /// than one match is reported, never deleted. ConditionalDelete, - /// Conditional patch (`PATCH [type]?criteria`). MongoDB and S3 do not - /// implement it. + /// Conditional patch (`PATCH [type]?criteria`). S3 does not serve it. ConditionalPatch, } diff --git a/crates/persistence/src/core/mod.rs b/crates/persistence/src/core/mod.rs index 3f5d8be6d..4aa9381ad 100644 --- a/crates/persistence/src/core/mod.rs +++ b/crates/persistence/src/core/mod.rs @@ -104,6 +104,7 @@ pub(crate) mod bulk_submit_receipts; pub mod bulk_submit_worker; pub mod capabilities; pub mod history; +pub mod patch; pub mod preconditions; pub mod search; pub mod sof_runner; @@ -158,6 +159,7 @@ pub use history::{ DifferentialHistoryProvider, HistoryEntry, HistoryMethod, HistoryPage, HistoryParams, InstanceHistoryProvider, SystemHistoryProvider, TypeHistoryProvider, }; +pub use patch::{PatchError, apply_patch}; pub use preconditions::{ EntityTag, EntityTagPrecondition, MalformedPrecondition, bundle_if_match_gate, bundle_if_none_exist_gate, conditional_if_match_gate, if_match_field_satisfied, diff --git a/crates/persistence/src/core/patch.rs b/crates/persistence/src/core/patch.rs new file mode 100644 index 000000000..5e0e6eda8 --- /dev/null +++ b/crates/persistence/src/core/patch.rs @@ -0,0 +1,290 @@ +//! The one patch applier. +//! +//! `PATCH [type]/[id]` (applied by the REST layer) and `PATCH [type]?criteria` +//! (applied by [`ConditionalStorage::conditional_patch`]) both go through +//! [`apply_patch`], so a patch document means the same thing on either +//! endpoint and on every backend (#1406). Before, REST, SQLite and PostgreSQL +//! each carried a private copy. +//! +//! # Formats +//! +//! * JSON Patch (RFC 6902) and JSON Merge Patch (RFC 7396) are applied with +//! the `json-patch` crate. +//! * FHIRPath Patch is **not implemented** and answers +//! [`PatchError::UnsupportedFormat`]. The private appliers this module +//! replaces had a stub for it that ignored every path but `Type.element`, +//! skipped unknown operation types, and so could change nothing — while the +//! caller still wrote a new version. +//! +//! # Invariants +//! +//! A patch cannot change or remove `resourceType` or `id`: they are the +//! identity of the resource the request named. Backends re-assert both on +//! every `update`, so such a patch could not corrupt a row — it would be +//! silently undone and answered as a success, which is why it is refused +//! here instead. +//! +//! `meta.versionId` and `meta.lastUpdated` are not guarded. They are +//! server-assigned on every write, exactly as for a `PUT` whose body carries +//! them, so a patch naming them has no lasting effect. +//! +//! [`ConditionalStorage::conditional_patch`]: super::ConditionalStorage::conditional_patch + +use serde_json::Value; +use thiserror::Error; + +use super::PatchFormat; + +/// Why a patch was not applied. Nothing has been written when one is returned. +/// +/// The variants are what a REST layer needs to choose a status: today all but +/// [`UnsupportedFormat`](Self::UnsupportedFormat) are a `400`, and a failed +/// `test` has a variant of its own so that can change on its own (#1393). +#[derive(Debug, Clone, PartialEq, Eq, Error)] +pub enum PatchError { + /// The patch document is not a document of its format — for JSON Patch, + /// not an array of RFC 6902 operations. + #[error("Invalid {format}: {message}")] + MalformedDocument { + /// The format, as a client would name it (`JSON Patch`). + format: &'static str, + /// The parser's complaint. + message: String, + }, + + /// A JSON Patch `test` operation compared unequal (RFC 6902 §4.6). The + /// document is well-formed and every path resolved; the resource is just + /// not in the state the client expected. + #[error("Failed to apply JSON Patch: {message}")] + TestFailed { + /// Which operation failed, and where. + message: String, + }, + + /// A JSON Patch operation could not be carried out against this resource: + /// a path that does not resolve, a `move` into its own source. + #[error("Failed to apply JSON Patch: {message}")] + OperationFailed { + /// Which operation failed, and where. + message: String, + }, + + /// The patch changes or removes an element that identifies the resource. + #[error("Cannot change {element} via patch")] + ImmutableElement { + /// `resourceType` or `id`. + element: &'static str, + }, + + /// The format is recognised but not implemented (FHIRPath Patch). + #[error("{format} is not implemented")] + UnsupportedFormat { + /// The format, as a client would name it (`FHIRPath Patch`). + format: &'static str, + }, +} + +/// The elements a patch may neither change nor remove. +const IMMUTABLE_ELEMENTS: [&str; 2] = ["resourceType", "id"]; + +/// Applies `patch` to `current` and returns the patched content. +/// +/// `current` is the content of the resource as stored; it is not modified. +/// The result still has the `resourceType` and `id` of `current` (see the +/// module docs), and is otherwise unvalidated: the caller writes it with +/// [`ResourceStorage::update`](super::ResourceStorage::update) like any other +/// content. +pub fn apply_patch(current: &Value, patch: &PatchFormat) -> Result { + let mut patched = current.clone(); + + match patch { + PatchFormat::JsonPatch(operations) => { + let operations: json_patch::Patch = serde_json::from_value(operations.clone()) + .map_err(|e| PatchError::MalformedDocument { + format: "JSON Patch", + message: e.to_string(), + })?; + + json_patch::patch(&mut patched, &operations).map_err(|e| { + let message = e.to_string(); + match e.kind { + json_patch::PatchErrorKind::TestFailed => PatchError::TestFailed { message }, + _ => PatchError::OperationFailed { message }, + } + })?; + } + PatchFormat::MergePatch(merge_doc) => json_patch::merge(&mut patched, merge_doc), + PatchFormat::FhirPathPatch(_) => { + return Err(PatchError::UnsupportedFormat { + format: "FHIRPath Patch", + }); + } + } + + for element in IMMUTABLE_ELEMENTS { + // Compared as JSON: a resource stored without the element (nothing + // requires `current` to carry its id) may not gain one either. + if patched.get(element) != current.get(element) { + return Err(PatchError::ImmutableElement { element }); + } + } + + Ok(patched) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn patient() -> Value { + json!({ + "resourceType": "Patient", + "id": "p1", + "active": false, + "name": [{"family": "Before"}] + }) + } + + #[test] + fn json_patch_is_applied_and_the_input_left_alone() { + let current = patient(); + let patched = apply_patch( + ¤t, + &PatchFormat::JsonPatch(json!([ + {"op": "test", "path": "/active", "value": false}, + {"op": "replace", "path": "/active", "value": true}, + {"op": "replace", "path": "/name/0/family", "value": "After"} + ])), + ) + .unwrap(); + + assert_eq!(patched["active"], json!(true)); + assert_eq!(patched["name"][0]["family"], json!("After")); + assert_eq!(current, patient()); + } + + #[test] + fn merge_patch_sets_and_removes() { + let patched = apply_patch( + &patient(), + &PatchFormat::MergePatch(json!({"active": true, "name": null, "gender": "other"})), + ) + .unwrap(); + + assert_eq!( + patched, + json!({"resourceType": "Patient", "id": "p1", "active": true, "gender": "other"}) + ); + } + + #[test] + fn a_document_that_is_not_a_json_patch_is_malformed() { + for document in [ + json!({"not": "a patch"}), + json!([{"op": "frobnicate", "path": "/active"}]), + json!([{"op": "replace", "path": "no-leading-slash", "value": 1}]), + ] { + let err = apply_patch(&patient(), &PatchFormat::JsonPatch(document.clone())); + assert!( + matches!( + err, + Err(PatchError::MalformedDocument { + format: "JSON Patch", + .. + }) + ), + "{document}: {err:?}" + ); + } + } + + #[test] + fn a_failed_test_is_told_apart_from_an_operation_that_cannot_apply() { + let failed_test = apply_patch( + &patient(), + &PatchFormat::JsonPatch(json!([{"op": "test", "path": "/active", "value": true}])), + ); + assert!( + matches!(failed_test, Err(PatchError::TestFailed { .. })), + "{failed_test:?}" + ); + + let bad_path = apply_patch( + &patient(), + &PatchFormat::JsonPatch(json!([{"op": "replace", "path": "/nope/deeper", "value": 1}])), + ); + assert!( + matches!(bad_path, Err(PatchError::OperationFailed { .. })), + "{bad_path:?}" + ); + } + + #[test] + fn resource_type_and_id_can_be_neither_changed_nor_removed() { + let refused = [ + ( + "resourceType", + PatchFormat::JsonPatch( + json!([{"op": "replace", "path": "/resourceType", "value": "Person"}]), + ), + ), + ( + "resourceType", + PatchFormat::JsonPatch(json!([{"op": "remove", "path": "/resourceType"}])), + ), + ( + "resourceType", + PatchFormat::MergePatch(json!({"resourceType": "Person"})), + ), + ( + "id", + PatchFormat::JsonPatch(json!([{"op": "replace", "path": "/id", "value": "p2"}])), + ), + ( + "id", + PatchFormat::JsonPatch(json!([{"op": "move", "from": "/id", "path": "/was"}])), + ), + ("id", PatchFormat::MergePatch(json!({"id": null}))), + ("id", PatchFormat::MergePatch(json!({"id": "p2"}))), + ]; + for (element, patch) in refused { + assert_eq!( + apply_patch(&patient(), &patch), + Err(PatchError::ImmutableElement { element }), + "{patch:?}" + ); + } + + // Naming them without changing them is not a change. + for patch in [ + PatchFormat::MergePatch(json!({"resourceType": "Patient", "id": "p1", "active": true})), + PatchFormat::JsonPatch(json!([ + {"op": "test", "path": "/id", "value": "p1"}, + {"op": "replace", "path": "/resourceType", "value": "Patient"}, + {"op": "replace", "path": "/active", "value": true} + ])), + ] { + let patched = apply_patch(&patient(), &patch).unwrap(); + assert_eq!(patched["active"], json!(true), "{patch:?}"); + } + } + + #[test] + fn fhirpath_patch_is_refused_rather_than_half_applied() { + let parameters = json!({ + "resourceType": "Parameters", + "parameter": [{"name": "operation", "part": [ + {"name": "type", "valueCode": "replace"}, + {"name": "path", "valueString": "Patient.active"}, + {"name": "value", "valueBoolean": true} + ]}] + }); + assert_eq!( + apply_patch(&patient(), &PatchFormat::FhirPathPatch(parameters)), + Err(PatchError::UnsupportedFormat { + format: "FHIRPath Patch" + }) + ); + } +} diff --git a/crates/persistence/src/core/storage.rs b/crates/persistence/src/core/storage.rs index fecc7b220..c6121492b 100644 --- a/crates/persistence/src/core/storage.rs +++ b/crates/persistence/src/core/storage.rs @@ -1230,15 +1230,42 @@ pub trait ConditionalStorage: ResourceStorage { /// /// The default mirrors the trait itself: `conditional_create`, /// `conditional_update` and `conditional_delete` are required methods, - /// `conditional_patch` defaults to `UnsupportedCapability`. A backend whose - /// methods differ from that — S3 refuses all four, SQLite and PostgreSQL - /// implement patch — overrides it from its declared - /// [`BackendCapability`](crate::core::BackendCapability) list, the same - /// list `tests/backend_capability_contract.rs` pins. + /// while `conditional_patch` works only once + /// [`resolve_conditional_matches`](Self::resolve_conditional_matches) is + /// provided. A backend whose methods differ from that — S3 refuses all + /// four; SQLite, PostgreSQL and MongoDB serve patch — overrides it from its + /// declared [`BackendCapability`](crate::core::BackendCapability) list, the + /// same list `tests/backend_capability_contract.rs` pins. fn supports_conditional(&self, interaction: ConditionalInteraction) -> bool { !matches!(interaction, ConditionalInteraction::Patch) } + /// Resolves conditional criteria to the resources they select — every + /// match, so the caller can tell one from several. + /// + /// This is the primitive the provided + /// [`conditional_patch`](Self::conditional_patch) is written in terms of. + /// Implementations build the query with + /// [`crate::search::build_conditional_query`], so criteria mean what they + /// mean as a direct search (#1312), and answer no match for empty criteria. + /// + /// The default refuses: a storage that cannot search cannot resolve + /// criteria. + async fn resolve_conditional_matches( + &self, + tenant: &TenantContext, + resource_type: &str, + search_params: &str, + ) -> StorageResult> { + let _ = (tenant, resource_type, search_params); + Err(StorageError::Backend( + crate::error::BackendError::UnsupportedCapability { + backend_name: self.backend_name().to_string(), + capability: "conditional_patch".to_string(), + }, + )) + } + /// Creates a resource only if no matching resource exists. /// /// # Arguments @@ -1336,8 +1363,35 @@ pub trait ConditionalStorage: ResourceStorage { /// /// # Errors /// - /// * `StorageError::Validation` - If the patch is invalid or would create invalid resource - /// * `StorageError::Backend(NotSupported)` - If conditional patch is not supported + /// * `StorageError::Validation(ValidationError::Patch(_))` - the patch is + /// malformed, does not apply, changes `resourceType` / `id`, or is a + /// FHIRPath Patch (not implemented); see [`PatchError`](super::PatchError) + /// * `StorageError::Concurrency(OptimisticLockFailure)` - `If-Match` was + /// supplied and is not satisfied + /// * `StorageError::Concurrency(VersionConflict)` - the resource changed + /// between resolving the criteria and writing + /// * `StorageError::Backend(UnsupportedCapability)` - this storage cannot + /// resolve criteria + /// + /// # Provided implementation + /// + /// The one implementation every backend uses (#1406), in terms of + /// primitives each already has: + /// + /// 1. [`resolve_conditional_matches`](Self::resolve_conditional_matches); + /// none is `NoMatch`, several `MultipleMatches`. + /// 2. `read` the match. On a [`CompositeStorage`](crate::composite) the + /// criteria are resolved by the search backend while `read` and `update` + /// go to the primary, so this is the authoritative content — the one the + /// patch has to apply to. A search copy of another version than the + /// primary's is a `VersionConflict`: the criteria were judged against + /// content that is no longer current, and whether the current content + /// still matches them is unknown. (Conditional update and delete reach + /// the same answer through the primary's compare-and-swap.) + /// 3. [`conditional_if_match_gate`](super::conditional_if_match_gate). + /// 4. [`apply_patch`](super::apply_patch). + /// 5. `update(current, patched)`, which compares-and-swaps on `current`'s + /// version: a writer landing after step 2 ends in `VersionConflict`. async fn conditional_patch( &self, tenant: &TenantContext, @@ -1346,14 +1400,42 @@ pub trait ConditionalStorage: ResourceStorage { patch: &PatchFormat, if_match: &EntityTagPrecondition, ) -> StorageResult { - // Default implementation returns NotSupported - let _ = (tenant, resource_type, search_params, patch, if_match); - Err(StorageError::Backend( - crate::error::BackendError::UnsupportedCapability { - backend_name: "unknown".to_string(), - capability: "conditional_patch".to_string(), - }, - )) + let mut matches = self + .resolve_conditional_matches(tenant, resource_type, search_params) + .await?; + + let matched = match matches.len() { + 0 => return Ok(ConditionalPatchResult::NoMatch), + 1 => matches.remove(0), + n => return Ok(ConditionalPatchResult::MultipleMatches(n)), + }; + + let current = match self.read(tenant, resource_type, matched.id()).await { + Ok(Some(current)) => current, + // Deleted since the index last saw it: there is no match any more. + Ok(None) | Err(StorageError::Resource(crate::error::ResourceError::Gone { .. })) => { + return Ok(ConditionalPatchResult::NoMatch); + } + Err(e) => return Err(e), + }; + if current.version_id() != matched.version_id() { + return Err(StorageError::Concurrency( + crate::error::ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: current.id().to_string(), + expected_version: matched.version_id().to_string(), + actual_version: current.version_id().to_string(), + }, + )); + } + + super::conditional_if_match_gate(if_match, resource_type, Some(¤t))?; + + let patched = super::apply_patch(current.content(), patch) + .map_err(crate::error::ValidationError::from)?; + + let updated = self.update(tenant, ¤t, patched).await?; + Ok(ConditionalPatchResult::Patched(updated)) } } diff --git a/crates/persistence/src/error.rs b/crates/persistence/src/error.rs index ef5d013b3..df41b658e 100644 --- a/crates/persistence/src/error.rs +++ b/crates/persistence/src/error.rs @@ -254,6 +254,11 @@ pub enum ValidationError { /// Human-readable failure detail. message: String, }, + + /// A patch document could not be applied; nothing was written. Kept typed + /// so the REST layer chooses the status per cause (#1406). + #[error(transparent)] + Patch(#[from] crate::core::PatchError), } /// Detailed validation error information. diff --git a/crates/persistence/tests/backend_capability_contract.rs b/crates/persistence/tests/backend_capability_contract.rs index e4bad8f8b..11cd52296 100644 --- a/crates/persistence/tests/backend_capability_contract.rs +++ b/crates/persistence/tests/backend_capability_contract.rs @@ -429,6 +429,7 @@ mod mongodb { BackendCapability::ConditionalCreate, BackendCapability::ConditionalUpdate, BackendCapability::ConditionalDelete, + BackendCapability::ConditionalPatch, BackendCapability::SharedSchema, ], ); @@ -444,11 +445,10 @@ mod mongodb { assert_shared_schema_instance_is_consistent("mongodb", &backend); } - /// No `ConditionalPatch`: `MongoBackend::conditional_patch` answers - /// `UnsupportedCapability`, and the CapabilityStatement advertised - /// `conditionalPatch` for it anyway (#1384). + /// All four: `conditional_patch` is the trait's provided implementation + /// over MongoDB's criteria resolver (#1406; it was unimplemented, #1384). #[test] - fn mongodb_declares_every_conditional_interaction_but_patch() { + fn mongodb_declares_every_conditional_interaction() { assert_declares_exactly_these_conditionals( "mongodb", &MongoBackend::declared_capabilities(), @@ -456,6 +456,7 @@ mod mongodb { BackendCapability::ConditionalCreate, BackendCapability::ConditionalUpdate, BackendCapability::ConditionalDelete, + BackendCapability::ConditionalPatch, ], ); } diff --git a/crates/persistence/tests/composite_conditional_capabilities.rs b/crates/persistence/tests/composite_conditional_capabilities.rs index 313f807e0..9e0b120b3 100644 --- a/crates/persistence/tests/composite_conditional_capabilities.rs +++ b/crates/persistence/tests/composite_conditional_capabilities.rs @@ -25,8 +25,10 @@ use helios_persistence::core::{ BackendKind, ConditionalDeleteResult, ConditionalInteraction, ConditionalPatchResult, ConditionalStorage, PatchFormat, ResourceStorage, }; -use helios_persistence::error::{BackendError, StorageError}; +use helios_persistence::core::{EntityTagPrecondition, SearchProvider}; +use helios_persistence::error::{ConcurrencyError, StorageError}; use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; +use helios_persistence::types::{SearchParamType, SearchParameter, SearchQuery, SearchValue}; use serde_json::{Value, json}; fn tenant() -> TenantContext { @@ -54,6 +56,14 @@ fn sqlite() -> SqliteBackend { /// A production-shaped composite: primary with its own index offloaded, a /// dedicated search secondary, synchronous sync. fn composite_with_search_backend(fhir_version: Option) -> CompositeStorage { + composite_and_its_primary(fhir_version).0 +} + +/// The same, with a handle on the primary so a test can write behind the +/// composite's back — what leaves the search backend's copy stale. +fn composite_and_its_primary( + fhir_version: Option, +) -> (CompositeStorage, Arc) { let mut primary = sqlite(); primary.set_search_offloaded(true); let primary = Arc::new(primary); @@ -75,11 +85,12 @@ fn composite_with_search_backend(fhir_version: Option) -> Composite providers.insert("sqlite".to_string(), primary.clone() as DynSearchProvider); providers.insert("search".to_string(), index as DynSearchProvider); - CompositeStorage::new(config, backends) + let composite = CompositeStorage::new(config, backends) .expect("composite") .with_search_providers(providers) - .with_full_primary(primary) - .start_sync_workers() + .with_full_primary(primary.clone()) + .start_sync_workers(); + (composite, primary) } /// A composite that is only its primary: the primary indexes and searches. @@ -115,57 +126,126 @@ fn rename() -> PatchFormat { ])) } -/// With a dedicated search backend the composite resolves create / update / -/// delete criteria itself, and cannot serve patch: the primary applies -/// patches, and resolves the criteria against an index that is offloaded and -/// empty. It used to delegate anyway, so every conditional patch was a silent -/// no-match; it now refuses, in step with what it declares. +/// Finds Organizations by `name` through the composite — that is, through +/// the search backend. +async fn names_found(composite: &CompositeStorage, t: &TenantContext, name: &str) -> Vec { + composite + .search( + t, + &SearchQuery::new("Organization").with_parameter(SearchParameter { + name: "name".to_string(), + param_type: SearchParamType::String, + values: vec![SearchValue::eq(name)], + ..Default::default() + }), + ) + .await + .expect("search through composite") + .resources + .items + .iter() + .map(|r| r.id().to_string()) + .collect() +} + +/// With a dedicated search backend the composite resolves the criteria +/// itself and needs only plain CRUD from the primary, for all four +/// interactions. Patch was the exception until #1406: only the primary could +/// apply one, and it resolved the criteria against an index that is offloaded +/// and empty — a silent no-match before #1384, a refusal after. The applier is +/// now shared, so the composite resolves through the search backend, reads and +/// writes through the primary, and syncs the result like any update. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn a_dedicated_search_backend_serves_all_but_conditional_patch() { +async fn a_dedicated_search_backend_serves_every_conditional_interaction() { let composite = composite_with_search_backend(None); let t = tenant(); for interaction in ConditionalInteraction::ALL { - assert_eq!( - composite.supports_conditional(interaction), - interaction != ConditionalInteraction::Patch, - "{interaction}" - ); + assert!(composite.supports_conditional(interaction), "{interaction}"); } let created = composite .create(&t, "Organization", organization("ORG-P"), FhirVersion::R4) .await .expect("create through composite"); + composite + .create( + &t, + "Organization", + organization("ORG-OTHER"), + FhirVersion::R4, + ) + .await + .expect("create decoy"); + let criteria = "identifier=urn:zzz:probe|ORG-P"; + + // A stale `If-Match` is refused and nothing is written. + let stale = EntityTagPrecondition::parse(["W/\"7\""]).expect("well-formed If-Match"); + let result = composite + .conditional_patch(&t, "Organization", criteria, &rename(), &stale) + .await; + assert!( + matches!( + result, + Err(StorageError::Concurrency( + ConcurrencyError::OptimisticLockFailure { .. } + )) + ), + "{result:?}" + ); match composite .conditional_patch( &t, "Organization", - "identifier=urn:zzz:probe|ORG-P", + criteria, &rename(), - &helios_persistence::core::EntityTagPrecondition::Absent, + &EntityTagPrecondition::Absent, ) .await + .expect("conditional patch") { - Err(StorageError::Backend(BackendError::UnsupportedCapability { capability, .. })) => { - assert_eq!(capability, "conditional_patch"); + ConditionalPatchResult::Patched(stored) => { + assert_eq!(stored.id(), created.id()); + assert_eq!(stored.version_id(), "2"); + assert_eq!(stored.content()["name"], "Patched"); } - Ok(ConditionalPatchResult::NoMatch) => { + ConditionalPatchResult::NoMatch => { panic!("a matching resource exists: NoMatch is the silent failure this guards") } - other => panic!("expected UnsupportedCapability, got {other:?}"), + other => panic!("expected Patched, got {other:?}"), } - // Positive control: the same criteria do resolve on this composite, for an - // interaction it declares. - match composite - .conditional_delete( + // The primary holds the patched content ... + let read = composite + .read(&t, "Organization", created.id()) + .await + .expect("read") + .expect("still there"); + assert_eq!(read.version_id(), "2"); + assert_eq!(read.content()["name"], "Patched"); + // ... and the search backend was told: the new name is found, the old one + // is not, the decoy is untouched. + assert_eq!(names_found(&composite, &t, "Patched").await, [created.id()]); + assert_eq!(names_found(&composite, &t, "ZZZ Probe Org").await.len(), 1); + + let result = composite + .conditional_patch( &t, "Organization", - "identifier=urn:zzz:probe|ORG-P", - &helios_persistence::core::EntityTagPrecondition::Absent, + "identifier=urn:zzz:probe|NOBODY", + &rename(), + &EntityTagPrecondition::Absent, ) + .await; + assert!( + matches!(result, Ok(ConditionalPatchResult::NoMatch)), + "{result:?}" + ); + + // The same criteria resolve for the other interactions too. + match composite + .conditional_delete(&t, "Organization", criteria, &EntityTagPrecondition::Absent) .await .expect("conditional delete") { @@ -174,6 +254,60 @@ async fn a_dedicated_search_backend_serves_all_but_conditional_patch() { } } +/// The match is found in the search backend; what gets patched is the +/// primary's content. When the two disagree on the version — a write the +/// search backend has not seen — the criteria were judged against content that +/// is no longer current, so the patch is a `VersionConflict` and writes +/// nothing, as conditional update and delete are through the primary's +/// compare-and-swap. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_stale_search_copy_is_a_conflict_not_a_patch_over_unseen_content() { + let (composite, primary) = composite_and_its_primary(None); + let t = tenant(); + let criteria = "identifier=urn:zzz:probe|ORG-S"; + + let created = composite + .create(&t, "Organization", organization("ORG-S"), FhirVersion::R4) + .await + .expect("create through composite"); + + // Behind the composite's back: the primary moves to version 2 and no + // longer carries the identifier; the search backend still has version 1. + let mut moved_on = created.content().clone(); + moved_on["identifier"] = json!([{"system": "urn:zzz:probe", "value": "ORG-ELSEWHERE"}]); + primary + .update(&t, &created, moved_on) + .await + .expect("direct primary update"); + + let result = composite + .conditional_patch( + &t, + "Organization", + criteria, + &rename(), + &EntityTagPrecondition::Absent, + ) + .await; + assert!( + matches!( + result, + Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { .. } + )) + ), + "{result:?}" + ); + + let read = composite + .read(&t, "Organization", created.id()) + .await + .expect("read") + .expect("still there"); + assert_eq!(read.version_id(), "2"); + assert_eq!(read.content()["name"], "ZZZ Probe Org"); +} + /// Without a dedicated search backend every conditional interaction is the /// primary's, so the composite declares exactly what the primary does — and /// the patch it declares works. @@ -197,7 +331,7 @@ async fn without_a_search_backend_the_composite_follows_its_primary() { "Organization", "identifier=urn:zzz:probe|ORG-Q", &rename(), - &helios_persistence::core::EntityTagPrecondition::Absent, + &EntityTagPrecondition::Absent, ) .await .expect("conditional patch") @@ -222,12 +356,7 @@ async fn a_configured_version_scopes_the_type_qualifier_of_conditional_criteria( let r4 = composite_with_search_backend(Some(FhirVersion::R4)); match r4 - .conditional_delete( - &t, - "Patient", - criteria, - &helios_persistence::core::EntityTagPrecondition::Absent, - ) + .conditional_delete(&t, "Patient", criteria, &EntityTagPrecondition::Absent) .await { Err(e) => assert!( @@ -242,12 +371,7 @@ async fn a_configured_version_scopes_the_type_qualifier_of_conditional_criteria( let unset = composite_with_search_backend(None); assert!(matches!( unset - .conditional_delete( - &t, - "Patient", - criteria, - &helios_persistence::core::EntityTagPrecondition::Absent - ) + .conditional_delete(&t, "Patient", criteria, &EntityTagPrecondition::Absent) .await, Ok(ConditionalDeleteResult::NoMatch) )); @@ -258,7 +382,7 @@ async fn a_configured_version_scopes_the_type_qualifier_of_conditional_criteria( &t, "Patient", "general-practitioner:Practitioner=p1", - &helios_persistence::core::EntityTagPrecondition::Absent, + &EntityTagPrecondition::Absent, ) .await, Ok(ConditionalDeleteResult::NoMatch) diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 3aa39e373..fd67648d2 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -716,8 +716,8 @@ async fn mongodb_system_qualified_tokens_in_chains() { mod conditional_if_match_suite; /// #1381: `If-Match` is evaluated against the resource the criteria resolve -/// to. MongoDB has no `conditional_patch`, so that arm asserts it stays -/// unsupported. Needs the full registry: `identifier` is not embedded. +/// to, on conditional update, delete and patch (#1406). Needs the full +/// registry: `identifier` is not embedded. #[tokio::test] async fn mongodb_conditional_writes_honour_if_match() { let Some(backend) = create_backend_with_full_registry("cond_if_match_1381").await else { @@ -727,7 +727,7 @@ async fn mongodb_conditional_writes_honour_if_match() { conditional_if_match_suite::if_match_is_evaluated_against_the_resolved_match( &backend, "cond-if-match-1381", - false, + true, ) .await; } @@ -759,6 +759,27 @@ async fn mongodb_empty_values_are_rejected_on_every_path() { empty_value_suite::empty_values_are_rejected_on_every_path(&backend, "empty-value-1380").await; } +/// The backend-agnostic conditional patch suite (#1406). Same `#[path]` +/// arrangement. +#[path = "search/conditional_patch_suite.rs"] +mod conditional_patch_suite; + +/// #1406: MongoDB had no `conditional_patch`; it now serves the trait's +/// provided implementation. Needs the full registry: `identifier` is not +/// embedded. +#[tokio::test] +async fn mongodb_conditional_patch() { + let Some(backend) = create_backend_with_full_registry("cond_patch_1406").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + conditional_patch_suite::conditional_patch_resolves_gates_applies_and_swaps( + &backend, + "cond-patch-1406", + ) + .await; +} + /// #1062: a comma-separated value list on one `SearchParameter` is OR per /// FHIR (https://build.fhir.org/search.html#combining) — for date same as /// every other type. Drives the real `SearchProvider::search` / diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 3db141d04..fd7045be2 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -108,6 +108,11 @@ mod conditional_if_match_suite; #[path = "search/empty_value_suite.rs"] mod empty_value_suite; +/// The backend-agnostic conditional patch suite (#1406). Same `#[path]` +/// arrangement. +#[path = "search/conditional_patch_suite.rs"] +mod conditional_patch_suite; + #[path = "common/container_cleanup.rs"] mod container_cleanup; @@ -19101,6 +19106,18 @@ mod postgres_integration { .await; } + /// #1406: conditional patch is the trait's provided implementation over + /// the backend's criteria resolver and the shared patch applier. + #[tokio::test] + async fn postgres_integration_conditional_patch() { + let backend = create_backend().await; + super::conditional_patch_suite::conditional_patch_resolves_gates_applies_and_swaps( + &backend, + &unique_base("cond_patch_1406"), + ) + .await; + } + #[tokio::test] async fn postgres_integration_distinct_tenant_ids_never_share_data() { let backend = create_backend().await; diff --git a/crates/persistence/tests/search/conditional_patch_suite.rs b/crates/persistence/tests/search/conditional_patch_suite.rs new file mode 100644 index 000000000..e76140d8c --- /dev/null +++ b/crates/persistence/tests/search/conditional_patch_suite.rs @@ -0,0 +1,304 @@ +//! Backend-agnostic conditional patch suite (issue #1406). +//! +//! `ConditionalStorage::conditional_patch` is one provided implementation — +//! resolve the criteria, read, `If-Match`, apply the shared patch, `update` +//! with compare-and-swap — that every backend reaches through its own +//! `resolve_conditional_matches`. This drives it on each backend: the patch +//! formats, the three match outcomes, the precondition, and the refusals that +//! must leave storage untouched. +//! +//! Opens with a positive control: a backend built without the spec search +//! parameters does not know `identifier`, never matches anything, and would +//! turn every "refused" assertion below into a vacuous no-match. +//! +//! Included by `#[path]` into each backend's test binary, the same arrangement +//! as `conditional_if_match_suite.rs`. + +#![allow(dead_code)] + +use serde_json::{Value, json}; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{ + ConditionalInteraction, ConditionalPatchResult, ConditionalStorage, EntityTagPrecondition, + PatchError, PatchFormat, ResourceStorage, SearchProvider, +}; +use helios_persistence::error::{ConcurrencyError, StorageError, ValidationError}; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; +use helios_persistence::types::{SearchParamType, SearchParameter, SearchQuery, SearchValue}; + +const CRITERIA: &str = "identifier=ne123"; +const NOBODY: &str = "identifier=nobody"; +/// Both `twin-a` and `twin-b`. +const TWINS: &str = "identifier=twin"; + +const ABSENT: &EntityTagPrecondition = &EntityTagPrecondition::Absent; + +fn patient(id: &str, family: &str, identifier: &str) -> Value { + json!({ + "resourceType": "Patient", + "id": id, + "active": false, + "name": [{"family": family}], + "identifier": [{"system": "http://example.org/mrn", "value": identifier}] + }) +} + +/// `(version, active, family)` of a Patient, or how it went missing. +async fn state_of(backend: &S, tenant: &TenantContext, id: &str) -> String { + match backend.read(tenant, "Patient", id).await { + Ok(Some(stored)) => format!( + "v{} {} {}", + stored.version_id(), + stored.content()["active"], + stored.content()["name"][0]["family"] + .as_str() + .unwrap_or("?") + ), + Ok(None) => "".to_string(), + Err(e) => format!("<{e}>"), + } +} + +fn patch_error(result: Result) -> PatchError { + match result { + Err(StorageError::Validation(ValidationError::Patch(e))) => e, + other => panic!("expected a typed patch error, got {other:?}"), + } +} + +/// Conditional patch, end to end, on one backend. +pub async fn conditional_patch_resolves_gates_applies_and_swaps(backend: &S, base: &str) +where + S: ResourceStorage + ConditionalStorage + SearchProvider, +{ + assert!( + backend.supports_conditional(ConditionalInteraction::Patch), + "the backend must declare what this suite shows it serves" + ); + + let t = TenantContext::new( + TenantId::new(format!("{base}-patch")), + TenantPermissions::full_access(), + ); + for (id, family, identifier) in [ + ("target", "Neal", "ne123"), + ("decoy", "Smith", "mrn-1"), + ("twin-a", "Twin", "twin"), + ("twin-b", "Twin", "twin"), + ] { + backend + .create( + &t, + "Patient", + patient(id, family, identifier), + FhirVersion::default(), + ) + .await + .expect("seed patient"); + } + + // Positive control: a plain search on the criteria's parameter finds the + // target, so a `NoMatch` below is about the criteria, not the registry. + let found = backend + .search( + &t, + &SearchQuery::new("Patient").with_parameter(SearchParameter { + name: "identifier".to_string(), + param_type: SearchParamType::Token, + values: vec![SearchValue::eq("ne123")], + ..Default::default() + }), + ) + .await + .expect("positive control search"); + let ids: Vec<&str> = found.resources.items.iter().map(|r| r.id()).collect(); + assert_eq!( + ids, + ["target"], + "positive control: is the backend built with the spec search parameters?" + ); + + let untouched = "v1 false Neal"; + + // ---- refusals: nothing is written -------------------------------------- + let activate = PatchFormat::JsonPatch(json!([ + {"op": "replace", "path": "/active", "value": true} + ])); + + let result = backend + .conditional_patch(&t, "Patient", NOBODY, &activate, ABSENT) + .await; + assert!( + matches!(result, Ok(ConditionalPatchResult::NoMatch)), + "{result:?}" + ); + + let result = backend + .conditional_patch(&t, "Patient", TWINS, &activate, ABSENT) + .await; + assert!( + matches!(result, Ok(ConditionalPatchResult::MultipleMatches(2))), + "{result:?}" + ); + assert_eq!(state_of(backend, &t, "twin-a").await, "v1 false Twin"); + assert_eq!(state_of(backend, &t, "twin-b").await, "v1 false Twin"); + + let stale = EntityTagPrecondition::parse(["W/\"7\""]).expect("well-formed If-Match"); + let result = backend + .conditional_patch(&t, "Patient", CRITERIA, &activate, &stale) + .await; + assert!( + matches!( + result, + Err(StorageError::Concurrency( + ConcurrencyError::OptimisticLockFailure { .. } + )) + ), + "stale If-Match: {result:?}" + ); + + for (element, patch) in [ + ( + "resourceType", + PatchFormat::JsonPatch( + json!([{"op": "replace", "path": "/resourceType", "value": "Person"}]), + ), + ), + ( + "resourceType", + PatchFormat::MergePatch(json!({"resourceType": "Person"})), + ), + ( + "id", + PatchFormat::JsonPatch(json!([{"op": "replace", "path": "/id", "value": "other"}])), + ), + ("id", PatchFormat::MergePatch(json!({"id": "other"}))), + ] { + let result = backend + .conditional_patch(&t, "Patient", CRITERIA, &patch, ABSENT) + .await; + assert_eq!( + patch_error(result), + PatchError::ImmutableElement { element }, + "{patch:?}" + ); + } + + // A failed `test` stops the whole document: the `replace` before it is + // not written either. + let result = backend + .conditional_patch( + &t, + "Patient", + CRITERIA, + &PatchFormat::JsonPatch(json!([ + {"op": "replace", "path": "/name/0/family", "value": "Half"}, + {"op": "test", "path": "/active", "value": true} + ])), + ABSENT, + ) + .await; + assert!( + matches!(patch_error(result), PatchError::TestFailed { .. }), + "failed test op" + ); + + let result = backend + .conditional_patch( + &t, + "Patient", + CRITERIA, + &PatchFormat::JsonPatch(json!({"not": "a patch"})), + ABSENT, + ) + .await; + assert!( + matches!(patch_error(result), PatchError::MalformedDocument { .. }), + "malformed document" + ); + + let result = backend + .conditional_patch( + &t, + "Patient", + CRITERIA, + &PatchFormat::JsonPatch(json!([{"op": "replace", "path": "/nope/deeper", "value": 1}])), + ABSENT, + ) + .await; + assert!( + matches!(patch_error(result), PatchError::OperationFailed { .. }), + "unresolvable path" + ); + + // FHIRPath Patch used to be a stub that changed nothing and still wrote a + // new version. It is refused, and no version is written. + let result = backend + .conditional_patch( + &t, + "Patient", + CRITERIA, + &PatchFormat::FhirPathPatch(json!({ + "resourceType": "Parameters", + "parameter": [{"name": "operation", "part": [ + {"name": "type", "valueCode": "replace"}, + {"name": "path", "valueString": "Patient.name[0].family"}, + {"name": "value", "valueString": "Changed"} + ]}] + })), + ABSENT, + ) + .await; + assert!( + matches!(patch_error(result), PatchError::UnsupportedFormat { .. }), + "FHIRPath Patch" + ); + + assert_eq!(state_of(backend, &t, "target").await, untouched); + + // ---- one match: JSON Patch, with a satisfied If-Match ------------------- + let current = EntityTagPrecondition::parse(["W/\"1\""]).expect("well-formed If-Match"); + let result = backend + .conditional_patch( + &t, + "Patient", + CRITERIA, + &PatchFormat::JsonPatch(json!([ + {"op": "test", "path": "/active", "value": false}, + {"op": "replace", "path": "/active", "value": true} + ])), + ¤t, + ) + .await; + match &result { + Ok(ConditionalPatchResult::Patched(stored)) => { + assert_eq!(stored.id(), "target"); + assert_eq!(stored.version_id(), "2"); + assert_eq!(stored.content()["active"], json!(true)); + assert_eq!(stored.content()["resourceType"], json!("Patient")); + assert_eq!(stored.content()["id"], json!("target")); + } + other => panic!("JSON Patch on one match: {other:?}"), + } + assert_eq!(state_of(backend, &t, "target").await, "v2 true Neal"); + + // ---- one match: Merge Patch. The criteria still resolve after the first + // patch, so the index followed the write. ------------------------------- + let result = backend + .conditional_patch( + &t, + "Patient", + CRITERIA, + &PatchFormat::MergePatch(json!({"name": [{"family": "Merged"}], "active": null})), + ABSENT, + ) + .await; + assert!( + matches!(&result, Ok(ConditionalPatchResult::Patched(s)) if s.version_id() == "3"), + "Merge Patch on one match: {result:?}" + ); + assert_eq!(state_of(backend, &t, "target").await, "v3 null Merged"); + + assert_eq!(state_of(backend, &t, "decoy").await, "v1 false Smith"); +} diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 33ddd74c7..463c1de86 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -98,6 +98,23 @@ async fn sqlite_conditional_writers_with_the_same_if_match_admit_one() { .await; } +/// The backend-agnostic conditional patch suite (#1406). Same `#[path]` +/// arrangement. +#[path = "search/conditional_patch_suite.rs"] +mod conditional_patch_suite; + +/// #1406: conditional patch is the trait's provided implementation over the +/// backend's criteria resolver and the shared patch applier. +#[tokio::test] +async fn sqlite_conditional_patch() { + let backend = create_backend(); + conditional_patch_suite::conditional_patch_resolves_gates_applies_and_swaps( + &backend, + "cond-patch-1406", + ) + .await; +} + fn create_backend() -> SqliteBackend { // Configure with data directory to load spec SearchParameters // CARGO_MANIFEST_DIR for tests is crates/persistence diff --git a/crates/rest/Cargo.toml b/crates/rest/Cargo.toml index defc6b46c..72f1d963e 100644 --- a/crates/rest/Cargo.toml +++ b/crates/rest/Cargo.toml @@ -94,7 +94,6 @@ chrono.workspace = true # Utilities uuid = { version = "1", features = ["v4", "serde"] } url = "2.5" -json-patch = "3" futures = "0.3" dashmap = "6" diff --git a/crates/rest/src/error.rs b/crates/rest/src/error.rs index 590e5c5c0..0c224e847 100644 --- a/crates/rest/src/error.rs +++ b/crates/rest/src/error.rs @@ -55,6 +55,7 @@ use axum::{ response::{IntoResponse, Response}, }; use helios_fhir::FhirVersion; +use helios_persistence::core::PatchError; use helios_persistence::error::{ BackendError, ConcurrencyError, ResourceError, SearchError, StorageError, TenantError, TransactionError, ValidationError, @@ -954,6 +955,31 @@ impl From for RestError { ValidationError::InvalidReference { reference, message } => RestError::BadRequest { message: format!("The reference '{}' is invalid: {}.", reference, message), }, + ValidationError::Patch(e) => e.into(), + } + } +} + +/// One mapping for both patch endpoints: `PATCH [type]/[id]` applies the patch +/// in the handler, `PATCH [type]?criteria` inside the storage layer, and both +/// get their [`PatchError`] from the same applier. +impl From for RestError { + fn from(err: PatchError) -> Self { + match err { + PatchError::UnsupportedFormat { format } => RestError::NotImplemented { + feature: format.to_string(), + }, + // #1393 asks for `422` here: the document is well-formed and + // applies, the resource is just not in the state it tested for. + // This arm is the only place that decides it. + PatchError::TestFailed { .. } => RestError::BadRequest { + message: err.to_string(), + }, + PatchError::MalformedDocument { .. } + | PatchError::OperationFailed { .. } + | PatchError::ImmutableElement { .. } => RestError::BadRequest { + message: err.to_string(), + }, } } } diff --git a/crates/rest/src/handlers/patch.rs b/crates/rest/src/handlers/patch.rs index b2060b396..5320fdc40 100644 --- a/crates/rest/src/handlers/patch.rs +++ b/crates/rest/src/handlers/patch.rs @@ -118,17 +118,10 @@ where }); } - // Apply the patch - let patched_content = apply_patch(existing.content(), &patch_format)?; - - // Validate that resourceType wasn't changed - if let Some(body_type) = patched_content.get("resourceType").and_then(|v| v.as_str()) { - if body_type != resource_type { - return Err(RestError::BadRequest { - message: "Cannot change resourceType via patch".to_string(), - }); - } - } + // Apply the patch: the applier `PATCH [type]?criteria` uses inside the + // storage layer (#1406). It refuses a patch that changes `resourceType` or + // `id`, and FHIRPath Patch (`501`). + let patched_content = helios_persistence::core::apply_patch(existing.content(), &patch_format)?; // Update the resource let stored = state @@ -266,11 +259,14 @@ where let patch_format = parse_patch_format(content_type, &body)?; - // Hold the patch to what `patch_handler` accepts *before* the backend sees - // it: the backend applies the document itself, and its FHIRPath Patch is a - // stub that ignores every path but `Type.element` and still writes a new - // version. - check_conditional_patch(&resource_type, &patch_format)?; + // FHIRPath Patch is not implemented. The storage layer's applier says so + // too, but only once the criteria have resolved to one resource; refused + // here, the answer does not depend on what the criteria match. + if matches!(patch_format, PatchFormat::FhirPathPatch(_)) { + return Err(RestError::NotImplemented { + feature: "FHIRPath Patch".to_string(), + }); + } let result = state .storage() @@ -322,45 +318,6 @@ where } } -/// The refusals [`patch_handler`] makes while or after applying a patch, made -/// up front for a conditional patch, where the backend does the applying. -/// -/// * FHIRPath Patch is not implemented (`501`), exactly as on the instance -/// endpoint. -/// * `resourceType` cannot be patched (`400`). Backends re-assert the stored -/// type and id on every update, so a patch naming them could not corrupt the -/// row — it would be silently undone, and answered with a `200`. -fn check_conditional_patch(resource_type: &str, patch: &PatchFormat) -> RestResult<()> { - let changes_type = match patch { - PatchFormat::FhirPathPatch(_) => { - return Err(RestError::NotImplemented { - feature: "FHIRPath Patch".to_string(), - }); - } - PatchFormat::JsonPatch(operations) => operations.as_array().is_some_and(|ops| { - ops.iter().any(|op| { - // `test` and the source of a `copy` only read the element. - let writes = - |key: &str| op.get(key).and_then(Value::as_str) == Some("/resourceType"); - match op.get("op").and_then(Value::as_str) { - Some("test") => false, - Some("move") => writes("path") || writes("from"), - _ => writes("path"), - } - }) - }), - PatchFormat::MergePatch(merge_doc) => merge_doc - .get("resourceType") - .is_some_and(|t| t.as_str() != Some(resource_type)), - }; - if changes_type { - return Err(RestError::BadRequest { - message: "Cannot change resourceType via patch".to_string(), - }); - } - Ok(()) -} - /// Parses the patch format from Content-Type and body. fn parse_patch_format(content_type: &str, body: &Bytes) -> RestResult { let patch_value: Value = serde_json::from_slice(body).map_err(|e| RestError::BadRequest { @@ -387,36 +344,6 @@ fn parse_patch_format(content_type: &str, body: &Bytes) -> RestResult RestResult { - match patch { - PatchFormat::JsonPatch(operations) => { - let patch: json_patch::Patch = - serde_json::from_value(operations.clone()).map_err(|e| RestError::BadRequest { - message: format!("Invalid JSON Patch: {}", e), - })?; - - let mut resource = resource.clone(); - json_patch::patch(&mut resource, &patch).map_err(|e| RestError::BadRequest { - message: format!("Failed to apply JSON Patch: {}", e), - })?; - - Ok(resource) - } - PatchFormat::MergePatch(merge_doc) => { - let mut resource = resource.clone(); - json_patch::merge(&mut resource, merge_doc); - Ok(resource) - } - PatchFormat::FhirPathPatch(_params) => { - // FHIRPath Patch is more complex and requires FHIRPath evaluation - Err(RestError::NotImplemented { - feature: "FHIRPath Patch".to_string(), - }) - } - } -} - /// Builds the response for a successful patch. fn build_patch_response( stored: &helios_persistence::types::StoredResource, From 1c989d2fcdf7000cd7726f24b74c95f1acc7f7df Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 08:59:32 -0400 Subject: [PATCH 03/10] fix(mongodb): serve :of-type, reference :identifier, :[type], :above/:below MongoDB refused three standard search modifiers that SQLite, PostgreSQL and Elasticsearch serve, so the same request succeeded or failed with HFS_STORAGE_BACKEND: token `:of-type`, reference `:identifier` and the reference `:[type]` qualifier all fell into the `UnsupportedModifier` catch-all of their filter builder. Reference `:above`/`:below` were refused one step earlier, by `validate_query_support`. Root cause: the builders were never written. Nothing was missing from the index - the writer has always stored `value_identifier_type_system` / `value_identifier_type_code` on identifier rows (and the partial index `idx_search_identifier_type_v2` exists for them), so no reindex is needed. - `:of-type` compares type system, type code and value; an empty part is not compared (as SQLite/PostgreSQL); anything but three parts matches nothing rather than dropping the condition. - `:[type]` is the qualified reference: `subject:Patient=1` builds the very filter `subject=Patient/1` builds, so it cannot match `Group/1`. A value naming another type matches nothing. Qualified reference values are now version-agnostic (`strip_reference_version`), like every other backend. - `:identifier` has the SQLite/PostgreSQL meaning (the reference's target carries the identifier). A filter document cannot join, so `matching_resource_ids` resolves the targets first - tenant-scoped, and bounded to the parameter's declared target types so the token index serves it - and turns the parameter into one index-bounded `$in` of `Type/id` plus an anchored `_history` regex each. More than 10 000 targets is refused (`TooManyResults`), never truncated. - reference `:above`/`:below` reuse the uri shapes. `modifiers_for_type` advertises what is now served, and the new cross-backend `modifier_parity_suite` walks every modifier `SearchModifier::is_valid_for` allows on every parameter type on all four backends, with each backend's known differences stated explicitly so no backend can silently fall behind again. Fixes #1408 --- .../src/backends/mongodb/backend.rs | 72 +- .../src/backends/mongodb/search_impl.rs | 661 ++++++++++++- .../persistence/tests/elasticsearch_tests.rs | 87 ++ crates/persistence/tests/mongodb_tests.rs | 21 + crates/persistence/tests/postgres_tests.rs | 54 ++ crates/persistence/tests/search/mod.rs | 3 + .../tests/search/modifier_parity_suite.rs | 904 ++++++++++++++++++ .../tests/search/modifier_tests.rs | 52 + 8 files changed, 1810 insertions(+), 44 deletions(-) create mode 100644 crates/persistence/tests/search/modifier_parity_suite.rs diff --git a/crates/persistence/src/backends/mongodb/backend.rs b/crates/persistence/src/backends/mongodb/backend.rs index 52b0e495b..548839b15 100644 --- a/crates/persistence/src/backends/mongodb/backend.rs +++ b/crates/persistence/src/backends/mongodb/backend.rs @@ -872,21 +872,31 @@ impl MongoBackend { /// rejected is a visible 400 rather than the old silent wrong answer; /// narrowing it would need a name-aware capability path. /// - /// `:above`/`:below` are now implemented for `uri` (#1002): - /// `build_uri_filter` resolves them segment-aware, mirroring SQLite and - /// Elasticsearch, so they are advertised there. `:in`/`:not-in` stay + /// `:above`/`:below` are implemented for `uri` (#1002) and `reference` + /// (#1408): `build_uri_filter` / `build_reference_filter` resolve them + /// segment-aware, mirroring SQLite and Elasticsearch, so they are + /// advertised there. Token `:of-type` and reference `:identifier` are + /// served too (#1408) — the latter by `resolve_reference_identifier` in + /// `matching_resource_ids`, ahead of the filter builders — as is the + /// reference `:[type]` qualifier, which like on the other backends is not + /// a named modifier and so is not listed. `:in`/`:not-in` stay /// unimplemented and unadvertised for every type (rejected outright by - /// `validate_query_support`), as do token `:above`/`:below`/`:of-type`/ - /// `:text-advanced` and reference `:identifier`/`:above`/`:below`/ - /// `:text-advanced` — token/reference `:above`/`:below` need terminology - /// subsumption and hierarchy resolution respectively, neither of which is - /// implemented — each still hits an `UnsupportedModifier` catch-all in - /// its type-specific builder. + /// `validate_query_support`), as do token `:above`/`:below` (terminology + /// subsumption) and `:text-advanced` on token and reference — each still + /// hits an `UnsupportedModifier` catch-all in its type-specific builder. pub(super) fn modifiers_for_type(param_type: SearchParamType) -> Vec<&'static str> { match param_type { SearchParamType::String => vec!["exact", "contains", "text", "missing"], - SearchParamType::Token => vec!["text", "code-text", "not", "missing"], - SearchParamType::Reference => vec!["contains", "text", "code-text", "missing"], + SearchParamType::Token => vec!["text", "code-text", "of-type", "not", "missing"], + SearchParamType::Reference => vec![ + "identifier", + "contains", + "text", + "code-text", + "below", + "above", + "missing", + ], SearchParamType::Uri => vec!["exact", "contains", "below", "above", "missing"], SearchParamType::Date | SearchParamType::Number | SearchParamType::Quantity => { vec!["missing"] @@ -971,20 +981,25 @@ mod capability_tests { // Token honors text/code-text but not the non-spec :code; :not and // :missing ARE honored (#881, generically in `matching_resource_ids`), - // but :of-type is still rejected by `build_token_filter`. + // and so is :of-type (#1408). Terminology-backed modifiers are not. let t = MongoBackend::modifiers_for_type(SearchParamType::Token); assert!(!t.contains(&"code")); assert!(t.contains(&"code-text")); assert!(t.contains(&"not")); assert!(t.contains(&"missing")); - assert!(!t.contains(&"of-type")); + assert!(t.contains(&"of-type")); + assert!(!t.contains(&"in")); + assert!(!t.contains(&"above")); - // Reference honors contains/text/code-text/missing but not :identifier. + // Reference honors contains/text/code-text/missing, and :identifier + // and :above/:below (#1408). let r = MongoBackend::modifiers_for_type(SearchParamType::Reference); assert!(r.contains(&"contains")); assert!(r.contains(&"text")); assert!(r.contains(&"missing")); - assert!(!r.contains(&"identifier")); + assert!(r.contains(&"identifier")); + assert!(r.contains(&"above")); + assert!(r.contains(&"below")); // Uri honors exact/contains/missing and :above/:below (#1002, // segment-aware in build_uri_filter). @@ -1090,8 +1105,8 @@ mod capability_tests { // `:not-in` outright for every parameter type; advertising // either would be a straightforward regression back to // over-promising. `:above`/`:below` are rejected for every - // type EXCEPT uri (#1002: build_uri_filter resolves them - // there, segment-aware). + // type EXCEPT uri (#1002) and reference (#1408), whose + // builders resolve them segment-aware. assert!( !matches!(modifier_str, "in" | "not-in"), "{param_type} advertises `{modifier_str}`, which \ @@ -1099,9 +1114,12 @@ mod capability_tests { ); assert!( !matches!(modifier_str, "above" | "below") - || param_type == SearchParamType::Uri, + || matches!( + param_type, + SearchParamType::Uri | SearchParamType::Reference + ), "{param_type} advertises `{modifier_str}`, which \ - validate_query_support rejects for every type except uri" + validate_query_support rejects for every type except uri and reference" ); // The advertised string must be a real, round-trippable @@ -1112,12 +1130,19 @@ mod capability_tests { SearchModifier::parse does not recognize" ) }); + // `Display` writes `of-type` in its legacy camelCase, which + // `parse` reads back; every other spelling is its own. assert_eq!( - parsed.to_string(), - modifier_str, + SearchModifier::parse(&parsed.to_string()), + Some(parsed.clone()), "{param_type}'s advertised `{modifier_str}` does not round-trip \ through SearchModifier::parse/Display" ); + assert!( + parsed.to_string() == modifier_str || modifier_str == "of-type", + "{param_type} advertises `{modifier_str}`, which is not the \ + spelling SearchModifier writes" + ); // `:missing` and `:not` are resolved generically in // `matching_resource_ids` *before* any type-specific value @@ -1126,8 +1151,11 @@ mod capability_tests { // than setting it — setting it would wrongly fail (e.g. token's // `build_token_filter` has no `Missing`/`Not` arm and would hit // its `Some(other) => Err(UnsupportedModifier)` catch-all). + // Reference `:identifier` is resolved there too (#1408), by + // `resolve_reference_identifier`: it needs the database, which + // a filter builder does not have, so the builder never sees it. let probe_modifier = match modifier_str { - "missing" | "not" => None, + "missing" | "not" | "identifier" => None, other => Some(SearchModifier::parse(other).unwrap()), }; diff --git a/crates/persistence/src/backends/mongodb/search_impl.rs b/crates/persistence/src/backends/mongodb/search_impl.rs index e67b5a3e8..9a1c0838d 100644 --- a/crates/persistence/src/backends/mongodb/search_impl.rs +++ b/crates/persistence/src/backends/mongodb/search_impl.rs @@ -128,6 +128,11 @@ const PROBE_ROW_LIMIT: u64 = 100_000; // 300k × ~45 bytes/UUID ≈ 13.5 MB — safely under the 16 MB BSON document cap. const MAX_RESULT_ID_SET: usize = 300_000; +/// Targets a reference `:identifier` search may resolve to (#1408). They +/// travel in one `$in`, two entries each; 10 000 stays near 1 MB, well under +/// the 16 MB BSON limit. Past it the search is refused, never truncated. +const MAX_IDENTIFIER_TARGETS: usize = 10_000; + async fn collect_documents(mut cursor: Cursor) -> StorageResult> { let mut docs = Vec::new(); while cursor @@ -1371,10 +1376,11 @@ impl MongoBackend { for param in &query.parameters { // `:in`/`:not-in` are unsupported for every parameter type. // `:above`/`:below` are served for `uri` by `build_uri_filter` - // (segment-aware, mirroring SQLite/Elasticsearch, #1002) but stay - // rejected for token/reference: token `:above`/`:below` need - // terminology subsumption and reference `:above`/`:below` need - // hierarchy resolution, neither of which is implemented here. + // (segment-aware, mirroring SQLite/Elasticsearch, #1002) and for + // `reference` by `build_reference_filter` (the same URL/path + // hierarchy on the stored reference, #1408) but stay rejected for + // token: token `:above`/`:below` need terminology subsumption, + // which is not implemented here. // Every modifier on a composite (#1206) is rejected here except // `:missing`: `composite_search::component_param` hardcodes // `modifier: None` when building each component's filter, so any @@ -1395,12 +1401,14 @@ impl MongoBackend { ) || (matches!( param.modifier, Some(SearchModifier::Above) | Some(SearchModifier::Below) - ) && param.param_type != SearchParamType::Uri) - || (param.param_type == SearchParamType::Composite - && param - .modifier - .as_ref() - .is_some_and(|m| !matches!(m, SearchModifier::Missing))); + ) && !matches!( + param.param_type, + SearchParamType::Uri | SearchParamType::Reference + )) || (param.param_type == SearchParamType::Composite + && param + .modifier + .as_ref() + .is_some_and(|m| !matches!(m, SearchModifier::Missing))); if modifier_unsupported { return Err(StorageError::Search(SearchError::UnsupportedModifier { modifier: param @@ -1859,6 +1867,31 @@ impl MongoBackend { .await; } + // Reference `:identifier` (#1408) names its targets by their + // identifier, which a filter document cannot join on: each such + // parameter is resolved to its complete filter here, once, and no + // target at all empties the search (parameters are ANDed). + let mut identifier_filters: HashMap = HashMap::new(); + for (i, param) in normal.iter().enumerate() { + if param.param_type == SearchParamType::Reference + && matches!(param.modifier, Some(SearchModifier::Identifier)) + { + match self + .resolve_reference_identifier(&search_index, tenant_id, resource_type, param) + .await? + { + Some(filter) => identifier_filters.insert(i, filter), + None => return Ok(Some(HashSet::new())), + }; + } + } + let normal_filter = |i: usize| -> StorageResult { + match identifier_filters.get(&i) { + Some(filter) => Ok(filter.clone()), + None => self.build_search_index_filter(tenant_id, resource_type, normal[i]), + } + }; + // #1206: a composite's probe must run over its component filters // regardless of how many normal params there are — unlike a plain // param, its own filter isn't a single document to count, and the @@ -1892,7 +1925,7 @@ impl MongoBackend { } } } else { - let filter = self.build_search_index_filter(tenant_id, resource_type, param)?; + let filter = normal_filter(i)?; let count = search_index .count_documents(filter) .limit(PROBE_ROW_LIMIT) @@ -1917,7 +1950,7 @@ impl MongoBackend { let driver_filter = if let Some((filter, _)) = composite_probes.remove(&driver_idx) { filter } else { - self.build_search_index_filter(tenant_id, resource_type, normal[driver_idx])? + normal_filter(driver_idx)? }; let mut driver_cursor = search_index @@ -1969,8 +2002,7 @@ impl MongoBackend { if i == driver_idx { continue; } - let param_filter = - self.build_search_index_filter(tenant_id, resource_type, param)?; + let param_filter = normal_filter(i)?; let bounded = doc! { "$and": [ param_filter, @@ -2787,6 +2819,7 @@ impl MongoBackend { "value_token_display": { "$regex": regex, "$options": "i" } }); } + Some(SearchModifier::OfType) => return Ok(Self::build_of_type_filter(&value.value)), Some(other) => { return Err(StorageError::Search(SearchError::UnsupportedModifier { modifier: other.to_string(), @@ -2815,6 +2848,178 @@ impl MongoBackend { } } + /// Builds the `:of-type` predicate (#1408): `type-system|type-code|value` + /// against the identifier row's `value_identifier_type_system` / + /// `value_identifier_type_code` / `value_token_code`. + /// + /// An empty part is not compared, as on SQLite and PostgreSQL + /// (`|MR|12345` is "typed MR, in any system"). Anything but three parts + /// matches nothing, as on PostgreSQL and Elasticsearch: the spec requires + /// all three, and guessing which one is absent would over-match. A row + /// written without the type fields — an identifier with no `type`, or one + /// indexed before they were stored — simply fails the equality. + fn build_of_type_filter(value: &str) -> Document { + let parts: Vec<&str> = value.splitn(3, '|').collect(); + let [type_system, type_code, identifier_value] = parts[..] else { + return Self::match_nothing(); + }; + + let mut filter = Document::new(); + if !identifier_value.is_empty() { + filter.insert("value_token_code", identifier_value); + } + if !type_system.is_empty() { + filter.insert("value_identifier_type_system", type_system); + } + if !type_code.is_empty() { + filter.insert("value_identifier_type_code", type_code); + } + if filter.is_empty() { + return Self::match_nothing(); + } + filter + } + + /// A `search_index` predicate no row satisfies. + fn match_nothing() -> Document { + doc! { "resource_id": { "$in": Bson::Array(Vec::new()) } } + } + + /// The `Type/id` (or absolute URL) a `:[type]` reference search names, or + /// `None` when the value names another type (`subject:Patient=Group/1`), + /// which no reference can satisfy. + fn typed_reference(type_name: &str, value: &str) -> Option { + let base = strip_reference_version(value); + if !base.contains('/') { + return Some(format!("{type_name}/{base}")); + } + let mut segments = base.rsplit('/'); + let _id = segments.next(); + (segments.next() == Some(type_name)).then(|| base.to_string()) + } + + /// The identifier-row predicate of one `:identifier` value, in the token + /// grammar SQLite and PostgreSQL use for it: `system|value`, `system|`, + /// `|value` (no system) or a bare value. + fn identifier_predicate(value: &str) -> Document { + match value.split_once('|') { + Some(("", code)) => doc! { + "value_token_system": { "$in": [Bson::Null, Bson::String(String::new())] }, + "value_token_code": code, + }, + Some((system, "")) => doc! { "value_token_system": system }, + Some((system, code)) => doc! { + "value_token_system": system, + "value_token_code": code, + }, + None => doc! { "value_token_code": value }, + } + } + + /// Resolves a reference `:identifier` parameter (#1408) into its complete + /// `search_index` filter, or `None` when no resource carries the + /// identifier — and so nothing can match. + /// + /// Same meaning as SQLite and PostgreSQL give it: the reference's *target* + /// has the identifier. Those backends express it as a sub-select on the + /// target's `identifier` rows; MongoDB has no join a filter document can + /// carry, so the targets are read first and the parameter becomes one + /// `$in` over their `Type/id` — each with an anchored `_history` regex, so + /// a versioned reference matches too and every entry stays index-bounded. + /// + /// The lookup is scoped to the tenant, which is load-bearing (another + /// tenant's identifiers must not decide this tenant's matches), and to the + /// parameter's declared target types, which is what lets it use the token + /// index: every value index leads with `resource_type`. + async fn resolve_reference_identifier( + &self, + search_index: &mongodb::Collection, + tenant_id: &str, + resource_type: &str, + param: &SearchParameter, + ) -> StorageResult> { + let predicates: Vec = param + .values + .iter() + .map(|value| Bson::Document(Self::identifier_predicate(&value.value))) + .collect(); + + let targets: Vec = { + let registry = self.tenant_registry(tenant_id); + let registry = registry.read(); + crate::search::resolve_param_targets(®istry, resource_type, ¶m.name) + }; + + let mut lookup = doc! { "tenant_id": tenant_id }; + if !targets.is_empty() { + lookup.insert("resource_type", doc! { "$in": targets }); + } + lookup.insert("param_name", "identifier"); + lookup.insert("$or", Bson::Array(predicates)); + + let mut cursor = search_index + .find(lookup) + .projection(doc! { "resource_type": 1, "resource_id": 1, "_id": 0 }) + .await + .or_query_error("Failed to resolve :identifier targets")?; + + let mut found: std::collections::BTreeSet = std::collections::BTreeSet::new(); + loop { + let batch = read_cursor_batch(&mut cursor, CANDIDATE_BATCH_SIZE).await?; + let read = batch.len(); + for row in &batch { + if let (Ok(target_type), Ok(target_id)) = + (row.get_str("resource_type"), row.get_str("resource_id")) + { + found.insert(format!("{target_type}/{target_id}")); + } + } + if found.len() > MAX_IDENTIFIER_TARGETS { + return Err(StorageError::Search(SearchError::TooManyResults { + count: found.len(), + max: MAX_IDENTIFIER_TARGETS, + })); + } + if read < CANDIDATE_BATCH_SIZE { + break; + } + } + if found.is_empty() { + return Ok(None); + } + + Ok(Some(Self::identifier_targets_filter( + tenant_id, + resource_type, + ¶m.name, + found, + ))) + } + + /// The filter a resolved `:identifier` parameter becomes: the referencing + /// rows whose `value_reference` is one of `targets`, at any version. + fn identifier_targets_filter( + tenant_id: &str, + resource_type: &str, + param_name: &str, + targets: impl IntoIterator, + ) -> Document { + let mut references: Vec = Vec::new(); + for target in targets { + references.push(Bson::RegularExpression(bson::Regex { + pattern: format!("^{}/_history/", regex_escape(&target)), + options: String::new(), + })); + references.push(Bson::String(target)); + } + doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "param_name": param_name, + "value_reference": { "$in": references }, + } + } + /// Builds the `search_index` filter for a `Reference`-typed parameter. /// /// #1083: bare-id search (`subject=123`) used to emit a single @@ -2875,20 +3080,63 @@ impl MongoBackend { }); } - if let Some(modifier) = ¶m.modifier { - return Err(StorageError::Search(SearchError::UnsupportedModifier { - modifier: modifier.to_string(), - param_type: "reference".to_string(), - })); + // `:below` / `:above` - URL/path hierarchy on the stored reference, + // the shapes `build_uri_filter` uses (#1408). Canonical `|version` + // comparison is not implemented, as on the other backends. + if matches!(param.modifier.as_ref(), Some(SearchModifier::Below)) { + return Ok(doc! { + "value_reference": { + "$regex": format!( + "^{}(/|$)", + regex_escape(value.value.trim_end_matches('/')) + ) + } + }); } + if matches!(param.modifier.as_ref(), Some(SearchModifier::Above)) { + return Ok(doc! { + "value_reference": { + "$in": crate::search::compute_parent_uris(&value.value) + } + }); + } + + // `:[type]` (#1408): `subject:Patient=123` is `subject=Patient/123`, + // so it takes the qualified branch below and can never match + // `Group/123`. + let typed; + let reference = match param.modifier.as_ref() { + None => value.value.as_str(), + Some(SearchModifier::Type(type_name)) => { + match Self::typed_reference(type_name, &value.value) { + Some(reference) => { + typed = reference; + typed.as_str() + } + None => return Ok(Self::match_nothing()), + } + } + // `:identifier` is resolved by `resolve_reference_identifier` + // before any filter is built; a path that does not do that has + // no way to honour it. + Some(modifier) => { + return Err(StorageError::Search(SearchError::UnsupportedModifier { + modifier: modifier.to_string(), + param_type: "reference".to_string(), + })); + } + }; - if value.value.contains('/') { + if reference.contains('/') { + // Version-agnostic, like every other backend: a versioned search + // value names the resource, not one version of it. + let base = strip_reference_version(reference); return Ok(doc! { "$or": [ - { "value_reference": &value.value }, + { "value_reference": base }, { "value_reference": { - "$regex": format!("^{}/_history/", regex_escape(&value.value)) + "$regex": format!("^{}/_history/", regex_escape(base)) } } ] @@ -5557,3 +5805,372 @@ mod cursor_keyset_tests { )); } } + +/// #1408: token `:of-type`, reference `:[type]`, `:identifier`, `:above` and +/// `:below` — refused as unsupported modifiers before. +#[cfg(test)] +mod modifier_parity_filter_tests { + use super::*; + use crate::backends::mongodb::MongoBackendConfig; + + const V2_0203: &str = "http://terminology.hl7.org/CodeSystem/v2-0203"; + + fn backend() -> MongoBackend { + MongoBackend::new(MongoBackendConfig::default()).unwrap() + } + + fn param( + name: &str, + param_type: SearchParamType, + modifier: SearchModifier, + values: &[&str], + ) -> SearchParameter { + SearchParameter { + name: name.to_string(), + param_type, + modifier: Some(modifier), + values: values.iter().map(|v| SearchValue::eq(*v)).collect(), + chain: vec![], + components: vec![], + } + } + + fn filter(param: &SearchParameter) -> Document { + backend() + .build_search_index_filter("t1", "Observation", param) + .unwrap() + } + + fn subject(modifier: SearchModifier, value: &str) -> Document { + filter(¶m( + "subject", + SearchParamType::Reference, + modifier, + &[value], + )) + } + + fn patient() -> SearchModifier { + SearchModifier::Type("Patient".to_string()) + } + + /// The envelope every filter carries, so a predicate is never evaluated + /// against another tenant's, type's or parameter's rows. + fn envelope(name: &str) -> Document { + doc! { "tenant_id": "t1", "resource_type": "Observation", "param_name": name } + } + + fn with(mut envelope: Document, predicate: Document) -> Document { + envelope.extend(predicate); + envelope + } + + #[test] + fn the_gate_admits_the_new_modifiers_and_still_refuses_terminology() { + let backend = backend(); + let admitted = [ + param( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &["s|c|v"], + ), + param("subject", SearchParamType::Reference, patient(), &["1"]), + param( + "subject", + SearchParamType::Reference, + SearchModifier::Identifier, + &["s|v"], + ), + param( + "subject", + SearchParamType::Reference, + SearchModifier::Above, + &["http://example.org/fhir/Patient/1"], + ), + param( + "subject", + SearchParamType::Reference, + SearchModifier::Below, + &["http://example.org/fhir"], + ), + ]; + for param in admitted { + let shown = format!("{param:?}"); + let query = SearchQuery::new("Observation").with_parameter(param); + assert!(backend.validate_query_support(&query).is_ok(), "{shown}"); + } + + for modifier in [ + SearchModifier::In, + SearchModifier::NotIn, + SearchModifier::Above, + SearchModifier::Below, + ] { + let query = SearchQuery::new("Observation").with_parameter(param( + "code", + SearchParamType::Token, + modifier.clone(), + &["http://loinc.org|1234-5"], + )); + assert!( + matches!( + backend.validate_query_support(&query), + Err(StorageError::Search( + SearchError::UnsupportedModifier { .. } + )) + ), + "token :{modifier} must stay refused" + ); + } + } + + #[test] + fn of_type_compares_type_system_type_code_and_value() { + let of_type = |value: &str| { + filter(¶m( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &[value], + )) + }; + + assert_eq!( + of_type(&format!("{V2_0203}|MR|12345")), + with( + envelope("identifier"), + doc! { + "value_token_code": "12345", + "value_identifier_type_system": V2_0203, + "value_identifier_type_code": "MR", + } + ) + ); + // An empty part is not compared. + assert_eq!( + of_type("|MR|12345"), + with( + envelope("identifier"), + doc! { "value_token_code": "12345", "value_identifier_type_code": "MR" } + ) + ); + // The identifier value may itself contain a pipe. + assert_eq!( + of_type("|MR|a|b"), + with( + envelope("identifier"), + doc! { "value_token_code": "a|b", "value_identifier_type_code": "MR" } + ) + ); + } + + /// Fewer than three parts — or three empty ones — must match nothing: + /// dropping the condition instead would return every resource of the type. + #[test] + fn a_malformed_of_type_value_matches_nothing() { + for value in ["12345", "MR|12345", "||"] { + let built = filter(¶m( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &[value], + )); + assert_eq!( + built, + with(envelope("identifier"), MongoBackend::match_nothing()), + "{value}" + ); + } + } + + #[test] + fn of_type_values_are_ored() { + let built = filter(¶m( + "identifier", + SearchParamType::Token, + SearchModifier::OfType, + &["|MR|1", "|SS|2"], + )); + assert_eq!( + built.get_array("$or").unwrap(), + &vec![ + Bson::Document( + doc! { "value_token_code": "1", "value_identifier_type_code": "MR" } + ), + Bson::Document( + doc! { "value_token_code": "2", "value_identifier_type_code": "SS" } + ), + ] + ); + } + + /// `subject:Patient=123` is `subject=Patient/123`: the same filter, and so + /// never `Group/123`. + #[test] + fn type_modifier_is_the_qualified_reference() { + let plain = filter(&SearchParameter { + name: "subject".to_string(), + param_type: SearchParamType::Reference, + modifier: None, + values: vec![SearchValue::eq("Patient/123")], + chain: vec![], + components: vec![], + }); + assert_eq!( + plain, + with( + envelope("subject"), + doc! { "$or": [ + { "value_reference": "Patient/123" }, + { "value_reference": { "$regex": "^Patient/123/_history/" } }, + ]} + ) + ); + assert_eq!(subject(patient(), "123"), plain); + assert_eq!(subject(patient(), "Patient/123"), plain); + // Version-agnostic, with or without the modifier. + assert_eq!(subject(patient(), "123/_history/2"), plain); + assert_eq!(subject(patient(), "Patient/123/_history/2"), plain); + assert_eq!( + filter(&SearchParameter { + name: "subject".to_string(), + param_type: SearchParamType::Reference, + modifier: None, + values: vec![SearchValue::eq("Patient/123/_history/2")], + chain: vec![], + components: vec![], + }), + plain + ); + } + + #[test] + fn type_modifier_keeps_an_absolute_url_of_that_type() { + assert_eq!( + subject(patient(), "http://example.org/fhir/Patient/123"), + with( + envelope("subject"), + doc! { "$or": [ + { "value_reference": "http://example.org/fhir/Patient/123" }, + { "value_reference": { + "$regex": "^http://example\\.org/fhir/Patient/123/_history/" + } }, + ]} + ) + ); + } + + #[test] + fn type_modifier_and_a_value_of_another_type_match_nothing() { + for value in ["Group/123", "http://example.org/fhir/Group/123"] { + assert_eq!( + subject(patient(), value), + with(envelope("subject"), MongoBackend::match_nothing()), + "{value}" + ); + } + } + + #[test] + fn type_modifier_escapes_regex_metacharacters() { + let built = subject(patient(), "1.5"); + let arms = built.get_array("$or").unwrap(); + assert_eq!( + arms[1].as_document().unwrap(), + &doc! { "value_reference": { "$regex": "^Patient/1\\.5/_history/" } } + ); + } + + #[test] + fn reference_below_and_above_mirror_the_uri_shapes() { + assert_eq!( + subject(SearchModifier::Below, "http://example.org/fhir/"), + with( + envelope("subject"), + doc! { "value_reference": { "$regex": "^http://example\\.org/fhir(/|$)" } } + ) + ); + let above = subject(SearchModifier::Above, "http://example.org/fhir/Patient/1"); + let parents = above + .get_document("value_reference") + .unwrap() + .get_array("$in") + .unwrap(); + assert!(parents.contains(&Bson::String( + "http://example.org/fhir/Patient/1".to_string() + ))); + assert!(parents.contains(&Bson::String("http://example.org/fhir".to_string()))); + } + + /// `:identifier` needs the database; a path that did not resolve it must + /// refuse rather than treat the identifier as a reference. + #[test] + fn an_unresolved_identifier_modifier_is_refused_by_the_builder() { + let error = backend() + .build_search_index_filter( + "t1", + "Observation", + ¶m( + "subject", + SearchParamType::Reference, + SearchModifier::Identifier, + &["http://example.org/mrn|12345"], + ), + ) + .unwrap_err(); + assert!(matches!( + error, + StorageError::Search(SearchError::UnsupportedModifier { ref modifier, .. }) + if modifier == "identifier" + )); + } + + #[test] + fn identifier_predicates_follow_the_token_grammar() { + assert_eq!( + MongoBackend::identifier_predicate("http://example.org/mrn|12345"), + doc! { "value_token_system": "http://example.org/mrn", "value_token_code": "12345" } + ); + assert_eq!( + MongoBackend::identifier_predicate("http://example.org/mrn|"), + doc! { "value_token_system": "http://example.org/mrn" } + ); + assert_eq!( + MongoBackend::identifier_predicate("12345"), + doc! { "value_token_code": "12345" } + ); + assert_eq!( + MongoBackend::identifier_predicate("|12345"), + doc! { + "value_token_system": { "$in": [Bson::Null, Bson::String(String::new())] }, + "value_token_code": "12345", + } + ); + } + + /// Each resolved target is matched exactly or at any version — never as a + /// prefix of another id (`Patient/p1` must not admit `Patient/p10`). + #[test] + fn resolved_identifier_targets_are_one_bounded_in() { + let built = MongoBackend::identifier_targets_filter( + "t1", + "Observation", + "subject", + vec!["Patient/p.1".to_string()], + ); + assert_eq!( + built, + with( + envelope("subject"), + doc! { "value_reference": { "$in": [ + Bson::RegularExpression(bson::Regex { + pattern: "^Patient/p\\.1/_history/".to_string(), + options: String::new(), + }), + Bson::String("Patient/p.1".to_string()), + ]}} + ) + ); + } +} diff --git a/crates/persistence/tests/elasticsearch_tests.rs b/crates/persistence/tests/elasticsearch_tests.rs index e4085e06d..cb4088bfe 100644 --- a/crates/persistence/tests/elasticsearch_tests.rs +++ b/crates/persistence/tests/elasticsearch_tests.rs @@ -689,6 +689,11 @@ mod token_code_system_suite; #[path = "search/empty_value_suite.rs"] mod empty_value_suite; +/// The backend-agnostic modifier parity suite (#1408). Same `#[path]` +/// arrangement. +#[path = "search/modifier_parity_suite.rs"] +mod modifier_parity_suite; + #[path = "common/container_cleanup.rs"] mod container_cleanup; @@ -1068,6 +1073,88 @@ mod es_integration { .await; } + /// #1408: every modifier `SearchModifier::is_valid_for` allows, on every + /// parameter type. + #[tokio::test] + async fn es_modifier_parity() { + use super::modifier_parity_suite::{Divergence, Expect}; + + let backend = create_backend().await; + super::modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + "modifier-parity-1408", + &[ + // A short `:of-type` value adds no condition at all: every Patient. + Divergence { + label: "Patient?identifier:ofType=MR|12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + Divergence { + label: "Patient?identifier:ofType=12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + // `:code-text` is a word-prefix match (`match_phrase_prefix`), not a + // starts-with on the whole display. + Divergence { + label: "Observation?code:code-text=rate", + expect: Expect::Ids(&["ob-pat"]), + }, + // Terminology-backed token modifiers are not refused but degraded: + // `:in` / `:not-in` match nothing, `:above` / `:below` match the code + // itself. Unreachable over REST, which expands them or answers 501 first. + Divergence { + label: "Observation?code:in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:not-in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:above=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?code:below=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + // `:[type]` filters on the indexed `resource_type`, which a versioned + // reference does not carry, and a bare id also matches an absolute URL. + Divergence { + label: "Observation?subject:Patient=p1", + expect: Expect::Ids(&["ob-abs", "ob-pat"]), + }, + Divergence { + label: "Observation?subject:Patient=Patient/p1", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?subject:Patient=Patient/p1/_history/2", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?subject:Patient=p1,nobody", + expect: Expect::Ids(&["ob-abs", "ob-pat"]), + }, + // `:identifier` looks for token rows under the reference parameter's own + // name, which nothing writes: it never matches. + Divergence { + label: "Observation?subject:identifier=http://example.org/mrn|12345", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?subject:identifier=http://example.org/mrn|", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?subject:identifier=12345", + expect: Expect::Ids(&[]), + }, + ], + ) + .await; + } + // ======================================================================== // Index-side date handling (#1314) // ======================================================================== diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 3aa39e373..a375895b5 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -759,6 +759,27 @@ async fn mongodb_empty_values_are_rejected_on_every_path() { empty_value_suite::empty_values_are_rejected_on_every_path(&backend, "empty-value-1380").await; } +/// The backend-agnostic modifier parity suite (#1408). Same `#[path]` +/// arrangement. +#[path = "search/modifier_parity_suite.rs"] +mod modifier_parity_suite; + +/// #1408: `:of-type`, reference `:identifier` and reference `:[type]` were +/// refused as unsupported modifiers. Needs the full registry. +#[tokio::test] +async fn mongodb_modifier_parity() { + let Some(backend) = create_backend_with_full_registry("modifier_parity").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + "modifier-parity-1408", + &[], + ) + .await; +} + /// #1062: a comma-separated value list on one `SearchParameter` is OR per /// FHIR (https://build.fhir.org/search.html#combining) — for date same as /// every other type. Drives the real `SearchProvider::search` / diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 3db141d04..2b4f32cf6 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -108,6 +108,11 @@ mod conditional_if_match_suite; #[path = "search/empty_value_suite.rs"] mod empty_value_suite; +/// The backend-agnostic modifier parity suite (#1408). Same `#[path]` +/// arrangement. +#[path = "search/modifier_parity_suite.rs"] +mod modifier_parity_suite; + #[path = "common/container_cleanup.rs"] mod container_cleanup; @@ -19101,6 +19106,55 @@ mod postgres_integration { .await; } + /// #1408: every modifier `SearchModifier::is_valid_for` allows, on every + /// parameter type. + #[tokio::test] + async fn postgres_integration_modifier_parity() { + use super::modifier_parity_suite::{Divergence, Expect}; + + let backend = create_backend().await; + super::modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + &unique_base("modifier_parity"), + &[ + // A short `:of-type` value adds no condition at all: every Patient. + Divergence { + label: "Patient?identifier:ofType=MR|12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + Divergence { + label: "Patient?identifier:ofType=12345", + expect: Expect::Ids(&["p1", "p2", "p3", "p4"]), + }, + // Terminology-backed token modifiers are not refused but degraded: + // `:in` / `:not-in` match nothing, `:above` / `:below` match the code + // itself. Unreachable over REST, which expands them or answers 501 first. + Divergence { + label: "Observation?code:in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:not-in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:above=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?code:below=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + // A value naming another type wins over the `:[type]` modifier. + Divergence { + label: "Observation?subject:Patient=Group/p1", + expect: Expect::Ids(&["ob-grp"]), + }, + ], + ) + .await; + } + #[tokio::test] async fn postgres_integration_distinct_tenant_ids_never_share_data() { let backend = create_backend().await; diff --git a/crates/persistence/tests/search/mod.rs b/crates/persistence/tests/search/mod.rs index 2c1bde65c..92f148d5f 100644 --- a/crates/persistence/tests/search/mod.rs +++ b/crates/persistence/tests/search/mod.rs @@ -70,6 +70,9 @@ pub mod include_tests; pub mod meta_params_suite; pub mod meta_params_suite_tests; pub mod meta_params_tests; +/// Backend-agnostic scenarios, shared with the PostgreSQL, MongoDB and +/// Elasticsearch test binaries via `#[path]` (#1408). +pub mod modifier_parity_suite; pub mod modifier_tests; /// Backend-agnostic scenarios, shared with the PostgreSQL, MongoDB and /// Elasticsearch test binaries via `#[path]` (#1337). diff --git a/crates/persistence/tests/search/modifier_parity_suite.rs b/crates/persistence/tests/search/modifier_parity_suite.rs new file mode 100644 index 000000000..350a034b4 --- /dev/null +++ b/crates/persistence/tests/search/modifier_parity_suite.rs @@ -0,0 +1,904 @@ +//! Backend-agnostic modifier parity suite (issue #1408). +//! +//! MongoDB answered `identifier:of-type=…`, `subject:identifier=…` and +//! `subject:Patient=…` with "unsupported modifier" while SQLite, PostgreSQL +//! and Elasticsearch served them, and nothing noticed: every backend had its +//! own modifier tests, none shared. This suite is the shared one. It walks +//! every modifier [`SearchModifier::is_valid_for`] allows on each parameter +//! type over one seeded data set and states, per cell, the ids that must +//! match. +//! +//! A backend that really does differ passes its differences in as +//! [`Divergence`]s, each keyed by the cell's label (`Type?param:modifier=value`) +//! with what that backend returns instead. A divergence that stops being one +//! fails the suite too, so the list cannot rot, and an unknown label is an +//! error. Anything not listed must agree with the table. +//! +//! Semantics worth knowing before reading the table: +//! +//! - `:identifier` on a reference is implemented by SQLite, PostgreSQL and +//! MongoDB as "the reference's *target* has this identifier" (a join on the +//! target's `identifier` rows). `Reference.identifier` itself — a logical +//! reference — is not indexed by the shared extractor, so `ob-logical` is +//! matched by no backend. +//! - `:[type]` with a bare id is `Type/id`, version-agnostic, and never the +//! same id under another type (`Group/p1`). +//! - `:in` / `:not-in` need a terminology server and `:above` / `:below` on a +//! token need subsumption; neither exists at this layer. The cells are here +//! so that what each backend does with them is on record. +//! +//! Included by `#[path]` into each backend's test binary, like +//! `token_code_system_suite.rs`. The backend must be built with the spec +//! search parameters loaded; the positive controls fail loudly if it was not. + +#![allow(dead_code)] + +use std::collections::BTreeSet; + +use serde_json::json; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{ResourceStorage, SearchProvider}; +use helios_persistence::error::{SearchError, StorageError}; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; +use helios_persistence::types::{ + SearchModifier, SearchParamType, SearchParameter, SearchQuery, SearchValue, +}; + +const V2_0203: &str = "http://terminology.hl7.org/CodeSystem/v2-0203"; +const MRN: &str = "http://example.org/mrn"; +const SSN: &str = "http://example.org/ssn"; +const LOINC: &str = "http://loinc.org"; +const VS: &str = "http://example.org/fhir/ValueSet/a"; +const ABS_PATIENT: &str = "http://example.org/fhir/Patient/p1"; + +/// What a cell must produce. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Expect { + /// The search succeeds and returns exactly these ids. + Ids(&'static [&'static str]), + /// The search is refused as an unsupported modifier. + Rejected, +} + +/// A cell on which one backend knowingly differs from the table. +pub struct Divergence { + /// The cell's label, `Type?param:modifier=value`. + pub label: &'static str, + /// What this backend produces instead. + pub expect: Expect, +} + +struct Case { + resource_type: &'static str, + param: &'static str, + param_type: SearchParamType, + modifier: SearchModifier, + value: String, + expect: Expect, +} + +impl Case { + fn label(&self) -> String { + format!( + "{}?{}:{}={}", + self.resource_type, self.param, self.modifier, self.value + ) + } +} + +fn case( + resource_type: &'static str, + param: &'static str, + param_type: SearchParamType, + modifier: SearchModifier, + value: impl Into, + expect: Expect, +) -> Case { + Case { + resource_type, + param, + param_type, + modifier, + value: value.into(), + expect, + } +} + +/// The matrix. See [`seed`] for the resources the ids name. +fn cases() -> Vec { + use Expect::{Ids, Rejected}; + use SearchModifier as M; + use SearchParamType as T; + + let patient = || M::Type("Patient".to_string()); + let group = || M::Type("Group".to_string()); + + vec![ + // ---- token --------------------------------------------------- + // `:of-type` is `type-system|type-code|identifier-value`. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|MR|12345"), + Ids(&["p1"]), + ), + // Same value, other type: the type really is compared. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|SS|12345"), + Ids(&["p2"]), + ), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|MR|99999"), + Ids(&[]), + ), + // An empty part is not compared. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + "|MR|12345", + Ids(&["p1"]), + ), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}||12345"), + Ids(&["p1", "p2"]), + ), + // The grammar has three parts; fewer name nothing. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + "MR|12345", + Ids(&[]), + ), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + "12345", + Ids(&[]), + ), + // An OR-list of `:of-type` values. + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}|MR|12345,{V2_0203}|SS|12345"), + Ids(&["p1", "p2"]), + ), + case( + "Observation", + "code", + T::Token, + M::Text, + "heart", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "code", + T::Token, + M::CodeText, + "heart", + Ids(&["ob-pat"]), + ), + // `:code-text` is starts-with, `:text` is not. + case( + "Observation", + "code", + T::Token, + M::CodeText, + "rate", + Ids(&[]), + ), + case( + "Observation", + "code", + T::Token, + M::Not, + format!("{LOINC}|1234-5"), + Ids(&["ob-abs", "ob-grp", "ob-logical", "ob-nosubj", "ob-ver"]), + ), + case( + "Observation", + "code", + T::Token, + M::Missing, + "true", + Ids(&["ob-nosubj"]), + ), + case( + "Observation", + "code", + T::Token, + M::Missing, + "false", + Ids(&["ob-abs", "ob-grp", "ob-logical", "ob-pat", "ob-ver"]), + ), + // Terminology-backed: not available at this layer. + case( + "Observation", + "code", + T::Token, + M::In, + "http://example.org/fhir/ValueSet/a", + Rejected, + ), + case( + "Observation", + "code", + T::Token, + M::NotIn, + "http://example.org/fhir/ValueSet/a", + Rejected, + ), + case( + "Observation", + "code", + T::Token, + M::Above, + format!("{LOINC}|1234-5"), + Rejected, + ), + case( + "Observation", + "code", + T::Token, + M::Below, + format!("{LOINC}|1234-5"), + Rejected, + ), + // ---- string -------------------------------------------------- + case( + "Patient", + "family", + T::String, + M::Exact, + "Smith", + Ids(&["p1"]), + ), + case( + "Patient", + "family", + T::String, + M::Exact, + "smith", + Ids(&["p3"]), + ), + case( + "Patient", + "family", + T::String, + M::Contains, + "MIT", + Ids(&["p1", "p2", "p3"]), + ), + case( + "Patient", + "family", + T::String, + M::Contains, + "thso", + Ids(&["p2"]), + ), + case( + "Patient", + "family", + T::String, + M::Text, + "thso", + Ids(&["p2"]), + ), + case( + "Patient", + "family", + T::String, + M::Missing, + "true", + Ids(&["p4"]), + ), + case( + "Patient", + "family", + T::String, + M::Missing, + "false", + Ids(&["p1", "p2", "p3"]), + ), + // ---- reference ----------------------------------------------- + // `subject:Patient=p1` is `subject=Patient/p1`: not `Group/p1`. + case( + "Observation", + "subject", + T::Reference, + patient(), + "p1", + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + group(), + "p1", + Ids(&["ob-grp"]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + "Patient/p1", + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + "Patient/p1/_history/2", + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + "nobody", + Ids(&[]), + ), + // The modifier and the value disagree on the type: nothing can be + // both. + case( + "Observation", + "subject", + T::Reference, + patient(), + "Group/p1", + Ids(&[]), + ), + case( + "Observation", + "subject", + T::Reference, + patient(), + ABS_PATIENT, + Ids(&["ob-abs"]), + ), + // Two ids under one type modifier. + case( + "Observation", + "subject", + T::Reference, + patient(), + "p1,nobody", + Ids(&["ob-pat", "ob-ver"]), + ), + // `:identifier`: the reference's target has the identifier. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|12345"), + Ids(&["ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|"), + Ids(&["ob-pat", "ob-ver"]), + ), + // Value alone: `p1` (a Patient) and the Group `p1` both carry 12345. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + "12345", + Ids(&["ob-grp", "ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|99999"), + Ids(&[]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Missing, + "true", + Ids(&["ob-logical", "ob-nosubj"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Missing, + "false", + Ids(&["ob-abs", "ob-grp", "ob-pat", "ob-ver"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Contains, + "example.org", + Ids(&["ob-abs"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Text, + "smith", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::CodeText, + "john", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Below, + "http://example.org/fhir/Patient", + Ids(&["ob-abs"]), + ), + case( + "Observation", + "subject", + T::Reference, + M::Above, + format!("{ABS_PATIENT}/_history/1"), + Ids(&["ob-abs"]), + ), + // ---- uri ----------------------------------------------------- + case( + "ValueSet", + "url", + T::Uri, + M::Below, + VS, + Ids(&["vs-a", "vs-sub"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Above, + format!("{VS}/sub"), + Ids(&["vs-a", "vs-sub"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Contains, + "ValueSet/a/s", + Ids(&["vs-sub"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Missing, + "true", + Ids(&["vs-none"]), + ), + case( + "ValueSet", + "url", + T::Uri, + M::Missing, + "false", + Ids(&["vs-a", "vs-sub"]), + ), + // ---- date / number / quantity ---------------------------------- + case( + "Patient", + "birthdate", + T::Date, + M::Missing, + "true", + Ids(&["p2", "p3", "p4"]), + ), + case( + "Patient", + "birthdate", + T::Date, + M::Missing, + "false", + Ids(&["p1"]), + ), + case( + "RiskAssessment", + "probability", + T::Number, + M::Missing, + "true", + Ids(&["ra-none"]), + ), + case( + "RiskAssessment", + "probability", + T::Number, + M::Missing, + "false", + Ids(&["ra-half"]), + ), + case( + "Observation", + "value-quantity", + T::Quantity, + M::Missing, + "false", + Ids(&["ob-pat"]), + ), + case( + "Observation", + "value-quantity", + T::Quantity, + M::Missing, + "true", + Ids(&["ob-abs", "ob-grp", "ob-logical", "ob-nosubj", "ob-ver"]), + ), + ] +} + +fn query( + resource_type: &str, + param: &str, + param_type: SearchParamType, + modifier: Option, + value: &str, +) -> SearchQuery { + SearchQuery::new(resource_type) + .with_parameter(SearchParameter { + name: param.to_string(), + param_type, + modifier, + // Comma-separated values are an OR-list, as the REST layer parses them. + values: value.split(',').map(SearchValue::eq).collect(), + ..Default::default() + }) + .with_count(100) +} + +/// Runs one query and reduces the result to what the table compares. +async fn outcome( + backend: &S, + tenant: &TenantContext, + query: &SearchQuery, +) -> Result, String> +where + S: ResourceStorage + SearchProvider, +{ + match backend.search(tenant, query).await { + Ok(result) => Ok(result + .resources + .items + .iter() + .map(|r| r.id().to_string()) + .collect()), + Err(StorageError::Search(SearchError::UnsupportedModifier { .. })) => { + Err("REJECTED".to_string()) + } + Err(other) => Err(format!("ERROR {other}")), + } +} + +fn render(outcome: &Result, String>) -> String { + match outcome { + Ok(ids) => format!("{:?}", ids.iter().collect::>()), + Err(e) => e.clone(), + } +} + +fn render_expect(expect: &Expect) -> String { + match expect { + Expect::Ids(ids) => { + let sorted: BTreeSet<&str> = ids.iter().copied().collect(); + format!("{:?}", sorted.iter().collect::>()) + } + Expect::Rejected => "REJECTED".to_string(), + } +} + +/// The seeded resources: +/// +/// - Patients: `p1` Smith, born 1980, MRN 12345 typed `MR`; `p2` Smithson, +/// SSN 12345 typed `SS`; `p3` smith (lower case), no identifier; `p4` has +/// neither name nor identifier. +/// - Group `p1`: the same id as the Patient, with an untyped identifier +/// `http://example.org/grp|12345`. +/// - Observations, by `subject`: `ob-pat` `Patient/p1` (display "John Smith", +/// coded LOINC 1234-5 "Heart rate", 5 mg); `ob-ver` `Patient/p1/_history/2`; +/// `ob-abs` the absolute URL; `ob-grp` `Group/p1`; `ob-logical` only a +/// `Reference.identifier`; `ob-nosubj` neither subject nor code. +/// - ValueSets `vs-a`, `vs-sub` (a URL below `vs-a`'s), `vs-none` (no URL). +/// - RiskAssessments `ra-half` (probability 0.5) and `ra-none`. +async fn seed(backend: &S, tenant: &TenantContext) +where + S: ResourceStorage + SearchProvider, +{ + let other_code = json!({"coding": [{"system": LOINC, "code": "9999-9"}]}); + let resources = [ + ( + "Patient", + json!({ + "id": "p1", + "identifier": [{ + "type": {"coding": [{"system": V2_0203, "code": "MR"}]}, + "system": MRN, + "value": "12345", + }], + "name": [{"family": "Smith", "given": ["John"]}], + "birthDate": "1980-01-01", + }), + ), + ( + "Patient", + json!({ + "id": "p2", + "identifier": [{ + "type": {"coding": [{"system": V2_0203, "code": "SS"}]}, + "system": SSN, + "value": "12345", + }], + "name": [{"family": "Smithson"}], + }), + ), + ( + "Patient", + json!({"id": "p3", "name": [{"family": "smith"}]}), + ), + ("Patient", json!({"id": "p4", "active": true})), + ( + "Group", + json!({ + "id": "p1", + "type": "person", + "actual": true, + "identifier": [{"system": "http://example.org/grp", "value": "12345"}], + }), + ), + ( + "Observation", + json!({ + "id": "ob-pat", + "status": "final", + "code": {"coding": [{"system": LOINC, "code": "1234-5", "display": "Heart rate"}]}, + "subject": {"reference": "Patient/p1", "display": "John Smith"}, + "valueQuantity": { + "value": 5, "unit": "mg", + "system": "http://unitsofmeasure.org", "code": "mg", + }, + }), + ), + ( + "Observation", + json!({ + "id": "ob-ver", + "status": "final", + "code": other_code.clone(), + "subject": {"reference": "Patient/p1/_history/2"}, + }), + ), + ( + "Observation", + json!({ + "id": "ob-abs", + "status": "final", + "code": other_code.clone(), + "subject": {"reference": ABS_PATIENT}, + }), + ), + ( + "Observation", + json!({ + "id": "ob-grp", + "status": "final", + "code": other_code.clone(), + "subject": {"reference": "Group/p1"}, + }), + ), + ( + "Observation", + json!({ + "id": "ob-logical", + "status": "final", + "code": other_code.clone(), + "subject": {"identifier": {"system": MRN, "value": "12345"}}, + }), + ), + ("Observation", json!({"id": "ob-nosubj", "status": "final"})), + ( + "ValueSet", + json!({"id": "vs-a", "status": "active", "url": VS}), + ), + ( + "ValueSet", + json!({"id": "vs-sub", "status": "active", "url": format!("{VS}/sub")}), + ), + ("ValueSet", json!({"id": "vs-none", "status": "active"})), + ( + "RiskAssessment", + json!({ + "id": "ra-half", + "status": "final", + "subject": {"reference": "Patient/p1"}, + "prediction": [{"probabilityDecimal": 0.5}], + }), + ), + ( + "RiskAssessment", + json!({ + "id": "ra-none", + "status": "final", + "subject": {"reference": "Patient/p1"}, + }), + ), + ]; + for (resource_type, resource) in resources { + let id = resource["id"].as_str().unwrap_or_default().to_string(); + backend + .create(tenant, resource_type, resource, FhirVersion::default()) + .await + .unwrap_or_else(|e| panic!("create {resource_type}/{id} failed: {e}")); + } +} + +/// Seeds the data under a caller-unique tenant and asserts the matrix, with +/// the caller's [`Divergence`]s applied. +pub async fn every_valid_modifier_agrees_across_backends( + backend: &S, + tenant_base: &str, + divergences: &[Divergence], +) where + S: ResourceStorage + SearchProvider, +{ + use SearchParamType as T; + + let tenant = TenantContext::new(TenantId::new(tenant_base), TenantPermissions::full_access()); + seed(backend, &tenant).await; + + // Positive controls: the unmodified search on every parameter the matrix + // uses. Polled because Elasticsearch is near-real-time. A failure here + // means the parameter did not index — a backend built without the spec + // search parameters — not that a modifier is wrong. + let controls: [(&str, &str, SearchParamType, &str, &[&str]); 9] = [ + ("Patient", "identifier", T::Token, "12345", &["p1", "p2"]), + ("Patient", "family", T::String, "smiths", &["p2"]), + ("Patient", "birthdate", T::Date, "1980-01-01", &["p1"]), + ("Observation", "code", T::Token, "1234-5", &["ob-pat"]), + ( + "Observation", + "subject", + T::Reference, + "Patient/p1", + &["ob-pat", "ob-ver"], + ), + ( + "Observation", + "subject", + T::Reference, + "Group/p1", + &["ob-grp"], + ), + ( + "Observation", + "value-quantity", + T::Quantity, + "5", + &["ob-pat"], + ), + ("ValueSet", "url", T::Uri, VS, &["vs-a"]), + ( + "RiskAssessment", + "probability", + T::Number, + "0.5", + &["ra-half"], + ), + ]; + for (resource_type, param, param_type, value, expected) in controls { + let control = query(resource_type, param, param_type, None, value); + let want: BTreeSet = expected.iter().map(|id| id.to_string()).collect(); + let mut got = Ok(BTreeSet::new()); + for _ in 0..60 { + got = outcome(backend, &tenant, &control).await; + if got.as_ref() == Ok(&want) { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(250)).await; + } + assert_eq!( + got, + Ok(want), + "positive control {resource_type}?{param}={value}" + ); + } + + let table = cases(); + let labels: BTreeSet = table.iter().map(Case::label).collect(); + for divergence in divergences { + assert!( + labels.contains(divergence.label), + "divergence names no cell of the matrix: {}", + divergence.label + ); + } + + let mut failures = Vec::new(); + for case in &table { + let label = case.label(); + let got = outcome( + backend, + &tenant, + &query( + case.resource_type, + case.param, + case.param_type, + Some(case.modifier.clone()), + &case.value, + ), + ) + .await; + let shown = render(&got); + println!("{label} -> {shown}"); + + let agreed = render_expect(&case.expect); + match divergences.iter().find(|d| d.label == label) { + Some(divergence) => { + let diverged = render_expect(&divergence.expect); + if shown == agreed { + failures.push(format!( + "{label}: now agrees with the matrix ({agreed}); remove the divergence" + )); + } else if shown != diverged { + failures.push(format!( + "{label}: got {shown}, expected the known divergence {diverged} \ + (matrix: {agreed})" + )); + } + } + None => { + if shown != agreed { + failures.push(format!("{label}: got {shown}, expected {agreed}")); + } + } + } + } + assert!(failures.is_empty(), "\n{}", failures.join("\n")); +} diff --git a/crates/persistence/tests/search/modifier_tests.rs b/crates/persistence/tests/search/modifier_tests.rs index 0bcce63d5..bb2ebc38c 100644 --- a/crates/persistence/tests/search/modifier_tests.rs +++ b/crates/persistence/tests/search/modifier_tests.rs @@ -1375,3 +1375,55 @@ async fn test_of_type_modifier_type_discrimination() { let _dl_result = backend.search(&tenant, &dl_query.with_count(100)).await; let _pp_result = backend.search(&tenant, &pp_query.with_count(100)).await; } + +/// #1408: every modifier `SearchModifier::is_valid_for` allows, on every +/// parameter type, over one data set. The scenarios are backend-agnostic +/// (`modifier_parity_suite.rs`): PostgreSQL, MongoDB and Elasticsearch run the +/// same ones. +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_modifier_parity_suite() { + use super::modifier_parity_suite::{Divergence, Expect}; + + let backend = create_sqlite_backend(); + super::modifier_parity_suite::every_valid_modifier_agrees_across_backends( + &backend, + "modifier-parity", + &[ + // A short `:of-type` value is read as `type-code|value`, or `value`. + Divergence { + label: "Patient?identifier:ofType=MR|12345", + expect: Expect::Ids(&["p1"]), + }, + Divergence { + label: "Patient?identifier:ofType=12345", + expect: Expect::Ids(&["p1", "p2"]), + }, + // Terminology-backed token modifiers are not refused but degraded: + // `:in` / `:not-in` match nothing, `:above` / `:below` match the code + // itself. Unreachable over REST, which expands them or answers 501 first. + Divergence { + label: "Observation?code:in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:not-in=http://example.org/fhir/ValueSet/a", + expect: Expect::Ids(&[]), + }, + Divergence { + label: "Observation?code:above=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + Divergence { + label: "Observation?code:below=http://loinc.org|1234-5", + expect: Expect::Ids(&["ob-pat"]), + }, + // A value naming another type wins over the `:[type]` modifier. + Divergence { + label: "Observation?subject:Patient=Group/p1", + expect: Expect::Ids(&["ob-grp"]), + }, + ], + ) + .await; +} From edf6afafff5a78ce91741db33d75d43565c512f0 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:01:24 -0400 Subject: [PATCH 04/10] test(persistence): modifier parity suite covers AND, search_count and tenants The suite's `:identifier` lookup is a second query; a cell now proves it is tenant-scoped, three cells AND a modified parameter with a plain one so it is also exercised as the non-driving filter, and every cell checks that `search_count` counts what `search` returns. Refs #1408 --- .../persistence/tests/elasticsearch_tests.rs | 8 ++ .../tests/search/modifier_parity_suite.rs | 122 ++++++++++++++++-- 2 files changed, 117 insertions(+), 13 deletions(-) diff --git a/crates/persistence/tests/elasticsearch_tests.rs b/crates/persistence/tests/elasticsearch_tests.rs index cb4088bfe..a52f638bc 100644 --- a/crates/persistence/tests/elasticsearch_tests.rs +++ b/crates/persistence/tests/elasticsearch_tests.rs @@ -1136,6 +1136,10 @@ mod es_integration { label: "Observation?subject:Patient=p1,nobody", expect: Expect::Ids(&["ob-abs", "ob-pat"]), }, + Divergence { + label: "Observation?subject:Patient=p1&code=9999-9", + expect: Expect::Ids(&["ob-abs"]), + }, // `:identifier` looks for token rows under the reference parameter's own // name, which nothing writes: it never matches. Divergence { @@ -1150,6 +1154,10 @@ mod es_integration { label: "Observation?subject:identifier=12345", expect: Expect::Ids(&[]), }, + Divergence { + label: "Observation?subject:identifier=http://example.org/mrn|12345&code=1234-5", + expect: Expect::Ids(&[]), + }, ], ) .await; diff --git a/crates/persistence/tests/search/modifier_parity_suite.rs b/crates/persistence/tests/search/modifier_parity_suite.rs index 350a034b4..965744738 100644 --- a/crates/persistence/tests/search/modifier_parity_suite.rs +++ b/crates/persistence/tests/search/modifier_parity_suite.rs @@ -75,16 +75,28 @@ struct Case { param_type: SearchParamType, modifier: SearchModifier, value: String, + /// A second, unmodified token parameter ANDed with the first, so the + /// modified one is also exercised where it does not drive the search. + and: Option<(&'static str, &'static str)>, expect: Expect, } impl Case { fn label(&self) -> String { + let and = self + .and + .map(|(param, value)| format!("&{param}={value}")) + .unwrap_or_default(); format!( - "{}?{}:{}={}", + "{}?{}:{}={}{and}", self.resource_type, self.param, self.modifier, self.value ) } + + fn and(mut self, param: &'static str, value: &'static str) -> Self { + self.and = Some((param, value)); + self + } } fn case( @@ -101,6 +113,7 @@ fn case( param_type, modifier, value: value.into(), + and: None, expect, } } @@ -483,6 +496,44 @@ fn cases() -> Vec { format!("{ABS_PATIENT}/_history/1"), Ids(&["ob-abs"]), ), + // Another tenant's Patient `p1` carries MRN 77777; this tenant's does + // not, and its references must not match on the strength of it. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|77777"), + Ids(&[]), + ), + // ANDed with a plain parameter, in both directions of selectivity. + case( + "Observation", + "subject", + T::Reference, + M::Identifier, + format!("{MRN}|12345"), + Ids(&["ob-pat"]), + ) + .and("code", "1234-5"), + case( + "Observation", + "subject", + T::Reference, + patient(), + "p1", + Ids(&["ob-ver"]), + ) + .and("code", "9999-9"), + case( + "Patient", + "identifier", + T::Token, + M::OfType, + format!("{V2_0203}||12345"), + Ids(&["p2"]), + ) + .and("family", "smithson"), // ---- uri ----------------------------------------------------- case( "ValueSet", @@ -595,6 +646,32 @@ fn query( .with_count(100) } +/// The query of one cell: the modified parameter, plus its companion if any. +fn case_query(case: &Case) -> SearchQuery { + let query = query( + case.resource_type, + case.param, + case.param_type, + Some(case.modifier.clone()), + &case.value, + ); + match case.and { + Some((param, value)) => query.with_parameter(SearchParameter { + name: param.to_string(), + // `family` is the one string companion; the rest are tokens. + param_type: if param == "family" { + SearchParamType::String + } else { + SearchParamType::Token + }, + modifier: None, + values: vec![SearchValue::eq(value)], + ..Default::default() + }), + None => query, + } +} + /// Runs one query and reduces the result to what the table compares. async fn outcome( backend: &S, @@ -793,6 +870,22 @@ pub async fn every_valid_modifier_agrees_across_backends( let tenant = TenantContext::new(TenantId::new(tenant_base), TenantPermissions::full_access()); seed(backend, &tenant).await; + // The same Patient id under another tenant, with an identifier this + // tenant's `p1` does not have. + let other = TenantContext::new( + TenantId::new(format!("{tenant_base}-other")), + TenantPermissions::full_access(), + ); + backend + .create( + &other, + "Patient", + json!({"id": "p1", "identifier": [{"system": MRN, "value": "77777"}]}), + FhirVersion::default(), + ) + .await + .unwrap_or_else(|e| panic!("create the other tenant's Patient/p1 failed: {e}")); + // Positive controls: the unmodified search on every parameter the matrix // uses. Polled because Elasticsearch is near-real-time. A failure here // means the parameter did not index — a backend built without the spec @@ -863,21 +956,24 @@ pub async fn every_valid_modifier_agrees_across_backends( let mut failures = Vec::new(); for case in &table { let label = case.label(); - let got = outcome( - backend, - &tenant, - &query( - case.resource_type, - case.param, - case.param_type, - Some(case.modifier.clone()), - &case.value, - ), - ) - .await; + let query = case_query(case); + let got = outcome(backend, &tenant, &query).await; let shown = render(&got); println!("{label} -> {shown}"); + // `search_count` takes its own route through some backends; it must + // count what `search` returns. + if let Ok(ids) = &got { + match backend.search_count(&tenant, &query).await { + Ok(count) if count == ids.len() as u64 => {} + Ok(count) => failures.push(format!( + "{label}: search_count is {count}, search returned {}", + ids.len() + )), + Err(e) => failures.push(format!("{label}: search_count failed: {e}")), + } + } + let agreed = render_expect(&case.expect); match divergences.iter().find(|d| d.label == label) { Some(divergence) => { From 863719074d6a7118fb7da6d6c1efdbfa97c5abce Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:04:25 -0400 Subject: [PATCH 05/10] test(rest): conditional patch is served and advertised on a composite with a search backend The #1384 tests pinned the composite's refusal (501, conditionalPatch: false). With the shared applier the composite serves it, so the same tests now pin that: PATCH [type]?criteria patches the one match (If-Match, no match, identity changes and a failed test op refused), the search backend follows the write, and /metadata advertises it. A patched id is refused on both patch endpoints. Refs #1406 --- crates/rest/src/handlers/capabilities.rs | 4 +- .../rest/src/handlers/conditional_support.rs | 5 +- crates/rest/src/handlers/patch.rs | 2 +- crates/rest/tests/conditional_capabilities.rs | 127 ++++++++++++------ crates/rest/tests/conditional_patch.rs | 8 +- 5 files changed, 100 insertions(+), 46 deletions(-) diff --git a/crates/rest/src/handlers/capabilities.rs b/crates/rest/src/handlers/capabilities.rs index 5836de488..920f9fa5f 100644 --- a/crates/rest/src/handlers/capabilities.rs +++ b/crates/rest/src/handlers/capabilities.rs @@ -174,8 +174,8 @@ where let revinclude_by_target = build_revinclude_index(®istry); // The conditional interactions are the storage's to declare, not literals: - // MongoDB has no conditional patch, S3 none at all, and a composite's - // answer depends on how it is composed (#1384). The conditional handlers + // S3 serves none on its own, and a composite's answer depends on how it + // is composed (#1384). The conditional handlers // refuse with `501` from this same source. let conditionals = super::conditional_support::advertised(state.storage(), version); diff --git a/crates/rest/src/handlers/conditional_support.rs b/crates/rest/src/handlers/conditional_support.rs index 552204834..616699dce 100644 --- a/crates/rest/src/handlers/conditional_support.rs +++ b/crates/rest/src/handlers/conditional_support.rs @@ -128,9 +128,10 @@ mod tests { /// nothing: only its declaration is under test. struct Declares(&'static [ConditionalInteraction]); - /// SQLite, PostgreSQL. + /// SQLite, PostgreSQL, MongoDB; a composite with a dedicated search backend. const ALL: Declares = Declares(&ConditionalInteraction::ALL); - /// MongoDB; a composite with a dedicated search backend. + /// What MongoDB and those composites declared before #1406; any storage + /// that resolves criteria but declines patch. const ALL_BUT_PATCH: Declares = Declares(&[ ConditionalInteraction::Create, ConditionalInteraction::Update, diff --git a/crates/rest/src/handlers/patch.rs b/crates/rest/src/handlers/patch.rs index 5320fdc40..54e63844e 100644 --- a/crates/rest/src/handlers/patch.rs +++ b/crates/rest/src/handlers/patch.rs @@ -191,7 +191,7 @@ where /// was supplied and is not satisfied /// - `415 Unsupported Media Type` - unknown patch format /// - `501 Not Implemented` - FHIRPath Patch, as for [`patch_handler`]; or a -/// backend without conditional patch (MongoDB) +/// storage without conditional patch (S3 on its own) /// /// # `If-Match` /// diff --git a/crates/rest/tests/conditional_capabilities.rs b/crates/rest/tests/conditional_capabilities.rs index c383dcf86..ee58abea2 100644 --- a/crates/rest/tests/conditional_capabilities.rs +++ b/crates/rest/tests/conditional_capabilities.rs @@ -1,21 +1,24 @@ //! #1384: `rest.resource.conditional*` and the `501` of an unsupported //! conditional interaction come from one source — what the storage declares. //! -//! The storage that lacks an interaction here is a production-shaped -//! composite (a primary with its index offloaded plus a dedicated search -//! backend — the `*-elasticsearch` arrangement, with a second SQLite standing -//! in for Elasticsearch so no container is needed). It resolves create / -//! update / delete criteria itself and cannot serve conditional patch. +//! The second storage here is a production-shaped composite (a primary with +//! its index offloaded plus a dedicated search backend — the +//! `*-elasticsearch` arrangement, with a second SQLite standing in for +//! Elasticsearch so no container is needed). It resolves conditional criteria +//! itself, through the search backend. //! //! The statement used to advertise every conditional interaction for every //! backend, and a conditional patch on such a composite answered `404` -//! whatever existed. Plain SQLite, which serves all four, is the -//! no-regression half; its `/metadata` is also asserted in -//! `conditional_patch.rs`. +//! whatever existed (then `501`, #1384). Since #1406 the patch applier is +//! shared at the persistence level, the composite serves conditional patch, +//! and declares it — so the statement and the behaviour below changed +//! together, from that one declaration. The `501` half is held by the unit +//! tests of `handlers::conditional_support` and by S3, the storage that still +//! declines. Plain SQLite is the no-regression half; its `/metadata` is also +//! asserted in `conditional_patch.rs`. //! -//! `conditionalPatch` exists from FHIR R5 on, so the statement only shows the -//! difference on a build with R5 or R6 enabled (`--features R4,R4B,R5,R6`); -//! the `501` shows on every build. +//! `conditionalPatch` exists from FHIR R5 on, so the statement only shows it +//! on a build with R5 or R6 enabled (`--features R4,R4B,R5,R6`). use std::collections::HashMap; use std::path::PathBuf; @@ -180,11 +183,11 @@ fn has_conditional_patch_element(version: FhirVersion) -> bool { matches!(version.as_mime_param(), "5.0" | "6.0") } -/// A storage that cannot serve conditional patch says so, and still advertises -/// what it does serve. Conditional read is the REST layer's own (ETag / -/// `Last-Modified`), so it does not vary with the storage. +/// A composite with a dedicated search backend serves all four, and says so +/// (`conditionalPatch` was `false` until #1406). Conditional read is the REST +/// layer's own (ETag / `Last-Modified`), so it does not vary with the storage. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn the_statement_omits_what_the_storage_does_not_serve() { +async fn the_statement_follows_what_the_composite_serves() { let server = composite_server(); for (version, entry) in patient_capabilities(&server).await { @@ -194,7 +197,7 @@ async fn the_statement_omits_what_the_storage_does_not_serve() { assert_eq!(entry["conditionalRead"], "full-support", "{version:?}"); assert_eq!( entry.get("conditionalPatch"), - has_conditional_patch_element(version).then_some(&Value::Bool(false)), + has_conditional_patch_element(version).then_some(&Value::Bool(true)), "{version:?}" ); } @@ -218,35 +221,61 @@ async fn sqlite_advertises_every_conditional_interaction() { } } -/// The interaction the statement leaves out is refused as `501` + -/// `not-supported`, naming what was asked — not answered `404` as though -/// nothing matched — and changes nothing. +/// `PATCH [type]?criteria` on the composite: resolved by the search backend, +/// applied to the primary's content, written through the primary, and visible +/// to the next search. It was `404` whatever existed, then `501` (#1384). #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn an_unsupported_conditional_patch_is_501_and_patches_nothing() { +async fn a_conditional_patch_on_the_composite_patches_the_one_match() { let server = composite_server(); let id = create(&server, "P-1").await; + create(&server, "P-1-decoy").await; // Positive control: the criteria do match, on this storage's search. assert_eq!(found(&server, "P-1").await, 1); - let response = server - .patch("/Patient?identifier=urn:zzz:probe|P-1") - .add_header(X_TENANT_ID, tenant()) - .add_header(header::CONTENT_TYPE, HeaderValue::from_static(JSON_PATCH)) - .bytes(serde_json::to_vec(&activate()).expect("patch").into()) - .await; + let patch = |url: &'static str, if_match: Option<&'static str>, body: Value| { + let mut request = server + .patch(url) + .add_header(X_TENANT_ID, tenant()) + .add_header(header::CONTENT_TYPE, HeaderValue::from_static(JSON_PATCH)); + if let Some(if_match) = if_match { + request = request.add_header(header::IF_MATCH, HeaderValue::from_static(if_match)); + } + request.bytes(serde_json::to_vec(&body).expect("patch").into()) + }; + + // Refused, and nothing written: a stale If-Match, no match, a patch that + // would change the resource's identity, a failed `test`. + patch( + "/Patient?identifier=urn:zzz:probe|P-1", + Some("W/\"7\""), + activate(), + ) + .await + .assert_status(StatusCode::PRECONDITION_FAILED); + patch("/Patient?identifier=urn:zzz:probe|NOBODY", None, activate()) + .await + .assert_status(StatusCode::NOT_FOUND); + for body in [ + json!([{"op": "replace", "path": "/id", "value": "other"}]), + json!([{"op": "replace", "path": "/resourceType", "value": "Person"}]), + json!([{"op": "test", "path": "/active", "value": true}]), + ] { + patch("/Patient?identifier=urn:zzz:probe|P-1", None, body) + .await + .assert_status(StatusCode::BAD_REQUEST); + } - response.assert_status(StatusCode::NOT_IMPLEMENTED); - let outcome: Value = response.json(); - assert_eq!(outcome["resourceType"], "OperationOutcome"); - assert_eq!(outcome["issue"][0]["code"], "not-supported"); - let text = outcome["issue"][0]["details"]["text"] - .as_str() - .or(outcome["issue"][0]["diagnostics"].as_str()) - .unwrap_or_default(); - assert!( - text.contains("conditional patch (PATCH [type]?criteria)"), - "{outcome}" - ); + let response = patch( + "/Patient?identifier=urn:zzz:probe|P-1", + Some("W/\"1\""), + activate(), + ) + .await; + response.assert_status_ok(); + let patched: Value = response.json(); + assert_eq!(patched["id"], id.as_str()); + assert_eq!(patched["active"], true); + assert_eq!(patched["meta"]["versionId"], "2"); let read = server .get(&format!("/Patient/{id}")) @@ -254,8 +283,26 @@ async fn an_unsupported_conditional_patch_is_501_and_patches_nothing() { .await; read.assert_status_ok(); let stored: Value = read.json(); - assert_eq!(stored["active"], false); - assert_eq!(stored["meta"]["versionId"], "1"); + assert_eq!(stored["active"], true); + assert_eq!(stored["meta"]["versionId"], "2"); + + // The search backend followed the write; the decoy did not move. + let active = server + .get("/Patient?active=true") + .add_header(X_TENANT_ID, tenant()) + .await; + active.assert_status_ok(); + let active: Value = active.json(); + let ids: Vec<&str> = active["entry"] + .as_array() + .map(|entries| { + entries + .iter() + .filter_map(|e| e["resource"]["id"].as_str()) + .collect() + }) + .unwrap_or_default(); + assert_eq!(ids, [id.as_str()]); } /// The interactions the same storage does advertise are served: the check is diff --git a/crates/rest/tests/conditional_patch.rs b/crates/rest/tests/conditional_patch.rs index a2d349c90..11983086c 100644 --- a/crates/rest/tests/conditional_patch.rs +++ b/crates/rest/tests/conditional_patch.rs @@ -429,13 +429,19 @@ async fn patch_documents_are_held_to_the_instance_endpoints_rules() { .await .assert_status(StatusCode::NOT_IMPLEMENTED); - // `resourceType` cannot be patched. + // `resourceType` cannot be patched, nor can `id` (#1406: a patched + // `id` used to be silently undone by the backend and answered `200`). for (content_type, body) in [ ( JSON_PATCH, json!([{"op": "replace", "path": "/resourceType", "value": "Person"}]), ), (MERGE_PATCH, json!({"resourceType": "Person"})), + ( + JSON_PATCH, + json!([{"op": "replace", "path": "/id", "value": "other"}]), + ), + (MERGE_PATCH, json!({"id": "other"})), ] { let response = patch(&server, url, content_type, &body).await; assert_outcome(&response, StatusCode::BAD_REQUEST, &format!("{url} {body}")); From 36741b73ecffdab7b20092cb7091d64c0a6c3739 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:05:11 -0400 Subject: [PATCH 06/10] fix(persistence): atomic versioned delete on every backend A `DELETE` carrying `If-Match` was check-then-act everywhere: the REST handler (and each backend's `delete_with_match`, and the conditional-delete gate from #1399) evaluated the precondition against one read and then called the unconditional `delete(id)`. A writer landing in between was deleted along with the version the client named - a version the client never saw - and the client was told 204. Reproduced deterministically over REST (a primary whose `read` is followed by another writer's update: `DELETE` + `If-Match: W/"1"` answered 204 and removed version 2) and by an 8-task race on SQLite and PostgreSQL (an `update` from version 1 and a delete "of version 1" both succeeded). `ResourceStorage::delete_versioned` is now one conditional write per backend: - PostgreSQL: the existing single-statement soft delete gains `AND ($5 IS NULL OR version_id = $5)`, evaluated on the locked row. - SQLite: version compare, tombstone UPDATE (version in its predicate), history row and index cleanup in one IMMEDIATE transaction. - MongoDB: the tombstone `update_one` already filtered on the version it read; the expected version is now compared against that same document. - S3: compared on the object whose ETag the conditional PUT is tied to. - Elasticsearch keeps the documented non-atomic default: it is a search secondary and never the system of record for a version. - CompositeStorage, CompositeSubmitJobs and IndexingSubmitJobs delegate to the primary instead of inheriting the default, and a refused precondition no longer counts against the primary's health. `core::delete_under_precondition` routes the instance `DELETE`, every `conditional_delete` and `delete_with_match` through it when `If-Match` is present; a delete without a precondition is unchanged. Losing the race is `VersionConflict` -> 409, what `PUT` + `If-Match` already answers. Fixes #1404 --- .../src/backends/mongodb/search_impl.rs | 2 +- .../src/backends/mongodb/storage.rs | 378 +++++++++++------- .../src/backends/postgres/storage.rs | 377 +++++++++-------- crates/persistence/src/backends/s3/storage.rs | 151 ++++--- .../src/backends/sqlite/storage.rs | 2 +- .../persistence/src/composite/bulk_submit.rs | 12 + .../src/composite/indexing_submit_jobs.rs | 12 + crates/persistence/src/composite/storage.rs | 64 ++- crates/persistence/src/core/mod.rs | 5 +- crates/persistence/src/core/preconditions.rs | 38 ++ crates/persistence/src/core/storage.rs | 6 +- crates/persistence/tests/mongodb_tests.rs | 16 + crates/persistence/tests/postgres_tests.rs | 40 ++ crates/rest/src/handlers/delete.rs | 32 +- crates/rest/tests/if_match_write_race.rs | 316 +++++++++++++++ 15 files changed, 1078 insertions(+), 373 deletions(-) create mode 100644 crates/rest/tests/if_match_write_race.rs diff --git a/crates/persistence/src/backends/mongodb/search_impl.rs b/crates/persistence/src/backends/mongodb/search_impl.rs index e67b5a3e8..14de7fe1d 100644 --- a/crates/persistence/src/backends/mongodb/search_impl.rs +++ b/crates/persistence/src/backends/mongodb/search_impl.rs @@ -873,7 +873,7 @@ impl ConditionalStorage for MongoBackend { 1 => { let current = matches.into_iter().next().expect("single match must exist"); crate::core::conditional_if_match_gate(if_match, resource_type, Some(¤t))?; - self.delete(tenant, resource_type, current.id()).await?; + crate::core::delete_under_precondition(self, tenant, if_match, ¤t).await?; Ok(ConditionalDeleteResult::Deleted(current)) } n => Ok(ConditionalDeleteResult::MultipleMatches(n)), diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index e9fed97bd..0237f69b1 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -1139,152 +1139,18 @@ impl ResourceStorage for MongoBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let db = self.get_database().await?; - let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); - let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); - let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; - let tenant_id = tenant.tenant_id().as_str(); - - let delete_lookup_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "is_deleted": false, - }; - - let maybe_existing = if let Some(active_session) = session.as_mut() { - resources - .find_one(delete_lookup_filter.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!( - "Failed to check resource before delete (session): {}", - e - )) - })? - } else { - resources - .find_one(delete_lookup_filter) - .await - .map_err(|e| { - internal_error(format!("Failed to check resource before delete: {}", e)) - })? - }; - - let Some(existing_doc) = maybe_existing else { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - }; - - let current_version = existing_doc - .get_str("version_id") - .map_err(|e| internal_error(format!("Missing current version: {}", e)))? - .to_string(); - let new_version = next_version(¤t_version)?; - - let payload = existing_doc - .get_document("data") - .map_err(|e| internal_error(format!("Missing resource payload: {}", e)))? - .clone(); - let fhir_version = existing_doc - .get_str("fhir_version") - .unwrap_or("4.0") - .to_string(); - let created_at = extract_created_at(&existing_doc, Utc::now()); - - let now = Utc::now(); - let now_bson = chrono_to_bson(now); - - let delete_update_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": ¤t_version, - "is_deleted": false, - }; - let delete_update_doc = doc! { - "$set": { - "version_id": &new_version, - "is_deleted": true, - "deleted_at": now_bson, - "last_updated": now_bson, - } - }; - - let update_result = if let Some(active_session) = session.as_mut() { - resources - .update_one(delete_update_filter.clone(), delete_update_doc.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!("Failed to soft-delete resource (session): {}", e)) - })? - } else { - resources - .update_one(delete_update_filter, delete_update_doc) - .await - .map_err(|e| internal_error(format!("Failed to soft-delete resource: {}", e)))? - }; - - if update_result.matched_count == 0 { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - - let history_doc = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": &new_version, - "data": Bson::Document(payload), - "created_at": chrono_to_bson(created_at), - "last_updated": now_bson, - "is_deleted": true, - "deleted_at": now_bson, - "fhir_version": fhir_version, - }; - - if let Some(active_session) = session.as_mut() { - history - .insert_one(history_doc) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!( - "Failed to insert deletion history row (session): {}", - e - )) - })?; - } else { - history.insert_one(history_doc).await.map_err(|e| { - internal_error(format!("Failed to insert deletion history row: {}", e)) - })?; - } - - self.delete_search_index(&db, tenant_id, resource_type, id, &mut session) - .await?; - - commit_best_effort_multi_write_session(&mut session, transaction_active, "delete").await?; - - // A SearchParameter delete may remove a tenant's overlay entry: refresh - // the stored-param cache and drop registries. This must run after the - // commit above: `reload_stored_cache` reads the `resources` collection - // without the session, so it cannot observe the delete while the - // transaction is still open. - if resource_type == "SearchParameter" { - if let Err(e) = self.reload_stored_cache().await { - tracing::warn!("SearchParameter cache reload failed: {e}"); - } - } + self.soft_delete(tenant, resource_type, id, None).await + } - Ok(()) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) + .await } async fn exists( @@ -1953,6 +1819,220 @@ impl SearchIndexDocuments { } impl MongoBackend { + /// The version of the live (not deleted) resource, read outside any + /// session — what a write that just lost a race reports as the version it + /// lost to. + async fn live_version( + &self, + resources: &Collection, + tenant_id: &str, + resource_type: &str, + id: &str, + ) -> StorageResult> { + let live = resources + .find_one(doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "is_deleted": false, + }) + .await + .map_err(|e| internal_error(format!("Failed to reload current version: {}", e)))?; + Ok(live.and_then(|d| d.get_str("version_id").ok().map(str::to_string))) + } + + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + /// + /// The tombstone `update_one` carries the version in its filter, so the + /// comparison and the delete are one conditional write whether or not the + /// deployment supports transactions. `expected_version` is checked against + /// the document that filter is then built from: a `DELETE` with `If-Match` + /// used to be evaluated above this layer against an earlier read and then + /// deleted whatever version was current by the time it got here (#1404). + async fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let db = self.get_database().await?; + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); + let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; + let tenant_id = tenant.tenant_id().as_str(); + + let delete_lookup_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "is_deleted": false, + }; + + let maybe_existing = if let Some(active_session) = session.as_mut() { + resources + .find_one(delete_lookup_filter.clone()) + .session(active_session) + .await + .map_err(|e| { + internal_error(format!( + "Failed to check resource before delete (session): {}", + e + )) + })? + } else { + resources + .find_one(delete_lookup_filter) + .await + .map_err(|e| { + internal_error(format!("Failed to check resource before delete: {}", e)) + })? + }; + + let Some(existing_doc) = maybe_existing else { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + }; + + let current_version = existing_doc + .get_str("version_id") + .map_err(|e| internal_error(format!("Missing current version: {}", e)))? + .to_string(); + if let Some(expected) = expected_version + && expected != current_version + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: current_version, + }, + )); + } + let new_version = next_version(¤t_version)?; + + let payload = existing_doc + .get_document("data") + .map_err(|e| internal_error(format!("Missing resource payload: {}", e)))? + .clone(); + let fhir_version = existing_doc + .get_str("fhir_version") + .unwrap_or("4.0") + .to_string(); + let created_at = extract_created_at(&existing_doc, Utc::now()); + + let now = Utc::now(); + let now_bson = chrono_to_bson(now); + + let delete_update_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": ¤t_version, + "is_deleted": false, + }; + let delete_update_doc = doc! { + "$set": { + "version_id": &new_version, + "is_deleted": true, + "deleted_at": now_bson, + "last_updated": now_bson, + } + }; + + let update_result = if let Some(active_session) = session.as_mut() { + resources + .update_one(delete_update_filter.clone(), delete_update_doc.clone()) + .session(active_session) + .await + .map_err(|e| { + internal_error(format!("Failed to soft-delete resource (session): {}", e)) + })? + } else { + resources + .update_one(delete_update_filter, delete_update_doc) + .await + .map_err(|e| internal_error(format!("Failed to soft-delete resource: {}", e)))? + }; + + if update_result.matched_count == 0 { + // A writer got in after the read above (only possible without a + // transaction). A versioned delete says which way it lost. + if let Some(expected) = expected_version + && let Some(actual) = self + .live_version(&resources, tenant_id, resource_type, id) + .await? + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: actual, + }, + )); + } + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + + let history_doc = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": &new_version, + "data": Bson::Document(payload), + "created_at": chrono_to_bson(created_at), + "last_updated": now_bson, + "is_deleted": true, + "deleted_at": now_bson, + "fhir_version": fhir_version, + }; + + if let Some(active_session) = session.as_mut() { + history + .insert_one(history_doc) + .session(active_session) + .await + .map_err(|e| { + internal_error(format!( + "Failed to insert deletion history row (session): {}", + e + )) + })?; + } else { + history.insert_one(history_doc).await.map_err(|e| { + internal_error(format!("Failed to insert deletion history row: {}", e)) + })?; + } + + self.delete_search_index(&db, tenant_id, resource_type, id, &mut session) + .await?; + + commit_best_effort_multi_write_session(&mut session, transaction_active, "delete").await?; + + // A SearchParameter delete may remove a tenant's overlay entry: refresh + // the stored-param cache and drop registries. This must run after the + // commit above: `reload_stored_cache` reads the `resources` collection + // without the session, so it cannot observe the delete while the + // transaction is still open. + if resource_type == "SearchParameter" { + if let Err(e) = self.reload_stored_cache().await { + tracing::warn!("SearchParameter cache reload failed: {e}"); + } + } + + Ok(()) + } + /// Brings a soft-deleted resource back to life with new content. /// /// FHIR permits a deleted resource to be restored by a subsequent update @@ -2687,7 +2767,13 @@ impl VersionedStorage for MongoBackend { )); } - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against. + // A plain `delete` here was check-then-act: a writer landing after the + // read above was deleted along with the version the client named + // (#1404). + let actual = actual.to_string(); + self.delete_versioned(tenant, resource_type, id, &actual) + .await } async fn list_versions( diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index 4c66e6515..dd13bb2e2 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -601,167 +601,18 @@ impl ResourceStorage for PostgresBackend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let client = self.get_client().await?; - let tenant_id = tenant.tenant_id().as_str(); - - let now = Utc::now(); + self.soft_delete(tenant, resource_type, id, None).await + } - // Soft delete the resource and write its deletion history row, in one - // statement, deriving the tombstone's version from the row itself. - // - // Three things used to be separate here: a `SELECT version_id`, an - // `UPDATE` compare-and-swapping against it, and an `INSERT` of the - // history row. The `INSERT` was folded into the `UPDATE` first; this - // folds in the `SELECT` too, so a delete is one round trip where it was - // three. On the crud suite that is 275,382 statements and 275,382 - // occupied-connection round trips removed from a workload that already - // demands ~36 cores' worth of PostgreSQL execution on a 4-core host — - // the round trip, not the 0.06 ms of execution behind it, is what is - // being bought back. - // - // ## Why this is *more* atomic, not less - // - // The read-then-CAS it replaces was correct but pessimistic. Under READ - // COMMITTED, a writer landing between the `SELECT` and the `UPDATE` - // meant the `version_id = ` predicate matched nothing, and this - // returned `NotFound` — a 404 for a resource that plainly existed and - // was live. Computing `version_id + 1` inside the `UPDATE`'s target list - // removes the window rather than detecting it: PostgreSQL takes the row - // lock, and if the row was concurrently updated it re-evaluates both the - // qualifier and the target list against the *committed new* version of - // the tuple (EvalPlanQual). So the tombstone's version is always exactly - // one more than whatever version is current at the instant the row is - // locked, never one more than a version that has since moved on. - // - // That is what preserves the primary-key fix the CAS was introduced for. - // `resource_history` is keyed `PRIMARY KEY (tenant_id, resource_type, - // id, version_id)` (schema.rs). The failure the CAS prevented was a - // history row computed from a stale read colliding with one a concurrent - // writer had already inserted. A version derived from the locked row - // cannot be stale, so it cannot collide — the invariant is enforced by - // construction instead of by a guard that has to lose a race to notice. - // - // A concurrent *delete* is still resolved correctly and still costs - // nothing extra: the loser re-evaluates `is_deleted = FALSE` against the - // committed tombstone, matches no row, and reports `NotFound`, which is - // exactly what it reported before. - // - // ## What changes, stated plainly - // - // An unconditional `DELETE` that races a concurrent `UPDATE` now - // succeeds — deleting the version that writer just committed — where it - // used to fail with `NotFound`. That is a deliberate correction: FHIR's - // delete interaction (https://hl7.org/fhir/http.html#delete) carries no - // precondition of its own, so "delete the current state" is the right - // reading and the 404 was spurious. Callers that *do* want a - // precondition use `If-Match`, which is evaluated above this layer. - // - // What this does NOT change: that `If-Match` on `DELETE` is evaluated by - // the REST handler (and by `delete_with_match`) against its own earlier - // read and is therefore still check-then-act. It was check-then-act - // before this change too — the CAS removed here guarded the version - // *this function* had read a microsecond earlier, never the version the - // caller's precondition was evaluated against — so no precondition - // guarantee moves in either direction. Making `If-Match` on `DELETE` - // atomic needs the expected version threaded into this statement, which - // is a signature change and a separate piece of work. - // - // ## The version arithmetic - // - // `version_id` is `TEXT`, so the increment is guarded rather than a bare - // cast: a non-numeric value would make `::bigint` raise 22P02 and turn a - // delete into a 500. The `CASE` reproduces the Rust it replaces — - // `current_version.parse::().unwrap_or(0) + 1` — for every value - // this server can have written (`'7'` -> 8, `'007'` -> 8, `''` and - // `'abc'` -> 1, matching `unwrap_or(0)`). `CASE` does not evaluate the - // branch it did not select, so the cast never runs on a value the regex - // rejected. Version ids are server-issued decimal integers on every - // write path in this backend, so the fallback is unreachable in - // practice and is here only so that it degrades the same way the Rust - // did rather than differently. - // - // `RETURNING` feeds the history row from the tuple just written, so the - // deletion entry carries the resource's own `fhir_version` without - // making a round trip through the client. As in `create` and `update`, - // no matching row means the CTE yields nothing, the insert selects - // nothing, and the statement reports zero rows affected — one signal for - // both writes. One statement is also one implicit transaction: the - // tombstone lands with the delete or neither does. - // - // ## The tombstone stores `'null'::jsonb`, not the resource - // - // A deletion entry is the record that the resource was deleted, not a - // version of the resource: FHIR gives it `request.method = DELETE` and - // no `resource` in a history Bundle, and `410 Gone` on a vread of that - // version. `history_entry_to_json` has always omitted the body, and the - // vread handler now answers `410`, so nothing can ask for these bytes. - // - // Storing them was not free. `data` is a JSONB body of a few kilobytes; - // the `UPDATE` above does not touch that column, so `resources` keeps - // its existing TOAST datum untouched, but a TOAST pointer cannot be - // shared across tables — inserting it into `resource_history` detoasts - // the value, re-compresses it, writes it, and puts the whole body in the - // WAL a second time. That was 10.6% of the crud suite's Postgres - // execution time on run 33213565802 for a row no reader can reach. - // - // `'null'::jsonb` rather than `NULL` because `resource_history.data` is - // `NOT NULL` (schema v1) and both `vread` and the history readers deserialise - // the column into a `serde_json::Value` with a non-nullable `FromSql`; - // a SQL `NULL` would panic in `row.get`, and widening the column would - // put a migration and six read sites in the way of a write-path change. - // `Value::Null` reaches the same readers as a well-formed value that - // renders as `null`, and they already discard it for a deleted version. - // - // Rows written by an older build keep their bodies and are read back - // exactly as before; nothing needs backfilling, because the only reader - // was already dropping the value on the floor. - let updated = execute_cached( - &client, - "WITH del AS ( - UPDATE resources - SET is_deleted = TRUE, - deleted_at = $1, - last_updated = $1, - version_id = ((CASE WHEN version_id ~ '^[0-9]+$' THEN version_id::bigint ELSE 0 END) + 1)::text - WHERE tenant_id = $2 AND resource_type = $3 AND id = $4 - AND is_deleted = FALSE - RETURNING tenant_id, resource_type, id, version_id, last_updated, is_deleted, fhir_version - ) - INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) - SELECT tenant_id, resource_type, id, version_id, 'null'::jsonb, last_updated, is_deleted, fhir_version FROM del", - &[&now, &tenant_id, &resource_type, &id], - ) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) .await - .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; - - if updated == 0 { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - } - - // Delete search index entries (skip when search is offloaded) - if !self.is_search_offloaded() { - execute_cached( - &client, - "DELETE FROM search_index WHERE tenant_id = $1 AND resource_type = $2 AND resource_id = $3", - &[&tenant_id, &resource_type, &id], - ) - .await - .map_err(|e| internal_error(format!("Failed to delete search index: {}", e)))?; - } - - // A SearchParameter delete invalidates the tenant overlays. - if resource_type == "SearchParameter" { - if let Err(e) = self.reload_stored_cache().await { - tracing::warn!("SearchParameter cache reload failed: {e}"); - } - } - - Ok(()) } async fn count( @@ -1276,6 +1127,204 @@ impl ResourceStorage for PostgresBackend { // ============================================================================ impl PostgresBackend { + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + async fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let client = self.get_client().await?; + let tenant_id = tenant.tenant_id().as_str(); + + let now = Utc::now(); + + // Soft delete the resource and write its deletion history row, in one + // statement, deriving the tombstone's version from the row itself. + // + // Three things used to be separate here: a `SELECT version_id`, an + // `UPDATE` compare-and-swapping against it, and an `INSERT` of the + // history row. The `INSERT` was folded into the `UPDATE` first; this + // folds in the `SELECT` too, so a delete is one round trip where it was + // three. On the crud suite that is 275,382 statements and 275,382 + // occupied-connection round trips removed from a workload that already + // demands ~36 cores' worth of PostgreSQL execution on a 4-core host — + // the round trip, not the 0.06 ms of execution behind it, is what is + // being bought back. + // + // ## Why this is *more* atomic, not less + // + // The read-then-CAS it replaces was correct but pessimistic. Under READ + // COMMITTED, a writer landing between the `SELECT` and the `UPDATE` + // meant the `version_id = ` predicate matched nothing, and this + // returned `NotFound` — a 404 for a resource that plainly existed and + // was live. Computing `version_id + 1` inside the `UPDATE`'s target list + // removes the window rather than detecting it: PostgreSQL takes the row + // lock, and if the row was concurrently updated it re-evaluates both the + // qualifier and the target list against the *committed new* version of + // the tuple (EvalPlanQual). So the tombstone's version is always exactly + // one more than whatever version is current at the instant the row is + // locked, never one more than a version that has since moved on. + // + // That is what preserves the primary-key fix the CAS was introduced for. + // `resource_history` is keyed `PRIMARY KEY (tenant_id, resource_type, + // id, version_id)` (schema.rs). The failure the CAS prevented was a + // history row computed from a stale read colliding with one a concurrent + // writer had already inserted. A version derived from the locked row + // cannot be stale, so it cannot collide — the invariant is enforced by + // construction instead of by a guard that has to lose a race to notice. + // + // A concurrent *delete* is still resolved correctly and still costs + // nothing extra: the loser re-evaluates `is_deleted = FALSE` against the + // committed tombstone, matches no row, and reports `NotFound`, which is + // exactly what it reported before. + // + // ## What changes, stated plainly + // + // An unconditional `DELETE` that races a concurrent `UPDATE` now + // succeeds — deleting the version that writer just committed — where it + // used to fail with `NotFound`. That is a deliberate correction: FHIR's + // delete interaction (https://hl7.org/fhir/http.html#delete) carries no + // precondition of its own, so "delete the current state" is the right + // reading and the 404 was spurious. Callers that *do* want a + // precondition use `If-Match`, which is evaluated above this layer. + // + // ## The versioned form (#1404) + // + // `If-Match` on `DELETE` used to be evaluated above this layer against + // an earlier read and followed by this unconditional statement — + // check-then-act, so a writer landing in between was deleted along with + // the version the client named. `expected_version` threads the + // precondition into the statement: `$5 IS NULL OR version_id = $5` is + // evaluated on the locked row, and re-evaluated by EvalPlanQual against + // the committed tuple if a writer got there first, so the comparison and + // the delete cannot be separated. With no expected version the + // predicate is constant-true and the statement is the one above. + // + // ## The version arithmetic + // + // `version_id` is `TEXT`, so the increment is guarded rather than a bare + // cast: a non-numeric value would make `::bigint` raise 22P02 and turn a + // delete into a 500. The `CASE` reproduces the Rust it replaces — + // `current_version.parse::().unwrap_or(0) + 1` — for every value + // this server can have written (`'7'` -> 8, `'007'` -> 8, `''` and + // `'abc'` -> 1, matching `unwrap_or(0)`). `CASE` does not evaluate the + // branch it did not select, so the cast never runs on a value the regex + // rejected. Version ids are server-issued decimal integers on every + // write path in this backend, so the fallback is unreachable in + // practice and is here only so that it degrades the same way the Rust + // did rather than differently. + // + // `RETURNING` feeds the history row from the tuple just written, so the + // deletion entry carries the resource's own `fhir_version` without + // making a round trip through the client. As in `create` and `update`, + // no matching row means the CTE yields nothing, the insert selects + // nothing, and the statement reports zero rows affected — one signal for + // both writes. One statement is also one implicit transaction: the + // tombstone lands with the delete or neither does. + // + // ## The tombstone stores `'null'::jsonb`, not the resource + // + // A deletion entry is the record that the resource was deleted, not a + // version of the resource: FHIR gives it `request.method = DELETE` and + // no `resource` in a history Bundle, and `410 Gone` on a vread of that + // version. `history_entry_to_json` has always omitted the body, and the + // vread handler now answers `410`, so nothing can ask for these bytes. + // + // Storing them was not free. `data` is a JSONB body of a few kilobytes; + // the `UPDATE` above does not touch that column, so `resources` keeps + // its existing TOAST datum untouched, but a TOAST pointer cannot be + // shared across tables — inserting it into `resource_history` detoasts + // the value, re-compresses it, writes it, and puts the whole body in the + // WAL a second time. That was 10.6% of the crud suite's Postgres + // execution time on run 33213565802 for a row no reader can reach. + // + // `'null'::jsonb` rather than `NULL` because `resource_history.data` is + // `NOT NULL` (schema v1) and both `vread` and the history readers deserialise + // the column into a `serde_json::Value` with a non-nullable `FromSql`; + // a SQL `NULL` would panic in `row.get`, and widening the column would + // put a migration and six read sites in the way of a write-path change. + // `Value::Null` reaches the same readers as a well-formed value that + // renders as `null`, and they already discard it for a deleted version. + // + // Rows written by an older build keep their bodies and are read back + // exactly as before; nothing needs backfilling, because the only reader + // was already dropping the value on the floor. + let updated = execute_cached( + &client, + "WITH del AS ( + UPDATE resources + SET is_deleted = TRUE, + deleted_at = $1, + last_updated = $1, + version_id = ((CASE WHEN version_id ~ '^[0-9]+$' THEN version_id::bigint ELSE 0 END) + 1)::text + WHERE tenant_id = $2 AND resource_type = $3 AND id = $4 + AND is_deleted = FALSE + AND ($5::text IS NULL OR version_id = $5::text) + RETURNING tenant_id, resource_type, id, version_id, last_updated, is_deleted, fhir_version + ) + INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) + SELECT tenant_id, resource_type, id, version_id, 'null'::jsonb, last_updated, is_deleted, fhir_version FROM del", + &[&now, &tenant_id, &resource_type, &id, &expected_version], + ) + .await + .map_err(|e| internal_error(format!("Failed to delete resource: {}", e)))?; + + if updated == 0 { + // Matched nothing. For a versioned delete that is either "nothing + // live" or "live at another version"; telling them apart costs a + // query, but only on the path that is already failing. + if let Some(expected) = expected_version { + let actual = client + .query_opt( + "SELECT version_id FROM resources + WHERE tenant_id = $1 AND resource_type = $2 AND id = $3 AND is_deleted = FALSE", + &[&tenant_id, &resource_type, &id], + ) + .await + .map_err(|e| internal_error(format!("Failed to get current version: {}", e)))?; + if let Some(row) = actual { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: row.get::<_, String>(0), + }, + )); + } + } + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + } + + // Delete search index entries (skip when search is offloaded) + if !self.is_search_offloaded() { + execute_cached( + &client, + "DELETE FROM search_index WHERE tenant_id = $1 AND resource_type = $2 AND resource_id = $3", + &[&tenant_id, &resource_type, &id], + ) + .await + .map_err(|e| internal_error(format!("Failed to delete search index: {}", e)))?; + } + + // A SearchParameter delete invalidates the tenant overlays. + if resource_type == "SearchParameter" { + if let Err(e) = self.reload_stored_cache().await { + tracing::warn!("SearchParameter cache reload failed: {e}"); + } + } + + Ok(()) + } + /// Brings a soft-deleted resource back to life with new content. /// /// FHIR permits a deleted resource to be restored by a subsequent update @@ -2136,8 +2185,12 @@ impl VersionedStorage for PostgresBackend { )); } - // Perform delete - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against. + // A plain `delete` here was check-then-act: a writer landing after the + // read above was deleted along with the version the client named + // (#1404). + self.delete_versioned(tenant, resource_type, id, ¤t_version) + .await } async fn list_versions( @@ -3163,7 +3216,7 @@ impl ConditionalStorage for PostgresBackend { // Exactly one match - delete it let existing = matches.into_iter().next().unwrap(); crate::core::conditional_if_match_gate(if_match, resource_type, Some(&existing))?; - self.delete(tenant, resource_type, existing.id()).await?; + crate::core::delete_under_precondition(self, tenant, if_match, &existing).await?; Ok(ConditionalDeleteResult::Deleted(existing)) } n => { diff --git a/crates/persistence/src/backends/s3/storage.rs b/crates/persistence/src/backends/s3/storage.rs index 9c4c3736b..f2a31fed1 100644 --- a/crates/persistence/src/backends/s3/storage.rs +++ b/crates/persistence/src/backends/s3/storage.rs @@ -44,6 +44,90 @@ pub(crate) struct CurrentResourceWithMeta { } impl S3Backend { + /// Soft-deletes a resource, optionally only at `expected_version` + /// ([`ResourceStorage::delete`] / [`ResourceStorage::delete_versioned`]). + /// + /// The tombstone is a conditional PUT on the ETag of the object loaded + /// here, and `expected_version` is compared on that same object — so a + /// writer landing after the comparison changes the ETag and the PUT is + /// refused (`OptimisticLockFailure`) rather than deleting a version the + /// caller never saw (#1404). That guarantee is the object store's + /// conditional write: a store that ignores `If-Match` on PUT gives none. + async fn soft_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> StorageResult<()> { + tenant.check_permission(Operation::Delete, resource_type)?; + + let location = self.tenant_location(tenant)?; + let current_key = location.keyspace.current_resource_key(resource_type, id); + + let Some(actual) = self + .load_current_with_meta(tenant, resource_type, id) + .await? + else { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })); + }; + + if actual.resource.is_deleted() { + return Err(StorageError::Resource(ResourceError::Gone { + resource_type: resource_type.to_string(), + id: id.to_string(), + deleted_at: actual.resource.deleted_at(), + })); + } + + if let Some(expected) = expected_version + && expected != actual.resource.version_id() + { + return Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected.to_string(), + actual_version: actual.resource.version_id().to_string(), + }, + )); + } + + let deleted = actual.resource.mark_deleted(); + let payload = self.serialize_json(&deleted)?; + + match self + .put_json_object( + &location.bucket, + ¤t_key, + &payload, + actual.etag.as_deref(), + None, + ) + .await + { + Ok(_) => { + self.put_history_and_indexes(&location, &deleted, HistoryMethod::Delete) + .await?; + self.maybe_reload_search_param_cache(tenant, resource_type, None) + .await; + Ok(()) + } + Err(StorageError::Backend(BackendError::QueryError { .. })) => Err( + StorageError::Concurrency(ConcurrencyError::OptimisticLockFailure { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_etag: actual.etag.unwrap_or_default(), + actual_etag: None, + }), + ), + Err(err) => Err(err), + } + } + /// Serialises `value` to a JSON byte vector. pub(crate) fn serialize_json(&self, value: &T) -> StorageResult> { serde_json::to_vec(value).map_err(|e| { @@ -939,59 +1023,18 @@ impl ResourceStorage for S3Backend { resource_type: &str, id: &str, ) -> StorageResult<()> { - tenant.check_permission(Operation::Delete, resource_type)?; - - let location = self.tenant_location(tenant)?; - let current_key = location.keyspace.current_resource_key(resource_type, id); - - let Some(actual) = self - .load_current_with_meta(tenant, resource_type, id) - .await? - else { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - }; - - if actual.resource.is_deleted() { - return Err(StorageError::Resource(ResourceError::Gone { - resource_type: resource_type.to_string(), - id: id.to_string(), - deleted_at: actual.resource.deleted_at(), - })); - } - - let deleted = actual.resource.mark_deleted(); - let payload = self.serialize_json(&deleted)?; + self.soft_delete(tenant, resource_type, id, None).await + } - match self - .put_json_object( - &location.bucket, - ¤t_key, - &payload, - actual.etag.as_deref(), - None, - ) + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.soft_delete(tenant, resource_type, id, Some(expected_version)) .await - { - Ok(_) => { - self.put_history_and_indexes(&location, &deleted, HistoryMethod::Delete) - .await?; - self.maybe_reload_search_param_cache(tenant, resource_type, None) - .await; - Ok(()) - } - Err(StorageError::Backend(BackendError::QueryError { .. })) => Err( - StorageError::Concurrency(ConcurrencyError::OptimisticLockFailure { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_etag: actual.etag.unwrap_or_default(), - actual_etag: None, - }), - ), - Err(err) => Err(err), - } } async fn count( @@ -1406,7 +1449,11 @@ impl VersionedStorage for S3Backend { )); } - self.delete(tenant, resource_type, id).await + // Delete exactly the version the precondition was evaluated against, + // not whatever is current by the time `delete` loads it again (#1404). + let actual_version = actual_version.to_string(); + self.delete_versioned(tenant, resource_type, id, &actual_version) + .await } async fn list_versions( diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index 2b7d41f69..c571bbf0d 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -3301,7 +3301,7 @@ impl ConditionalStorage for SqliteBackend { // Exactly one match - delete it let existing = matches.into_iter().next().unwrap(); crate::core::conditional_if_match_gate(if_match, resource_type, Some(&existing))?; - self.delete(tenant, resource_type, existing.id()).await?; + crate::core::delete_under_precondition(self, tenant, if_match, &existing).await?; Ok(ConditionalDeleteResult::Deleted(existing)) } n => { diff --git a/crates/persistence/src/composite/bulk_submit.rs b/crates/persistence/src/composite/bulk_submit.rs index 7110bfad7..de91bc782 100644 --- a/crates/persistence/src/composite/bulk_submit.rs +++ b/crates/persistence/src/composite/bulk_submit.rs @@ -470,6 +470,18 @@ impl ResourceStorage for CompositeSubmitJobs { self.composite.delete(tenant, resource_type, id).await } + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.composite + .delete_versioned(tenant, resource_type, id, expected_version) + .await + } + async fn exists( &self, tenant: &TenantContext, diff --git a/crates/persistence/src/composite/indexing_submit_jobs.rs b/crates/persistence/src/composite/indexing_submit_jobs.rs index ff794e327..918eccfe4 100644 --- a/crates/persistence/src/composite/indexing_submit_jobs.rs +++ b/crates/persistence/src/composite/indexing_submit_jobs.rs @@ -239,6 +239,18 @@ impl ResourceStorage for IndexingSubmitJobs { self.inner.delete(tenant, resource_type, id).await } + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.inner + .delete_versioned(tenant, resource_type, id, expected_version) + .await + } + async fn exists( &self, tenant: &TenantContext, diff --git a/crates/persistence/src/composite/storage.rs b/crates/persistence/src/composite/storage.rs index 1eab608e6..cd60bc584 100644 --- a/crates/persistence/src/composite/storage.rs +++ b/crates/persistence/src/composite/storage.rs @@ -1080,6 +1080,57 @@ impl ResourceStorage for CompositeStorage { Ok(()) } + /// The primary is the system of record for versions, so the compare-and- + /// swap is its alone; secondaries are told about the delete only once it + /// has won. Inheriting the trait's default here would turn the primary's + /// atomic delete back into read-compare-delete (#1404). + #[instrument(skip(self, tenant), fields(resource_type = %resource_type, id = %id))] + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + let result = self + .primary + .delete_versioned(tenant, resource_type, id, expected_version) + .await; + + // A refused precondition is the primary working, not the primary + // failing: racing clients must not be able to mark it unhealthy. + let refused = matches!( + result, + Err(StorageError::Concurrency(_) | StorageError::Resource(_)) + ); + let primary_id = self.config.primary_id().unwrap_or("primary"); + self.update_health( + primary_id, + result.is_ok() || refused, + result + .as_ref() + .err() + .filter(|_| !refused) + .map(|e| e.to_string()), + ); + + result?; + + // Sync to secondaries + if let Err(e) = self + .sync_to_secondaries(SyncEvent::Delete { + resource_type: resource_type.to_string(), + resource_id: id.to_string(), + tenant_id: tenant.tenant_id().clone(), + }) + .await + { + warn!(error = %e, "Failed to sync delete to secondaries"); + } + + Ok(()) + } + async fn count( &self, tenant: &TenantContext, @@ -1609,9 +1660,16 @@ impl ConditionalStorage for CompositeStorage { resource_type, Some(¤t), )?; - self.primary - .delete(tenant, resource_type, current.id()) - .await?; + // `current` is the search backend's copy; the primary's + // compare-and-swap runs on its version, so a stale copy + // is a 409, not a delete (#1404). + crate::core::delete_under_precondition( + self.primary.as_ref(), + tenant, + if_match, + ¤t, + ) + .await?; if let Err(e) = self .sync_to_secondaries(SyncEvent::Delete { diff --git a/crates/persistence/src/core/mod.rs b/crates/persistence/src/core/mod.rs index 7c17469c7..ed035fb5a 100644 --- a/crates/persistence/src/core/mod.rs +++ b/crates/persistence/src/core/mod.rs @@ -160,8 +160,9 @@ pub use history::{ }; pub use preconditions::{ EntityTag, EntityTagPrecondition, MalformedPrecondition, bundle_if_match_gate, - bundle_if_none_exist_gate, conditional_if_match_gate, if_match_field_satisfied, - multiple_matches_entry, not_supported_entry, precondition_failed_entry, + bundle_if_none_exist_gate, conditional_if_match_gate, delete_under_precondition, + if_match_field_satisfied, multiple_matches_entry, not_supported_entry, + precondition_failed_entry, }; pub use search::{ ChainedSearchProvider, FullSearchProvider, INCLUDE_TRUNCATION_OUTCOME_ID, IncludeProvider, diff --git a/crates/persistence/src/core/preconditions.rs b/crates/persistence/src/core/preconditions.rs index aff395673..8f967d909 100644 --- a/crates/persistence/src/core/preconditions.rs +++ b/crates/persistence/src/core/preconditions.rs @@ -496,6 +496,44 @@ pub fn conditional_if_match_gate( )) } +/// Deletes `current` — the resource an `If-Match` precondition has just been +/// evaluated against — so that the evaluation and the delete are one step. +/// +/// With a precondition the delete goes through +/// [`ResourceStorage::delete_versioned`](super::ResourceStorage::delete_versioned), +/// pinned to `current`'s version: a writer landing after the evaluation ends +/// in `VersionConflict` instead of being deleted along with the version the +/// client named (#1404). Without one it is the plain, unconditional +/// [`delete`](super::ResourceStorage::delete) it always was — FHIR's delete +/// carries no precondition of its own. +/// +/// Shared by `DELETE [type]/[id]` and every +/// [`ConditionalStorage::conditional_delete`](super::ConditionalStorage::conditional_delete). +pub async fn delete_under_precondition( + storage: &S, + tenant: &crate::tenant::TenantContext, + if_match: &EntityTagPrecondition, + current: &StoredResource, +) -> crate::error::StorageResult<()> +where + S: super::ResourceStorage + ?Sized, +{ + if if_match.is_present() { + storage + .delete_versioned( + tenant, + current.resource_type(), + current.id(), + current.version_id(), + ) + .await + } else { + storage + .delete(tenant, current.resource_type(), current.id()) + .await + } +} + /// Builds the `412` bundle entry result used by every backend. pub fn precondition_failed_entry(diagnostics: &str) -> BundleEntryResult { BundleEntryResult::error( diff --git a/crates/persistence/src/core/storage.rs b/crates/persistence/src/core/storage.rs index 1d4a49566..ac210860c 100644 --- a/crates/persistence/src/core/storage.rs +++ b/crates/persistence/src/core/storage.rs @@ -574,9 +574,11 @@ pub trait ResourceStorage: Send + Sync { /// # Errors /// /// * `StorageError::Resource(NotFound)` - If no live resource exists - /// (never created, or already deleted) + /// (never created, or already deleted; S3 reports the latter as `Gone`, + /// as its `delete` does) /// * `StorageError::Concurrency(VersionConflict)` - If the current version - /// is not `expected_version`; nothing is deleted + /// is not `expected_version`; nothing is deleted. S3 reports a writer + /// that lands after its comparison as `OptimisticLockFailure`. /// * `StorageError::Tenant` - If the tenant doesn't have delete permission async fn delete_versioned( &self, diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 3aa39e373..088995591 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -759,6 +759,22 @@ async fn mongodb_empty_values_are_rejected_on_every_path() { empty_value_suite::empty_values_are_rejected_on_every_path(&backend, "empty-value-1380").await; } +/// The backend-agnostic race suite for version-aware writes (#1404, #1405). +/// Same `#[path]` arrangement. +#[path = "search/versioned_write_race_suite.rs"] +mod versioned_write_race_suite; + +/// #1404: `delete_versioned` compares and deletes in one step. +#[tokio::test] +async fn mongodb_versioned_delete_is_a_compare_and_swap() { + let Some(backend) = create_backend("delete_cas_1404").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::versioned_delete_is_a_compare_and_swap(&backend, "delete-cas-1404") + .await; +} + /// #1062: a comma-separated value list on one `SearchParameter` is OR per /// FHIR (https://build.fhir.org/search.html#combining) — for date same as /// every other type. Drives the real `SearchProvider::search` / diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 3db141d04..ecf702851 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -108,6 +108,11 @@ mod conditional_if_match_suite; #[path = "search/empty_value_suite.rs"] mod empty_value_suite; +/// The backend-agnostic race suite for version-aware writes (#1404, #1405). +/// Same `#[path]` arrangement. +#[path = "search/versioned_write_race_suite.rs"] +mod versioned_write_race_suite; + #[path = "common/container_cleanup.rs"] mod container_cleanup; @@ -19190,4 +19195,39 @@ mod postgres_integration { assert!(included.is_empty()); } + + /// #1404: of several writers holding the same version, one `update` writes. + #[tokio::test(flavor = "multi_thread", worker_threads = 8)] + async fn postgres_integration_concurrent_updates_from_the_same_version_admit_one() { + let backend = create_backend().await; + super::versioned_write_race_suite::concurrent_updates_from_the_same_version_admit_one( + std::sync::Arc::new(backend), + &unique_base("update_race_1404"), + 10, + ) + .await; + } + + /// #1404: an update and a versioned delete of the same version: one wins. + #[tokio::test(flavor = "multi_thread", worker_threads = 8)] + async fn postgres_integration_concurrent_update_and_versioned_delete_admit_one() { + let backend = create_backend().await; + super::versioned_write_race_suite::concurrent_update_and_versioned_delete_admit_one( + std::sync::Arc::new(backend), + &unique_base("delete_race_1404"), + 10, + ) + .await; + } + + /// #1404: `delete_versioned` compares and deletes in one step. + #[tokio::test] + async fn postgres_integration_versioned_delete_is_a_compare_and_swap() { + let backend = create_backend().await; + super::versioned_write_race_suite::versioned_delete_is_a_compare_and_swap( + &backend, + &unique_base("delete_cas_1404"), + ) + .await; + } } diff --git a/crates/rest/src/handlers/delete.rs b/crates/rest/src/handlers/delete.rs index 726c84fb3..1f6c6e17b 100644 --- a/crates/rest/src/handlers/delete.rs +++ b/crates/rest/src/handlers/delete.rs @@ -37,6 +37,8 @@ use crate::state::AppState; /// - `200 OK` - Resource deleted, returning OperationOutcome /// - `404 Not Found` - Resource does not exist, or is already deleted /// - `412 Precondition Failed` - `If-Match` was supplied and is not satisfied +/// - `409 Conflict` - `If-Match` was satisfied, but another writer changed the +/// resource before the delete landed (#1404); nothing is deleted /// - `405 Method Not Allowed` - `AuditEvent` resources are immutable /// /// # The `If-Match` precondition @@ -169,10 +171,32 @@ where // Perform the delete. Everything above this line is a refusal path; nothing // below it may run for a request that failed its precondition. - state - .storage() - .delete(tenant.context(), &resource_type, &id) - .await?; + // + // With `If-Match` the delete is pinned to the version the precondition was + // just evaluated against, inside storage's own compare-and-swap. A plain + // `delete` here was check-then-act: a writer landing after the read above + // was deleted along with the version the client named, one it never saw + // (#1404). Losing that race is `VersionConflict` -> 409, what `PUT` with + // `If-Match` answers for the same race. A satisfied precondition implies a + // current resource, so the `None` arm is the precondition-less delete of + // something absent: storage's own `NotFound` -> 404, as before. + match existing_resource.as_ref() { + Some(current) => { + helios_persistence::core::delete_under_precondition( + state.storage(), + tenant.context(), + &if_match, + current, + ) + .await? + } + None => { + state + .storage() + .delete(tenant.context(), &resource_type, &id) + .await? + } + } debug!( resource_type = %resource_type, diff --git a/crates/rest/tests/if_match_write_race.rs b/crates/rest/tests/if_match_write_race.rs new file mode 100644 index 000000000..fbe5639bc --- /dev/null +++ b/crates/rest/tests/if_match_write_race.rs @@ -0,0 +1,316 @@ +//! #1404: `If-Match` on the instance `PUT`, `PATCH` and `DELETE` is a +//! compare-and-swap in storage, not a check in the handler followed by an +//! unconditional write. +//! +//! The handlers read the current resource to evaluate the precondition and +//! then write. These tests land a second writer *between* those two steps, +//! deterministically: the primary backend is wrapped so that a `read` can be +//! armed to update the resource right after it has produced its answer. The +//! handler therefore sees version 1, the precondition `W/"1"` is satisfied, +//! and by the time the handler writes, version 2 — which the client never saw +//! — is current. The write must be refused (409) and version 2 must survive. +//! +//! Before the fix `DELETE` answered `204` and deleted version 2. + +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::{Arc, Mutex}; + +use async_trait::async_trait; +use axum::http::StatusCode; +use axum_test::TestServer; +use helios_fhir::FhirVersion; +use helios_persistence::backends::sqlite::{SqliteBackend, SqliteBackendConfig}; +use helios_persistence::composite::{CompositeConfig, CompositeStorage, DynStorage}; +use helios_persistence::core::{BackendKind, ResourceStorage}; +use helios_persistence::error::StorageResult; +use helios_persistence::tenant::TenantContext; +use helios_persistence::types::StoredResource; +use helios_rest::ServerConfig; +use serde_json::{Value, json}; + +fn sqlite() -> SqliteBackend { + let data_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../../data") + .canonicalize() + .expect("repo data dir"); + let backend = SqliteBackend::with_config( + ":memory:", + SqliteBackendConfig { + data_dir: Some(data_dir), + ..Default::default() + }, + ) + .expect("in-memory SQLite"); + backend.init_schema().expect("init schema"); + backend +} + +/// A primary whose next `read` is followed — before the caller can act on what +/// it read — by another writer's update. +struct RacingPrimary { + inner: SqliteBackend, + /// Content the interloper writes after the next `read`; taken when used. + interloper: Mutex>, +} + +#[async_trait] +impl ResourceStorage for RacingPrimary { + fn backend_name(&self) -> &'static str { + "racing-primary" + } + + async fn create( + &self, + tenant: &TenantContext, + resource_type: &str, + resource: Value, + fhir_version: FhirVersion, + ) -> StorageResult { + self.inner + .create(tenant, resource_type, resource, fhir_version) + .await + } + + async fn create_or_update( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + resource: Value, + fhir_version: FhirVersion, + ) -> StorageResult<(StoredResource, bool)> { + self.inner + .create_or_update(tenant, resource_type, id, resource, fhir_version) + .await + } + + async fn read( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + ) -> StorageResult> { + let seen = self.inner.read(tenant, resource_type, id).await?; + let interloper = self.interloper.lock().expect("interloper lock").take(); + if let (Some(seen), Some(content)) = (seen.as_ref(), interloper) { + self.inner + .update(tenant, seen, content) + .await + .expect("the interloper's update lands"); + } + Ok(seen) + } + + async fn update( + &self, + tenant: &TenantContext, + current: &StoredResource, + resource: Value, + ) -> StorageResult { + self.inner.update(tenant, current, resource).await + } + + async fn delete( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + ) -> StorageResult<()> { + self.inner.delete(tenant, resource_type, id).await + } + + // A wrapper around a real backend delegates; the trait's default is + // read-compare-delete and would reopen the window. + async fn delete_versioned( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + ) -> StorageResult<()> { + self.inner + .delete_versioned(tenant, resource_type, id, expected_version) + .await + } + + async fn count( + &self, + tenant: &TenantContext, + resource_type: Option<&str>, + ) -> StorageResult { + self.inner.count(tenant, resource_type).await + } +} + +fn server() -> (TestServer, Arc) { + let primary = Arc::new(RacingPrimary { + inner: sqlite(), + interloper: Mutex::new(None), + }); + + let config = CompositeConfig::builder() + .primary("sqlite", BackendKind::Sqlite) + .build() + .expect("composite config"); + let mut backends: HashMap = HashMap::new(); + backends.insert("sqlite".to_string(), primary.clone() as DynStorage); + let composite = Arc::new(CompositeStorage::new(config, backends).expect("composite")); + + let state = helios_rest::AppState::new( + composite, + ServerConfig { + base_url: "http://localhost:8080".to_string(), + ..ServerConfig::for_testing() + }, + ); + let server = TestServer::new(helios_rest::routing::fhir_routes::create_routes(state)) + .expect("create test server"); + (server, primary) +} + +fn patient(id: &str, family: &str) -> Value { + json!({"resourceType": "Patient", "id": id, "name": [{"family": family}]}) +} + +/// Creates `Patient/{id}` and proves it is live at version 1 with `If-Match` +/// honoured at all — the positive control for every refusal below. +async fn seed(server: &TestServer, id: &str) { + let created = server + .put(&format!("/Patient/{id}")) + .json(&patient(id, "Seed")) + .await; + assert_eq!( + created.status_code(), + StatusCode::CREATED, + "{}", + created.text() + ); + + let read = server.get(&format!("/Patient/{id}")).await; + assert_eq!(read.status_code(), StatusCode::OK); + assert_eq!(read.json::()["meta"]["versionId"], "1"); + + let stale = server + .delete(&format!("/Patient/{id}")) + .add_header("If-Match", "W/\"7\"") + .await; + assert_eq!( + stale.status_code(), + StatusCode::PRECONDITION_FAILED, + "an unsatisfied If-Match is refused by the handler's own check" + ); +} + +/// The interloper's version 2 is current and intact. +async fn assert_interloper_survives(server: &TestServer, id: &str) { + let read = server.get(&format!("/Patient/{id}")).await; + assert_eq!( + read.status_code(), + StatusCode::OK, + "version 2 was never seen by the client and must not have been removed: {}", + read.text() + ); + let body = read.json::(); + assert_eq!(body["meta"]["versionId"], "2"); + assert_eq!(body["name"][0]["family"], "Interloper"); +} + +#[tokio::test] +async fn delete_with_if_match_does_not_delete_a_version_written_after_the_check() { + let (server, primary) = server(); + seed(&server, "del").await; + + *primary.interloper.lock().unwrap() = Some(patient("del", "Interloper")); + let response = server + .delete("/Patient/del") + .add_header("If-Match", "W/\"1\"") + .await; + + assert_eq!( + response.status_code(), + StatusCode::CONFLICT, + "the delete lost to a concurrent writer: {}", + response.text() + ); + assert_interloper_survives(&server, "del").await; + + // The client re-reads, sees version 2, and may delete that. + let retry = server + .delete("/Patient/del") + .add_header("If-Match", "W/\"2\"") + .await; + assert_eq!( + retry.status_code(), + StatusCode::NO_CONTENT, + "{}", + retry.text() + ); + assert_eq!( + server.get("/Patient/del").await.status_code(), + StatusCode::GONE + ); +} + +#[tokio::test] +async fn delete_without_if_match_still_deletes_whatever_is_current() { + let (server, primary) = server(); + seed(&server, "plain").await; + + *primary.interloper.lock().unwrap() = Some(patient("plain", "Interloper")); + let response = server.delete("/Patient/plain").await; + + assert_eq!( + response.status_code(), + StatusCode::NO_CONTENT, + "FHIR's delete carries no precondition of its own: {}", + response.text() + ); + assert_eq!( + server.get("/Patient/plain").await.status_code(), + StatusCode::GONE + ); +} + +#[tokio::test] +async fn put_with_if_match_does_not_overwrite_a_version_written_after_the_check() { + let (server, primary) = server(); + seed(&server, "put").await; + + *primary.interloper.lock().unwrap() = Some(patient("put", "Interloper")); + let response = server + .put("/Patient/put") + .add_header("If-Match", "W/\"1\"") + .json(&patient("put", "Overwriter")) + .await; + + assert_eq!( + response.status_code(), + StatusCode::CONFLICT, + "{}", + response.text() + ); + assert_interloper_survives(&server, "put").await; +} + +#[tokio::test] +async fn patch_with_if_match_does_not_overwrite_a_version_written_after_the_check() { + let (server, primary) = server(); + seed(&server, "patch").await; + + *primary.interloper.lock().unwrap() = Some(patient("patch", "Interloper")); + let response = server + .patch("/Patient/patch") + .add_header("If-Match", "W/\"1\"") + .text(r#"[{"op":"replace","path":"/name/0/family","value":"Patcher"}]"#) + .content_type("application/json-patch+json") + .await; + + assert_eq!( + response.status_code(), + StatusCode::CONFLICT, + "{}", + response.text() + ); + assert_interloper_survives(&server, "patch").await; +} From 073ed60a52079412fdc370878eb84f5d4614c14d Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:11:19 -0400 Subject: [PATCH 07/10] fix(mongodb): report a lost write race as a version conflict, not a 500 MongoDB's `update` and `delete` run in a multi-document transaction on a replica set. MongoDB does not queue a conflicting writer behind the first one the way PostgreSQL queues it behind a row lock: the loser's `update_one` fails immediately with Error code 112 (WriteConflict) ... labels: {"TransientTransactionError"} (captured from the replica-set test container with eight tasks holding the same version; it is raised by the write inside the transaction, not at commit, and no duplicate-key error on the history collection was seen). Every driver error on that path was wrapped as `BackendError::Internal`, so the client got `500 Internal Server Error` for "you lost a race, read and retry". The loser wrote nothing; only the classification was wrong. Code 112 and the `TransientTransactionError` label (the transaction did not and will not commit) now surface as `ConcurrencyError::VersionConflict` against whatever is live afterwards - `NotFound` when the winner was a delete - which REST already renders as 409, the answer PostgreSQL gives for the same race. `UnknownTransactionCommitResult` is deliberately not classified: there the write may have landed. No retry where the write carries a precondition: `update` always names a version and so does `delete_versioned`, and the writer they lost to has moved the resource on. The unconditional `delete` - "delete whatever is current" - gets ONE retry after a short pause, the driver's recommended handling of a transient transaction error. Removes the MongoDB exemption from #1399's eight-writer test, and adds the MongoDB runs of `versioned_write_race_suite.rs` plus a race between updates and unconditional deletes (the retried path) on all three backends. Fixes #1405 --- .../src/backends/mongodb/storage.rs | 631 ++++++++++++------ crates/persistence/tests/mongodb_tests.rs | 50 +- crates/persistence/tests/postgres_tests.rs | 14 +- .../search/conditional_if_match_suite.rs | 21 +- .../search/versioned_write_race_suite.rs | 101 +++ crates/persistence/tests/sqlite_tests.rs | 14 +- 6 files changed, 606 insertions(+), 225 deletions(-) diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index 0237f69b1..a8ac8ec0e 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -61,6 +61,67 @@ pub(super) fn is_duplicate_key_error(err: &MongoError) -> bool { err.to_string().contains("E11000") } +/// The server's `WriteConflict` code. +const WRITE_CONFLICT_CODE: i32 = 112; + +/// Pause before the single retry of an unconditional delete that hit a write +/// conflict: long enough for the winner's transaction to commit, so the retry +/// does not just collide with it again. +const WRITE_CONFLICT_RETRY_DELAY: std::time::Duration = std::time::Duration::from_millis(25); + +/// Why one attempt at a versioned write failed. +enum WriteAttemptError { + /// The server refused the write because a concurrent writer got there + /// first ([`is_write_conflict`]); nothing was written. + Conflict(MongoError), + /// Anything else, already in its final form. + Storage(StorageError), +} + +impl WriteAttemptError { + /// Classifies a driver error raised inside the attempt's session. + fn driver(context: &str, err: MongoError) -> Self { + if is_write_conflict(&err) { + Self::Conflict(err) + } else { + Self::Storage(internal_error(format!("{}: {}", context, err))) + } + } +} + +impl> From for WriteAttemptError { + fn from(err: E) -> Self { + Self::Storage(err.into()) + } +} + +/// True when the server refused (and rolled back) a write because another +/// operation got to the document first. +/// +/// Inside a multi-document transaction MongoDB does not queue behind a +/// conflicting writer the way PostgreSQL queues behind a row lock: the loser's +/// write fails at once with `WriteConflict` (112), labelled +/// `TransientTransactionError`, and its transaction is aborted. The label on +/// its own means the same thing for our purposes — the transaction did not and +/// will not commit, and running it again is safe — so both are classified. +/// `UnknownTransactionCommitResult` is deliberately not: there the write may +/// have landed. +/// +/// This used to reach the client as `BackendError::Internal` -> 500 (#1405): +/// "the server failed", for what is "you lost a race, read and retry". +pub(super) fn is_write_conflict(err: &MongoError) -> bool { + if err.contains_label(mongodb::error::TRANSIENT_TRANSACTION_ERROR) { + return true; + } + match err.kind.as_ref() { + MongoErrorKind::Command(command) => command.code == WRITE_CONFLICT_CODE, + MongoErrorKind::Write(mongodb::error::WriteFailure::WriteError(write)) => { + write.code == WRITE_CONFLICT_CODE + } + _ => false, + } +} + pub(super) fn ensure_resource_identity(resource_type: &str, id: &str, resource: &mut Value) { if let Some(obj) = resource.as_object_mut() { obj.insert( @@ -616,17 +677,28 @@ async fn commit_best_effort_multi_write_session( transaction_active: bool, operation: &str, ) -> StorageResult<()> { + try_commit_best_effort_multi_write_session(session, transaction_active) + .await + .map_err(|e| { + internal_error(format!( + "Failed to commit MongoDB transaction after {}: {}", + operation, e + )) + }) +} + +/// [`commit_best_effort_multi_write_session`] with the driver's error intact, +/// for the writes that classify a commit-time `WriteConflict`. +async fn try_commit_best_effort_multi_write_session( + session: &mut Option, + transaction_active: bool, +) -> Result<(), MongoError> { if !transaction_active { return Ok(()); } if let Some(active_session) = session.as_mut() { - active_session.commit_transaction().await.map_err(|e| { - internal_error(format!( - "Failed to commit MongoDB transaction after {}: {}", - operation, e - )) - })?; + active_session.commit_transaction().await?; } Ok(()) @@ -945,192 +1017,23 @@ impl ResourceStorage for MongoBackend { current: &StoredResource, resource: Value, ) -> StorageResult { - let resource_type = current.resource_type(); - tenant.check_permission(Operation::Update, resource_type)?; - - let db = self.get_database().await?; - let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); - let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); - let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; - let tenant_id = tenant.tenant_id().as_str(); - let id = current.id(); - - let current_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "is_deleted": false, - }; - - let maybe_existing = if let Some(active_session) = session.as_mut() { - resources - .find_one(current_filter.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!("Failed to load current resource (session): {}", e)) - })? - } else { - resources - .find_one(current_filter) - .await - .map_err(|e| internal_error(format!("Failed to load current resource: {}", e)))? - }; - - let Some(existing_doc) = maybe_existing else { - return Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })); - }; - - let actual_version = existing_doc - .get_str("version_id") - .map_err(|e| internal_error(format!("Missing current version: {}", e)))? - .to_string(); - - if actual_version != current.version_id() { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_version: current.version_id().to_string(), - actual_version, - }, - )); - } - - let new_version = next_version(current.version_id())?; - - let mut resource = resource; - ensure_resource_identity(resource_type, id, &mut resource); - let payload = value_to_document(&resource)?; - - let now = Utc::now(); - let now_bson = chrono_to_bson(now); - let fhir_version = current.fhir_version(); - let fhir_version_str = fhir_version.as_mime_param().to_string(); - - let update_filter = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": current.version_id(), - "is_deleted": false, - }; - let update_doc = doc! { - "$set": { - "version_id": &new_version, - "data": Bson::Document(payload.clone()), - "last_updated": now_bson, - "is_deleted": false, - "deleted_at": Bson::Null, - "fhir_version": &fhir_version_str, - } - }; - - let update_result = if let Some(active_session) = session.as_mut() { - resources - .update_one(update_filter.clone(), update_doc.clone()) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!("Failed to update resource (session): {}", e)) - })? - } else { - resources - .update_one(update_filter, update_doc) - .await - .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))? - }; - - if update_result.matched_count == 0 { - let latest = resources - .find_one(doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - }) - .await - .map_err(|e| { - internal_error(format!("Failed to reload version conflict state: {}", e)) - })?; - - let actual = latest - .as_ref() - .and_then(|d| d.get_str("version_id").ok()) - .unwrap_or("unknown") - .to_string(); - - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_version: current.version_id().to_string(), - actual_version: actual, - }, - )); - } - - let created_at = extract_created_at(&existing_doc, now); - - let history_doc = doc! { - "tenant_id": tenant_id, - "resource_type": resource_type, - "id": id, - "version_id": &new_version, - "data": Bson::Document(payload), - "created_at": chrono_to_bson(created_at), - "last_updated": now_bson, - "is_deleted": false, - "deleted_at": Bson::Null, - "fhir_version": fhir_version_str, - }; - - if let Some(active_session) = session.as_mut() { - history - .insert_one(history_doc) - .session(active_session) - .await - .map_err(|e| { - internal_error(format!( - "Failed to insert updated history row (session): {}", - e - )) - })?; - } else { - history.insert_one(history_doc).await.map_err(|e| { - internal_error(format!("Failed to insert updated history row: {}", e)) - })?; - } - - self.index_resource(&db, tenant_id, resource_type, id, &resource, &mut session) - .await?; - - commit_best_effort_multi_write_session(&mut session, transaction_active, "update").await?; - - // A SearchParameter update may change a tenant's overlay (status flips, - // expression edits): refresh the stored-param cache and drop registries. - // This must run after the commit above: `reload_stored_cache` reads the - // `resources` collection without the session, so it cannot observe the - // update while the transaction is still open. - if resource_type == "SearchParameter" { - if let Err(e) = self.reload_stored_cache().await { - tracing::warn!("SearchParameter cache reload failed: {e}"); - } + // No retry: `update` always carries a precondition (`current`'s + // version), and a writer that beat us to the document has, or is about + // to have, moved it on. The honest answer is the one PostgreSQL gives + // for the same race — `VersionConflict` -> 409 — not a second attempt. + match self.update_attempt(tenant, current, resource).await { + Ok(stored) => Ok(stored), + Err(WriteAttemptError::Storage(e)) => Err(e), + Err(WriteAttemptError::Conflict(e)) => Err(self + .lost_race( + tenant, + current.resource_type(), + current.id(), + current.version_id(), + &e, + ) + .await), } - - Ok(StoredResource::from_storage( - resource_type, - id, - new_version, - tenant.tenant_id().clone(), - resource, - created_at, - now, - None, - fhir_version, - )) } async fn delete( @@ -1819,6 +1722,207 @@ impl SearchIndexDocuments { } impl MongoBackend { + /// One attempt at [`ResourceStorage::update`]: the compare-and-swap on + /// `current`'s version, the history row and the search index, in one + /// transaction where the deployment has them. A write the server refused + /// because another writer holds the document comes back as + /// [`WriteAttemptError::Conflict`] rather than as an `Internal` error. + async fn update_attempt( + &self, + tenant: &TenantContext, + current: &StoredResource, + resource: Value, + ) -> Result { + let resource_type = current.resource_type(); + tenant.check_permission(Operation::Update, resource_type)?; + + let db = self.get_database().await?; + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); + let (mut session, transaction_active) = begin_best_effort_multi_write_session(&db).await; + let tenant_id = tenant.tenant_id().as_str(); + let id = current.id(); + + let current_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "is_deleted": false, + }; + + let maybe_existing = if let Some(active_session) = session.as_mut() { + resources + .find_one(current_filter.clone()) + .session(active_session) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to load current resource (session)", e) + })? + } else { + resources + .find_one(current_filter) + .await + .map_err(|e| internal_error(format!("Failed to load current resource: {}", e)))? + }; + + let Some(existing_doc) = maybe_existing else { + return Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + }) + .into()); + }; + + let actual_version = existing_doc + .get_str("version_id") + .map_err(|e| internal_error(format!("Missing current version: {}", e)))? + .to_string(); + + if actual_version != current.version_id() { + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: current.version_id().to_string(), + actual_version, + }) + .into(), + ); + } + + let new_version = next_version(current.version_id())?; + + let mut resource = resource; + ensure_resource_identity(resource_type, id, &mut resource); + let payload = value_to_document(&resource)?; + + let now = Utc::now(); + let now_bson = chrono_to_bson(now); + let fhir_version = current.fhir_version(); + let fhir_version_str = fhir_version.as_mime_param().to_string(); + + let update_filter = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": current.version_id(), + "is_deleted": false, + }; + let update_doc = doc! { + "$set": { + "version_id": &new_version, + "data": Bson::Document(payload.clone()), + "last_updated": now_bson, + "is_deleted": false, + "deleted_at": Bson::Null, + "fhir_version": &fhir_version_str, + } + }; + + let update_result = if let Some(active_session) = session.as_mut() { + resources + .update_one(update_filter.clone(), update_doc.clone()) + .session(active_session) + .await + .map_err(|e| WriteAttemptError::driver("Failed to update resource (session)", e))? + } else { + resources + .update_one(update_filter, update_doc) + .await + .map_err(|e| internal_error(format!("Failed to update resource: {}", e)))? + }; + + if update_result.matched_count == 0 { + let latest = resources + .find_one(doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + }) + .await + .map_err(|e| { + internal_error(format!("Failed to reload version conflict state: {}", e)) + })?; + + let actual = latest + .as_ref() + .and_then(|d| d.get_str("version_id").ok()) + .unwrap_or("unknown") + .to_string(); + + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: current.version_id().to_string(), + actual_version: actual, + }) + .into(), + ); + } + + let created_at = extract_created_at(&existing_doc, now); + + let history_doc = doc! { + "tenant_id": tenant_id, + "resource_type": resource_type, + "id": id, + "version_id": &new_version, + "data": Bson::Document(payload), + "created_at": chrono_to_bson(created_at), + "last_updated": now_bson, + "is_deleted": false, + "deleted_at": Bson::Null, + "fhir_version": fhir_version_str, + }; + + if let Some(active_session) = session.as_mut() { + history + .insert_one(history_doc) + .session(active_session) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to insert updated history row (session)", e) + })?; + } else { + history.insert_one(history_doc).await.map_err(|e| { + internal_error(format!("Failed to insert updated history row: {}", e)) + })?; + } + + self.index_resource(&db, tenant_id, resource_type, id, &resource, &mut session) + .await?; + + try_commit_best_effort_multi_write_session(&mut session, transaction_active) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to commit MongoDB transaction after update", e) + })?; + + // A SearchParameter update may change a tenant's overlay (status flips, + // expression edits): refresh the stored-param cache and drop registries. + // This must run after the commit above: `reload_stored_cache` reads the + // `resources` collection without the session, so it cannot observe the + // update while the transaction is still open. + if resource_type == "SearchParameter" { + if let Err(e) = self.reload_stored_cache().await { + tracing::warn!("SearchParameter cache reload failed: {e}"); + } + } + + Ok(StoredResource::from_storage( + resource_type, + id, + new_version, + tenant.tenant_id().clone(), + resource, + created_at, + now, + None, + fhir_version, + )) + } + /// The version of the live (not deleted) resource, read outside any /// session — what a write that just lost a race reports as the version it /// lost to. @@ -1857,6 +1961,109 @@ impl MongoBackend { id: &str, expected_version: Option<&str>, ) -> StorageResult<()> { + let mut retried = false; + loop { + let conflict = match self + .soft_delete_attempt(tenant, resource_type, id, expected_version) + .await + { + Ok(()) => return Ok(()), + Err(WriteAttemptError::Storage(e)) => return Err(e), + Err(WriteAttemptError::Conflict(e)) => e, + }; + + // ONE more attempt, and only for a delete with no precondition: + // "delete whatever is current" means the same thing after the + // other writer commits, which is the driver's recommended handling + // of a transient transaction error. A delete that names a version + // is never run again — the writer it lost to has moved the resource + // on, and the client must see that (`VersionConflict` -> 409). + if expected_version.is_none() && !retried { + retried = true; + tracing::debug!( + resource_type, + id, + error = %conflict, + "MongoDB write conflict on an unconditional delete; retrying once" + ); + tokio::time::sleep(WRITE_CONFLICT_RETRY_DELAY).await; + continue; + } + + return Err(self + .lost_race( + tenant, + resource_type, + id, + expected_version.unwrap_or("unknown"), + &conflict, + ) + .await); + } + } + + /// Reports a write the server refused with `WriteConflict` as the + /// concurrency error it is: `VersionConflict` against whatever is live now, + /// or `NotFound` when the winner was a delete. The winner may not have + /// committed yet, in which case the live version still reads as the + /// expected one and is reported as `unknown` rather than as a conflict of a + /// version with itself. A failure of this read must not turn a 409 back + /// into a 500, so it degrades to `unknown` too. + async fn lost_race( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: &str, + conflict: &MongoError, + ) -> StorageError { + tracing::debug!( + resource_type, + id, + expected_version, + error = %conflict, + "MongoDB write conflict: a concurrent writer won" + ); + + let live = match self.get_database().await { + Ok(db) => { + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + self.live_version(&resources, tenant.tenant_id().as_str(), resource_type, id) + .await + } + Err(e) => Err(e), + }; + + match live { + Ok(None) => StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + }), + Ok(Some(actual)) if actual != expected_version => { + StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version: actual, + }) + } + Ok(Some(_)) | Err(_) => StorageError::Concurrency(ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version: expected_version.to_string(), + actual_version: "unknown".to_string(), + }), + } + } + + /// One attempt at [`Self::soft_delete`]; see [`Self::update_attempt`]. + async fn soft_delete_attempt( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + expected_version: Option<&str>, + ) -> Result<(), WriteAttemptError> { tenant.check_permission(Operation::Delete, resource_type)?; let db = self.get_database().await?; @@ -1878,10 +2085,7 @@ impl MongoBackend { .session(active_session) .await .map_err(|e| { - internal_error(format!( - "Failed to check resource before delete (session): {}", - e - )) + WriteAttemptError::driver("Failed to check resource before delete (session)", e) })? } else { resources @@ -1896,7 +2100,8 @@ impl MongoBackend { return Err(StorageError::Resource(ResourceError::NotFound { resource_type: resource_type.to_string(), id: id.to_string(), - })); + }) + .into()); }; let current_version = existing_doc @@ -1906,14 +2111,15 @@ impl MongoBackend { if let Some(expected) = expected_version && expected != current_version { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { resource_type: resource_type.to_string(), id: id.to_string(), expected_version: expected.to_string(), actual_version: current_version, - }, - )); + }) + .into(), + ); } let new_version = next_version(¤t_version)?; @@ -1952,7 +2158,7 @@ impl MongoBackend { .session(active_session) .await .map_err(|e| { - internal_error(format!("Failed to soft-delete resource (session): {}", e)) + WriteAttemptError::driver("Failed to soft-delete resource (session)", e) })? } else { resources @@ -1969,19 +2175,21 @@ impl MongoBackend { .live_version(&resources, tenant_id, resource_type, id) .await? { - return Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { + return Err( + StorageError::Concurrency(ConcurrencyError::VersionConflict { resource_type: resource_type.to_string(), id: id.to_string(), expected_version: expected.to_string(), actual_version: actual, - }, - )); + }) + .into(), + ); } return Err(StorageError::Resource(ResourceError::NotFound { resource_type: resource_type.to_string(), id: id.to_string(), - })); + }) + .into()); } let history_doc = doc! { @@ -2003,10 +2211,7 @@ impl MongoBackend { .session(active_session) .await .map_err(|e| { - internal_error(format!( - "Failed to insert deletion history row (session): {}", - e - )) + WriteAttemptError::driver("Failed to insert deletion history row (session)", e) })?; } else { history.insert_one(history_doc).await.map_err(|e| { @@ -2017,7 +2222,11 @@ impl MongoBackend { self.delete_search_index(&db, tenant_id, resource_type, id, &mut session) .await?; - commit_best_effort_multi_write_session(&mut session, transaction_active, "delete").await?; + try_commit_best_effort_multi_write_session(&mut session, transaction_active) + .await + .map_err(|e| { + WriteAttemptError::driver("Failed to commit MongoDB transaction after delete", e) + })?; // A SearchParameter delete may remove a tenant's overlay entry: refresh // the stored-param cache and drop registries. This must run after the diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 088995591..995a4e475 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -742,7 +742,6 @@ async fn mongodb_conditional_writers_with_the_same_if_match_admit_one() { conditional_if_match_suite::concurrent_writers_with_the_same_if_match_admit_one( &backend, "cond-if-match-race-1381", - false, ) .await; } @@ -764,6 +763,55 @@ async fn mongodb_empty_values_are_rejected_on_every_path() { #[path = "search/versioned_write_race_suite.rs"] mod versioned_write_race_suite; +/// #1405: of several writers holding the same version, one `update` writes and +/// every loser is a `ConcurrencyError` — the server's `WriteConflict` used to +/// reach them as `BackendError::Internal`. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn mongodb_concurrent_updates_from_the_same_version_admit_one() { + let Some(backend) = create_backend("update_race_1405").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::concurrent_updates_from_the_same_version_admit_one( + std::sync::Arc::new(backend), + "update-race-1405", + 10, + ) + .await; +} + +/// #1404: an update and a versioned delete of the same version: one wins. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn mongodb_concurrent_update_and_versioned_delete_admit_one() { + let Some(backend) = create_backend("delete_race_1404").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::concurrent_update_and_versioned_delete_admit_one( + std::sync::Arc::new(backend), + "delete-race-1404", + 10, + ) + .await; +} + +/// #1405: an update racing an unconditional delete — the one write that is +/// retried after a `WriteConflict` — leaves a contiguous history and no +/// `Internal` error. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn mongodb_concurrent_update_and_plain_delete_stay_consistent() { + let Some(backend) = create_backend("plain_delete_race_1405").await else { + eprintln!("skipping: no MongoDB container available"); + return; + }; + versioned_write_race_suite::concurrent_update_and_plain_delete_stay_consistent( + std::sync::Arc::new(backend), + "plain-delete-race-1405", + 10, + ) + .await; +} + /// #1404: `delete_versioned` compares and deletes in one step. #[tokio::test] async fn mongodb_versioned_delete_is_a_compare_and_swap() { diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index ecf702851..099e34a56 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -19089,7 +19089,6 @@ mod postgres_integration { super::conditional_if_match_suite::concurrent_writers_with_the_same_if_match_admit_one( &backend, &unique_base("cond_if_match_race_1381"), - true, ) .await; } @@ -19220,6 +19219,19 @@ mod postgres_integration { .await; } + /// #1404: an update racing an unconditional delete leaves a contiguous + /// history. + #[tokio::test(flavor = "multi_thread", worker_threads = 8)] + async fn postgres_integration_concurrent_update_and_plain_delete_stay_consistent() { + let backend = create_backend().await; + super::versioned_write_race_suite::concurrent_update_and_plain_delete_stay_consistent( + std::sync::Arc::new(backend), + &unique_base("plain_delete_race_1404"), + 10, + ) + .await; + } + /// #1404: `delete_versioned` compares and deletes in one step. #[tokio::test] async fn postgres_integration_versioned_delete_is_a_compare_and_swap() { diff --git a/crates/persistence/tests/search/conditional_if_match_suite.rs b/crates/persistence/tests/search/conditional_if_match_suite.rs index 2017c7e90..63b6994dc 100644 --- a/crates/persistence/tests/search/conditional_if_match_suite.rs +++ b/crates/persistence/tests/search/conditional_if_match_suite.rs @@ -328,16 +328,16 @@ pub async fn if_match_is_evaluated_against_the_resolved_match( /// resolved after the winner, or by the swap (`VersionConflict`) if it resolved /// before. None of them may write version 3. /// -/// `losers_are_concurrency_errors` is `false` for MongoDB: its `update` runs in -/// a multi-document transaction, and a racing loser there surfaces the server's -/// `WriteConflict` as `BackendError::Internal` rather than as a -/// `ConcurrencyError`. The loser still writes nothing — which is what this -/// asserts for every backend — but its error is not classified. -pub async fn concurrent_writers_with_the_same_if_match_admit_one( - backend: &S, - base: &str, - losers_are_concurrency_errors: bool, -) where +/// That holds on every backend. MongoDB used to be exempt from the second +/// half: its `update` runs in a multi-document transaction, and a racing loser +/// there surfaced the server's `WriteConflict` as `BackendError::Internal` +/// (#1405). +/// +/// These writers are futures on one task, so a backend whose calls never yield +/// (SQLite) runs them one after another; `versioned_write_race_suite.rs` is the +/// test with real parallelism. +pub async fn concurrent_writers_with_the_same_if_match_admit_one(backend: &S, base: &str) +where S: ResourceStorage + ConditionalStorage + SearchProvider, { let t = tenant(base, "race"); @@ -374,7 +374,6 @@ pub async fn concurrent_writers_with_the_same_if_match_admit_one( for result in &results { match result { Ok(ConditionalUpdateResult::Updated(_)) | Err(StorageError::Concurrency(_)) => {} - Err(_) if !losers_are_concurrency_errors => {} other => panic!("a loser is a concurrency refusal, nothing else: {other:?}"), } } diff --git a/crates/persistence/tests/search/versioned_write_race_suite.rs b/crates/persistence/tests/search/versioned_write_race_suite.rs index 956db52a6..b8626626e 100644 --- a/crates/persistence/tests/search/versioned_write_race_suite.rs +++ b/crates/persistence/tests/search/versioned_write_race_suite.rs @@ -274,6 +274,107 @@ pub async fn concurrent_update_and_versioned_delete_admit_one( } } +/// Half the tasks `update` from version 1 and half `delete` with no +/// precondition. A plain delete removes whatever is current, so an update and +/// a delete may both land (as versions 2 and 3) — but only one of each, every +/// other task is refused cleanly, and the history is contiguous. On MongoDB +/// this is the path with the one bounded retry of a write conflict (#1405). +pub async fn concurrent_update_and_plain_delete_stay_consistent( + backend: Arc, + base: &str, + rounds: usize, +) where + S: ResourceStorage + VersionedStorage + Send + Sync + 'static, +{ + let t = tenant(base, "plain-delete-race"); + + for round in 0..rounds { + let id = format!("race-{round}"); + let current = seed(backend.as_ref(), &t, &id).await; + let barrier = Arc::new(Barrier::new(WRITERS)); + + let mut tasks = Vec::new(); + for n in 0..WRITERS { + let (backend, t, current, barrier, id) = ( + backend.clone(), + t.clone(), + current.clone(), + barrier.clone(), + id.clone(), + ); + tasks.push(tokio::spawn(async move { + barrier.wait().await; + if n % 2 == 0 { + match backend + .update(&t, ¤t, patient(&id, &format!("Writer{n}"))) + .await + { + Ok(stored) => Outcome::Updated(stored), + Err(e) => Outcome::Failed(e), + } + } else { + match backend.delete(&t, "Patient", &id).await { + Ok(()) => Outcome::Deleted, + Err(e) => Outcome::Failed(e), + } + } + })); + } + let mut outcomes = Vec::new(); + for task in tasks { + outcomes.push(task.await.expect("writer task")); + } + let context = format!("round {round}"); + + let updates = outcomes + .iter() + .filter(|o| matches!(o, Outcome::Updated(_))) + .count(); + let deletes = outcomes + .iter() + .filter(|o| matches!(o, Outcome::Deleted)) + .count(); + assert!( + updates <= 1, + "{context}: one update from version 1: {outcomes:?}" + ); + assert!( + deletes <= 1, + "{context}: a resource is deleted once: {outcomes:?}" + ); + assert!( + updates + deletes >= 1, + "{context}: somebody wins: {outcomes:?}" + ); + assert_losers_are_concurrency_errors(&outcomes, true, &context); + + let expected_versions: Vec = + (1..=1 + updates + deletes).map(|v| v.to_string()).collect(); + assert_eq!( + backend + .list_versions(&t, "Patient", &id) + .await + .expect("list versions"), + expected_versions, + "{context}: one version per successful write, no gap, no duplicate: {outcomes:?}" + ); + + let after = backend.read(&t, "Patient", &id).await; + if deletes == 1 { + assert!( + matches!( + after, + Ok(None) | Err(StorageError::Resource(ResourceError::Gone { .. })) + ), + "{context}: a delete landed, so nothing is live: {after:?}" + ); + } else { + let stored = after.expect("read after race").expect("still live"); + assert_eq!(stored.version_id(), "2", "{context}"); + } + } +} + /// `delete_versioned` is a compare-and-swap on the *current* version: a stale /// version is refused and deletes nothing, the current one deletes, and a /// second delete of the same version finds nothing live. diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index 6168f3709..756c26b51 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -93,7 +93,6 @@ async fn sqlite_conditional_writers_with_the_same_if_match_admit_one() { conditional_if_match_suite::concurrent_writers_with_the_same_if_match_admit_one( &backend, "cond-if-match-race-1381", - true, ) .await; } @@ -141,6 +140,19 @@ async fn sqlite_concurrent_update_and_versioned_delete_admit_one() { .await; } +/// #1404: an update racing an unconditional delete leaves a contiguous history. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn sqlite_concurrent_update_and_plain_delete_stay_consistent() { + let dir = tempfile::tempdir().expect("tempdir"); + let backend = std::sync::Arc::new(create_file_backend(&dir)); + versioned_write_race_suite::concurrent_update_and_plain_delete_stay_consistent( + backend, + "plain-delete-race-1404", + 40, + ) + .await; +} + /// #1404: `delete_versioned` compares and deletes in one step. #[tokio::test] async fn sqlite_versioned_delete_is_a_compare_and_swap() { From 06b2cb1d38f61c163fad8a5b5a7e21570785f0c7 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 09:14:08 -0400 Subject: [PATCH 08/10] test(persistence): MongoDB conditional patch is served, not refused mongodb_integration_conditional_patch_not_supported pinned the pre-#1406 UnsupportedCapability; it now pins a patch through the embedded _id parameter. README feature matrix: MongoDB conditional patch is implemented. Refs #1406 --- crates/persistence/README.md | 2 +- crates/persistence/tests/mongodb_tests.rs | 38 +++++++++++++------ .../search/conditional_if_match_suite.rs | 5 ++- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/crates/persistence/README.md b/crates/persistence/README.md index 1d09d6d13..78e373043 100644 --- a/crates/persistence/README.md +++ b/crates/persistence/README.md @@ -354,7 +354,7 @@ For a capability-by-capability narrative of FHIR Search against the [spec](https | [Batch Bundles](https://build.fhir.org/http.html#batch) | ✓ | ✓ | ✓ | ○ | ○ | ○ | ✓ | | [Transaction Bundles](https://build.fhir.org/http.html#transaction) | ✓ | ✓ | ✓ | ✗ | ○ | ✗ | ◐ | | [Conditional Operations](https://build.fhir.org/http.html#cond-update) | ✓ | ✓ | ✓ | ✗ | ○ | ○ | ✗ | -| [Conditional Patch](https://build.fhir.org/http.html#patch) | ✓ | ✓ | ○ | ✗ | ○ | ○ | ✗ | +| [Conditional Patch](https://build.fhir.org/http.html#patch) | ✓ | ✓ | ✓ | ✗ | ○ | ○ | ✗ | | [Delete History](https://build.fhir.org/http.html#delete) | ✓ | ✓ | ○ | ✗ | ○ | ✗ | ✗ | | Per-User Settings (`/_user/settings`) | ✓ | ✓ | ✓ | ✗ | ✗ | ✗ | ✓ | | **Multitenancy** | diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index fd67648d2..5334f89bb 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -7327,33 +7327,49 @@ async fn mongodb_integration_conditional_create_multiple_matches() { } } +/// Was `..._not_supported`, asserting `UnsupportedCapability`: MongoDB had no +/// `conditional_patch` until #1406. `_id` is one of the embedded parameters, +/// so this needs no spec registry; `mongodb_conditional_patch` is the full +/// suite. #[tokio::test] -async fn mongodb_integration_conditional_patch_not_supported() { - let Some(backend) = create_backend("conditional_patch_not_supported").await else { +async fn mongodb_integration_conditional_patch_is_supported() { + let Some(backend) = create_backend("conditional_patch_supported").await else { eprintln!( - "Skipping mongodb_integration_conditional_patch_not_supported (requires Docker or HFS_TEST_MONGODB_URL)" + "Skipping mongodb_integration_conditional_patch_is_supported (requires Docker or HFS_TEST_MONGODB_URL)" ); return; }; let tenant = create_tenant("tenant-conditional-patch"); + backend + .create( + &tenant, + "Patient", + json!({"resourceType": "Patient", "id": "cond-patch-1", "active": false}), + FhirVersion::default(), + ) + .await + .unwrap(); let result = backend .conditional_patch( &tenant, "Patient", - "identifier=http://hospital.org/mrn|MRN-COND-PATCH", + "_id=cond-patch-1", &PatchFormat::MergePatch(json!({ "active": true })), &helios_persistence::core::EntityTagPrecondition::Absent, ) - .await; + .await + .unwrap(); - assert!(matches!( - result, - Err(StorageError::Backend( - BackendError::UnsupportedCapability { .. } - )) - )); + match result { + helios_persistence::core::ConditionalPatchResult::Patched(stored) => { + assert_eq!(stored.id(), "cond-patch-1"); + assert_eq!(stored.version_id(), "2"); + assert_eq!(stored.content()["active"], json!(true)); + } + other => panic!("expected Patched, got {:?}", other), + } } #[tokio::test] diff --git a/crates/persistence/tests/search/conditional_if_match_suite.rs b/crates/persistence/tests/search/conditional_if_match_suite.rs index 2017c7e90..9cfaad81e 100644 --- a/crates/persistence/tests/search/conditional_if_match_suite.rs +++ b/crates/persistence/tests/search/conditional_if_match_suite.rs @@ -134,8 +134,9 @@ fn is_precondition_failure(result: &Result) /// `OptimisticLockFailure` that leaves storage untouched, and a no-match /// update does not fall through to its create. /// -/// `supports_patch` is `false` for a backend without `conditional_patch` -/// (MongoDB), which must then refuse it whatever the precondition says. +/// `supports_patch` is `false` for a backend that declines `conditional_patch` +/// (none of the current callers, since #1406), which must then refuse it +/// whatever the precondition says. pub async fn if_match_is_evaluated_against_the_resolved_match( backend: &S, base: &str, From e325b0bcaace2ba211104d44c3e290c9a03393a6 Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 13:11:30 -0400 Subject: [PATCH 09/10] docs(persistence): the If-Match check and the delete are now one step Doc comments on conditional_if_match_gate, VersionedStorage::delete_with_match and the conditional DELETE handler still described the check-then-act window that #1404 closed: a delete under a precondition now goes through ResourceStorage::delete_versioned, pinned to the version that was compared. --- crates/persistence/src/core/preconditions.rs | 3 ++- crates/persistence/src/core/versioned.rs | 5 ++++- crates/rest/src/handlers/delete.rs | 9 +++++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/persistence/src/core/preconditions.rs b/crates/persistence/src/core/preconditions.rs index 8f967d909..184a74a9d 100644 --- a/crates/persistence/src/core/preconditions.rs +++ b/crates/persistence/src/core/preconditions.rs @@ -460,7 +460,8 @@ pub fn if_match_field_satisfied(raw: &str, current_version_id: &str) -> bool { /// this between resolving the match and writing, and then hands *that* row to /// `update`, whose compare-and-swap is keyed on the version evaluated here. A /// writer landing in between therefore ends in `VersionConflict`, never in a -/// write over a version the client did not name. +/// write over a version the client did not name. A delete gets the same +/// guarantee from [`delete_under_precondition`]. /// /// The failure is [`ConcurrencyError::OptimisticLockFailure`], which the REST /// layer already renders as `412`. `id` is empty when nothing matched. diff --git a/crates/persistence/src/core/versioned.rs b/crates/persistence/src/core/versioned.rs index 12bddda07..35dee5594 100644 --- a/crates/persistence/src/core/versioned.rs +++ b/crates/persistence/src/core/versioned.rs @@ -135,7 +135,10 @@ pub trait VersionedStorage: ResourceStorage { /// # Errors /// /// * `StorageError::Resource(NotFound)` - If the resource doesn't exist - /// * `StorageError::Concurrency(VersionConflict)` - If versions don't match + /// * `StorageError::Concurrency(VersionConflict)` - If versions don't match, + /// including when a writer lands after the comparison: implementations + /// delete through [`ResourceStorage::delete_versioned`], pinned to the + /// version they compared (#1404) /// * `StorageError::Tenant` - If the tenant doesn't have delete permission async fn delete_with_match( &self, diff --git a/crates/rest/src/handlers/delete.rs b/crates/rest/src/handlers/delete.rs index 1f6c6e17b..3bc3c6fe7 100644 --- a/crates/rest/src/handlers/delete.rs +++ b/crates/rest/src/handlers/delete.rs @@ -266,10 +266,11 @@ where /// below — as it does on `DELETE [type]/[id]` for a resource that does not /// exist: no current representation satisfies `If-Match` (RFC 9110 §13.1.1). /// -/// The check and the delete are not one atomic step. No backend's `delete` -/// compares-and-swaps on a version (`update` does), so a writer landing between -/// the two is deleted along with the version the client named — the same -/// window [`delete_handler`] has, not a wider one. +/// The check and the delete are one step (#1404): with `If-Match` the backend +/// deletes through `ResourceStorage::delete_versioned`, pinned to the version +/// it evaluated, so a writer landing between the two is answered `409` instead +/// of being deleted along with the version the client named — as on +/// [`delete_handler`]. /// /// # No match /// From ec4587bebc3ba06bf93a74f564d491a16a9a5d9f Mon Sep 17 00:00:00 2001 From: smunini Date: Mon, 21 Sep 2026 13:32:01 -0400 Subject: [PATCH 10/10] style(rest): drop a needless borrow in delete_handler clippy 1.98.1 (needless_borrow): `if_match` is already a reference. --- crates/rest/src/handlers/delete.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rest/src/handlers/delete.rs b/crates/rest/src/handlers/delete.rs index a0aba2756..87575d288 100644 --- a/crates/rest/src/handlers/delete.rs +++ b/crates/rest/src/handlers/delete.rs @@ -185,7 +185,7 @@ where helios_persistence::core::delete_under_precondition( state.storage(), tenant.context(), - &if_match, + if_match, current, ) .await?