From 6fe5d95616b14d226155c5872551081ce0d76b95 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 2 Sep 2026 20:34:27 -0400 Subject: [PATCH 1/7] docs: draft issues surfaced while implementing #859 [skip ci] Side findings for review before filing: transactional instance deletes never reach composite secondaries, the spec fixture's `$lookup` entry, the test-hfs skill's missing backend commands, and `BundleError` indexes referring to the sorted entry order. --- docs/draft-issues-from-859.md | 50 +++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/draft-issues-from-859.md diff --git a/docs/draft-issues-from-859.md b/docs/draft-issues-from-859.md new file mode 100644 index 000000000..5b5e99800 --- /dev/null +++ b/docs/draft-issues-from-859.md @@ -0,0 +1,50 @@ +# Draft issues surfaced while implementing #859 — for review before filing + +Found while resolving `PUT/DELETE [type]?[criteria]` inside transactions (branch +`feat/859-transaction-conditional-url`). None is fixed by that PR unless stated. Line numbers +are current-tree at the time of writing. + +## D1. Transactional `DELETE [type]/[id]` never reaches a composite's secondaries + +`CompositeStorage::sync_bundle_results` (`crates/persistence/src/composite/storage.rs`) syncs +a transaction's entries to secondaries by reading each `BundleEntryResult.resource`. An +instance-addressed `DELETE` answers `204` with no body and no `location`, so it is skipped: +on `sqlite-elasticsearch` a resource deleted inside a transaction stays searchable in +Elasticsearch until the next reindex. #859 fixes the *conditional* delete (its `204` now +carries `location`, and the sync emits `SyncEvent::Delete` for a `204` with one), but the +explicit form still answers a bare `204`. + +Fix: have the three executors set `location` on every delete result (the deleted version's +URL), or have the composite derive the identity from the entry's URL, which it does not see +today. Test: `composite_conformance_sync` — a transaction `DELETE Patient/p1` followed by an +Elasticsearch search that must not return `p1`. + +## D2. The spec fixture's `POST ValueSet/$lookup` entry is not a create + +`crates/fhir/tests/data/json/R4/bundle-transaction.json` entry 7 is `POST ValueSet/$lookup` +with a `Parameters` body. Nothing in the bundle path recognises an operation URL: the REST +layer admits it as a mutation of type `ValueSet`, and the backends' `parse_url` would take +`$lookup` as an id. Today the entry fails on the resource-type mismatch (`Parameters` body under +`ValueSet`), which is at least a refusal; but the fixture as a whole cannot be replayed until +`$op` entries are either executed or declined with a message that names the operation. + +Fix: detect `[type]/$op` and `[type]/[id]/$op` in `parse_bundle_entry`, and either dispatch +to the operation router or return `501` naming the operation. Then the fixture (minus the GET +entries, which #478 covers) becomes an end-to-end test. + +## D3. `test-hfs` skill lacks the backend-specific test commands + +`.claude/skills/test-hfs/SKILL.md` names testcontainers and the ES heap cap but not the +commands the suites actually need: `cargo test -p helios-persistence --features postgres -- +postgres_integration`, `--features mongodb -- mongodb_integration`, the +`HFS_TEST_MONGODB_URL` escape hatch, or that MongoDB transaction tests skip on a standalone +topology. Every backend suite spells these out in its module docs instead. + +## D4. `BundleError` index refers to the sorted entry order + +`process_transaction` sorts entries DELETE → POST → PUT → GET before calling the backend, and +`TransactionError::BundleError { index }` (rendered as "Transaction failed at entry N") is the +index in *that* order, not the client's. The #859 overlap message names both entries by the +same sorted index. Mapping back to the original index is one lookup in the REST layer +(`indexed_entries[index].0`). Pre-existing; noticed because the overlap message makes it +visible. From c72186cba79e0a838d61f36e8d7e1723374fb03b Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 2 Sep 2026 20:33:54 -0400 Subject: [PATCH 2/7] feat(persistence): resolve conditional URL entries inside transactions via ConditionalTransaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A transaction Bundle could not carry `PUT [type]?[criteria]` or `DELETE [type]?[criteria]`: the backends' `parse_url` is query-blind, and `ConditionalStorage` writes through the backend's own connection, outside the open transaction, so nothing could resolve criteria within the atomic scope. Add the shape design discussion #28 proposed. `ConditionalTransaction: Transaction` is a separate trait whose one required method, `find_matching`, is the transaction-scoped search; `create_if_none_exist`, `update_conditional` and `delete_conditional` are defaulted on top of it and reuse the existing outcome enums. `BundleEntry` gains typed `criteria`, so modifiers, chains and prefixes reach the backend as `SearchParameter`s rather than a `k=v&k=v` string. `BundleProvider` gains the required predicate `supports_conditional_in_transaction`, false when search is offloaded to a secondary (empty local index) and for S3. `core::bundle_conditionals` holds what every executor shares: resolution of every conditional entry against the transaction's starting view before any write, `412 multiple-matches` for the whole bundle on several matches, the R4 §3.1.0.11.2 overlapping-identity check (a resolved identity another entry addresses fails the bundle naming both entries), and the delete result that names what it deleted through `location`. A matched entry's `fullUrl` is seeded into the reference map before execution, so `urn:uuid` references to it resolve from any position. - SQLite and PostgreSQL implement the trait on their transaction types (PostgreSQL's transaction now carries a cloneable backend handle) and pin the pre-pass target in their PUT/DELETE arms; the string `ifNoneExist` matcher now runs through the same typed search. - MongoDB has no `Transaction` impl, so it runs the same pre-pass on its session; criteria its in-memory matcher cannot evaluate (modifier, chain, prefix, OR-list) refuse the entry with 501 rather than matching nothing. - CompositeStorage delegates the predicate and syncs a 204 carrying a `location` as a delete to secondaries. Tests: unit tests for the overlap check, URL parsing and entry results in `bundle_conditionals`; the backend suites land in the next commit. --- .../src/backends/mongodb/storage.rs | 216 ++++++++- .../src/backends/postgres/backend.rs | 3 + .../src/backends/postgres/storage.rs | 111 ++++- .../src/backends/postgres/transaction.rs | 41 +- crates/persistence/src/backends/s3/bundle.rs | 5 + .../src/backends/sqlite/storage.rs | 117 ++++- .../src/backends/sqlite/transaction.rs | 30 +- crates/persistence/src/composite/storage.rs | 34 ++ .../src/core/bundle_conditionals.rs | 429 ++++++++++++++++++ crates/persistence/src/core/mod.rs | 9 +- crates/persistence/src/core/transaction.rs | 135 +++++- crates/persistence/tests/mongodb_tests.rs | 13 + crates/persistence/tests/postgres_tests.rs | 3 + .../tests/transactions/bundle_tests.rs | 18 + .../tests/transactions/if_match_suite.rs | 2 + crates/rest/src/handlers/batch.rs | 1 + 16 files changed, 1122 insertions(+), 45 deletions(-) create mode 100644 crates/persistence/src/core/bundle_conditionals.rs diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index c56d1f7e5..16aac695d 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -364,6 +364,45 @@ fn parse_history_row( }) } +/// Flattens typed conditional criteria into the `(name, value)` pairs the +/// session-scoped matcher evaluates, or names the first criterion whose shape +/// that matcher cannot evaluate: a modifier, a chain, composite components, +/// an OR-list, or a comparison prefix. +/// +/// Refusing is the honest answer — the SQL backends evaluate all of these +/// through their query builders, and a silent "no match" here would create a +/// duplicate or delete nothing (#865, #709). +fn bundle_criteria_pairs( + criteria: &[crate::types::SearchParameter], +) -> Result, String> { + use crate::types::SearchPrefix; + + criteria + .iter() + .map(|param| { + let plain = param.modifier.is_none() + && param.chain.is_empty() + && param.components.is_empty() + && param.values.len() == 1 + && param.values[0].prefix == SearchPrefix::Eq; + if !plain { + let values = param + .values + .iter() + .map(|v| format!("{}{}", v.prefix, v.value)) + .collect::>() + .join(","); + let name = match ¶m.modifier { + Some(modifier) => format!("{}:{modifier}", param.name), + None => param.name.clone(), + }; + return Err(format!("{name}={values}")); + } + Ok((param.name.clone(), param.values[0].value.clone())) + }) + .collect() +} + fn parse_simple_bundle_search_params(params: &str) -> Vec<(String, String)> { params .split('&') @@ -2542,6 +2581,14 @@ impl BundleProvider for MongoBackend { true } + /// The in-transaction matcher scans the resources collection itself + /// rather than the search index, so an offloaded index does not blind + /// it. Criteria shapes it cannot evaluate refuse the entry instead + /// (see `bundle_criteria_pairs`). + fn supports_conditional_in_transaction(&self) -> bool { + true + } + async fn process_transaction( &self, tenant: &TenantContext, @@ -2557,9 +2604,34 @@ impl BundleProvider for MongoBackend { let mut session = begin_required_bundle_transaction_session(&db).await?; + // URL-borne conditional entries (`PUT/DELETE [type]?[criteria]`) + // resolve against the transaction's starting view before any entry is + // written, and an overlap between resolved identities and the other + // entries fails the bundle (R4 §3.1.0.11.2; #859). + let targets = match self + .resolve_conditional_targets_in_bundle_transaction(&db, &mut session, tenant, &entries) + .await + { + Ok(targets) => targets, + Err(e) => { + let _ = session.abort_transaction().await; + return Err(e); + } + }; + let mut results = Vec::with_capacity(entries.len()); let mut error_info: Option<(usize, String)> = None; + // A conditional entry that matched is known now, so `urn:uuid` + // references to it resolve regardless of entry order. let mut reference_map: HashMap = HashMap::new(); + for target in targets.values() { + if let (Some(full_url), Some(identity)) = ( + entries[target.entry_index].full_url.as_ref(), + target.identity(), + ) { + reference_map.insert(full_url.clone(), identity); + } + } let mut pending_search_parameter_changes: Vec = Vec::new(); let mut entries = entries; @@ -2576,6 +2648,7 @@ impl BundleProvider for MongoBackend { entry, fhir_version, &mut pending_search_parameter_changes, + targets.get(&idx), ) .await; @@ -2589,7 +2662,9 @@ impl BundleProvider for MongoBackend { break; } - if entry.method == BundleMethod::Post { + // A create (POST, or a conditional PUT that created) with a + // fullUrl records the assigned identity for later references. + if matches!(entry.method, BundleMethod::Post | BundleMethod::Put) { if let Some(full_url) = entry.full_url.as_ref() { if let Some(location) = entry_result.location.as_ref() { let reference = location @@ -2640,6 +2715,75 @@ impl BundleProvider for MongoBackend { } impl MongoBackend { + /// Resolves every URL-borne conditional entry inside the session, before + /// any write, and enforces the R4 §3.1.0.11.2 overlap rule (#859). + /// + /// The session-scoped matcher evaluates criteria in application memory + /// and understands plain `name=value` only, so a criterion with a + /// modifier, chain, prefix or OR-list refuses the entry with the `501` + /// the offloaded-search case answers on the other backends; #709 owns + /// an index-backed matcher. + async fn resolve_conditional_targets_in_bundle_transaction( + &self, + db: &mongodb::Database, + session: &mut ClientSession, + tenant: &TenantContext, + entries: &[BundleEntry], + ) -> Result, TransactionError> { + let mut targets = Vec::new(); + for (index, entry) in entries.iter().enumerate() { + let Some(criteria) = entry.criteria.as_deref() else { + continue; + }; + let resource_type = crate::core::conditional_resource_type(entry) + .filter(|t| !t.is_empty()) + .ok_or_else(|| TransactionError::BundleError { + index, + message: format!("Entry request.url '{}' names no resource type", entry.url), + })? + .to_string(); + let pairs = bundle_criteria_pairs(criteria).map_err(|shape| { + crate::core::unsupported_conditional_entry( + index, + &format!( + "criteria '{shape}' cannot be evaluated inside a MongoDB transaction; \ + submit the entry in a batch Bundle instead" + ), + ) + })?; + let matches = self + .find_matching_pairs_in_bundle_transaction( + db, + session, + tenant, + &resource_type, + &pairs, + ) + .await + .map_err(|e| TransactionError::BundleError { + index, + message: format!("Entry processing failed: {e}"), + })?; + targets.push(crate::core::conditional_target( + index, + entry, + &resource_type, + matches, + )?); + } + crate::core::check_identity_overlap(entries, &targets)?; + Ok(targets + .into_iter() + .map(|target| (target.entry_index, target)) + .collect()) + } + + /// Process a single bundle entry within the session's transaction. + /// + /// `target` is the pre-pass resolution of a URL-borne conditional entry + /// (#859): its `PUT` updates the match or creates, its `DELETE` deletes + /// the match or is a no-op `204`, without re-resolving the criteria. + #[allow(clippy::too_many_arguments)] async fn process_bundle_entry_transaction( &self, db: &mongodb::Database, @@ -2648,6 +2792,7 @@ impl MongoBackend { entry: &BundleEntry, fhir_version: helios_fhir::FhirVersion, pending_search_parameter_changes: &mut Vec, + target: Option<&crate::core::ConditionalTarget>, ) -> StorageResult { match entry.method { BundleMethod::Get => { @@ -2724,6 +2869,34 @@ impl MongoBackend { }) })?; + if let Some(target) = target { + return Ok(match &target.resolved { + Some(existing) => crate::core::conditional_update_entry( + self.update_resource_in_bundle_transaction( + db, + session, + tenant, + existing, + resource, + pending_search_parameter_changes, + ) + .await?, + ), + None => BundleEntryResult::created( + self.create_resource_in_bundle_transaction( + db, + session, + tenant, + &target.resource_type, + resource, + fhir_version, + pending_search_parameter_changes, + ) + .await?, + ), + }); + } + let (resource_type, id) = self.parse_url(&entry.url)?; match self @@ -2782,6 +2955,24 @@ impl MongoBackend { } } BundleMethod::Delete => { + if let Some(target) = target { + return Ok(match &target.resolved { + Some(existing) => { + self.delete_resource_in_bundle_transaction( + db, + session, + tenant, + &target.resource_type, + existing.id(), + pending_search_parameter_changes, + ) + .await?; + crate::core::conditional_delete_entry(existing) + } + None => BundleEntryResult::deleted(), + }); + } + let (resource_type, id) = self.parse_url(&entry.url)?; if let Some(if_match) = entry.if_match.as_ref() { @@ -3324,6 +3515,27 @@ impl MongoBackend { search_params: &str, ) -> StorageResult> { let parsed_params = parse_simple_bundle_search_params(search_params); + self.find_matching_pairs_in_bundle_transaction( + db, + session, + tenant, + resource_type, + &parsed_params, + ) + .await + } + + /// The session-scoped matcher both criteria forms end in: `ifNoneExist`'s + /// string, parsed above, and a URL-borne conditional entry's typed + /// criteria, flattened by `bundle_criteria_pairs` (#859). + async fn find_matching_pairs_in_bundle_transaction( + &self, + db: &mongodb::Database, + session: &mut ClientSession, + tenant: &TenantContext, + resource_type: &str, + parsed_params: &[(String, String)], + ) -> StorageResult> { if parsed_params.is_empty() { return Ok(Vec::new()); } @@ -3358,7 +3570,7 @@ impl MongoBackend { })?; let resource = document_to_value(payload)?; - if resource_matches_bundle_search_params(&resource, &parsed_params) + if resource_matches_bundle_search_params(&resource, parsed_params) && doc .get_str("resource_type") .map(|rt| rt == resource_type) diff --git a/crates/persistence/src/backends/postgres/backend.rs b/crates/persistence/src/backends/postgres/backend.rs index 82277535e..03f697b6a 100644 --- a/crates/persistence/src/backends/postgres/backend.rs +++ b/crates/persistence/src/backends/postgres/backend.rs @@ -28,6 +28,9 @@ use crate::search::{ type StoredByTenant = Arc>>>; /// PostgreSQL backend for FHIR resource storage. +/// +/// Cheap to clone: a pool handle, shared registries and the config. +#[derive(Clone)] pub struct PostgresBackend { pool: Pool, config: PostgresConfig, diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index fc09cd028..5a7851b14 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -2643,6 +2643,12 @@ impl PurgableStorage for PostgresBackend { // Helper function to parse simple search parameters // Supports basic formats like: identifier=X, _id=Y, name=Z +/// Why conditional criteria cannot be resolved inside a transaction when +/// search is offloaded to a secondary backend (#511, #859). +const OFFLOADED_CONDITIONAL_REFUSAL: &str = "conditional criteria cannot be resolved inside a \ + transaction when search is offloaded to a secondary backend; submit the entry in a batch \ + Bundle instead"; + fn parse_simple_search_params(params: &str) -> Vec<(String, String)> { params .split('&') @@ -2830,6 +2836,10 @@ impl PostgresBackend { /// Buffered creates are flushed first, exactly as `read` does, so they are /// visible too; a bundle that puts `ifNoneExist` on every entry therefore /// forfeits create batching, which is the correct trade. + /// + /// The string form `ifNoneExist` carries is parsed here and handed to the + /// typed [`ConditionalTransaction::find_matching`], so both criteria + /// forms run one search body (#859). async fn find_matching_resources_in_tx( &self, tenant: &TenantContext, @@ -2837,17 +2847,12 @@ impl PostgresBackend { resource_type: &str, search_params_str: &str, ) -> StorageResult> { + use crate::core::ConditionalTransaction; + let Some(query) = self.conditional_query(tenant, resource_type, search_params_str)? else { return Ok(Vec::new()); }; - - tx.flush().await?; - let client = tx.client()?; - let result = self - .search_with_client(client, tenant, &query, None) - .await?; - - Ok(result.resources.items) + tx.find_matching(resource_type, &query.parameters).await } /// Builds the search a conditional interaction's criteria describe, or @@ -3097,6 +3102,14 @@ impl BundleProvider for PostgresBackend { true } + /// With search offloaded to a secondary backend the local index is empty + /// for every row, so an in-transaction search would always find nothing + /// and every conditional write would create the duplicate its criteria + /// exist to prevent. + fn supports_conditional_in_transaction(&self) -> bool { + !self.is_search_offloaded() + } + async fn process_transaction( &self, tenant: &TenantContext, @@ -3117,6 +3130,25 @@ impl BundleProvider for PostgresBackend { let mut results = Vec::with_capacity(entries.len()); let mut error_info: Option<(usize, String)> = None; + // URL-borne conditional entries (`PUT/DELETE [type]?[criteria]`) + // resolve against the transaction's starting view before any entry is + // written, and an overlap between resolved identities and the other + // entries fails the bundle (R4 §3.1.0.11.2; #859). Nothing is + // buffered yet, so no flush precedes these searches. + let targets = match crate::core::resolve_conditional_targets( + &mut tx, + &entries, + (!self.supports_conditional_in_transaction()).then_some(OFFLOADED_CONDITIONAL_REFUSAL), + ) + .await + { + Ok(targets) => targets, + Err(e) => { + let _ = Box::new(tx).rollback().await; + return Err(e); + } + }; + // `create` no longer sends its insert on the spot — the transaction // batches consecutive creates and flushes them together, which is what // takes a 1,632-entry import bundle from 3,264 statements to 26. A @@ -3126,8 +3158,18 @@ impl BundleProvider for PostgresBackend { // n-th `create` call back to the entry that made it. let mut create_entry_index: Vec = Vec::with_capacity(entries.len()); - // Build a map of fullUrl -> assigned reference for reference resolution + // Build a map of fullUrl -> assigned reference for reference resolution. + // A conditional entry that matched is known now, so `urn:uuid` + // references to it resolve regardless of entry order. let mut reference_map: HashMap = HashMap::new(); + for target in targets.values() { + if let (Some(full_url), Some(identity)) = ( + entries[target.entry_index].full_url.as_ref(), + target.identity(), + ) { + reference_map.insert(full_url.clone(), identity); + } + } // Whether any entry in this transaction writes a SearchParameter that // affects this tenant's cached overlay (#787: transaction-bundle writes @@ -3149,7 +3191,9 @@ impl BundleProvider for PostgresBackend { } let creates_before = tx.creates_seen(); - let result = self.process_bundle_entry_tx(tenant, &mut tx, entry).await; + let result = self + .process_bundle_entry_tx(tenant, &mut tx, entry, targets.get(&idx)) + .await; for _ in creates_before..tx.creates_seen() { create_entry_index.push(idx); } @@ -3187,17 +3231,22 @@ impl BundleProvider for PostgresBackend { }) == Some("SearchParameter") } // Deleted: the emptied result carries no resource, so - // parse the type from the entry's URL instead. - 204 => self - .parse_url(&entry.url) - .map(|(resource_type, _)| resource_type == "SearchParameter") + // take the type from the entry's URL instead. + 204 => crate::core::conditional_resource_type(entry) + .map(|resource_type| resource_type == "SearchParameter") + .or_else(|| { + self.parse_url(&entry.url).ok().map(|(resource_type, _)| { + resource_type == "SearchParameter" + }) + }) .unwrap_or(false), _ => false, }; } - // If this was a create (POST) and we have a fullUrl, record the mapping - if entry.method == BundleMethod::Post { + // A create (POST, or a conditional PUT that created) with a + // fullUrl records the assigned identity for later references. + if matches!(entry.method, BundleMethod::Post | BundleMethod::Put) { if let Some(ref full_url) = entry.full_url { if let Some(ref location) = entry_result.location { let reference = location @@ -3292,11 +3341,16 @@ fn attribute_entry_error( impl PostgresBackend { /// Process a single bundle entry within a transaction. + /// + /// `target` is the pre-pass resolution of a URL-borne conditional entry + /// (#859): its `PUT` updates the match or creates, its `DELETE` deletes + /// the match or is a no-op `204`, without re-resolving the criteria. async fn process_bundle_entry_tx( &self, tenant: &TenantContext, tx: &mut super::transaction::PostgresTransaction, entry: &BundleEntry, + target: Option<&crate::core::ConditionalTarget>, ) -> StorageResult { use crate::core::transaction::Transaction; @@ -3341,9 +3395,7 @@ impl PostgresBackend { // Refuse the entry instead; the bundle rolls back (#511). if self.is_search_offloaded() { return Ok(crate::core::not_supported_entry( - "ifNoneExist cannot be resolved inside a transaction when search \ - is offloaded to a secondary backend; submit the entry in a batch \ - Bundle instead", + OFFLOADED_CONDITIONAL_REFUSAL, )); } let matches = self @@ -3364,6 +3416,17 @@ impl PostgresBackend { }) })?; + if let Some(target) = target { + return Ok(match &target.resolved { + Some(existing) => crate::core::conditional_update_entry( + tx.update(existing, resource).await?, + ), + None => BundleEntryResult::created( + tx.create(&target.resource_type, resource).await?, + ), + }); + } + let (resource_type, id) = self.parse_url(&entry.url)?; let existing = tx.read(&resource_type, &id).await?; @@ -3396,6 +3459,16 @@ impl PostgresBackend { } } BundleMethod::Delete => { + if let Some(target) = target { + return Ok(match &target.resolved { + Some(existing) => { + tx.delete(&target.resource_type, existing.id()).await?; + crate::core::conditional_delete_entry(existing) + } + None => BundleEntryResult::deleted(), + }); + } + let (resource_type, id) = self.parse_url(&entry.url)?; // Honor `ifMatch` on DELETE — previously ignored here, so a diff --git a/crates/persistence/src/backends/postgres/transaction.rs b/crates/persistence/src/backends/postgres/transaction.rs index 25d0134ec..dd7edced5 100644 --- a/crates/persistence/src/backends/postgres/transaction.rs +++ b/crates/persistence/src/backends/postgres/transaction.rs @@ -9,13 +9,15 @@ use deadpool_postgres::Client; use helios_fhir::FhirVersion; use serde_json::Value; -use crate::core::{Transaction, TransactionOptions, TransactionProvider}; +use crate::core::{ + ConditionalTransaction, Transaction, TransactionOptions, TransactionProvider, conditional_query, +}; use crate::error::{ BackendError, ConcurrencyError, ResourceError, StorageError, StorageResult, TransactionError, }; use crate::search::SearchParameterExtractor; use crate::tenant::{Operation, TenantContext}; -use crate::types::StoredResource; +use crate::types::{SearchParameter, StoredResource}; use super::PostgresBackend; use super::cached::{execute_cached, query_cached, query_opt_cached}; @@ -46,6 +48,10 @@ pub struct PostgresTransaction { active: bool, /// The tenant context for this transaction. tenant: TenantContext, + /// The parent backend, for the transaction-scoped search + /// ([`ConditionalTransaction`]) to run the backend's own search body on + /// this transaction's client (#859). + backend: PostgresBackend, /// Search parameter extractor for indexing resources. search_extractor: Arc, /// When true, search indexing is offloaded to a secondary backend. @@ -126,6 +132,7 @@ impl PostgresTransaction { async fn new( client: Client, tenant: TenantContext, + backend: PostgresBackend, search_extractor: Arc, search_offloaded: bool, fhir_version: FhirVersion, @@ -152,6 +159,7 @@ impl PostgresTransaction { client: Some(client), active: true, tenant, + backend, search_extractor, search_offloaded, fhir_version, @@ -935,6 +943,34 @@ impl Drop for PostgresTransaction { } } +/// The transaction-scoped search surface (#859). +/// +/// Runs the backend's search body on this transaction's client, so the match +/// set includes what earlier entries of the same bundle wrote (#511). +/// Buffered creates are flushed first, exactly as `read` does, so they are +/// visible too; a bundle that resolves criteria on every entry therefore +/// forfeits create batching, which is the correct trade. +#[async_trait] +impl ConditionalTransaction for PostgresTransaction { + async fn find_matching( + &mut self, + resource_type: &str, + criteria: &[SearchParameter], + ) -> StorageResult> { + if criteria.is_empty() { + return Ok(Vec::new()); + } + let query = conditional_query(resource_type, criteria); + self.flush().await?; + let client = self.client()?; + let result = self + .backend + .search_with_client(client, &self.tenant, &query, None) + .await?; + Ok(result.resources.items) + } +} + #[async_trait] impl TransactionProvider for PostgresBackend { type Transaction = PostgresTransaction; @@ -948,6 +984,7 @@ impl TransactionProvider for PostgresBackend { PostgresTransaction::new( client, tenant.clone(), + self.clone(), std::sync::Arc::new(self.tenant_extractor(tenant.tenant_id().as_str())), self.is_search_offloaded(), options.fhir_version.unwrap_or(self.config().fhir_version), diff --git a/crates/persistence/src/backends/s3/bundle.rs b/crates/persistence/src/backends/s3/bundle.rs index 2e31a057e..18c832861 100644 --- a/crates/persistence/src/backends/s3/bundle.rs +++ b/crates/persistence/src/backends/s3/bundle.rs @@ -48,6 +48,11 @@ impl BundleProvider for S3Backend { false } + /// No transaction, so nothing to resolve inside one. + fn supports_conditional_in_transaction(&self) -> bool { + false + } + async fn process_transaction( &self, _tenant: &TenantContext, diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index a52cb211f..17b4a56b1 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -2703,6 +2703,12 @@ impl DifferentialHistoryProvider for SqliteBackend { // Helper function to parse simple search parameters // Supports basic formats like: identifier=X, _id=Y, name=Z +/// Why conditional criteria cannot be resolved inside a transaction when +/// search is offloaded to a secondary backend (#511, #859). +const OFFLOADED_CONDITIONAL_REFUSAL: &str = "conditional criteria cannot be resolved inside a \ + transaction when search is offloaded to a secondary backend; submit the entry in a batch \ + Bundle instead"; + fn parse_simple_search_params(params: &str) -> Vec<(String, String)> { params .split('&') @@ -2899,19 +2905,23 @@ impl SqliteBackend { /// the match set includes what earlier entries of the same bundle wrote /// (#511). The pooled-connection twin above cannot see those rows under /// `BEGIN IMMEDIATE`. - fn find_matching_resources_in_tx( + /// + /// The string form `ifNoneExist` carries is parsed here and handed to the + /// typed [`ConditionalTransaction::find_matching`], so both criteria + /// forms run one search body (#859). + async fn find_matching_resources_in_tx( &self, tenant: &TenantContext, - tx: &crate::backends::sqlite::transaction::SqliteTransaction, + tx: &mut crate::backends::sqlite::transaction::SqliteTransaction, resource_type: &str, search_params_str: &str, ) -> StorageResult> { + use crate::core::ConditionalTransaction; + let Some(query) = self.conditional_query(tenant, resource_type, search_params_str)? else { return Ok(Vec::new()); }; - - tx.with_connection(|conn| self.search_with_connection(conn, tenant, &query, None)) - .map(|result| result.resources.items) + tx.find_matching(resource_type, &query.parameters).await } /// Builds the search a conditional interaction's criteria describe, or @@ -3201,6 +3211,14 @@ impl BundleProvider for SqliteBackend { true } + /// With search offloaded to a secondary backend the local index is empty + /// for every row, so an in-transaction search would always find nothing + /// and every conditional write would create the duplicate its criteria + /// exist to prevent. + fn supports_conditional_in_transaction(&self) -> bool { + !self.is_search_offloaded() + } + async fn process_transaction( &self, tenant: &TenantContext, @@ -3225,6 +3243,34 @@ impl BundleProvider for SqliteBackend { // This maps urn:uuid:xxx to ResourceType/assigned-id after creates let mut reference_map: HashMap = HashMap::new(); + // URL-borne conditional entries (`PUT/DELETE [type]?[criteria]`) + // resolve against the transaction's starting view before any entry is + // written, and an overlap between resolved identities and the other + // entries fails the bundle (R4 §3.1.0.11.2; #859). A match with a + // `fullUrl` is known now, so `urn:uuid` references to it resolve + // regardless of entry order. + let targets = match crate::core::resolve_conditional_targets( + &mut tx, + &entries, + (!self.supports_conditional_in_transaction()).then_some(OFFLOADED_CONDITIONAL_REFUSAL), + ) + .await + { + Ok(targets) => targets, + Err(e) => { + let _ = Box::new(tx).rollback().await; + return Err(e); + } + }; + for target in targets.values() { + if let (Some(full_url), Some(identity)) = ( + entries[target.entry_index].full_url.as_ref(), + target.identity(), + ) { + reference_map.insert(full_url.clone(), identity); + } + } + // Whether any entry in this transaction writes a SearchParameter that // affects this tenant's cached overlay (#787: transaction-bundle writes // never invalidated the registry, so a SearchParameter POSTed inside a @@ -3244,7 +3290,9 @@ impl BundleProvider for SqliteBackend { resolve_bundle_references(resource, &reference_map); } - let result = self.process_bundle_entry_tx(tenant, &mut tx, entry).await; + let result = self + .process_bundle_entry_tx(tenant, &mut tx, entry, targets.get(&idx)) + .await; match result { Ok(entry_result) => { @@ -3280,17 +3328,22 @@ impl BundleProvider for SqliteBackend { }) == Some("SearchParameter") } // Deleted: the emptied result carries no resource, so - // parse the type from the entry's URL instead. - 204 => self - .parse_url(&entry.url) - .map(|(resource_type, _)| resource_type == "SearchParameter") + // take the type from the entry's URL instead. + 204 => crate::core::conditional_resource_type(entry) + .map(|resource_type| resource_type == "SearchParameter") + .or_else(|| { + self.parse_url(&entry.url).ok().map(|(resource_type, _)| { + resource_type == "SearchParameter" + }) + }) .unwrap_or(false), _ => false, }; } - // If this was a create (POST) and we have a fullUrl, record the mapping - if entry.method == BundleMethod::Post { + // A create (POST, or a conditional PUT that created) with a + // fullUrl records the assigned identity for later references. + if matches!(entry.method, BundleMethod::Post | BundleMethod::Put) { if let Some(ref full_url) = entry.full_url { if let Some(ref location) = entry_result.location { // location is in format "ResourceType/id/_history/version" @@ -3345,11 +3398,16 @@ impl BundleProvider for SqliteBackend { impl SqliteBackend { /// Process a single bundle entry within a transaction. + /// + /// `target` is the pre-pass resolution of a URL-borne conditional entry + /// (#859): its `PUT` updates the match or creates, its `DELETE` deletes + /// the match or is a no-op `204`, without re-resolving the criteria. async fn process_bundle_entry_tx( &self, tenant: &TenantContext, tx: &mut crate::backends::sqlite::transaction::SqliteTransaction, entry: &BundleEntry, + target: Option<&crate::core::ConditionalTarget>, ) -> StorageResult { use crate::core::transaction::Transaction; @@ -3396,13 +3454,12 @@ impl SqliteBackend { // Refuse the entry instead; the bundle rolls back (#511). if self.is_search_offloaded() { return Ok(crate::core::not_supported_entry( - "ifNoneExist cannot be resolved inside a transaction when search \ - is offloaded to a secondary backend; submit the entry in a batch \ - Bundle instead", + OFFLOADED_CONDITIONAL_REFUSAL, )); } - let matches = - self.find_matching_resources_in_tx(tenant, tx, &resource_type, criteria)?; + let matches = self + .find_matching_resources_in_tx(tenant, tx, &resource_type, criteria) + .await?; if let Some(gated) = crate::core::bundle_if_none_exist_gate(matches) { return Ok(gated); } @@ -3419,6 +3476,17 @@ impl SqliteBackend { }) })?; + if let Some(target) = target { + return Ok(match &target.resolved { + Some(existing) => crate::core::conditional_update_entry( + tx.update(existing, resource).await?, + ), + None => BundleEntryResult::created( + tx.create(&target.resource_type, resource).await?, + ), + }); + } + let (resource_type, id) = self.parse_url(&entry.url)?; // Check if resource exists @@ -3452,6 +3520,16 @@ impl SqliteBackend { } } BundleMethod::Delete => { + if let Some(target) = target { + return Ok(match &target.resolved { + Some(existing) => { + tx.delete(&target.resource_type, existing.id()).await?; + crate::core::conditional_delete_entry(existing) + } + None => BundleEntryResult::deleted(), + }); + } + let (resource_type, id) = self.parse_url(&entry.url)?; // Honor `ifMatch` on DELETE — it was previously ignored here, so @@ -6288,6 +6366,7 @@ mod tests { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }; let tx_result = backend @@ -6336,6 +6415,7 @@ mod tests { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, // This should fail (duplicate ID) BundleEntry { @@ -6346,6 +6426,7 @@ mod tests { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, ]; @@ -6377,6 +6458,7 @@ mod tests { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, BundleEntry { method: BundleMethod::Post, @@ -6386,6 +6468,7 @@ mod tests { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, ]; diff --git a/crates/persistence/src/backends/sqlite/transaction.rs b/crates/persistence/src/backends/sqlite/transaction.rs index 9023f9827..3dd232027 100644 --- a/crates/persistence/src/backends/sqlite/transaction.rs +++ b/crates/persistence/src/backends/sqlite/transaction.rs @@ -11,12 +11,14 @@ use r2d2_sqlite::SqliteConnectionManager; use rusqlite::params; use serde_json::Value; -use crate::core::{Transaction, TransactionOptions, TransactionProvider}; +use crate::core::{ + ConditionalTransaction, Transaction, TransactionOptions, TransactionProvider, conditional_query, +}; use crate::error::{ BackendError, ConcurrencyError, ResourceError, StorageError, StorageResult, TransactionError, }; use crate::tenant::{Operation, TenantContext}; -use crate::types::StoredResource; +use crate::types::{SearchParameter, StoredResource}; use super::SqliteBackend; use super::backend::load_tenant_stored_params_with_conn; @@ -550,6 +552,30 @@ impl Drop for SqliteTransaction { } } +/// The transaction-scoped search surface (#859). +/// +/// Runs the backend's own search body on this transaction's connection, so +/// the match set includes what earlier entries of the same bundle wrote; the +/// pooled-connection twin on the backend cannot see those rows under +/// `BEGIN IMMEDIATE` (#511). +#[async_trait] +impl ConditionalTransaction for SqliteTransaction { + async fn find_matching( + &mut self, + resource_type: &str, + criteria: &[SearchParameter], + ) -> StorageResult> { + if criteria.is_empty() { + return Ok(Vec::new()); + } + let query = conditional_query(resource_type, criteria); + let backend = self.backend.clone(); + let tenant = self.tenant.clone(); + self.with_connection(|conn| backend.search_with_connection(conn, &tenant, &query, None)) + .map(|result| result.resources.items) + } +} + #[async_trait] impl TransactionProvider for SqliteBackend { type Transaction = SqliteTransaction; diff --git a/crates/persistence/src/composite/storage.rs b/crates/persistence/src/composite/storage.rs index 2144276f0..c1b8594b2 100644 --- a/crates/persistence/src/composite/storage.rs +++ b/crates/persistence/src/composite/storage.rs @@ -672,6 +672,31 @@ impl CompositeStorage { fhir_version: FhirVersion, ) { for entry_result in &result.entries { + // A conditional delete answers 204 with no body but names what it + // deleted through `location` (#859); the secondaries must drop it + // too, or a deleted resource stays searchable there. + if entry_result.status == 204 + && let Some(location) = entry_result.location.as_deref() + { + let mut segments = location.split('/').filter(|s| !s.is_empty()); + if let (Some(resource_type), Some(resource_id)) = (segments.next(), segments.next()) + && let Err(e) = self + .sync_to_secondaries(SyncEvent::Delete { + resource_type: resource_type.to_string(), + resource_id: resource_id.to_string(), + tenant_id: tenant.tenant_id().clone(), + }) + .await + { + warn!( + error = %e, + resource_type = resource_type, + resource_id = resource_id, + "Failed to sync bundle entry delete to secondaries" + ); + } + continue; + } // Only sync successful mutating operations that have a resource body if let Some(ref resource_json) = entry_result.resource { let resource_type = resource_json @@ -1700,6 +1725,15 @@ impl BundleProvider for CompositeStorage { .is_some_and(|p| p.supports_atomic_transactions()) } + /// The primary's answer: it is the primary that opens the transaction, + /// and a primary whose search is offloaded to this composite's secondary + /// says so itself. + fn supports_conditional_in_transaction(&self) -> bool { + self.bundle_provider + .as_ref() + .is_some_and(|p| p.supports_conditional_in_transaction()) + } + async fn process_transaction( &self, tenant: &TenantContext, diff --git a/crates/persistence/src/core/bundle_conditionals.rs b/crates/persistence/src/core/bundle_conditionals.rs new file mode 100644 index 000000000..137447c73 --- /dev/null +++ b/crates/persistence/src/core/bundle_conditionals.rs @@ -0,0 +1,429 @@ +//! Shared handling of URL-borne conditional entries in transaction Bundles. +//! +//! A transaction Bundle may address a resource by search criteria rather than +//! by id: `PUT [type]?[criteria]` and `DELETE [type]?[criteria]`. Every +//! transaction executor (SQLite, PostgreSQL, MongoDB) has to do the same three +//! things with such an entry, and this module is the single place they are +//! written down so the backends cannot drift apart (#859): +//! +//! 1. **Resolve before any write.** R4 §3.1.0.11.2 lists the resolution of +//! conditional identities among the steps that happen *before* the entries +//! are processed, and the overlap rule below is only meaningful if every +//! target is known up front. So an executor resolves every conditional +//! entry against the transaction's starting view, pins the outcome in a +//! [`ConditionalTarget`], and executes against that pin rather than +//! re-resolving mid-bundle. (`ifNoneExist` is resolved in entry order +//! instead, and sees earlier writes of the same bundle; the spec ties it to +//! the POST it decorates rather than to this pre-pass.) +//! 2. **Fail the bundle on an overlap.** "If any resource identities +//! (including resolved identities from conditional update/delete) overlap +//! in steps 1-3, then the transaction SHALL fail" — [`check_identity_overlap`]. +//! 3. **Answer with the resource endpoints' status mapping.** Several matches +//! are a `412 multiple-matches` for the whole bundle +//! ([`TransactionError::MultipleMatches`]); a delete names what it deleted +//! through `location`, because a `204` has no body and the URL no id. +//! +//! Criteria arrive typed ([`SearchParameter`]) on [`BundleEntry::criteria`]; +//! the caller parses them with the search parser, so the backends run the +//! same query builder the search endpoint runs. + +use std::collections::HashMap; + +use super::transaction::{BundleEntry, BundleEntryResult, BundleMethod, ConditionalTransaction}; +use crate::error::TransactionError; +use crate::types::{SearchParameter, SearchQuery, StoredResource}; + +/// Upper bound on the matches fetched for one conditional interaction. +/// +/// Two would prove non-uniqueness, but the `412` names the count it found, as +/// the resource endpoints' conditional interactions do, so the same bound they +/// use applies here. +pub const CONDITIONAL_MATCH_LIMIT: u32 = 1000; + +/// The search a conditional interaction's criteria describe. +pub fn conditional_query(resource_type: &str, criteria: &[SearchParameter]) -> SearchQuery { + SearchQuery { + resource_type: resource_type.to_string(), + parameters: criteria.to_vec(), + count: Some(CONDITIONAL_MATCH_LIMIT), + ..Default::default() + } +} + +/// The resource type a conditional entry addresses: the last path segment +/// before the `?`, with any scheme, host and server prefix stripped. +/// +/// `None` when the entry carries no criteria — such an entry addresses an +/// instance and goes through the executor's instance URL parser. +pub fn conditional_resource_type(entry: &BundleEntry) -> Option<&str> { + entry.criteria.as_ref()?; + let path = entry + .url + .split_once('?') + .map_or(entry.url.as_str(), |(p, _)| p); + let path = strip_origin(path); + path.rsplit('/').find(|segment| !segment.is_empty()) +} + +/// A conditional entry's resolution, produced inside the transaction before +/// any entry is written. +#[derive(Debug)] +pub struct ConditionalTarget { + /// Index of the entry in the ordered list the executor received. + pub entry_index: usize, + /// The type the entry's URL names. + pub resource_type: String, + /// The single match, or `None` when the criteria matched nothing (a `PUT` + /// then creates; a `DELETE` is a no-op `204`). + pub resolved: Option, +} + +impl ConditionalTarget { + /// The resolved identity as `Type/id`, when there is one. + pub fn identity(&self) -> Option { + self.resolved + .as_ref() + .map(|r| format!("{}/{}", r.resource_type(), r.id())) + } +} + +/// Turns a conditional entry's match set into its pinned target, or into the +/// whole-bundle `412` several matches call for. +pub fn conditional_target( + entry_index: usize, + entry: &BundleEntry, + resource_type: &str, + matches: Vec, +) -> Result { + match matches.len() { + 0 | 1 => Ok(ConditionalTarget { + entry_index, + resource_type: resource_type.to_string(), + resolved: matches.into_iter().next(), + }), + count => Err(TransactionError::MultipleMatches { + operation: conditional_operation(entry.method).to_string(), + count, + }), + } +} + +/// R4 §3.1.0.11.2: a conditional entry whose resolved identity is also +/// addressed by another entry — an instance-addressed `PUT`/`DELETE`/`PATCH`, +/// or another conditional entry that resolved to the same resource — fails +/// the bundle. +/// +/// Two instance-addressed entries naming the same id are not reported here: +/// that has never been detected on this path, and the entries execute in +/// order as they always did. The rule is enforced where a *resolved* identity +/// is involved, which is what the conditional pre-pass exists to see. +pub fn check_identity_overlap( + entries: &[BundleEntry], + targets: &[ConditionalTarget], +) -> Result<(), TransactionError> { + let mut seen: HashMap = HashMap::new(); + for (index, entry) in entries.iter().enumerate() { + if entry.criteria.is_some() + || !matches!( + entry.method, + BundleMethod::Put | BundleMethod::Delete | BundleMethod::Patch + ) + { + continue; + } + if let Some(identity) = instance_identity(&entry.url) { + seen.entry(identity).or_insert(index); + } + } + + for target in targets { + let Some(identity) = target.identity() else { + continue; + }; + if let Some(&other) = seen.get(&identity) { + let entry = &entries[target.entry_index]; + let other_entry = &entries[other]; + return Err(TransactionError::BundleError { + index: target.entry_index, + message: format!( + "{} {} resolves to {identity}, which entry {other} ({} {}) also \ + addresses; a transaction whose resolved identities overlap \ + fails as a whole (R4 §3.1.0.11.2)", + entry.method, entry.url, other_entry.method, other_entry.url + ), + }); + } + seen.insert(identity, target.entry_index); + } + Ok(()) +} + +/// Resolves every conditional entry of a transaction against `tx`'s view, +/// before any entry is written, and enforces [`check_identity_overlap`]. +/// +/// `unsupported` names why this backend cannot evaluate criteria inside its +/// transaction (search offloaded to a secondary, so the local index is empty); +/// when set, the first conditional entry fails the bundle with that reason +/// rather than resolving to "no match" and duplicating. +/// +/// Returns the targets keyed by entry index; an entry without criteria has no +/// target and executes through the instance path as before. +pub async fn resolve_conditional_targets( + tx: &mut T, + entries: &[BundleEntry], + unsupported: Option<&str>, +) -> Result, TransactionError> +where + T: ConditionalTransaction + ?Sized, +{ + let mut targets = Vec::new(); + for (index, entry) in entries.iter().enumerate() { + let Some(criteria) = entry.criteria.as_deref() else { + continue; + }; + if let Some(reason) = unsupported { + return Err(unsupported_conditional_entry(index, reason)); + } + let resource_type = conditional_resource_type(entry) + .filter(|t| !t.is_empty()) + .ok_or_else(|| TransactionError::BundleError { + index, + message: format!("Entry request.url '{}' names no resource type", entry.url), + })? + .to_string(); + let matches = tx + .find_matching(&resource_type, criteria) + .await + .map_err(|e| TransactionError::BundleError { + index, + message: format!("Entry processing failed: {e}"), + })?; + targets.push(conditional_target(index, entry, &resource_type, matches)?); + } + check_identity_overlap(entries, &targets)?; + Ok(targets + .into_iter() + .map(|target| (target.entry_index, target)) + .collect()) +} + +/// The `200` a conditional update answers when its criteria matched. +/// +/// Carries `location`, as the batch arm's and `ifNoneExist`'s `200`s do, so a +/// `urn:uuid` reference to the entry resolves to the match. +pub fn conditional_update_entry(updated: StoredResource) -> BundleEntryResult { + let location = updated.versioned_url(); + let mut result = BundleEntryResult::ok(updated); + result.location = Some(location); + result +} + +/// The `204` a conditional delete answers, naming the resource it deleted +/// through `location` (the version that was current when it was deleted). +/// +/// A `204` has no body and a criteria URL no id, so without this the audit +/// trail and a composite's secondary sync would have nothing to name. +pub fn conditional_delete_entry(deleted: &StoredResource) -> BundleEntryResult { + let mut result = BundleEntryResult::deleted(); + result.location = Some(deleted.versioned_url()); + result +} + +/// The whole-bundle error for a conditional entry a backend cannot evaluate +/// inside its transaction, carrying the `501` the entry would have answered. +pub fn unsupported_conditional_entry(index: usize, diagnostics: &str) -> TransactionError { + TransactionError::BundleError { + index, + message: format!("Entry failed with status 501: {diagnostics}"), + } +} + +fn conditional_operation(method: BundleMethod) -> &'static str { + match method { + BundleMethod::Put | BundleMethod::Patch => "update", + BundleMethod::Delete => "delete", + BundleMethod::Post => "create", + BundleMethod::Get => "read", + } +} + +fn strip_origin(url: &str) -> &str { + let without_scheme = url + .strip_prefix("http://") + .or_else(|| url.strip_prefix("https://")); + match without_scheme { + Some(rest) => rest.find('/').map(|i| &rest[i..]).unwrap_or(""), + None => url, + } +} + +/// `Type/id` of an instance-addressed entry URL, or `None` for a type-level +/// one. Tolerates a server prefix and a `/_history/{v}` suffix. +fn instance_identity(url: &str) -> Option { + let path = url.split_once('?').map_or(url, |(p, _)| p); + let segments: Vec<&str> = strip_origin(path) + .split('/') + .filter(|s| !s.is_empty()) + .collect(); + let type_index = segments + .iter() + .rposition(|s| s.chars().next().is_some_and(|c| c.is_ascii_uppercase()))?; + let id = segments.get(type_index + 1)?; + Some(format!("{}/{}", segments[type_index], id)) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tenant::TenantId; + use crate::types::SearchParamType; + + fn entry(method: BundleMethod, url: &str, conditional: bool) -> BundleEntry { + BundleEntry { + method, + url: url.to_string(), + criteria: conditional.then(|| { + vec![SearchParameter { + name: "identifier".to_string(), + param_type: SearchParamType::Token, + ..Default::default() + }] + }), + ..Default::default() + } + } + + fn stored(resource_type: &str, id: &str) -> StoredResource { + StoredResource::new( + resource_type, + id, + TenantId::new("t"), + serde_json::json!({"resourceType": resource_type, "id": id}), + helios_fhir::FhirVersion::R4, + ) + } + + fn target(index: usize, resolved: Option) -> ConditionalTarget { + ConditionalTarget { + entry_index: index, + resource_type: "Patient".to_string(), + resolved, + } + } + + #[test] + fn resource_type_is_the_segment_before_the_query() { + for url in [ + "Patient?identifier=x", + "/Patient?identifier=x", + "http://example.org/fhir/Patient?identifier=x", + ] { + let e = entry(BundleMethod::Put, url, true); + assert_eq!(conditional_resource_type(&e), Some("Patient"), "{url}"); + } + let plain = entry(BundleMethod::Put, "Patient/1", false); + assert_eq!(conditional_resource_type(&plain), None); + } + + #[test] + fn instance_identity_tolerates_prefix_and_history() { + assert_eq!(instance_identity("Patient/1"), Some("Patient/1".into())); + assert_eq!( + instance_identity("/fhir/Patient/1/_history/3"), + Some("Patient/1".into()) + ); + assert_eq!( + instance_identity("https://h/fhir/Patient/1?_format=json"), + Some("Patient/1".into()) + ); + assert_eq!(instance_identity("Patient"), None); + assert_eq!(instance_identity("Patient?identifier=x"), None); + } + + #[test] + fn several_matches_are_a_412_naming_the_operation() { + let e = entry(BundleMethod::Delete, "Patient?identifier=x", true); + let err = conditional_target( + 0, + &e, + "Patient", + vec![stored("Patient", "a"), stored("Patient", "b")], + ) + .expect_err("two matches"); + match err { + TransactionError::MultipleMatches { operation, count } => { + assert_eq!(operation, "delete"); + assert_eq!(count, 2); + } + other => panic!("unexpected {other:?}"), + } + let none = conditional_target(0, &e, "Patient", vec![]).expect("no match is fine"); + assert!(none.resolved.is_none()); + } + + #[test] + fn resolved_identity_colliding_with_an_instance_entry_fails() { + let entries = vec![ + entry(BundleMethod::Put, "Patient/p1", false), + entry(BundleMethod::Put, "Patient?identifier=x", true), + ]; + let targets = vec![target(1, Some(stored("Patient", "p1")))]; + let err = check_identity_overlap(&entries, &targets).expect_err("overlap"); + match err { + TransactionError::BundleError { index, message } => { + assert_eq!(index, 1); + assert!(message.contains("Patient/p1"), "{message}"); + assert!(message.contains("entry 0"), "{message}"); + } + other => panic!("unexpected {other:?}"), + } + } + + #[test] + fn two_conditional_entries_resolving_to_one_resource_fail() { + let entries = vec![ + entry(BundleMethod::Delete, "Patient?identifier=x", true), + entry(BundleMethod::Put, "Patient?identifier=x", true), + ]; + let targets = vec![ + target(0, Some(stored("Patient", "p1"))), + target(1, Some(stored("Patient", "p1"))), + ]; + let err = check_identity_overlap(&entries, &targets).expect_err("overlap"); + assert!( + matches!(err, TransactionError::BundleError { index: 1, .. }), + "{err:?}" + ); + } + + #[test] + fn distinct_ids_gets_and_unresolved_targets_do_not_overlap() { + let entries = vec![ + entry(BundleMethod::Get, "Patient/p1", false), + entry(BundleMethod::Put, "Patient/p2", false), + entry(BundleMethod::Put, "Patient?identifier=x", true), + entry(BundleMethod::Delete, "Patient?identifier=y", true), + ]; + let targets = vec![target(2, Some(stored("Patient", "p1"))), target(3, None)]; + check_identity_overlap(&entries, &targets).expect("no overlap"); + } + + #[test] + fn update_entry_names_the_updated_version() { + let result = conditional_update_entry(stored("Patient", "p1")); + assert_eq!(result.status, 200); + assert_eq!(result.location.as_deref(), Some("Patient/p1/_history/1")); + assert_eq!( + result.resource.as_ref().and_then(|r| r["id"].as_str()), + Some("p1") + ); + } + + #[test] + fn delete_entry_names_the_deleted_version() { + let result = conditional_delete_entry(&stored("Patient", "p1")); + assert_eq!(result.status, 204); + assert_eq!(result.location.as_deref(), Some("Patient/p1/_history/1")); + assert!(result.resource.is_none()); + } +} diff --git a/crates/persistence/src/core/mod.rs b/crates/persistence/src/core/mod.rs index d58cf3b9e..9970dd288 100644 --- a/crates/persistence/src/core/mod.rs +++ b/crates/persistence/src/core/mod.rs @@ -97,6 +97,7 @@ pub mod bulk_provider; pub mod bulk_submit; pub mod bulk_submit_input; pub mod bulk_submit_worker; +pub mod bundle_conditionals; pub mod capabilities; pub mod history; pub mod preconditions; @@ -138,6 +139,11 @@ pub use bulk_submit_worker::{ ManifestWorkerView, PollTokenTarget, SubmitClaimStrategy, SubmitFileRecord, SubmitFileRow, SubmitWorkerStorage, }; +pub use bundle_conditionals::{ + CONDITIONAL_MATCH_LIMIT, ConditionalTarget, check_identity_overlap, conditional_delete_entry, + conditional_query, conditional_resource_type, conditional_target, conditional_update_entry, + resolve_conditional_targets, unsupported_conditional_entry, +}; pub use capabilities::{ CapabilityProvider, GlobalSearchCapabilities, Interaction, ResourceCapabilities, ResourceSearchCapabilities, SearchCapabilityProvider, SearchParamCapability, @@ -165,7 +171,8 @@ pub use storage::{ }; pub use transaction::{ BundleEntry, BundleEntryResult, BundleMethod, BundleProvider, BundleResult, BundleType, - IsolationLevel, LockingStrategy, Transaction, TransactionOptions, TransactionProvider, + ConditionalTransaction, IsolationLevel, LockingStrategy, Transaction, TransactionOptions, + TransactionProvider, }; pub use user_settings::{ BY_TENANT_KEY, GLOBAL_SETTINGS_KEYS, SettingsStore, StoredUserSettings, apply_merge_patch, diff --git a/crates/persistence/src/core/transaction.rs b/crates/persistence/src/core/transaction.rs index 957bc236e..2428afdb0 100644 --- a/crates/persistence/src/core/transaction.rs +++ b/crates/persistence/src/core/transaction.rs @@ -9,9 +9,11 @@ use serde_json::Value; use crate::error::{StorageResult, TransactionError}; use crate::tenant::TenantContext; -use crate::types::StoredResource; +use crate::types::{SearchParameter, StoredResource}; -use super::storage::ResourceStorage; +use super::storage::{ + ConditionalCreateResult, ConditionalDeleteResult, ConditionalUpdateResult, ResourceStorage, +}; /// Transaction isolation levels. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -248,6 +250,106 @@ pub trait TransactionProvider: ResourceStorage { } } +/// Conditional interactions inside an open transaction. +/// +/// [`Transaction`] addresses resources by id. FHIR's conditional interactions +/// address them by search criteria, and a transaction Bundle may carry them +/// (`PUT [type]?[criteria]`, `DELETE [type]?[criteria]`, `ifNoneExist`). The +/// backend-level [`ConditionalStorage`](super::storage::ConditionalStorage) +/// cannot serve those: it searches and writes through the backend's own +/// connection, outside the open transaction, so a bundle that used it would +/// commit one entry while the rest could still roll back (#859). +/// +/// This is a separate trait rather than a widening of [`Transaction`] — the +/// shape design discussion #28 proposed — so a backend adopts it once it has a +/// transaction-scoped search, instead of every implementor of the base trait +/// being widened at once. Only [`find_matching`](Self::find_matching) is +/// required: it is the search surface, resolving criteria against *this +/// transaction's* view, earlier writes of the same transaction included. The +/// three interactions are defaulted on top of it and [`Transaction`]'s +/// id-addressed writes, answering with the same outcome enums the +/// backend-level trait returns. +/// +/// `criteria` is typed, not a `k=v&k=v` string: the caller has parsed and +/// validated it with the search parser, so modifiers and chains reach the +/// query builder intact (#861, #865). Empty criteria match nothing — matching +/// everything would be the literal reading, but no conditional interaction +/// means that. +#[async_trait] +pub trait ConditionalTransaction: Transaction { + /// Resolves `criteria` against `resource_type` as this transaction sees + /// it. + /// + /// Returns every match up to the backend's conditional match limit, so a + /// caller can distinguish none, one, and several. + async fn find_matching( + &mut self, + resource_type: &str, + criteria: &[SearchParameter], + ) -> StorageResult>; + + /// Conditional create: creates `resource` only when `criteria` match + /// nothing; one match is answered as it stands. + async fn create_if_none_exist( + &mut self, + resource_type: &str, + resource: Value, + criteria: &[SearchParameter], + ) -> StorageResult { + let mut matches = self.find_matching(resource_type, criteria).await?; + match matches.len() { + 0 => Ok(ConditionalCreateResult::Created( + self.create(resource_type, resource).await?, + )), + 1 => Ok(ConditionalCreateResult::Exists(matches.remove(0))), + n => Ok(ConditionalCreateResult::MultipleMatches(n)), + } + } + + /// Conditional update with upsert: one match is updated, no match creates + /// `resource`, several matches are refused. + async fn update_conditional( + &mut self, + resource_type: &str, + resource: Value, + criteria: &[SearchParameter], + ) -> StorageResult { + let mut matches = self.find_matching(resource_type, criteria).await?; + match matches.len() { + 0 => Ok(ConditionalUpdateResult::Created( + self.create(resource_type, resource).await?, + )), + 1 => { + let existing = matches.remove(0); + Ok(ConditionalUpdateResult::Updated( + self.update(&existing, resource).await?, + )) + } + n => Ok(ConditionalUpdateResult::MultipleMatches(n)), + } + } + + /// Conditional delete: one match is deleted, no match is not an error, + /// several matches are refused (this server elects + /// `conditionalDelete: "single"`). + async fn delete_conditional( + &mut self, + resource_type: &str, + criteria: &[SearchParameter], + ) -> StorageResult { + let mut matches = self.find_matching(resource_type, criteria).await?; + match matches.len() { + 0 => Ok(ConditionalDeleteResult::NoMatch), + 1 => { + let existing = matches.remove(0); + self.delete(resource_type, existing.id()).await?; + Ok(ConditionalDeleteResult::Deleted(existing)) + } + n => Ok(ConditionalDeleteResult::MultipleMatches(n)), + } + } +} + /// Entry in a FHIR transaction or batch bundle. #[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct BundleEntry { @@ -273,6 +375,18 @@ pub struct BundleEntry { /// Typically a urn:uuid: for new resources in transactions. #[serde(default, skip_serializing_if = "Option::is_none")] pub full_url: Option, + /// Typed criteria of a `PUT [type]?[criteria]` or `DELETE [type]?[criteria]` + /// entry (#859); `None` for every other entry. + /// + /// Set by the caller, which percent-decodes and parses the entry URL's + /// query with the same parser the search endpoint uses, so modifiers, + /// chains and prefixes reach the backend typed rather than as a `k=v&k=v` + /// string it would have to re-parse (#861, #865). `url` keeps its + /// `[type]?[criteria]` form for audit and messages; a transaction executor + /// takes the type from the text before `?` and never routes such a URL + /// through its instance parser. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub criteria: Option>, } /// HTTP method for bundle entries. @@ -432,6 +546,23 @@ pub trait BundleProvider: ResourceStorage { /// transaction support. fn supports_atomic_transactions(&self) -> bool; + /// Whether this provider resolves conditional interactions inside the + /// transaction it opens for a Bundle (#28's + /// `supports_conditional_in_transaction`; #859). + /// + /// Covers `PUT [type]?[criteria]`, `DELETE [type]?[criteria]` and + /// `ifNoneExist`. A backend whose local search index is empty because + /// search is offloaded to a secondary (composite SQLite/PostgreSQL + + /// Elasticsearch) answers `false`: its transaction-scoped search would + /// find nothing, and "no match" on a conditional write is a create, so + /// the bundle would duplicate exactly what the criteria exist to prevent. + /// The REST layer consults this before anything executes, so such a + /// bundle is declined intact with `501` rather than failing at the entry. + /// + /// Required, not defaulted, for the reason given on + /// [`supports_atomic_transactions`](Self::supports_atomic_transactions). + fn supports_conditional_in_transaction(&self) -> bool; + /// Processes a transaction bundle (all-or-nothing). /// /// All entries are processed atomically. If any entry fails, diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 572bcaf1f..3ef074676 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -883,6 +883,7 @@ async fn mongodb_integration_transaction_bundle_create_and_resolve_references() if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:new-patient".to_string()), + criteria: None, }, BundleEntry { method: BundleMethod::Post, @@ -897,6 +898,7 @@ async fn mongodb_integration_transaction_bundle_create_and_resolve_references() if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:new-observation".to_string()), + criteria: None, }, ]; @@ -992,6 +994,7 @@ async fn mongodb_integration_transaction_bundle_mixed_operations_and_idempotent_ if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, BundleEntry { method: BundleMethod::Post, @@ -1005,6 +1008,7 @@ async fn mongodb_integration_transaction_bundle_mixed_operations_and_idempotent_ if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:new-created".to_string()), + criteria: None, }, BundleEntry { method: BundleMethod::Put, @@ -1018,6 +1022,7 @@ async fn mongodb_integration_transaction_bundle_mixed_operations_and_idempotent_ if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, ]; @@ -1064,6 +1069,7 @@ async fn mongodb_integration_transaction_bundle_mixed_operations_and_idempotent_ if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }]; let Some(idempotent_result) = process_transaction_or_skip( @@ -1119,6 +1125,7 @@ async fn mongodb_integration_transaction_if_none_exist_match_resolves_urn_refere if_none_match: None, if_none_exist: Some("identifier=http://example.org/mrn|MRN-URN-1".to_string()), full_url: Some("urn:uuid:patient".to_string()), + criteria: None, }, BundleEntry { method: BundleMethod::Post, @@ -1133,6 +1140,7 @@ async fn mongodb_integration_transaction_if_none_exist_match_resolves_urn_refere if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:observation".to_string()), + criteria: None, }, ]; @@ -1187,6 +1195,7 @@ async fn mongodb_integration_transaction_bundle_conditional_headers() { if_none_match: None, if_none_exist: Some("identifier=http://example.org/mrn|MRN-TX-COND-1".to_string()), full_url: Some("urn:uuid:conditional-create".to_string()), + criteria: None, }]; let Some(first_create) = process_transaction_or_skip( @@ -1246,6 +1255,7 @@ async fn mongodb_integration_transaction_bundle_conditional_headers() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }]; let Some(good_if_match_result) = process_transaction_or_skip( @@ -1272,6 +1282,7 @@ async fn mongodb_integration_transaction_bundle_conditional_headers() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }]; match backend @@ -1338,6 +1349,7 @@ async fn mongodb_integration_transaction_bundle_rolls_back_on_failure() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:rollback-created".to_string()), + criteria: None, }, BundleEntry { method: BundleMethod::Post, @@ -1351,6 +1363,7 @@ async fn mongodb_integration_transaction_bundle_rolls_back_on_failure() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:rollback-fail".to_string()), + criteria: None, }, ]; diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 40e92be1c..9a11d9a6e 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -4581,6 +4581,7 @@ mod postgres_integration { if_none_match: None, if_none_exist: Some("identifier=http://example.org/mrn|MRN-TX-COND-1".to_string()), full_url: Some(full_url.to_string()), + criteria: None, } } @@ -4682,6 +4683,7 @@ mod postgres_integration { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:observation".to_string()), + criteria: None, }, ], FhirVersion::default(), @@ -4726,6 +4728,7 @@ mod postgres_integration { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, if_none_exist_entry("Ambiguous", "urn:uuid:ambiguous"), ], diff --git a/crates/persistence/tests/transactions/bundle_tests.rs b/crates/persistence/tests/transactions/bundle_tests.rs index eaec4554c..7b6b9ac2b 100644 --- a/crates/persistence/tests/transactions/bundle_tests.rs +++ b/crates/persistence/tests/transactions/bundle_tests.rs @@ -54,6 +54,7 @@ fn if_none_exist_entry(family: &str, full_url: &str) -> BundleEntry { if_none_match: None, if_none_exist: Some("identifier=http://example.org|12345".to_string()), full_url: Some(full_url.to_string()), + criteria: None, } } @@ -87,6 +88,7 @@ async fn test_bundle_create_entries() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:patient-1".to_string()), + criteria: None, }, BundleEntry { method: BundleMethod::Post, @@ -99,6 +101,7 @@ async fn test_bundle_create_entries() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:patient-2".to_string()), + criteria: None, }, ]; @@ -140,6 +143,7 @@ async fn test_bundle_put_entries() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:patient-put".to_string()), + criteria: None, }]; let result = backend @@ -186,6 +190,7 @@ async fn test_bundle_delete_entries() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }]; let result = backend @@ -251,6 +256,7 @@ async fn test_bundle_mixed_operations() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:new-patient".to_string()), + criteria: None, }, // UPDATE BundleEntry { @@ -265,6 +271,7 @@ async fn test_bundle_mixed_operations() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, // DELETE BundleEntry { @@ -275,6 +282,7 @@ async fn test_bundle_mixed_operations() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, ]; @@ -330,6 +338,7 @@ async fn test_bundle_internal_references() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:new-patient".to_string()), + criteria: None, }, // Create observation referencing patient by urn:uuid BundleEntry { @@ -345,6 +354,7 @@ async fn test_bundle_internal_references() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:new-observation".to_string()), + criteria: None, }, ]; @@ -472,6 +482,7 @@ async fn test_bundle_if_none_exist_match_resolves_urn_references() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:observation".to_string()), + criteria: None, }, ]; @@ -523,6 +534,7 @@ async fn test_bundle_if_none_exist_multiple_matches_rolls_back() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, if_none_exist_entry("Ambiguous", "urn:uuid:ambiguous"), ]; @@ -634,6 +646,7 @@ async fn test_bundle_conditional_update_if_match() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }]; let result = backend @@ -683,6 +696,7 @@ async fn test_bundle_if_match_failure() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }]; let result = backend @@ -719,6 +733,7 @@ async fn test_bundle_atomicity() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:valid".to_string()), + criteria: None, }, // Invalid - delete non-existent BundleEntry { @@ -729,6 +744,7 @@ async fn test_bundle_atomicity() { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, }, ]; @@ -781,6 +797,7 @@ async fn test_bundle_single_entry() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:single".to_string()), + criteria: None, }]; let result = backend @@ -810,6 +827,7 @@ async fn test_bundle_tenant_isolation() { if_none_match: None, if_none_exist: None, full_url: Some("urn:uuid:tenant-patient".to_string()), + criteria: None, }]; let result = backend diff --git a/crates/persistence/tests/transactions/if_match_suite.rs b/crates/persistence/tests/transactions/if_match_suite.rs index f6b09f191..a6b78636e 100644 --- a/crates/persistence/tests/transactions/if_match_suite.rs +++ b/crates/persistence/tests/transactions/if_match_suite.rs @@ -66,6 +66,7 @@ pub fn put_entry(id: &str, family: &str, if_match: Option<&str>) -> BundleEntry if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, } } @@ -79,6 +80,7 @@ pub fn delete_entry(id: &str, if_match: Option<&str>) -> BundleEntry { if_none_match: None, if_none_exist: None, full_url: None, + criteria: None, } } diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index 2d89293fd..9b4a884b4 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -2122,6 +2122,7 @@ fn parse_bundle_entry(entry: &Value) -> Result<(BundleEntry, Option), En if_none_match, if_none_exist, full_url: None, // Will be set later + criteria: None, }, full_url, )) From 07ca13f41e3801ba0572042de2376b4b56f9122e Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 2 Sep 2026 20:34:06 -0400 Subject: [PATCH 3/7] test(persistence): shared conditional-URL transaction suite for SQLite, PostgreSQL and MongoDB Add `tests/transactions/conditional_url_suite.rs`, generic over `BundleProvider` and `#[path]`-included by all three backend binaries the way `if_match_suite.rs` is, so the same nine scenarios run everywhere: update the single match (200 naming the new version), create on no match, 412 with the sibling create rolled back, delete the match (204 naming the deleted version), no-match 204, several-match delete rolled back, overlap with an instance-addressed entry, two conditional entries resolving to one resource, and a matched PUT's `fullUrl` resolving a `urn:uuid` reference from an earlier entry. SQLite additionally covers the offloaded-search 501 refusal and the `ConditionalTransaction` defaults (upsert, single-match delete, empty criteria match nothing). MongoDB additionally asserts the 501 for a criterion with a modifier. Postgres and Mongo wrappers use per-scenario tenants on the shared container; Mongo probes the topology first and skips on standalone. Tests: transactions_suite 53 pass; postgres_integration 18 matching pass on testcontainers; the 11 MongoDB transaction cases pass against a replica-set container (they skip on the standalone testcontainer). --- crates/persistence/tests/mongodb_tests.rs | 117 +++++ crates/persistence/tests/postgres_tests.rs | 55 +++ .../tests/transactions/bundle_tests.rs | 127 ++++++ .../transactions/conditional_url_suite.rs | 404 ++++++++++++++++++ crates/persistence/tests/transactions/mod.rs | 4 + 5 files changed, 707 insertions(+) create mode 100644 crates/persistence/tests/transactions/conditional_url_suite.rs diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 3ef074676..243fb8d58 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -301,6 +301,11 @@ mod shared_mongo { #[path = "multitenancy/tenant_id_fidelity_suite.rs"] mod tenant_id_fidelity_suite; +/// Backend-agnostic `PUT/DELETE [type]?[criteria]` transaction scenarios +/// (#859), shared with the SQLite and PostgreSQL suites. +#[path = "transactions/conditional_url_suite.rs"] +mod conditional_url_suite; + /// The backend-agnostic day-precision date-boundary suite (issue #519) — the /// #456 table that #463 pinned for SQLite only. Same `#[path]` arrangement. #[path = "search/date_boundary_suite.rs"] @@ -4528,3 +4533,115 @@ async fn mongodb_integration_export_until_is_inclusive() { "a resource exactly on the bound is included" ); } + +// ============================================================================ +// Issue #859 — `PUT/DELETE [type]?[criteria]` inside a transaction +// ============================================================================ + +/// Runs one shared #859 scenario on its own database and tenant, skipping when +/// Docker is unavailable or the topology cannot run transactions (probed with an +/// empty bundle, the way `mongodb_integration_transaction_bundle_topology_behavior` +/// does, since the scenarios call `process_transaction` directly). +macro_rules! mongodb_conditional_url_test { + ($test_name:ident, $scenario:ident) => { + #[tokio::test] + async fn $test_name() { + let Some(backend) = create_backend(stringify!($scenario)).await else { + eprintln!( + "Skipping {} (requires Docker or HFS_TEST_MONGODB_URL)", + stringify!($test_name) + ); + return; + }; + let tenant = create_tenant(concat!("tenant-cond-url-", stringify!($scenario))); + if process_transaction_or_skip(&backend, &tenant, vec![], stringify!($test_name)) + .await + .is_none() + { + return; + } + conditional_url_suite::$scenario(&backend, &tenant).await; + } + }; +} + +mongodb_conditional_url_test!( + mongodb_integration_conditional_put_updates_the_single_match, + conditional_put_updates_the_single_match +); +mongodb_conditional_url_test!( + mongodb_integration_conditional_put_creates_when_nothing_matches, + conditional_put_creates_when_nothing_matches +); +mongodb_conditional_url_test!( + mongodb_integration_conditional_put_with_several_matches_rolls_back, + conditional_put_with_several_matches_rolls_back +); +mongodb_conditional_url_test!( + mongodb_integration_conditional_delete_removes_the_single_match, + conditional_delete_removes_the_single_match +); +mongodb_conditional_url_test!( + mongodb_integration_conditional_delete_with_no_match_is_204, + conditional_delete_with_no_match_is_204 +); +mongodb_conditional_url_test!( + mongodb_integration_conditional_delete_with_several_matches_rolls_back, + conditional_delete_with_several_matches_rolls_back +); +mongodb_conditional_url_test!( + mongodb_integration_overlap_with_an_instance_entry_fails_the_bundle, + overlap_with_an_instance_entry_fails_the_bundle +); +mongodb_conditional_url_test!( + mongodb_integration_two_conditional_entries_resolving_to_one_resource_fail, + two_conditional_entries_resolving_to_one_resource_fail +); +mongodb_conditional_url_test!( + mongodb_integration_matched_conditional_put_resolves_urn_references, + matched_conditional_put_resolves_urn_references +); + +/// The session-scoped matcher evaluates plain `name=value` only; a modifier +/// refuses the entry with the 501 the offloaded-search case answers elsewhere, +/// rather than silently matching nothing and creating a duplicate (#865, #709). +#[tokio::test] +async fn mongodb_integration_conditional_url_with_a_modifier_is_refused() { + let Some(backend) = create_backend("conditional_url_modifier").await else { + eprintln!( + "Skipping mongodb_integration_conditional_url_with_a_modifier_is_refused (requires Docker or HFS_TEST_MONGODB_URL)" + ); + return; + }; + let tenant = create_tenant("tenant-cond-url-modifier"); + if process_transaction_or_skip( + &backend, + &tenant, + vec![], + "mongodb_integration_conditional_url_with_a_modifier_is_refused", + ) + .await + .is_none() + { + return; + } + + let mut entry = conditional_url_suite::conditional_put("Modifier", None); + let criteria = entry.criteria.as_mut().expect("criteria"); + criteria[0].modifier = Some(helios_persistence::types::SearchModifier::Exact); + entry.url = "Patient?identifier:exact=http://example.org|12345".to_string(); + + let err = backend + .process_transaction(&tenant, vec![entry], FhirVersion::default()) + .await + .expect_err("a modifier cannot be evaluated in memory"); + match err { + TransactionError::BundleError { index, message } => { + assert_eq!(index, 0); + assert!(message.contains("501"), "{message}"); + assert!(message.contains("identifier:exact"), "{message}"); + } + other => panic!("unexpected error: {other:?}"), + } + assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 0); +} diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 9a11d9a6e..c4af3d870 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -21,6 +21,11 @@ use helios_persistence::core::BackendKind; #[path = "transactions/if_match_suite.rs"] mod if_match_suite; +/// Backend-agnostic `PUT/DELETE [type]?[criteria]` transaction scenarios +/// (#859), shared with the SQLite and MongoDB suites. +#[path = "transactions/conditional_url_suite.rs"] +mod conditional_url_suite; + /// The backend-agnostic tenant-id fidelity scenarios (issue #447), shared /// verbatim with the SQLite and MongoDB suites. Declared at the top level for /// the same `#[path]` resolution reason as `if_match_suite` above. @@ -6192,6 +6197,56 @@ mod postgres_integration { }; } + /// One `#[tokio::test]` per shared #859 scenario, each on its own + /// UUID-suffixed tenant so they cannot collide on the shared container. + macro_rules! pg_conditional_url_test { + ($test_name:ident, $scenario:ident) => { + #[tokio::test] + async fn $test_name() { + let backend = create_backend().await; + let tenant = create_tenant(concat!("cond_url_", stringify!($scenario))); + super::conditional_url_suite::$scenario(&backend, &tenant).await; + } + }; + } + + pg_conditional_url_test!( + postgres_integration_conditional_put_updates_the_single_match, + conditional_put_updates_the_single_match + ); + pg_conditional_url_test!( + postgres_integration_conditional_put_creates_when_nothing_matches, + conditional_put_creates_when_nothing_matches + ); + pg_conditional_url_test!( + postgres_integration_conditional_put_with_several_matches_rolls_back, + conditional_put_with_several_matches_rolls_back + ); + pg_conditional_url_test!( + postgres_integration_conditional_delete_removes_the_single_match, + conditional_delete_removes_the_single_match + ); + pg_conditional_url_test!( + postgres_integration_conditional_delete_with_no_match_is_204, + conditional_delete_with_no_match_is_204 + ); + pg_conditional_url_test!( + postgres_integration_conditional_delete_with_several_matches_rolls_back, + conditional_delete_with_several_matches_rolls_back + ); + pg_conditional_url_test!( + postgres_integration_overlap_with_an_instance_entry_fails_the_bundle, + overlap_with_an_instance_entry_fails_the_bundle + ); + pg_conditional_url_test!( + postgres_integration_two_conditional_entries_resolving_to_one_resource_fail, + two_conditional_entries_resolving_to_one_resource_fail + ); + pg_conditional_url_test!( + postgres_integration_matched_conditional_put_resolves_urn_references, + matched_conditional_put_resolves_urn_references + ); + pg_if_match_test!( postgres_integration_multi_valued_if_match_matches_any_member, multi_valued_if_match_matches_any_member diff --git a/crates/persistence/tests/transactions/bundle_tests.rs b/crates/persistence/tests/transactions/bundle_tests.rs index 7b6b9ac2b..5d5b4c2b9 100644 --- a/crates/persistence/tests/transactions/bundle_tests.rs +++ b/crates/persistence/tests/transactions/bundle_tests.rs @@ -883,3 +883,130 @@ sqlite_if_match_test!(multi_valued_if_match_fails_when_no_member_matches); sqlite_if_match_test!(strong_form_if_match_matches_weak_etag); sqlite_if_match_test!(transaction_delete_honors_stale_if_match); sqlite_if_match_test!(transaction_delete_accepts_matching_if_match); + +// ============================================================================ +// Issue #859 — `PUT/DELETE [type]?[criteria]` inside a transaction +// +// The scenarios live in `super::conditional_url_suite` so PostgreSQL and +// MongoDB run the same assertions. Each wrapper gets its own in-memory backend +// with the spec search parameters loaded (`identifier` is not in the embedded +// minimal set). +// ============================================================================ + +macro_rules! sqlite_conditional_url_test { + ($name:ident) => { + #[cfg(feature = "sqlite")] + #[tokio::test] + async fn $name() { + let backend = create_sqlite_backend_with_spec_params(); + super::conditional_url_suite::$name(&backend, &create_tenant()).await; + } + }; +} + +sqlite_conditional_url_test!(conditional_put_updates_the_single_match); +sqlite_conditional_url_test!(conditional_put_creates_when_nothing_matches); +sqlite_conditional_url_test!(conditional_put_with_several_matches_rolls_back); +sqlite_conditional_url_test!(conditional_delete_removes_the_single_match); +sqlite_conditional_url_test!(conditional_delete_with_no_match_is_204); +sqlite_conditional_url_test!(conditional_delete_with_several_matches_rolls_back); +sqlite_conditional_url_test!(overlap_with_an_instance_entry_fails_the_bundle); +sqlite_conditional_url_test!(two_conditional_entries_resolving_to_one_resource_fail); +sqlite_conditional_url_test!(matched_conditional_put_resolves_urn_references); + +/// With search offloaded the local index is empty, so a URL-borne conditional +/// entry is refused with the same 501 `ifNoneExist` gets, before any write. +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_bundle_conditional_url_is_refused_when_search_is_offloaded() { + let mut backend = create_sqlite_backend_with_spec_params(); + backend.set_search_offloaded(true); + assert!(!backend.supports_conditional_in_transaction()); + let tenant = create_tenant(); + + let err = backend + .process_transaction( + &tenant, + vec![super::conditional_url_suite::conditional_put( + "Sibling", None, + )], + FhirVersion::default(), + ) + .await + .expect_err("must be refused"); + match err { + helios_persistence::error::TransactionError::BundleError { index, message } => { + assert_eq!(index, 0); + assert!(message.contains("501"), "{message}"); + } + other => panic!("unexpected error: {other:?}"), + } + assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 0); +} + +/// The `ConditionalTransaction` defaults on top of `find_matching`: upsert +/// semantics for `update_conditional`, single-match delete, and the shared +/// outcome enums (#28, #859). +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_conditional_transaction_defaults() { + use helios_persistence::core::{ + ConditionalDeleteResult, ConditionalTransaction, ConditionalUpdateResult, Transaction, + TransactionOptions, TransactionProvider, + }; + + let backend = create_sqlite_backend_with_spec_params(); + let tenant = create_tenant(); + let criteria = super::conditional_url_suite::identifier_criteria(); + let resource = json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": "First"}] + }); + + let mut tx = backend + .begin_transaction(&tenant, TransactionOptions::new()) + .await + .unwrap(); + + let created = tx + .update_conditional("Patient", resource.clone(), &criteria) + .await + .unwrap(); + let ConditionalUpdateResult::Created(created) = created else { + panic!("no match creates: {created:?}"); + }; + + let matches = tx.find_matching("Patient", &criteria).await.unwrap(); + assert_eq!(matches.len(), 1, "the transaction sees its own create"); + assert_eq!(matches[0].id(), created.id()); + + let mut second = resource.clone(); + second["name"][0]["family"] = json!("Second"); + let updated = tx + .update_conditional("Patient", second, &criteria) + .await + .unwrap(); + let ConditionalUpdateResult::Updated(updated) = updated else { + panic!("one match updates: {updated:?}"); + }; + assert_eq!(updated.id(), created.id()); + assert_eq!(updated.content()["name"][0]["family"], "Second"); + + let deleted = tx.delete_conditional("Patient", &criteria).await.unwrap(); + let ConditionalDeleteResult::Deleted(deleted) = deleted else { + panic!("one match deletes: {deleted:?}"); + }; + assert_eq!(deleted.id(), created.id()); + assert!(matches!( + tx.delete_conditional("Patient", &criteria).await.unwrap(), + ConditionalDeleteResult::NoMatch + )); + assert!( + tx.find_matching("Patient", &[]).await.unwrap().is_empty(), + "empty criteria match nothing" + ); + + Box::new(tx).commit().await.unwrap(); + assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 0); +} diff --git a/crates/persistence/tests/transactions/conditional_url_suite.rs b/crates/persistence/tests/transactions/conditional_url_suite.rs new file mode 100644 index 000000000..c52718982 --- /dev/null +++ b/crates/persistence/tests/transactions/conditional_url_suite.rs @@ -0,0 +1,404 @@ +//! Backend-agnostic conformance suite for URL-borne conditional entries in +//! transaction Bundles — `PUT [type]?[criteria]` and `DELETE [type]?[criteria]` +//! (issue #859). +//! +//! Every scenario is generic over [`BundleProvider`], so the *same* assertions +//! run against each backend. The resolution rules are shared through +//! `helios_persistence::core::bundle_conditionals`; what these scenarios pin +//! down is each backend's wiring: that criteria are resolved inside the open +//! transaction before any write, that the outcome is pinned, that several +//! matches and overlapping identities fail the whole bundle and roll back +//! earlier entries, and that a matched entry's `fullUrl` resolves `urn:uuid` +//! references from any position. +//! +//! Criteria arrive typed on `BundleEntry::criteria`, as the REST layer sends +//! them; the scenarios build a plain `identifier` token, the one shape every +//! backend's in-transaction matcher evaluates (MongoDB's refuses the rest, see +//! `mongodb_tests.rs`). +//! +//! Like `if_match_suite.rs`, this file is `#[path]`-included by each backend's +//! test binary rather than living in `tests/common/` (issue #306). The +//! PostgreSQL and MongoDB suites run against one long-lived database, so +//! callers must pass a **distinct tenant per scenario**. + +#![allow(dead_code)] + +use serde_json::json; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{BundleEntry, BundleMethod, BundleProvider}; +use helios_persistence::error::TransactionError; +use helios_persistence::tenant::TenantContext; +use helios_persistence::types::{SearchParamType, SearchParameter, SearchValue, StoredResource}; + +const IDENTIFIER: &str = "http://example.org|12345"; + +/// The typed form of `identifier=http://example.org|12345`. +pub fn identifier_criteria() -> Vec { + vec![SearchParameter { + name: "identifier".to_string(), + param_type: SearchParamType::Token, + values: vec![SearchValue::eq(IDENTIFIER)], + ..Default::default() + }] +} + +fn patient(family: &str) -> serde_json::Value { + json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": family}] + }) +} + +/// `PUT Patient?identifier=…` carrying `family`. +pub fn conditional_put(family: &str, full_url: Option<&str>) -> BundleEntry { + BundleEntry { + method: BundleMethod::Put, + url: format!("Patient?identifier={IDENTIFIER}"), + resource: Some(patient(family)), + full_url: full_url.map(String::from), + criteria: Some(identifier_criteria()), + ..Default::default() + } +} + +/// `DELETE Patient?identifier=…`. +pub fn conditional_delete() -> BundleEntry { + BundleEntry { + method: BundleMethod::Delete, + url: format!("Patient?identifier={IDENTIFIER}"), + criteria: Some(identifier_criteria()), + ..Default::default() + } +} + +fn plain_post(family: &str) -> BundleEntry { + BundleEntry { + method: BundleMethod::Post, + url: "Patient".to_string(), + resource: Some(json!({"resourceType": "Patient", "name": [{"family": family}]})), + ..Default::default() + } +} + +/// Seeds `Patient/{id}` carrying the suite's identifier and `family`. +pub async fn seed_identified_patient( + backend: &B, + tenant: &TenantContext, + id: &str, + family: &str, +) -> StoredResource { + let mut resource = patient(family); + resource["id"] = json!(id); + let (stored, _) = backend + .create_or_update(tenant, "Patient", id, resource, FhirVersion::default()) + .await + .expect("seed patient"); + stored +} + +async fn patient_count(backend: &B, tenant: &TenantContext) -> u64 { + backend.count(tenant, Some("Patient")).await.expect("count") +} + +async fn family_of(backend: &B, tenant: &TenantContext, id: &str) -> String { + backend + .read(tenant, "Patient", id) + .await + .expect("read") + .expect("patient exists") + .content()["name"][0]["family"] + .as_str() + .expect("family") + .to_string() +} + +/// One match: the entry updates it and answers `200` naming the new version. +pub async fn conditional_put_updates_the_single_match( + backend: &B, + tenant: &TenantContext, +) { + let seeded = seed_identified_patient(backend, tenant, "p1", "Original").await; + + let result = backend + .process_transaction( + tenant, + vec![conditional_put("Updated", None)], + FhirVersion::default(), + ) + .await + .expect("transaction"); + + let entry = &result.entries[0]; + assert_eq!(entry.status, 200, "{entry:?}"); + assert_eq!( + entry.location.as_deref(), + Some("Patient/p1/_history/2"), + "the 200 names the updated version so a urn:uuid reference resolves" + ); + assert_eq!( + entry.resource.as_ref().and_then(|r| r["id"].as_str()), + Some("p1") + ); + assert_eq!(family_of(backend, tenant, "p1").await, "Updated"); + assert_eq!( + patient_count(backend, tenant).await, + 1, + "an update creates nothing" + ); + assert_ne!( + entry + .resource + .as_ref() + .and_then(|r| r["meta"]["versionId"].as_str()), + Some(seeded.version_id()), + "a new version was written" + ); +} + +/// No match: the entry creates and answers `201`. +pub async fn conditional_put_creates_when_nothing_matches( + backend: &B, + tenant: &TenantContext, +) { + let result = backend + .process_transaction( + tenant, + vec![conditional_put("New", None)], + FhirVersion::default(), + ) + .await + .expect("transaction"); + + let entry = &result.entries[0]; + assert_eq!(entry.status, 201, "{entry:?}"); + assert!( + entry + .location + .as_deref() + .is_some_and(|l| l.starts_with("Patient/") && l.ends_with("/_history/1")), + "{entry:?}" + ); + assert_eq!(patient_count(backend, tenant).await, 1); +} + +/// Several matches fail the bundle with `412 multiple-matches`, and the plain +/// create that preceded the conditional entry is rolled back. +pub async fn conditional_put_with_several_matches_rolls_back( + backend: &B, + tenant: &TenantContext, +) { + seed_identified_patient(backend, tenant, "p1", "One").await; + seed_identified_patient(backend, tenant, "p2", "Two").await; + + let err = backend + .process_transaction( + tenant, + vec![plain_post("Sibling"), conditional_put("Ambiguous", None)], + FhirVersion::default(), + ) + .await + .expect_err("two matches must fail the bundle"); + + match err { + TransactionError::MultipleMatches { operation, count } => { + assert_eq!(operation, "update"); + assert_eq!(count, 2); + } + other => panic!("unexpected error: {other:?}"), + } + assert_eq!( + patient_count(backend, tenant).await, + 2, + "the plain create in entry 0 must have been rolled back" + ); + assert_eq!(family_of(backend, tenant, "p1").await, "One"); +} + +/// One match: deleted, `204` naming the deleted version. +pub async fn conditional_delete_removes_the_single_match( + backend: &B, + tenant: &TenantContext, +) { + seed_identified_patient(backend, tenant, "p1", "Original").await; + + let result = backend + .process_transaction(tenant, vec![conditional_delete()], FhirVersion::default()) + .await + .expect("transaction"); + + let entry = &result.entries[0]; + assert_eq!(entry.status, 204, "{entry:?}"); + assert_eq!( + entry.location.as_deref(), + Some("Patient/p1/_history/1"), + "the 204 names what it deleted, for the audit trail and secondary sync" + ); + assert!(entry.resource.is_none()); + assert!( + !backend + .exists(tenant, "Patient", "p1") + .await + .expect("exists"), + "the match is gone" + ); + assert_eq!(patient_count(backend, tenant).await, 0); +} + +/// No match is not an error: `204`, nothing written. +pub async fn conditional_delete_with_no_match_is_204( + backend: &B, + tenant: &TenantContext, +) { + let result = backend + .process_transaction(tenant, vec![conditional_delete()], FhirVersion::default()) + .await + .expect("transaction"); + + let entry = &result.entries[0]; + assert_eq!(entry.status, 204, "{entry:?}"); + assert!(entry.location.is_none()); + assert_eq!(patient_count(backend, tenant).await, 0); +} + +/// Several matches fail the bundle and delete nothing; the sibling create +/// rolls back. +pub async fn conditional_delete_with_several_matches_rolls_back( + backend: &B, + tenant: &TenantContext, +) { + seed_identified_patient(backend, tenant, "p1", "One").await; + seed_identified_patient(backend, tenant, "p2", "Two").await; + + let err = backend + .process_transaction( + tenant, + vec![plain_post("Sibling"), conditional_delete()], + FhirVersion::default(), + ) + .await + .expect_err("two matches must fail the bundle"); + + assert!( + matches!( + err, + TransactionError::MultipleMatches { ref operation, count: 2 } if operation == "delete" + ), + "{err:?}" + ); + assert_eq!( + patient_count(backend, tenant).await, + 2, + "nothing deleted, nothing created" + ); +} + +/// R4 §3.1.0.11.2: a conditional entry resolving to a resource another entry +/// addresses by id fails the bundle, and neither write lands. +pub async fn overlap_with_an_instance_entry_fails_the_bundle( + backend: &B, + tenant: &TenantContext, +) { + seed_identified_patient(backend, tenant, "p1", "Original").await; + let mut explicit = patient("ByInstance"); + explicit["id"] = json!("p1"); + + let err = backend + .process_transaction( + tenant, + vec![ + BundleEntry { + method: BundleMethod::Put, + url: "Patient/p1".to_string(), + resource: Some(explicit), + ..Default::default() + }, + conditional_put("ByCriteria", None), + ], + FhirVersion::default(), + ) + .await + .expect_err("overlapping identities must fail the bundle"); + + match err { + TransactionError::BundleError { index, message } => { + assert_eq!(index, 1, "{message}"); + assert!(message.contains("Patient/p1"), "{message}"); + assert!(message.contains("entry 0"), "{message}"); + } + other => panic!("unexpected error: {other:?}"), + } + assert_eq!( + family_of(backend, tenant, "p1").await, + "Original", + "neither write may land" + ); +} + +/// Two conditional entries resolving to the same resource fail the bundle. +pub async fn two_conditional_entries_resolving_to_one_resource_fail( + backend: &B, + tenant: &TenantContext, +) { + seed_identified_patient(backend, tenant, "p1", "Original").await; + + let err = backend + .process_transaction( + tenant, + vec![conditional_delete(), conditional_put("Again", None)], + FhirVersion::default(), + ) + .await + .expect_err("overlapping identities must fail the bundle"); + + assert!( + matches!(err, TransactionError::BundleError { index: 1, .. }), + "{err:?}" + ); + assert_eq!(family_of(backend, tenant, "p1").await, "Original"); +} + +/// A matched conditional `PUT` is resolved before anything executes, so a +/// `urn:uuid` reference to its `fullUrl` resolves even from an earlier entry. +pub async fn matched_conditional_put_resolves_urn_references( + backend: &B, + tenant: &TenantContext, +) { + seed_identified_patient(backend, tenant, "p1", "Original").await; + + let result = backend + .process_transaction( + tenant, + vec![ + BundleEntry { + method: BundleMethod::Post, + url: "Observation".to_string(), + resource: Some(json!({ + "resourceType": "Observation", + "status": "final", + "code": {"text": "test"}, + "subject": {"reference": "urn:uuid:patient"} + })), + full_url: Some("urn:uuid:observation".to_string()), + ..Default::default() + }, + conditional_put("Updated", Some("urn:uuid:patient")), + ], + FhirVersion::default(), + ) + .await + .expect("transaction"); + + assert_eq!(result.entries[0].status, 201, "{:?}", result.entries[0]); + assert_eq!(result.entries[1].status, 200, "{:?}", result.entries[1]); + let observation = result.entries[0] + .resource + .as_ref() + .expect("observation echoed"); + assert_eq!( + observation["subject"]["reference"], + json!("Patient/p1"), + "a urn:uuid reference to the matched entry resolves to the match" + ); +} diff --git a/crates/persistence/tests/transactions/mod.rs b/crates/persistence/tests/transactions/mod.rs index b753958e6..b4a7d2a6b 100644 --- a/crates/persistence/tests/transactions/mod.rs +++ b/crates/persistence/tests/transactions/mod.rs @@ -10,3 +10,7 @@ pub mod rollback_tests; /// Backend-agnostic `ifMatch` scenarios shared with the PostgreSQL suite, which /// `#[path]`-includes this same file (see the module docs). pub mod if_match_suite; + +/// Backend-agnostic `PUT/DELETE [type]?[criteria]` scenarios (#859), shared +/// with the PostgreSQL and MongoDB suites the same way. +pub mod conditional_url_suite; From 3edd5104226aeab37184c8bf9d9cbaef15df1d8e Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 2 Sep 2026 20:34:27 -0400 Subject: [PATCH 4/7] feat(rest): admit and resolve conditional URL entries in transaction bundles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transaction arm declined any non-GET entry whose URL carried a query string with `400 not-supported` before anything executed (#503), while the batch arm resolved the same entries. With the backends now resolving typed criteria inside the open transaction, the guard becomes admission: - `PUT/DELETE [type]?[criteria]` criteria are percent-decoded with repeated keys kept and parsed with the search parser against the tenant's registry, so an unknown parameter, a modifier the type does not define, or a result-shaping `_` parameter (`_count`, `_sort`, `_include`, `_has`, …) is a 400 for the whole bundle rather than a silent "matches nothing" — which on a conditional write is a duplicate (#865) and on a `_`-name matched everything (#866). The typed criteria travel on `BundleEntry.criteria`. - The batch arm's refusals apply: criteria on a POST, an empty query, and `ifMatch` on a conditional entry are 400. A control parameter on an instance URL (`PUT Patient/123?_format=json`) is dropped; the entry addresses the instance either way. - A backend answering `supports_conditional_in_transaction() == false` declines a bundle carrying URL criteria or `ifNoneExist` intact with 501, instead of the 400 the backend's mid-bundle refusal used to surface as. - A conditional entry's audit event names the resource the backend resolved, read from the result's `location`, since its URL has no id and a delete no body. README: the transaction bullet describes the new behavior, the snapshot-resolution rule, the overlap and several-matches outcomes, the 501 gate and MongoDB's criteria-shape limit; the limitation line is removed. Tests: `AuditTarget::from_location` unit test; helios-rest lib 580 pass; the conformance cases land in the next commit. Closes #859. --- crates/rest/README.md | 38 ++++- crates/rest/src/handlers/batch.rs | 240 +++++++++++++++++++++++++----- 2 files changed, 231 insertions(+), 47 deletions(-) diff --git a/crates/rest/README.md b/crates/rest/README.md index acefdc27b..7d664b1a4 100644 --- a/crates/rest/README.md +++ b/crates/rest/README.md @@ -571,11 +571,38 @@ Conditional interactions expressed in the entry URL (`PUT [type]?[criteria]`, compare-and-swap. `ifMatch` on a conditional entry is `400`: it names a version of an instance the server has yet to resolve. Criteria on a `POST` are `400`; a conditional create is expressed through `ifNoneExist`. -- In a `transaction`, any non-`GET` entry whose URL carries a query string still - declines the whole bundle with `400 not-supported` before anything executes. - Resolving URL criteria inside a transaction's atomic scope needs a search - surface on the `Transaction` trait and the R4 §3.1.0.11.2 overlapping-identity - pre-pass, and is tracked by #859. +- In a `transaction`, they are **resolved inside the open transaction** (#859). + The criteria are percent-decoded and parsed with the search parser against the + tenant's search-parameter registry before anything executes, so a modifier the + parameter's type does not define, an unknown parameter, or a result-shaping `_` + parameter (`_count`, `_sort`, `_include`, `_has`, …) is a `400` for the whole + bundle rather than a silent "matches nothing". The typed criteria reach the + backend on `BundleEntry.criteria` and are resolved through the + `ConditionalTransaction` trait, the search surface design discussion #28 + proposed. Every conditional entry is resolved against the transaction's + starting view **before any entry is written**, per the R4 transaction + processing rules, and the outcome is pinned: `PUT` updates the match (`200`, + `location` = the updated version) or creates (`201`); `DELETE` deletes the match + (`204`, `location` = the deleted version, so the AuditEvent and a composite's + secondaries can name it) or is a no-op `204`. Several matches fail the whole + bundle with `412 multiple-matches`. Per R4 §3.1.0.11.2, a resolved identity that + another entry also addresses — an instance-addressed `PUT`/`DELETE`, or another + conditional entry resolving to the same resource — fails the bundle with `400` + naming both entries. Because resolution precedes execution, a conditional entry + does not see a sibling `POST`'s write (unlike `ifNoneExist`, which resolves in + entry order); a conditional `PUT` that matched can be referenced by `urn:uuid` + from any entry, whatever its position. `ifMatch` on a conditional entry and + criteria on a `POST` are `400`, as in a batch. A control parameter on an + instance URL (`PUT Patient/123?_format=json`) is dropped; the entry addresses + the instance either way. +- A backend answers `BundleProvider::supports_conditional_in_transaction`. When it + is `false` — search offloaded to a secondary (composite SQLite/PostgreSQL + + Elasticsearch), whose local index is empty — a transaction carrying URL criteria + or `ifNoneExist` is declined intact with `501` before anything executes, rather + than failing at the entry. MongoDB's session-scoped matcher evaluates criteria in + application memory and understands plain `name=value` only: a criterion with a + modifier, chain, comparison prefix, or comma OR-list fails the bundle with the + same `501` text (#709 owns an index-backed matcher). Note that `/metadata` advertises `conditionalCreate`, `conditionalUpdate` and `conditionalDelete` for every resource type regardless of backend; gating it per @@ -584,7 +611,6 @@ backend is #514. ### Current Limitations The following FHIR transaction features are not yet implemented: -- **Conditional URL criteria in transactions** - `[type]?[criteria]` entries are declined whole in a `transaction` (resolved in a `batch`; #859) - **Conditional reference resolution** - References like `Patient?identifier=12345` are not resolved - **PATCH method** - PATCH operations in bundles return 501 Not Implemented, in both `batch` (per entry) and `transaction` (whole bundle). Send the patch to the instance endpoint instead - **HEAD entries** - refused with 405. `HEAD` is a legal `http-verb` code and is served on the instance-read route, but not inside a Bundle diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index 9b4a884b4..33d3cc39b 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -22,11 +22,12 @@ use helios_persistence::core::{ ResourceStorage, RevincludeProvider, SearchProvider, bundle_if_match_gate, }; use helios_persistence::error::{ResourceError, StorageError, TransactionError}; +use helios_persistence::types::SearchParameter; use serde_json::Value; use tracing::{debug, error, warn}; use crate::error::{RestError, RestResult}; -use crate::extractors::{FhirVersionExtractor, TenantExtractor}; +use crate::extractors::{FhirVersionExtractor, SearchParams, TenantExtractor}; use crate::fhir_types::{admit_resource_type, is_valid_resource_type}; use crate::handlers::extract_patient_from_resource; use crate::middleware::prefer::PreferHeader; @@ -442,46 +443,30 @@ where for (index, entry) in json_entries.iter().enumerate() { match parse_bundle_entry(entry) { - Ok((bundle_entry, full_url)) => { - // Entry URLs reach the backends unparsed, and every backend's - // `parse_url` splits on `/` alone and takes the last two - // segments — sqlite, postgres and mongodb carry byte-equivalent - // copies. A query string therefore lands in storage as part of - // the resource type or the id: `PUT Patient?identifier=http://…` - // commits a row typed `Patient?identifier=http:`, and - // `PUT Patient/123?_format=json` commits one whose id is - // `123?_format=json`. `PUT Patient?name=peter` yields a single - // segment and fails the whole bundle with a message about the - // URL format instead. Decline here, before anything executes, so - // the bundle is declined intact (#503). + Ok((mut bundle_entry, full_url)) => { + // A query string on a non-GET entry is either conditional + // criteria on a type-level URL (`PUT Patient?identifier=…`) or + // a control parameter on an instance URL + // (`PUT Patient/123?_format=json`). Every backend's `parse_url` + // is query-blind and takes the last two path segments, so + // neither may reach storage as written: the first committed a + // row typed `Patient?identifier=http:` before #503 declined + // both up front. Criteria now go to the backend typed, on + // `BundleEntry::criteria`, and are resolved inside the open + // transaction (#859); a control parameter is dropped from the + // URL, because the entry addresses the instance either way. // - // GET is exempt — but not because this path resolves searches. - // It does not: a GET entry still reaches the backend's - // `parse_url`, and a query-bearing one still fails there. The - // exemption keeps this guard off the arm #478 is rewriting, so - // that work lands on an untouched dispatch path instead of - // merging against a refusal it is about to replace. - // - // `ifNoneExist` is left alone: every backend resolves it inside - // the open transaction (#511). Resolving URL-borne criteria - // (`PUT [type]?[criteria]`) within a transaction's atomic scope - // needs a search surface on the `Transaction` trait and the - // R4 §3.1.0.11.2 overlapping-identity pre-pass, and remains a - // follow-up; the batch arm resolves them. - if !matches!(bundle_entry.method, BundleMethod::Get) - && bundle_entry.url.contains('?') - { - return Err(RestError::NotSupported { - feature: format!( - "Transaction entry {} ({} {}) carries a query string. This \ - server cannot resolve one inside a transaction's atomic \ - scope, so no entries were applied. Submit it in a batch \ - Bundle, or address the instance directly.", - index, - bundle_method_to_http_method(&bundle_entry.method), - bundle_entry.url - ), - }); + // GET is exempt: a search entry is partitioned out below and + // runs against the committed state (#478). + if !matches!(bundle_entry.method, BundleMethod::Get) { + match conditional_entry_criteria(state, &tenant, index, &bundle_entry)? { + Some(criteria) => bundle_entry.criteria = Some(criteria), + None => { + if let Some((path, _)) = bundle_entry.url.split_once('?') { + bundle_entry.url = path.to_string(); + } + } + } } // Enforce per-entry scope authorization for transactions. @@ -527,6 +512,32 @@ where } } + // A backend that cannot resolve criteria inside its transaction — its + // search index lives in a secondary backend, so an in-transaction search + // would find nothing and every conditional write would duplicate — says + // so through `supports_conditional_in_transaction`. Decline the bundle + // intact here, at the 501 the backend would otherwise answer from inside + // the transaction after earlier entries had executed (#511, #859). + if !state.storage().supports_conditional_in_transaction() + && let Some((index, entry, _)) = indexed_entries + .iter() + .find(|(_, entry, _)| entry.criteria.is_some() || entry.if_none_exist.is_some()) + { + return Err(RestError::NotImplemented { + feature: format!( + "Transaction entry {} ({} {}) is a conditional interaction, which the \ + configured storage backend ('{}') cannot resolve inside a transaction \ + because its search index is held by a secondary backend, so no entries \ + were applied. Submit it in a batch Bundle, or address the instance \ + directly.", + index, + bundle_method_to_http_method(&entry.method), + entry.url, + state.storage().backend_name() + ), + }); + } + // Admit every mutation before reference resolution, configurable // validation, or storage. A transaction with one invalid write is declined // whole, so none of its otherwise valid siblings can commit or delete. @@ -1290,6 +1301,20 @@ struct AuditTarget { } impl AuditTarget { + /// The entity a `[type]/[id]` or `[type]/[id]/_history/[v]` location + /// names. The patient reference is left to the response body, when the + /// entry has one. + fn from_location(location: &str) -> Option { + let mut segments = location.split('/').filter(|s| !s.is_empty()); + let resource_type = segments.next()?; + let id = segments.next()?; + Some(Self { + resource_type: resource_type.to_string(), + id: id.to_string(), + patient_reference: None, + }) + } + fn from_stored(stored: &helios_persistence::types::StoredResource) -> Self { Self { resource_type: stored.resource_type().to_string(), @@ -1316,6 +1341,122 @@ fn normalize_criteria(raw: &str) -> String { .join("&") } +/// `_`-prefixed names a conditional interaction may carry: the search +/// criteria among FHIR's common parameters. Everything else that starts with +/// `_` shapes a result (`_count`, `_sort`, `_include`, …) or is not evaluated +/// by the backends' conditional query (`_has`, `_list`), and either one would +/// silently widen the match — `_count` alone would match every resource of the +/// type — so it is refused instead. +const CONDITIONAL_COMMON_PARAMS: &[&str] = &[ + "_id", + "_lastUpdated", + "_tag", + "_profile", + "_security", + "_source", + "_language", + "_text", + "_content", +]; + +/// Admits and parses a transaction entry's URL-borne conditional criteria. +/// +/// `Ok(None)` for an entry that carries none: an instance URL (its query, if +/// any, is a control parameter) or a bare type URL. `Ok(Some)` carries the +/// typed criteria a backend resolves inside its transaction (#859). +/// +/// The refusals mirror the batch arm's (#511): criteria on a `POST` (a +/// conditional create is `ifNoneExist`), an empty query (`Patient?` would +/// match everything), and `ifMatch` on a conditional entry (it names a version +/// of an instance the server has yet to resolve). Beyond those, the criteria +/// are parsed with the search parser against the tenant's registry, so a +/// modifier the parameter's type does not define, an unknown parameter, or a +/// result-shaping `_` parameter is a `400` here rather than a silent +/// "matches nothing" — which on a conditional write is a duplicate (#865) and +/// on a `_`-name matched everything (#866). +fn conditional_entry_criteria( + state: &AppState, + tenant: &TenantExtractor, + index: usize, + entry: &BundleEntry, +) -> RestResult>> +where + S: ResourceStorage + SearchProvider + Send + Sync, +{ + let (resource_type, id) = parse_request_url(&entry.url).map_err(|e| RestError::BadRequest { + message: format!("Entry {index}: {e}"), + })?; + if !entry.url.contains('?') || !id.is_empty() || matches!(entry.method, BundleMethod::Patch) { + return Ok(None); + } + let method = bundle_method_to_http_method(&entry.method); + let url = &entry.url; + if matches!(entry.method, BundleMethod::Post) { + return Err(RestError::BadRequest { + message: format!( + "Entry {index}: POST {url} carries criteria, but a conditional create is \ + expressed through request.ifNoneExist, not the URL" + ), + }); + } + let Some(raw) = conditional_criteria(url, &id) else { + return Err(RestError::BadRequest { + message: format!("Entry {index}: {method} {url} carries no usable criteria"), + }); + }; + if entry.if_match.is_some() { + return Err(RestError::BadRequest { + message: format!( + "Entry {index}: ifMatch cannot be combined with a conditional interaction \ + ({method} {url}); address the instance directly" + ), + }); + } + + let pairs = crate::extractors::query_pairs::parse_query_pairs(Some(raw)); + for (name, _) in &pairs { + let base = name.split(':').next().unwrap_or(name); + if base.starts_with('_') && !CONDITIONAL_COMMON_PARAMS.contains(&base) { + return Err(RestError::BadRequest { + message: format!( + "Entry {index}: {method} {url}: '{name}' is not a search criterion a \ + conditional interaction can be resolved by" + ), + }); + } + } + let registry = state.storage().search_param_registry(tenant.context()); + let registry = registry.read(); + let params = SearchParams::from_pairs(pairs.clone()); + let unknown = crate::extractors::search_query_builder::unknown_search_params( + &resource_type, + ¶ms, + ®istry, + ); + if !unknown.is_empty() { + return Err(RestError::BadRequest { + message: format!( + "Entry {index}: {method} {url}: unknown search parameter(s) for \ + {resource_type}: {}", + unknown.join(", ") + ), + }); + } + let query = crate::extractors::build_search_query_from_pairs(&resource_type, &pairs, ®istry) + .map_err(|e| RestError::BadRequest { + message: format!( + "Entry {index}: {method} {url}: invalid criteria: {}", + e.client_response().2 + ), + })?; + if query.parameters.is_empty() { + return Err(RestError::BadRequest { + message: format!("Entry {index}: {method} {url} carries no usable criteria"), + }); + } + Ok(Some(query.parameters)) +} + fn admit_bundle_mutation( method: &BundleMethod, resource_type: &str, @@ -1409,13 +1550,21 @@ fn emit_transaction_entry_audit( ) where S: ResourceStorage + Send + Sync, { + // A conditional entry's URL carries criteria, not an id, and a delete's + // 204 has no body; the backend names the resource it resolved through + // `location` (#859). + let target = entry + .criteria + .as_ref() + .and(result.location.as_deref()) + .and_then(AuditTarget::from_location); emit_entry_audit( state, bundle_method_to_http_method(&entry.method), &entry.url, entry.resource.as_ref(), result, - None, + target.as_ref(), principal, rollback_reason, correlation, @@ -3410,6 +3559,15 @@ mod tests { } } + #[test] + fn audit_target_from_location_names_type_and_id() { + let target = AuditTarget::from_location("Patient/p1/_history/2").expect("target"); + assert_eq!(target.resource_type, "Patient"); + assert_eq!(target.id, "p1"); + assert!(target.patient_reference.is_none()); + assert!(AuditTarget::from_location("Patient").is_none()); + } + #[test] fn conditional_criteria_only_fires_on_a_type_level_url() { assert_eq!( From a3ebb8ff90a46e755d3192ee80e0bafc8f2e879a Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 2 Sep 2026 20:34:27 -0400 Subject: [PATCH 5/7] test(rest): transaction conditional-URL conformance cases Replace the two #503 guard tests (declined intact; GET exempt) with seventeen transaction cases on the in-memory SQLite harness: PUT updates the match, creates on none, 412 with the sibling rolled back; DELETE removes the match naming it, no-match 204, 412 deleting nothing; overlap with an instance entry and two conditional entries resolving to one resource are 400 with nothing written; a `urn:uuid` reference to a matched PUT resolves; percent-encoded criteria; `family:exact` honoured and case-sensitive; `_count`/`_sort`/`_include` refused and `_id=nonexistent` deleting nothing; unknown parameter, `ifMatch` and POST-with-criteria 400; an instance URL's control parameter dropped; and the 501 gate on an offloaded-search backend for URL criteria and `ifNoneExist` alike. `create_test_server` is split so a test can hand in a configured backend. Tests: batch_conformance 89 pass. --- crates/rest/tests/batch_conformance.rs | 483 ++++++++++++++++++++++--- 1 file changed, 432 insertions(+), 51 deletions(-) diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index 35eea1564..8e53186a8 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -27,8 +27,9 @@ const X_TENANT_ID: HeaderName = HeaderName::from_static("x-tenant-id"); const CONTENT_TYPE: HeaderName = HeaderName::from_static("content-type"); const PREFER: HeaderName = HeaderName::from_static("prefer"); -/// Creates a test server with a known base URL. -async fn create_test_server() -> (TestServer, Arc) { +/// An in-memory SQLite backend with the spec search parameters loaded, so +/// conditional criteria on `identifier` and friends resolve. +fn create_test_backend() -> SqliteBackend { let data_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) .parent() .and_then(|p| p.parent()) @@ -42,6 +43,16 @@ async fn create_test_server() -> (TestServer, Arc) { let backend = SqliteBackend::with_config(":memory:", backend_config) .expect("Failed to create SQLite backend"); backend.init_schema().expect("Failed to init schema"); + backend +} + +/// Creates a test server with a known base URL. +async fn create_test_server() -> (TestServer, Arc) { + create_test_server_from(create_test_backend()).await +} + +/// Creates a test server over a caller-configured backend. +async fn create_test_server_from(backend: SqliteBackend) -> (TestServer, Arc) { let backend = Arc::new(backend); let config = ServerConfig { @@ -1684,83 +1695,453 @@ mod conditional_entries { assert_eq!(body["entry"][0]["resource"]["id"], "p1"); } - /// A transaction carrying a query-bearing non-GET entry is declined whole, - /// before anything executes — so the sibling create in the same bundle must - /// not have landed. Backends parse entry URLs query-blind, so letting it - /// through commits the criteria as part of the resource type or the id. + // ── Transactions: resolved inside the atomic scope (#859) ────────────── + + const CRITERIA_URL: &str = "Patient?identifier=http://example.org|12345"; + + fn transaction(entries: Vec) -> Value { + json!({ "resourceType": "Bundle", "type": "transaction", "entry": entries }) + } + + fn put_entry(url: &str, family: &str) -> Value { + json!({ + "request": { "method": "PUT", "url": url }, + "resource": { + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": family}] + } + }) + } + + fn delete_entry(url: &str) -> Value { + json!({ "request": { "method": "DELETE", "url": url } }) + } + + fn sibling_post() -> Value { + json!({ + "request": { "method": "POST", "url": "Patient" }, + "resource": { "resourceType": "Patient", "name": [{"family": "Sibling"}] } + }) + } + + /// The bundle #503 used to decline whole: the conditional entry now + /// updates its match inside the transaction, and the sibling commits. + #[tokio::test] + async fn a_transaction_conditional_put_updates_the_single_match() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + let before = patient_count(&backend).await; + + let body = post_batch( + &server, + transaction(vec![sibling_post(), put_entry(CRITERIA_URL, "Updated")]), + ) + .await; + + assert_eq!(body["type"], "transaction-response"); + assert_eq!( + body["entry"][0]["response"]["status"], "201 Created", + "{body}" + ); + let entry = &body["entry"][1]; + assert_eq!(entry["response"]["status"], "200 OK", "{entry}"); + assert_eq!(entry["response"]["location"], "Patient/p1/_history/2"); + assert_eq!(entry["resource"]["id"], "p1"); + assert_eq!(family_of(&backend, "p1").await, "Updated"); + assert_eq!( + patient_count(&backend).await, + before + 1, + "the sibling committed" + ); + } + #[tokio::test] - async fn a_transaction_with_a_conditional_url_is_declined_intact() { + async fn a_transaction_conditional_put_creates_when_nothing_matches() { let (server, backend) = create_test_server().await; let before = patient_count(&backend).await; + let body = post_batch(&server, transaction(vec![put_entry(CRITERIA_URL, "New")])).await; + + let entry = &body["entry"][0]; + assert_eq!(entry["response"]["status"], "201 Created", "{entry}"); + assert!( + entry["response"]["location"] + .as_str() + .is_some_and(|l| l.starts_with("Patient/") && l.contains("/_history/1")), + "{entry}" + ); + assert_eq!(patient_count(&backend).await, before + 1); + } + + #[tokio::test] + async fn a_transaction_conditional_put_with_several_matches_is_412_and_rolls_back() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "One").await; + seed_patient_with_identifier(&backend, "p2", "Two").await; + let before = patient_count(&backend).await; + let response = post_bundle( &server, - json!({ - "resourceType": "Bundle", - "type": "transaction", - "entry": [ - { - "request": { "method": "POST", "url": "Patient" }, - "resource": { "resourceType": "Patient", "name": [{"family": "Sibling"}] } - }, - { - "request": { - "method": "PUT", - "url": "Patient?identifier=http://example.org|12345" - }, - "resource": { "resourceType": "Patient" } - } - ] - }), + transaction(vec![sibling_post(), put_entry(CRITERIA_URL, "Ambiguous")]), ) .await; - response.assert_status(StatusCode::BAD_REQUEST); + response.assert_status(StatusCode::PRECONDITION_FAILED); let body: Value = response.json(); assert_eq!(body["resourceType"], "OperationOutcome"); - assert_eq!(body["issue"][0]["code"], "not-supported"); + assert_eq!(body["issue"][0]["code"], "multiple-matches", "{body}"); assert_eq!( patient_count(&backend).await, before, - "the bundle must be declined before any entry is applied" + "the sibling rolled back" ); + assert_eq!(family_of(&backend, "p1").await, "One"); } - /// GET entries are left to #478: a transaction search URL is not declined - /// by the query guard, so that work lands on an untouched arm. #[tokio::test] - async fn a_transaction_get_with_a_query_is_not_declined_by_the_query_guard() { + async fn a_transaction_conditional_delete_removes_the_single_match() { let (server, backend) = create_test_server().await; - seed_patient(&backend, "p1", "Nguyen").await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let body = post_batch(&server, transaction(vec![delete_entry(CRITERIA_URL)])).await; + + let entry = &body["entry"][0]; + assert_eq!(entry["response"]["status"], "204 No Content", "{entry}"); + assert_eq!( + entry["response"]["location"], "Patient/p1/_history/1", + "the 204 names what it deleted" + ); + assert!(entry.get("resource").is_none(), "{entry}"); + assert!( + !backend + .exists(&test_tenant(), "Patient", "p1") + .await + .expect("exists"), + "the match is gone" + ); + } + + #[tokio::test] + async fn a_transaction_conditional_delete_with_no_match_is_204() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "other", "Other").await; + let before = patient_count(&backend).await; + + let body = post_batch(&server, transaction(vec![delete_entry(CRITERIA_URL)])).await; + + assert_eq!( + body["entry"][0]["response"]["status"], "204 No Content", + "{body}" + ); + assert_eq!(patient_count(&backend).await, before); + } + + #[tokio::test] + async fn a_transaction_conditional_delete_with_several_matches_is_412_and_deletes_nothing() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "One").await; + seed_patient_with_identifier(&backend, "p2", "Two").await; + let before = patient_count(&backend).await; let response = post_bundle( &server, - json!({ - "resourceType": "Bundle", - "type": "transaction", - "entry": [{ - "request": { "method": "GET", "url": "Patient?name=Nguyen" } - }] - }), + transaction(vec![sibling_post(), delete_entry(CRITERIA_URL)]), + ) + .await; + + response.assert_status(StatusCode::PRECONDITION_FAILED); + let body: Value = response.json(); + assert_eq!(body["issue"][0]["code"], "multiple-matches", "{body}"); + assert_eq!(patient_count(&backend).await, before); + } + + /// R4 §3.1.0.11.2: a resolved identity another entry addresses by id + /// fails the bundle, naming both entries; neither write lands. + #[tokio::test] + async fn a_transaction_conditional_entry_overlapping_an_instance_entry_is_400() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let response = post_bundle( + &server, + transaction(vec![ + json!({ + "request": { "method": "PUT", "url": "Patient/p1" }, + "resource": { + "resourceType": "Patient", + "id": "p1", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": "ByInstance"}] + } + }), + put_entry(CRITERIA_URL, "ByCriteria"), + ]), + ) + .await; + + response.assert_status(StatusCode::BAD_REQUEST); + let body: Value = response.json(); + let text = body["issue"][0]["details"]["text"] + .as_str() + .unwrap_or_default(); + assert!(text.contains("Patient/p1"), "{body}"); + assert!( + text.contains("Patient/p1") && text.contains("resolves to"), + "{body}" + ); + assert_eq!( + family_of(&backend, "p1").await, + "Nguyen", + "neither write may land" + ); + } + + #[tokio::test] + async fn two_transaction_conditional_entries_resolving_to_one_resource_are_400() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let response = post_bundle( + &server, + transaction(vec![ + delete_entry(CRITERIA_URL), + put_entry(CRITERIA_URL, "Again"), + ]), + ) + .await; + + response.assert_status(StatusCode::BAD_REQUEST); + assert_eq!(family_of(&backend, "p1").await, "Nguyen"); + } + + /// A conditional PUT that matched is resolved before anything executes, + /// so a `urn:uuid` reference to it resolves from any entry. + #[tokio::test] + async fn a_urn_uuid_reference_to_a_matched_transaction_conditional_put_resolves() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let mut put = put_entry(CRITERIA_URL, "Updated"); + put["fullUrl"] = json!("urn:uuid:patient"); + let body = post_batch( + &server, + transaction(vec![ + json!({ + "fullUrl": "urn:uuid:observation", + "request": { "method": "POST", "url": "Observation" }, + "resource": { + "resourceType": "Observation", + "status": "final", + "code": {"text": "test"}, + "subject": {"reference": "urn:uuid:patient"} + } + }), + put, + ]), + ) + .await; + + assert_eq!( + body["entry"][0]["response"]["status"], "201 Created", + "{body}" + ); + assert_eq!(body["entry"][1]["response"]["status"], "200 OK", "{body}"); + assert_eq!( + body["entry"][0]["resource"]["subject"]["reference"], "Patient/p1", + "{body}" + ); + } + + #[tokio::test] + async fn percent_encoded_criteria_resolve_in_a_transaction() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let body = post_batch( + &server, + transaction(vec![put_entry( + "Patient?identifier=http%3A%2F%2Fexample.org%7C12345", + "Decoded", + )]), + ) + .await; + + assert_eq!(body["entry"][0]["response"]["status"], "200 OK", "{body}"); + assert_eq!(family_of(&backend, "p1").await, "Decoded"); + } + + /// Criteria are parsed with the search parser, so a modifier reaches the + /// query builder instead of being dropped and matching nothing (#865). + #[tokio::test] + async fn a_modifier_in_transaction_criteria_is_honoured() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + let before = patient_count(&backend).await; + + let body = post_batch( + &server, + transaction(vec![put_entry("Patient?family:exact=Nguyen", "Exact")]), + ) + .await; + assert_eq!(body["entry"][0]["response"]["status"], "200 OK", "{body}"); + assert_eq!(family_of(&backend, "p1").await, "Exact"); + assert_eq!(patient_count(&backend).await, before); + + let body = post_batch( + &server, + transaction(vec![put_entry("Patient?family:exact=exact", "Case")]), + ) + .await; + assert_eq!( + body["entry"][0]["response"]["status"], "201 Created", + ":exact is case-sensitive, so this is no match: {body}" + ); + assert_eq!(patient_count(&backend).await, before + 1); + } + + /// A `_` parameter that shapes results rather than matching is refused: + /// dropped, it would match every resource of the type (#866). + #[tokio::test] + async fn a_result_parameter_in_transaction_criteria_is_400_and_writes_nothing() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + let before = patient_count(&backend).await; + + for url in [ + "Patient?_count=1", + "Patient?_sort=name", + "Patient?_include=Patient:link", + ] { + let response = post_bundle(&server, transaction(vec![delete_entry(url)])).await; + response.assert_status(StatusCode::BAD_REQUEST); + let body: Value = response.json(); + assert!( + body["issue"][0]["details"]["text"] + .as_str() + .is_some_and(|t| t.contains("not a search criterion")), + "{url}: {body}" + ); + } + let response = post_bundle( + &server, + transaction(vec![delete_entry("Patient?_id=nonexistent")]), + ) + .await; + assert!(response.status_code() != StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!(patient_count(&backend).await, before, "nothing was deleted"); + assert_eq!(family_of(&backend, "p1").await, "Nguyen"); + } + + #[tokio::test] + async fn an_unknown_parameter_in_transaction_criteria_is_400() { + let (server, backend) = create_test_server().await; + let before = patient_count(&backend).await; + + let response = post_bundle( + &server, + transaction(vec![put_entry("Patient?ident'ifier=x", "Unknown")]), ) .await; + response.assert_status(StatusCode::BAD_REQUEST); + let body: Value = response.json(); + assert!( + body["issue"][0]["details"]["text"] + .as_str() + .is_some_and(|t| t.contains("unknown search parameter")), + "{body}" + ); + assert_eq!(patient_count(&backend).await, before); + } + + #[tokio::test] + async fn if_match_on_a_transaction_conditional_entry_is_400() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let mut put = put_entry(CRITERIA_URL, "Guarded"); + put["request"]["ifMatch"] = json!("W/\"1\""); + let response = post_bundle(&server, transaction(vec![sibling_post(), put])).await; + + response.assert_status(StatusCode::BAD_REQUEST); + assert_eq!(family_of(&backend, "p1").await, "Nguyen"); + } + + #[tokio::test] + async fn a_transaction_post_with_url_criteria_is_400() { + let (server, backend) = create_test_server().await; + let before = patient_count(&backend).await; + + let mut post = put_entry(CRITERIA_URL, "Posted"); + post["request"]["method"] = json!("POST"); + let response = post_bundle(&server, transaction(vec![post])).await; + + response.assert_status(StatusCode::BAD_REQUEST); let body: Value = response.json(); - // `details.text`, not `diagnostics`: the guard raises - // `RestError::NotSupported`, and `create_operation_outcome` writes only - // `details.text` — `RestError` never renders a `diagnostics` field. As - // written against `diagnostics` the third conjunct was unsatisfiable, - // so `declined_by_the_guard` was permanently false and the negated - // assert below held even if a GET entry *were* declined by the guard, - // which is the one thing this test exists to catch. - let declined_by_the_guard = body["resourceType"] == "OperationOutcome" - && body["issue"][0]["code"] == "not-supported" - && body["issue"][0]["details"]["text"] + assert!( + body["issue"][0]["details"]["text"] .as_str() - .is_some_and(|d| d.contains("carries a query string")); + .is_some_and(|t| t.contains("ifNoneExist")), + "{body}" + ); + assert_eq!(patient_count(&backend).await, before); + } + + /// An instance URL's query is a control parameter, not criteria; it is + /// dropped rather than reaching the backend as part of the id (#503). + #[tokio::test] + async fn an_instance_url_control_parameter_is_dropped_in_a_transaction() { + let (server, backend) = create_test_server().await; + + let body = post_batch( + &server, + transaction(vec![json!({ + "request": { "method": "PUT", "url": "Patient/p9?_format=json" }, + "resource": { "resourceType": "Patient", "id": "p9" } + })]), + ) + .await; + + assert_eq!( + body["entry"][0]["response"]["status"], "201 Created", + "{body}" + ); assert!( - !declined_by_the_guard, - "GET entries must stay on #478's path, not this guard: {body}" + backend + .read(&test_tenant(), "Patient", "p9") + .await + .expect("read") + .is_some(), + "the row is keyed by the id, not by `p9?_format=json`" + ); + } + + /// A backend whose search index lives in a secondary declines the bundle + /// intact with 501, for URL criteria and `ifNoneExist` alike. + #[tokio::test] + async fn a_transaction_conditional_entry_is_501_when_search_is_offloaded() { + let mut offloaded = create_test_backend(); + offloaded.set_search_offloaded(true); + let (server, backend) = create_test_server_from(offloaded).await; + let before = patient_count(&backend).await; + + for bundle in [ + transaction(vec![sibling_post(), put_entry(CRITERIA_URL, "Offloaded")]), + transaction(vec![ + sibling_post(), + if_none_exist_post("identifier=http://example.org|12345", "Offloaded", None), + ]), + ] { + let response = post_bundle(&server, bundle).await; + response.assert_status(StatusCode::NOT_IMPLEMENTED); + let body: Value = response.json(); + assert_eq!(body["issue"][0]["code"], "not-supported", "{body}"); + } + assert_eq!( + patient_count(&backend).await, + before, + "declined before anything executes" ); } } From 98367f1de4ec1696b915956fff0604683f572053 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 2 Sep 2026 21:00:36 -0400 Subject: [PATCH 6/7] docs: point the #859 draft issues at #921, #922, #868 and the #390 comment --- docs/draft-issues-from-859.md | 57 ++++++----------------------------- 1 file changed, 9 insertions(+), 48 deletions(-) diff --git a/docs/draft-issues-from-859.md b/docs/draft-issues-from-859.md index 5b5e99800..cf546fc22 100644 --- a/docs/draft-issues-from-859.md +++ b/docs/draft-issues-from-859.md @@ -1,50 +1,11 @@ -# Draft issues surfaced while implementing #859 — for review before filing +# Draft issues surfaced while implementing #859 — filed 2026-09-02 -Found while resolving `PUT/DELETE [type]?[criteria]` inside transactions (branch -`feat/859-transaction-conditional-url`). None is fixed by that PR unless stated. Line numbers -are current-tree at the time of writing. +Found while resolving `PUT/DELETE [type]?[criteria]` inside transactions (PR #919). Each finding +was validated at runtime before filing; the reproductions live in the issues. -## D1. Transactional `DELETE [type]/[id]` never reaches a composite's secondaries - -`CompositeStorage::sync_bundle_results` (`crates/persistence/src/composite/storage.rs`) syncs -a transaction's entries to secondaries by reading each `BundleEntryResult.resource`. An -instance-addressed `DELETE` answers `204` with no body and no `location`, so it is skipped: -on `sqlite-elasticsearch` a resource deleted inside a transaction stays searchable in -Elasticsearch until the next reindex. #859 fixes the *conditional* delete (its `204` now -carries `location`, and the sync emits `SyncEvent::Delete` for a `204` with one), but the -explicit form still answers a bare `204`. - -Fix: have the three executors set `location` on every delete result (the deleted version's -URL), or have the composite derive the identity from the entry's URL, which it does not see -today. Test: `composite_conformance_sync` — a transaction `DELETE Patient/p1` followed by an -Elasticsearch search that must not return `p1`. - -## D2. The spec fixture's `POST ValueSet/$lookup` entry is not a create - -`crates/fhir/tests/data/json/R4/bundle-transaction.json` entry 7 is `POST ValueSet/$lookup` -with a `Parameters` body. Nothing in the bundle path recognises an operation URL: the REST -layer admits it as a mutation of type `ValueSet`, and the backends' `parse_url` would take -`$lookup` as an id. Today the entry fails on the resource-type mismatch (`Parameters` body under -`ValueSet`), which is at least a refusal; but the fixture as a whole cannot be replayed until -`$op` entries are either executed or declined with a message that names the operation. - -Fix: detect `[type]/$op` and `[type]/[id]/$op` in `parse_bundle_entry`, and either dispatch -to the operation router or return `501` naming the operation. Then the fixture (minus the GET -entries, which #478 covers) becomes an end-to-end test. - -## D3. `test-hfs` skill lacks the backend-specific test commands - -`.claude/skills/test-hfs/SKILL.md` names testcontainers and the ES heap cap but not the -commands the suites actually need: `cargo test -p helios-persistence --features postgres -- -postgres_integration`, `--features mongodb -- mongodb_integration`, the -`HFS_TEST_MONGODB_URL` escape hatch, or that MongoDB transaction tests skip on a standalone -topology. Every backend suite spells these out in its module docs instead. - -## D4. `BundleError` index refers to the sorted entry order - -`process_transaction` sorts entries DELETE → POST → PUT → GET before calling the backend, and -`TransactionError::BundleError { index }` (rendered as "Transaction failed at entry N") is the -index in *that* order, not the client's. The #859 overlap message names both entries by the -same sorted index. Mapping back to the original index is one lookup in the REST layer -(`indexed_entries[index].0`). Pre-existing; noticed because the overlap message makes it -visible. +| Finding | Status | +|---|---| +| D1. Transactional `DELETE [type]/[id]` never reaches composite secondaries | Filed as **#921**. Reproduced on a SQLite/SQLite composite in synchronous mode. Relates to discussion #223's E1 durable sync outbox (which would not close it by itself) and #28's CQRS projection framing. | +| D2. The spec fixture's `POST ValueSet/$lookup` entry is not a create | Already tracked by **#868**. | +| D3. MongoDB transaction tests skip silently on the standalone testcontainer; `test-hfs` skill lacks the backend commands | Added as a [comment on #390](https://github.com/HeliosSoftware/hfs/issues/390#issuecomment-5518697101), the umbrella issue for vacuously passing suites. | +| D4. `BundleError` index refers to the sorted processing order, not the client's Bundle index | Filed as **#922**. Reproduced on the batch_conformance harness. | From 8b200f09db2a9f4d9010c81071ac2f64991b1d8b Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Fri, 4 Sep 2026 12:16:07 -0400 Subject: [PATCH 7/7] test(persistence,rest): cover the conditional-transaction paths CI can reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `codecov/patch` failed the PR at 70.44% of the diff hit against an 82.08% target: 183 of the 619 measured new lines were uncovered, and two files carried 159 of them with no coverage at all. The MongoDB backend's share is structural — the coverage job's Mongo testcontainer is standalone, so every transaction test skips there (#390) — but its criteria flattener is pure, and nothing outside a live session was covered either. - `mongodb`: unit tests for `bundle_criteria_pairs`, the flattener that decides which criteria the session-scoped matcher can evaluate — plain pairs, and the refusal naming the first modifier, chain, prefix, OR-list, composite or valueless criterion. The refusal spelled out the implicit `eq` prefix (`family:exact=eqNguyen`), echoing back a criterion in a form the client never sent; it now prints only a prefix that was written. - `composite`: a recording secondary pins the delete fan-out a conditional delete's `204`+`location` triggers, that a locationless `204` syncs nothing, that a create beside it still fans out, and that a fan-out that cannot be queued is logged rather than failing the committed bundle. - `bundle_conditionals`: a fake `ConditionalTransaction` covers the pre-pass without a backend — targets pinned per entry index, a URL naming no resource type, a failing search, the 501 for a backend that cannot resolve conditionals, the `412`'s per-method operation naming, and the three `ConditionalTransaction` defaults reading their outcome from `find_matching`. - `transactions_suite`: `create_if_none_exist`'s three outcomes on a real SQLite transaction, beside the update and delete defaults already there. - `batch_conformance`: a modifier the parameter's type does not define is a 400 for the whole bundle. The ~109 lines still uncovered are the MongoDB session code that only a replica-set container reaches; making the test harness start one is its own change. Tests: persistence lib 1015, transactions_suite 53, batch_conformance 90. fmt clean; CI's clippy line clean on both crates. --- .../src/backends/mongodb/storage.rs | 149 +++++++++- crates/persistence/src/composite/storage.rs | 251 +++++++++++++++- .../src/core/bundle_conditionals.rs | 278 ++++++++++++++++++ .../tests/transactions/bundle_tests.rs | 36 ++- crates/rest/tests/batch_conformance.rs | 28 ++ 5 files changed, 738 insertions(+), 4 deletions(-) diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index 16aac695d..d29632680 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -386,10 +386,15 @@ fn bundle_criteria_pairs( && param.values.len() == 1 && param.values[0].prefix == SearchPrefix::Eq; if !plain { + // `eq` is the implicit prefix: spelling it out would echo the + // criterion back in a form the client never sent. let values = param .values .iter() - .map(|v| format!("{}{}", v.prefix, v.value)) + .map(|v| match v.prefix { + SearchPrefix::Eq => v.value.clone(), + prefix => format!("{prefix}{}", v.value), + }) .collect::>() .join(","); let name = match ¶m.modifier { @@ -4121,3 +4126,145 @@ fn resolve_bundle_references(value: &mut Value, reference_map: &HashMap {} } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::types::{ + ChainedParameter, CompositeSearchComponent, SearchModifier, SearchParamType, + SearchParameter, SearchPrefix, SearchValue, + }; + + fn plain(name: &str, value: &str) -> SearchParameter { + SearchParameter { + name: name.to_string(), + param_type: SearchParamType::Token, + values: vec![SearchValue::eq(value)], + ..Default::default() + } + } + + /// The shapes the session-scoped matcher does understand: one `eq` value + /// per criterion, flattened in order. + #[test] + fn plain_criteria_flatten_to_name_value_pairs() { + let pairs = bundle_criteria_pairs(&[ + plain("identifier", "http://example.org|12345"), + plain("family", "Nguyen"), + ]) + .expect("plain criteria are evaluable"); + + assert_eq!( + pairs, + vec![ + ( + "identifier".to_string(), + "http://example.org|12345".to_string() + ), + ("family".to_string(), "Nguyen".to_string()), + ] + ); + assert_eq!(bundle_criteria_pairs(&[]).expect("no criteria"), vec![]); + } + + /// A modifier the matcher cannot evaluate names itself in the refusal, so + /// the 501 says which criterion the entry has to lose (#709, #865). + #[test] + fn a_modifier_is_refused_and_named_with_its_modifier() { + let mut param = plain("family", "Nguyen"); + param.modifier = Some(SearchModifier::Exact); + + assert_eq!( + bundle_criteria_pairs(&[param]).expect_err("a modifier is not evaluable"), + "family:exact=Nguyen" + ); + } + + /// A chain resolves through another resource, which the in-memory matcher + /// never loads: refusing beats silently matching nothing. + #[test] + fn a_chain_is_refused() { + let mut param = plain("subject", "Nguyen"); + param.chain = vec![ChainedParameter { + reference_param: "subject".to_string(), + target_type: Some("Patient".to_string()), + target_param: "family".to_string(), + }]; + + assert_eq!( + bundle_criteria_pairs(&[param]).expect_err("a chain is not evaluable"), + "subject=Nguyen" + ); + } + + /// A comparison prefix is kept in the refusal's value, so `gt2020` reads + /// back as it was sent rather than as a bare `2020`. + #[test] + fn a_comparison_prefix_is_refused_and_shown() { + let param = SearchParameter { + name: "birthdate".to_string(), + param_type: SearchParamType::Date, + values: vec![SearchValue::new(SearchPrefix::Gt, "2020-01-01")], + ..Default::default() + }; + + assert_eq!( + bundle_criteria_pairs(&[param]).expect_err("a prefix is not evaluable"), + "birthdate=gt2020-01-01" + ); + } + + /// An OR-list is one criterion with several values; the matcher tests a + /// single value, so the whole list is refused, comma-joined as sent. + #[test] + fn an_or_list_is_refused_whole() { + let mut param = plain("identifier", "12345"); + param.values.push(SearchValue::eq("67890")); + + assert_eq!( + bundle_criteria_pairs(&[param]).expect_err("an OR-list is not evaluable"), + "identifier=12345,67890" + ); + } + + /// A composite parameter carries components rather than a plain value. + #[test] + fn a_composite_is_refused() { + let mut param = plain("component-code-value-quantity", "loinc|8480-6$lt60"); + param.components = vec![CompositeSearchComponent { + param_type: SearchParamType::Token, + param_name: "component-code".to_string(), + }]; + + assert!(bundle_criteria_pairs(&[param]).is_err()); + } + + /// The first criterion the matcher cannot evaluate decides the refusal, + /// even when a later one could have been flattened. + #[test] + fn the_first_unevaluable_criterion_refuses_the_entry() { + let mut param = plain("family", "Nguyen"); + param.modifier = Some(SearchModifier::Contains); + + assert_eq!( + bundle_criteria_pairs(&[param, plain("identifier", "12345")]) + .expect_err("one unevaluable criterion refuses the entry"), + "family:contains=Nguyen" + ); + } + + /// A criterion with no value at all is not a plain `name=value` either. + #[test] + fn a_valueless_criterion_is_refused() { + let param = SearchParameter { + name: "identifier".to_string(), + param_type: SearchParamType::Token, + ..Default::default() + }; + + assert_eq!( + bundle_criteria_pairs(&[param]).expect_err("no value is not evaluable"), + "identifier=" + ); + } +} diff --git a/crates/persistence/src/composite/storage.rs b/crates/persistence/src/composite/storage.rs index c1b8594b2..d4b215b83 100644 --- a/crates/persistence/src/composite/storage.rs +++ b/crates/persistence/src/composite/storage.rs @@ -2526,7 +2526,9 @@ impl GroupExportProvider for CompositeStorage { #[cfg(test)] mod tests { use super::*; - use crate::core::{BackendKind, CapabilityProvider}; + use crate::core::{ + BackendKind, BundleEntryResult, BundleResult, BundleType, CapabilityProvider, + }; use crate::error::{BackendError, StorageError, StorageResult}; use crate::tenant::{TenantContext, TenantId, TenantPermissions}; use crate::types::{ @@ -4183,4 +4185,251 @@ mod tests { )) } } + + // ── sync_bundle_results: a conditional delete's 204 (#859) ───── + + /// Records the writes a secondary is asked to make, so a bundle's + /// fan-out can be asserted without a live search backend. + struct SpySecondary { + calls: Arc>>, + } + + impl SpySecondary { + fn new(calls: Arc>>) -> Arc { + Arc::new(Self { calls }) + } + } + + #[async_trait] + impl ResourceStorage for SpySecondary { + fn backend_name(&self) -> &'static str { + "spy-secondary" + } + + async fn create( + &self, + tenant: &TenantContext, + resource_type: &str, + resource: Value, + fhir_version: FhirVersion, + ) -> StorageResult { + let id = resource + .get("id") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + self.calls + .lock() + .push(format!("create {resource_type}/{id}")); + MockStorage + .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.calls + .lock() + .push(format!("upsert {resource_type}/{id}")); + MockStorage + .create_or_update(tenant, resource_type, id, resource, fhir_version) + .await + } + + async fn read( + &self, + tenant: &TenantContext, + resource_type: &str, + id: &str, + ) -> StorageResult> { + MockStorage.read(tenant, resource_type, id).await + } + + async fn update( + &self, + tenant: &TenantContext, + current: &StoredResource, + resource: Value, + ) -> StorageResult { + MockStorage.update(tenant, current, resource).await + } + + async fn delete( + &self, + _tenant: &TenantContext, + resource_type: &str, + id: &str, + ) -> StorageResult<()> { + self.calls + .lock() + .push(format!("delete {resource_type}/{id}")); + Ok(()) + } + + async fn count( + &self, + _tenant: &TenantContext, + _resource_type: Option<&str>, + ) -> StorageResult { + Ok(0) + } + } + + /// A composite whose secondary is the spy, syncing synchronously so the + /// calls have landed by the time `sync_bundle_results` returns. + fn make_composite_with_spy(spy: Arc) -> CompositeStorage { + let config = CompositeConfig::builder() + .primary("primary", BackendKind::Sqlite) + .search_backend("es", BackendKind::Elasticsearch) + .sync_mode(SyncMode::Synchronous) + .build() + .unwrap(); + let mut backends = HashMap::new(); + backends.insert("primary".to_string(), Arc::new(MockStorage) as DynStorage); + backends.insert("es".to_string(), spy as DynStorage); + CompositeStorage::new(config, backends).unwrap() + } + + /// The same pair, left on the default asynchronous sync mode with no + /// worker started, so every fan-out fails to queue. + fn make_composite_with_spy_async(spy: Arc) -> CompositeStorage { + let config = CompositeConfig::builder() + .primary("primary", BackendKind::Sqlite) + .search_backend("es", BackendKind::Elasticsearch) + .build() + .unwrap(); + let mut backends = HashMap::new(); + backends.insert("primary".to_string(), Arc::new(MockStorage) as DynStorage); + backends.insert("es".to_string(), spy as DynStorage); + CompositeStorage::new(config, backends).unwrap() + } + + fn deleted_entry(location: Option<&str>) -> BundleEntryResult { + BundleEntryResult { + status: 204, + location: location.map(str::to_string), + etag: None, + last_modified: None, + resource: None, + outcome: None, + } + } + + /// A transactional conditional delete answers `204` with no body; the + /// secondaries learn what to drop from `location` alone (#859, #921). + #[tokio::test] + async fn a_bundle_delete_syncs_the_resource_named_by_location() { + let calls = Arc::new(parking_lot::Mutex::new(Vec::new())); + let composite = make_composite_with_spy(SpySecondary::new(calls.clone())); + + composite + .sync_bundle_results( + &make_tenant(), + &BundleResult { + bundle_type: BundleType::Transaction, + entries: vec![deleted_entry(Some("Patient/p1/_history/2"))], + }, + FhirVersion::default(), + ) + .await; + + assert_eq!(*calls.lock(), vec!["delete Patient/p1".to_string()]); + } + + /// A `204` with nothing to name — an unconditional delete, or a delete + /// that matched nothing — leaves the secondaries alone. + #[tokio::test] + async fn a_bundle_delete_without_a_location_syncs_nothing() { + let calls = Arc::new(parking_lot::Mutex::new(Vec::new())); + let composite = make_composite_with_spy(SpySecondary::new(calls.clone())); + + composite + .sync_bundle_results( + &make_tenant(), + &BundleResult { + bundle_type: BundleType::Transaction, + entries: vec![deleted_entry(None), deleted_entry(Some("Patient"))], + }, + FhirVersion::default(), + ) + .await; + + assert!(calls.lock().is_empty(), "{:?}", calls.lock()); + } + + /// The delete arm is additional to the existing body-carrying arm, not a + /// replacement: a create in the same bundle still fans out. + #[tokio::test] + async fn a_bundle_delete_does_not_displace_the_writes_beside_it() { + let calls = Arc::new(parking_lot::Mutex::new(Vec::new())); + let composite = make_composite_with_spy(SpySecondary::new(calls.clone())); + + composite + .sync_bundle_results( + &make_tenant(), + &BundleResult { + bundle_type: BundleType::Transaction, + entries: vec![ + deleted_entry(Some("Patient/p1/_history/2")), + BundleEntryResult { + status: 201, + location: Some("Patient/p2/_history/1".to_string()), + etag: None, + last_modified: None, + resource: Some(json!({"resourceType": "Patient", "id": "p2"})), + outcome: None, + }, + ], + }, + FhirVersion::default(), + ) + .await; + + assert_eq!( + *calls.lock(), + vec![ + "delete Patient/p1".to_string(), + "create Patient/p2".to_string() + ] + ); + } + + /// A fan-out that cannot even be queued — async sync configured, worker + /// never started — is logged, not propagated: the primary has already + /// committed the transaction, so the bundle's own result stands. + #[tokio::test] + async fn a_fan_out_failure_does_not_fail_the_bundle() { + let calls = Arc::new(parking_lot::Mutex::new(Vec::new())); + let composite = make_composite_with_spy_async(SpySecondary::new(calls.clone())); + + composite + .sync_bundle_results( + &make_tenant(), + &BundleResult { + bundle_type: BundleType::Transaction, + entries: vec![deleted_entry(Some("Patient/p1/_history/2"))], + }, + FhirVersion::default(), + ) + .await; + + assert!( + calls.lock().is_empty(), + "the event never reached the secondary: {:?}", + calls.lock() + ); + } + + /// Without a primary that can process bundles at all, the composite + /// cannot promise conditional resolution inside one. + #[test] + fn conditional_in_transaction_follows_the_primary() { + use crate::core::BundleProvider; + assert!(!make_composite_with_secondary().supports_conditional_in_transaction()); + } } diff --git a/crates/persistence/src/core/bundle_conditionals.rs b/crates/persistence/src/core/bundle_conditionals.rs index 137447c73..8572f276c 100644 --- a/crates/persistence/src/core/bundle_conditionals.rs +++ b/crates/persistence/src/core/bundle_conditionals.rs @@ -426,4 +426,282 @@ mod tests { assert_eq!(result.location.as_deref(), Some("Patient/p1/_history/1")); assert!(result.resource.is_none()); } + + /// A transaction that answers `find_matching` from a fixed list, or + /// refuses it, so the pre-pass can be exercised without a backend. + struct FakeTransaction { + matches: Vec, + fail: bool, + tenant: crate::tenant::TenantContext, + } + + impl FakeTransaction { + fn returning(matches: Vec) -> Self { + Self { + matches, + fail: false, + tenant: crate::tenant::TenantContext::new( + TenantId::new("t"), + crate::tenant::TenantPermissions::full_access(), + ), + } + } + + fn failing() -> Self { + Self { + fail: true, + ..Self::returning(Vec::new()) + } + } + } + + #[async_trait::async_trait] + impl super::super::transaction::Transaction for FakeTransaction { + async fn create( + &mut self, + resource_type: &str, + _resource: serde_json::Value, + ) -> crate::error::StorageResult { + Ok(stored(resource_type, "created")) + } + + async fn read( + &mut self, + _resource_type: &str, + _id: &str, + ) -> crate::error::StorageResult> { + Ok(None) + } + + async fn update( + &mut self, + current: &StoredResource, + _resource: serde_json::Value, + ) -> crate::error::StorageResult { + Ok(current.clone()) + } + + async fn delete( + &mut self, + _resource_type: &str, + _id: &str, + ) -> crate::error::StorageResult<()> { + Ok(()) + } + + async fn commit(self: Box) -> crate::error::StorageResult<()> { + Ok(()) + } + + async fn rollback(self: Box) -> crate::error::StorageResult<()> { + Ok(()) + } + + fn tenant(&self) -> &crate::tenant::TenantContext { + &self.tenant + } + + fn is_active(&self) -> bool { + true + } + } + + #[async_trait::async_trait] + impl ConditionalTransaction for FakeTransaction { + async fn find_matching( + &mut self, + _resource_type: &str, + _criteria: &[SearchParameter], + ) -> crate::error::StorageResult> { + if self.fail { + return Err(crate::error::StorageError::Backend( + crate::error::BackendError::Unavailable { + backend_name: "fake".to_string(), + message: "search is down".to_string(), + }, + )); + } + Ok(self.matches.clone()) + } + } + + /// The pre-pass pins one target per conditional entry, keyed by the + /// entry's own index, and leaves the instance entries alone. + #[tokio::test] + async fn the_pre_pass_pins_a_target_per_conditional_entry() { + let entries = vec![ + entry(BundleMethod::Put, "Patient/p9", false), + entry(BundleMethod::Put, "Patient?identifier=x", true), + ]; + let mut tx = FakeTransaction::returning(vec![stored("Patient", "p1")]); + + let targets = resolve_conditional_targets(&mut tx, &entries, None) + .await + .expect("one match resolves"); + + assert_eq!(targets.len(), 1); + let target = targets.get(&1).expect("the conditional entry's index"); + assert_eq!(target.resource_type, "Patient"); + assert_eq!(target.identity().as_deref(), Some("Patient/p1")); + } + + /// A URL with criteria but no type before them cannot be resolved; the + /// entry is named so the client can see which one. + #[tokio::test] + async fn a_url_naming_no_resource_type_fails_the_bundle() { + let entries = vec![entry(BundleMethod::Delete, "?identifier=x", true)]; + let mut tx = FakeTransaction::returning(Vec::new()); + + let err = resolve_conditional_targets(&mut tx, &entries, None) + .await + .expect_err("no resource type"); + + let TransactionError::BundleError { index, message } = err else { + panic!("expected a bundle error"); + }; + assert_eq!(index, 0); + assert!(message.contains("names no resource type"), "{message}"); + } + + /// A search the backend cannot run fails the bundle rather than being + /// read as "nothing matched" — which on a conditional PUT is a duplicate. + #[tokio::test] + async fn a_failing_search_fails_the_bundle() { + let entries = vec![entry(BundleMethod::Put, "Patient?identifier=x", true)]; + let mut tx = FakeTransaction::failing(); + + let err = resolve_conditional_targets(&mut tx, &entries, None) + .await + .expect_err("the search failed"); + + let TransactionError::BundleError { index, message } = err else { + panic!("expected a bundle error"); + }; + assert_eq!(index, 0); + assert!(message.starts_with("Entry processing failed"), "{message}"); + assert!(message.contains("fake"), "{message}"); + } + + /// A backend that cannot resolve conditionals inside a transaction at all + /// refuses the first such entry with the 501 the REST gate mirrors. + #[tokio::test] + async fn an_unsupported_backend_refuses_the_first_conditional_entry() { + let entries = vec![entry(BundleMethod::Put, "Patient?identifier=x", true)]; + let mut tx = FakeTransaction::returning(Vec::new()); + + let err = resolve_conditional_targets(&mut tx, &entries, Some("search is offloaded")) + .await + .expect_err("unsupported"); + + let TransactionError::BundleError { message, .. } = err else { + panic!("expected a bundle error"); + }; + assert!(message.contains("501"), "{message}"); + assert!(message.contains("search is offloaded"), "{message}"); + } + + /// The three defaults are composed from `find_matching` alone, so the + /// outcome follows from the number of matches whatever the backend is. + #[tokio::test] + async fn the_conditional_defaults_read_their_outcome_from_find_matching() { + use super::super::storage::{ + ConditionalCreateResult, ConditionalDeleteResult, ConditionalUpdateResult, + }; + use super::super::transaction::Transaction; + + let criteria = [SearchParameter { + name: "identifier".to_string(), + param_type: SearchParamType::Token, + ..Default::default() + }]; + let resource = serde_json::json!({"resourceType": "Patient"}); + + let mut none = FakeTransaction::returning(Vec::new()); + assert!(matches!( + none.create_if_none_exist("Patient", resource.clone(), &criteria) + .await + .unwrap(), + ConditionalCreateResult::Created(_) + )); + assert!(matches!( + none.update_conditional("Patient", resource.clone(), &criteria) + .await + .unwrap(), + ConditionalUpdateResult::Created(_) + )); + assert!(matches!( + none.delete_conditional("Patient", &criteria).await.unwrap(), + ConditionalDeleteResult::NoMatch + )); + assert!(none.is_active()); + assert_eq!(none.tenant().tenant_id().as_str(), "t"); + + let mut one = FakeTransaction::returning(vec![stored("Patient", "p1")]); + assert!(matches!( + one.create_if_none_exist("Patient", resource.clone(), &criteria) + .await + .unwrap(), + ConditionalCreateResult::Exists(existing) if existing.id() == "p1" + )); + assert!(matches!( + one.update_conditional("Patient", resource.clone(), &criteria) + .await + .unwrap(), + ConditionalUpdateResult::Updated(updated) if updated.id() == "p1" + )); + assert!(matches!( + one.delete_conditional("Patient", &criteria).await.unwrap(), + ConditionalDeleteResult::Deleted(deleted) if deleted.id() == "p1" + )); + + let mut several = + FakeTransaction::returning(vec![stored("Patient", "a"), stored("Patient", "b")]); + assert!(matches!( + several + .create_if_none_exist("Patient", resource.clone(), &criteria) + .await + .unwrap(), + ConditionalCreateResult::MultipleMatches(2) + )); + assert!(matches!( + several + .update_conditional("Patient", resource, &criteria) + .await + .unwrap(), + ConditionalUpdateResult::MultipleMatches(2) + )); + assert!(matches!( + several + .delete_conditional("Patient", &criteria) + .await + .unwrap(), + ConditionalDeleteResult::MultipleMatches(2) + )); + + Box::new(one).commit().await.unwrap(); + Box::new(several).rollback().await.unwrap(); + } + + /// The `412` names the interaction the entry's method describes. + #[test] + fn the_multiple_matches_error_names_each_interaction() { + let matches = || vec![stored("Patient", "a"), stored("Patient", "b")]; + for (method, operation) in [ + (BundleMethod::Put, "update"), + (BundleMethod::Patch, "update"), + (BundleMethod::Delete, "delete"), + (BundleMethod::Post, "create"), + (BundleMethod::Get, "read"), + ] { + let e = entry(method, "Patient?identifier=x", true); + let err = conditional_target(0, &e, "Patient", matches()).expect_err("two matches"); + assert!( + matches!( + &err, + TransactionError::MultipleMatches { operation: op, count: 2 } if op == operation + ), + "{method:?}: {err:?}" + ); + } + } } diff --git a/crates/persistence/tests/transactions/bundle_tests.rs b/crates/persistence/tests/transactions/bundle_tests.rs index 5d5b4c2b9..ebbd6bd42 100644 --- a/crates/persistence/tests/transactions/bundle_tests.rs +++ b/crates/persistence/tests/transactions/bundle_tests.rs @@ -951,8 +951,8 @@ async fn test_bundle_conditional_url_is_refused_when_search_is_offloaded() { #[tokio::test] async fn test_conditional_transaction_defaults() { use helios_persistence::core::{ - ConditionalDeleteResult, ConditionalTransaction, ConditionalUpdateResult, Transaction, - TransactionOptions, TransactionProvider, + ConditionalCreateResult, ConditionalDeleteResult, ConditionalTransaction, + ConditionalUpdateResult, Transaction, TransactionOptions, TransactionProvider, }; let backend = create_sqlite_backend_with_spec_params(); @@ -1007,6 +1007,38 @@ async fn test_conditional_transaction_defaults() { "empty criteria match nothing" ); + // `create_if_none_exist`: the third default, and the one `ifNoneExist` + // rides on. No match creates; the same criteria then answer the existing + // resource rather than creating a second one. + let created = tx + .create_if_none_exist("Patient", resource.clone(), &criteria) + .await + .unwrap(); + let ConditionalCreateResult::Created(created) = created else { + panic!("no match creates: {created:?}"); + }; + let existing = tx + .create_if_none_exist("Patient", resource.clone(), &criteria) + .await + .unwrap(); + let ConditionalCreateResult::Exists(existing) = existing else { + panic!("one match is answered as it stands: {existing:?}"); + }; + assert_eq!(existing.id(), created.id()); + + // A second resource carrying the same identifier makes the criteria + // ambiguous, which is the `412` the bundle arm renders. + let duplicate = tx.create("Patient", resource.clone()).await.unwrap(); + assert!(matches!( + tx.create_if_none_exist("Patient", resource, &criteria) + .await + .unwrap(), + ConditionalCreateResult::MultipleMatches(2) + )); + + tx.delete("Patient", created.id()).await.unwrap(); + tx.delete("Patient", duplicate.id()).await.unwrap(); + Box::new(tx).commit().await.unwrap(); assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 0); } diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index 8e53186a8..b5b6e29ce 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -2055,6 +2055,34 @@ mod conditional_entries { assert_eq!(patient_count(&backend).await, before); } + /// A modifier the parameter's type does not define is refused by the + /// search parser; dropping it would silently widen the criteria (#865). + #[tokio::test] + async fn an_invalid_modifier_in_transaction_criteria_is_400() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + let before = patient_count(&backend).await; + + let response = post_bundle( + &server, + transaction(vec![put_entry( + "Patient?birthdate:contains=1980", + "Invalid", + )]), + ) + .await; + + response.assert_status(StatusCode::BAD_REQUEST); + let body: Value = response.json(); + assert!( + body["issue"][0]["details"]["text"] + .as_str() + .is_some_and(|t| t.contains("invalid criteria")), + "{body}" + ); + assert_eq!(patient_count(&backend).await, before); + } + #[tokio::test] async fn if_match_on_a_transaction_conditional_entry_is_400() { let (server, backend) = create_test_server().await;