diff --git a/crates/persistence/src/backends/mongodb/search_impl.rs b/crates/persistence/src/backends/mongodb/search_impl.rs index 2544c67b3..84498d512 100644 --- a/crates/persistence/src/backends/mongodb/search_impl.rs +++ b/crates/persistence/src/backends/mongodb/search_impl.rs @@ -477,7 +477,7 @@ impl ConditionalStorage for MongoBackend { 1 => { let current = matches.into_iter().next().expect("single match must exist"); self.delete(tenant, resource_type, current.id()).await?; - Ok(ConditionalDeleteResult::Deleted) + Ok(ConditionalDeleteResult::Deleted(current)) } n => Ok(ConditionalDeleteResult::MultipleMatches(n)), } diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index 0ce14a329..c56d1f7e5 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -17,7 +17,8 @@ use crate::core::{ BundleEntry, BundleEntryResult, BundleMethod, BundleProvider, BundleResult, BundleType, HistoryEntry, HistoryMethod, HistoryPage, HistoryParams, InstanceHistoryProvider, PurgableStorage, ResourceStorage, SettingsStore, SystemHistoryProvider, TypeHistoryProvider, - VersionedStorage, bundle_if_match_gate, if_match_field_satisfied, normalize_etag, + VersionedStorage, bundle_if_match_gate, bundle_if_none_exist_gate, if_match_field_satisfied, + normalize_etag, }; use crate::error::{ BackendError, ConcurrencyError, QueryErrorExt, ResourceError, StorageError, StorageResult, @@ -2695,29 +2696,11 @@ impl MongoBackend { ) .await?; - match matches.len() { - 0 => {} - 1 => { - return Ok(BundleEntryResult::ok( - matches.into_iter().next().expect("single match must exist"), - )); - } - n => { - return Ok(BundleEntryResult::error( - 412, - serde_json::json!({ - "resourceType": "OperationOutcome", - "issue": [{ - "severity": "error", - "code": "multiple-matches", - "diagnostics": format!( - "Conditional create matched {} resources", - n - ) - }] - }), - )); - } + // Shared with the SQLite and PostgreSQL executors so all + // three answer 200-with-location / 412 identically, and so + // the matched id reaches the fullUrl reference map. + if let Some(gated) = bundle_if_none_exist_gate(matches) { + return Ok(gated); } } diff --git a/crates/persistence/src/backends/postgres/search_impl.rs b/crates/persistence/src/backends/postgres/search_impl.rs index 3f34b74d2..395efd015 100644 --- a/crates/persistence/src/backends/postgres/search_impl.rs +++ b/crates/persistence/src/backends/postgres/search_impl.rs @@ -195,31 +195,22 @@ fn open_range_needs_empty_guard(query: &SearchQuery) -> bool { }) } -#[async_trait] -impl SearchProvider for PostgresBackend { - async fn search( +impl PostgresBackend { + /// The body of [`SearchProvider::search`], run on a caller-supplied client. + /// + /// `SearchProvider::search` takes a fresh pooled client, which cannot see + /// rows an open transaction has written. A bundle entry that must resolve + /// `ifNoneExist` against what earlier entries in the same transaction wrote + /// runs this on the transaction's own client instead (#511). `total` is + /// computed by the caller so the count query's client is not held across + /// this one's await points. + pub(crate) async fn search_with_client( &self, + client: &deadpool_postgres::Client, tenant: &TenantContext, query: &SearchQuery, + total: Option, ) -> StorageResult { - reject_contained_missing(query)?; - - // `_contained` search uses a dedicated path (different index columns and - // heterogeneous result types); standard search handles `_contained=false`. - if query.contained != crate::types::ContainedMode::Off { - return self.search_contained(tenant, query).await; - } - - // Populate Bundle.total only when the client asked for it - // (`_total=accurate|estimate`). Computed up-front so the count query's - // client is not held across the main query's await points. - let total = if query.wants_total() { - Some(self.search_count(tenant, query).await?) - } else { - None - }; - - let client = self.get_client().await?; let tenant_id = tenant.tenant_id().as_str(); let resource_type = &query.resource_type; @@ -406,7 +397,7 @@ impl SearchProvider for PostgresBackend { .map(|p| p.as_ref() as &(dyn tokio_postgres::types::ToSql + Sync)) .collect(); let rows = if statement_is_reusable(query) { - query_dyn_cached(&client, &sql, ¶m_refs).await + query_dyn_cached(client, &sql, ¶m_refs).await } else { client.query(&sql, ¶m_refs).await } @@ -490,6 +481,35 @@ impl SearchProvider for PostgresBackend { scores: Default::default(), }) } +} + +#[async_trait] +impl SearchProvider for PostgresBackend { + async fn search( + &self, + tenant: &TenantContext, + query: &SearchQuery, + ) -> StorageResult { + reject_contained_missing(query)?; + + // `_contained` search uses a dedicated path (different index columns and + // heterogeneous result types); standard search handles `_contained=false`. + if query.contained != crate::types::ContainedMode::Off { + return self.search_contained(tenant, query).await; + } + + // Populate Bundle.total only when the client asked for it + // (`_total=accurate|estimate`). Computed up-front so the count query's + // client is not held across the main query's await points. + let total = if query.wants_total() { + Some(self.search_count(tenant, query).await?) + } else { + None + }; + + let client = self.get_client().await?; + self.search_with_client(&client, tenant, query, total).await + } async fn search_count( &self, diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index 525117089..f446dfffd 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -2753,7 +2753,7 @@ impl ConditionalStorage for PostgresBackend { // Exactly one match - delete it let existing = matches.into_iter().next().unwrap(); self.delete(tenant, resource_type, existing.id()).await?; - Ok(ConditionalDeleteResult::Deleted) + Ok(ConditionalDeleteResult::Deleted(existing)) } n => { // Multiple matches - error condition @@ -2815,29 +2815,66 @@ impl PostgresBackend { resource_type: &str, search_params_str: &str, ) -> StorageResult> { + let Some(query) = self.conditional_query(tenant, resource_type, search_params_str)? else { + return Ok(Vec::new()); + }; + + // Use the SearchProvider implementation + let result = ::search(self, tenant, &query).await?; + + Ok(result.resources.items) + } + + /// Resolves conditional criteria on the transaction's own 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 puts `ifNoneExist` on every entry therefore + /// forfeits create batching, which is the correct trade. + async fn find_matching_resources_in_tx( + &self, + tenant: &TenantContext, + tx: &mut crate::backends::postgres::transaction::PostgresTransaction, + resource_type: &str, + search_params_str: &str, + ) -> StorageResult> { + 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) + } + + /// Builds the search a conditional interaction's criteria describe, or + /// `None` when the criteria are empty — matching everything would be the + /// literal reading, but no conditional interaction means that. + fn conditional_query( + &self, + tenant: &TenantContext, + resource_type: &str, + search_params_str: &str, + ) -> StorageResult> { // Parse search parameters into (name, value) pairs let parsed_params = parse_simple_search_params(search_params_str); if parsed_params.is_empty() { - // No search params means match all - but for conditional ops this is unusual - return Ok(Vec::new()); + return Ok(None); } // Build SearchParameter objects by looking up types from the registry let search_params = self.build_search_parameters(tenant, resource_type, &parsed_params)?; - // Build a SearchQuery - let query = SearchQuery { + Ok(Some(SearchQuery { resource_type: resource_type.to_string(), parameters: search_params, count: Some(1000), ..Default::default() - }; - - // Use the SearchProvider implementation - let result = ::search(self, tenant, &query).await?; - - Ok(result.resources.items) + })) } /// Builds SearchParameter objects from parsed (name, value) pairs. @@ -3103,7 +3140,7 @@ impl BundleProvider for PostgresBackend { } let creates_before = tx.creates_seen(); - let result = self.process_bundle_entry_tx(&mut tx, entry).await; + let result = self.process_bundle_entry_tx(tenant, &mut tx, entry).await; for _ in creates_before..tx.creates_seen() { create_entry_index.push(idx); } @@ -3202,6 +3239,7 @@ impl PostgresBackend { /// Process a single bundle entry within a transaction. async fn process_bundle_entry_tx( &self, + tenant: &TenantContext, tx: &mut super::transaction::PostgresTransaction, entry: &BundleEntry, ) -> StorageResult { @@ -3240,6 +3278,27 @@ impl PostgresBackend { ) })?; + if let Some(criteria) = entry.if_none_exist.as_deref() { + // 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 this arm would + // create the duplicate `ifNoneExist` exists to prevent. + // 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", + )); + } + 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); + } + } + let created = tx.create(&resource_type, resource).await?; Ok(BundleEntryResult::created(created)) } diff --git a/crates/persistence/src/backends/postgres/transaction.rs b/crates/persistence/src/backends/postgres/transaction.rs index 37d07902f..f4a5c981f 100644 --- a/crates/persistence/src/backends/postgres/transaction.rs +++ b/crates/persistence/src/backends/postgres/transaction.rs @@ -163,7 +163,7 @@ impl PostgresTransaction { }) } - fn client(&self) -> StorageResult<&Client> { + pub(crate) fn client(&self) -> StorageResult<&Client> { self.client .as_ref() .ok_or_else(|| StorageError::Transaction(TransactionError::InvalidTransaction)) diff --git a/crates/persistence/src/backends/sqlite/search_impl.rs b/crates/persistence/src/backends/sqlite/search_impl.rs index 72b0c9bb9..0619dfd7c 100644 --- a/crates/persistence/src/backends/sqlite/search_impl.rs +++ b/crates/persistence/src/backends/sqlite/search_impl.rs @@ -83,31 +83,24 @@ fn bind_cursor_value( Ok(()) } -#[async_trait] -impl SearchProvider for SqliteBackend { - async fn search( +impl SqliteBackend { + /// The body of [`SearchProvider::search`], run on a caller-supplied + /// connection. + /// + /// `SearchProvider::search` takes a fresh pooled connection, which under + /// `BEGIN IMMEDIATE` sees pre-transaction state only. A bundle entry that + /// must resolve `ifNoneExist` against what earlier entries in the same + /// transaction wrote runs this on the transaction's own connection instead + /// (via `SqliteTransaction::with_connection`, #511). `total` is computed by + /// the caller because `search_count` is async and this is not. + #[allow(clippy::type_complexity)] + pub(crate) fn search_with_connection( &self, + conn: &rusqlite::Connection, tenant: &TenantContext, query: &SearchQuery, + total: Option, ) -> StorageResult { - reject_contained_missing(query)?; - - // `_contained` search uses a dedicated path (different index columns and - // heterogeneous result types); standard search handles `_contained=false`. - if query.contained != crate::types::ContainedMode::Off { - return self.search_contained(tenant, query).await; - } - - // Populate Bundle.total only when the client asked for it - // (`_total=accurate|estimate`). Computed up-front, before acquiring the - // (non-Send) connection, so it is not held across this await. - let total = if query.wants_total() { - Some(self.search_count(tenant, query).await?) - } else { - None - }; - - let conn = self.get_connection()?; let tenant_id = tenant.tenant_id().as_str(); let resource_type = &query.resource_type; @@ -352,6 +345,35 @@ impl SearchProvider for SqliteBackend { scores: Default::default(), }) } +} + +#[async_trait] +impl SearchProvider for SqliteBackend { + async fn search( + &self, + tenant: &TenantContext, + query: &SearchQuery, + ) -> StorageResult { + reject_contained_missing(query)?; + + // `_contained` search uses a dedicated path (different index columns and + // heterogeneous result types); standard search handles `_contained=false`. + if query.contained != crate::types::ContainedMode::Off { + return self.search_contained(tenant, query).await; + } + + // Populate Bundle.total only when the client asked for it + // (`_total=accurate|estimate`). Computed up-front, before acquiring the + // (non-Send) connection, so it is not held across this await. + let total = if query.wants_total() { + Some(self.search_count(tenant, query).await?) + } else { + None + }; + + let conn = self.get_connection()?; + self.search_with_connection(&conn, tenant, query, total) + } async fn search_count( &self, diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index 705b3c711..423f7e374 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -2813,7 +2813,7 @@ impl ConditionalStorage for SqliteBackend { // Exactly one match - delete it let existing = matches.into_iter().next().unwrap(); self.delete(tenant, resource_type, existing.id()).await?; - Ok(ConditionalDeleteResult::Deleted) + Ok(ConditionalDeleteResult::Deleted(existing)) } n => { // Multiple matches - error condition @@ -2885,31 +2885,61 @@ impl SqliteBackend { resource_type: &str, search_params_str: &str, ) -> StorageResult> { + let Some(query) = self.conditional_query(tenant, resource_type, search_params_str)? else { + return Ok(Vec::new()); + }; + + // Use the SearchProvider implementation which uses the search index + let result = ::search(self, tenant, &query).await?; + + Ok(result.resources.items) + } + + /// Resolves conditional criteria on the transaction's own connection, so + /// 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( + &self, + tenant: &TenantContext, + tx: &crate::backends::sqlite::transaction::SqliteTransaction, + resource_type: &str, + search_params_str: &str, + ) -> StorageResult> { + 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) + } + + /// Builds the search a conditional interaction's criteria describe, or + /// `None` when the criteria are empty — matching everything would be the + /// literal reading, but no conditional interaction means that. + fn conditional_query( + &self, + tenant: &TenantContext, + resource_type: &str, + search_params_str: &str, + ) -> StorageResult> { // Parse search parameters into (name, value) pairs let parsed_params = parse_simple_search_params(search_params_str); if parsed_params.is_empty() { - // No search params means match all - but for conditional ops this is unusual - // Return empty to avoid unintended matches - return Ok(Vec::new()); + return Ok(None); } // Build SearchParameter objects by looking up types from the registry let search_params = self.build_search_parameters(tenant, resource_type, &parsed_params)?; - // Build a SearchQuery - let query = SearchQuery { + Ok(Some(SearchQuery { resource_type: resource_type.to_string(), parameters: search_params, // No pagination limit for conditional operations - we need all matches count: Some(1000), // Reasonable upper limit for conditional matching ..Default::default() - }; - - // Use the SearchProvider implementation which uses the search index - let result = ::search(self, tenant, &query).await?; - - Ok(result.resources.items) + })) } /// Builds SearchParameter objects from parsed (name, value) pairs. @@ -3205,7 +3235,7 @@ impl BundleProvider for SqliteBackend { resolve_bundle_references(resource, &reference_map); } - let result = self.process_bundle_entry_tx(&mut tx, entry).await; + let result = self.process_bundle_entry_tx(tenant, &mut tx, entry).await; match result { Ok(entry_result) => { @@ -3268,6 +3298,7 @@ impl SqliteBackend { /// Process a single bundle entry within a transaction. async fn process_bundle_entry_tx( &self, + tenant: &TenantContext, tx: &mut crate::backends::sqlite::transaction::SqliteTransaction, entry: &BundleEntry, ) -> StorageResult { @@ -3308,6 +3339,26 @@ impl SqliteBackend { ) })?; + if let Some(criteria) = entry.if_none_exist.as_deref() { + // 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 this arm would + // create the duplicate `ifNoneExist` exists to prevent. + // 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", + )); + } + let matches = + self.find_matching_resources_in_tx(tenant, tx, &resource_type, criteria)?; + if let Some(gated) = crate::core::bundle_if_none_exist_gate(matches) { + return Ok(gated); + } + } + let created = tx.create(&resource_type, resource).await?; Ok(BundleEntryResult::created(created)) } @@ -5994,7 +6045,7 @@ mod tests { .unwrap(); match result { - ConditionalDeleteResult::Deleted => { + ConditionalDeleteResult::Deleted(_) => { // Verify resource is deleted (read returns Gone error or None) let read_result = backend.read(&tenant, "Patient", "p1").await; match read_result { diff --git a/crates/persistence/src/composite/storage.rs b/crates/persistence/src/composite/storage.rs index 0033e6fd4..2144276f0 100644 --- a/crates/persistence/src/composite/storage.rs +++ b/crates/persistence/src/composite/storage.rs @@ -1401,7 +1401,7 @@ impl ConditionalStorage for CompositeStorage { warn!(error = %e, "Failed to sync conditional_delete to secondaries"); } - Ok(ConditionalDeleteResult::Deleted) + Ok(ConditionalDeleteResult::Deleted(current)) } n => Ok(ConditionalDeleteResult::MultipleMatches(n)), }; @@ -1418,8 +1418,19 @@ impl ConditionalStorage for CompositeStorage { .conditional_delete(tenant, resource_type, search_params) .await?; - // Note: We don't have the resource ID for sync here — the primary already - // performed the delete. The sync_manager will handle it if configured. + // The primary resolved the criteria and performed the delete; its + // result names the row it removed, so the secondaries can drop it too. + if let ConditionalDeleteResult::Deleted(deleted) = &result + && let Err(e) = self + .sync_to_secondaries(SyncEvent::Delete { + resource_type: resource_type.to_string(), + resource_id: deleted.id().to_string(), + tenant_id: tenant.tenant_id().clone(), + }) + .await + { + warn!(error = %e, "Failed to sync conditional_delete to secondaries"); + } Ok(result) } diff --git a/crates/persistence/src/core/mod.rs b/crates/persistence/src/core/mod.rs index 938bafc06..d58cf3b9e 100644 --- a/crates/persistence/src/core/mod.rs +++ b/crates/persistence/src/core/mod.rs @@ -149,7 +149,8 @@ pub use history::{ }; pub use preconditions::{ EntityTag, EntityTagPrecondition, MalformedPrecondition, bundle_if_match_gate, - if_match_field_satisfied, precondition_failed_entry, + bundle_if_none_exist_gate, if_match_field_satisfied, multiple_matches_entry, + not_supported_entry, precondition_failed_entry, }; pub use search::{ ChainedSearchProvider, FullSearchProvider, IncludeProvider, MultiTypeSearchProvider, diff --git a/crates/persistence/src/core/preconditions.rs b/crates/persistence/src/core/preconditions.rs index 0dd683509..a32dc79a5 100644 --- a/crates/persistence/src/core/preconditions.rs +++ b/crates/persistence/src/core/preconditions.rs @@ -55,6 +55,7 @@ use std::fmt; use super::transaction::BundleEntryResult; +use crate::types::StoredResource; /// A single parsed entity-tag. /// @@ -461,6 +462,68 @@ pub fn precondition_failed_entry(diagnostics: &str) -> BundleEntryResult { ) } +/// Resolves a bundle entry's `ifNoneExist` matches into the entry result FHIR +/// prescribes for a conditional create, so the backends' transaction executors +/// and the REST batch arm agree by construction (the same discipline as +/// [`bundle_if_match_gate`]). +/// +/// - No match: `None` — proceed with the create. +/// - One match: `Some(200)` carrying the existing resource. Its `location` is +/// set to the match's versioned URL even though nothing was written, because +/// every `process_transaction` loop records a POST entry's `fullUrl` → +/// `Type/id` mapping from `entry_result.location`. Without it a later +/// `urn:uuid:` reference to a matched entry stayed unresolved, which R4 +/// §3.1.0.11.2 forbids: references to a conditionally created entry must +/// resolve to the match. +/// - Several matches: `Some(412 multiple-matches)`. +pub fn bundle_if_none_exist_gate(matches: Vec) -> Option { + match matches.len() { + 0 => None, + 1 => { + let existing = matches.into_iter().next().expect("length checked"); + let location = existing.versioned_url(); + let mut result = BundleEntryResult::ok(existing); + result.location = Some(location); + Some(result) + } + n => Some(multiple_matches_entry("create", n)), + } +} + +/// Builds the `412 multiple-matches` bundle entry result for a conditional +/// `operation` (`create`, `update`, `delete`) whose criteria resolved to +/// `count` resources. +pub fn multiple_matches_entry(operation: &str, count: usize) -> BundleEntryResult { + BundleEntryResult::error( + 412, + serde_json::json!({ + "resourceType": "OperationOutcome", + "issue": [{ + "severity": "error", + "code": "multiple-matches", + "diagnostics": format!("Conditional {operation} matched {count} resources"), + }] + }), + ) +} + +/// Builds the `501 not-supported` bundle entry result a backend records when +/// it cannot honour an entry's conditional semantics inside the transaction +/// rather than silently applying the unconditional interaction. +pub fn not_supported_entry(diagnostics: &str) -> BundleEntryResult { + BundleEntryResult::error( + 501, + serde_json::json!({ + "resourceType": "OperationOutcome", + "issue": [{ + "severity": "error", + "code": "not-supported", + "diagnostics": diagnostics, + }] + }), + ) +} + #[cfg(test)] mod tests { use super::*; @@ -772,4 +835,59 @@ mod tests { assert_eq!(err.reason, "entity-tag contains an unescaped double quote"); assert_eq!(err.raw, r#""a"b""#); } + + // ── ifNoneExist gate ───────────────────────────────────────────────────── + + fn stored(id: &str) -> StoredResource { + StoredResource::new( + "Patient", + id, + crate::tenant::TenantId::new("t"), + serde_json::json!({"resourceType": "Patient", "id": id}), + helios_fhir::FhirVersion::default(), + ) + } + + #[test] + fn if_none_exist_gate_proceeds_when_nothing_matches() { + assert!(bundle_if_none_exist_gate(Vec::new()).is_none()); + } + + #[test] + fn if_none_exist_gate_answers_200_with_a_location_for_the_single_match() { + let result = bundle_if_none_exist_gate(vec![stored("p1")]).expect("gated"); + 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.get("id")) + .and_then(|v| v.as_str()), + Some("p1") + ); + assert!(result.outcome.is_none()); + } + + #[test] + fn if_none_exist_gate_answers_412_for_several_matches() { + let result = bundle_if_none_exist_gate(vec![stored("p1"), stored("p2")]).expect("gated"); + assert_eq!(result.status, 412); + assert!(result.resource.is_none()); + let outcome = result.outcome.expect("outcome"); + assert_eq!(outcome["issue"][0]["code"], "multiple-matches"); + assert_eq!( + outcome["issue"][0]["diagnostics"], + "Conditional create matched 2 resources" + ); + } + + #[test] + fn not_supported_entry_is_a_501_with_the_diagnostics() { + let result = not_supported_entry("why"); + assert_eq!(result.status, 501); + let outcome = result.outcome.expect("outcome"); + assert_eq!(outcome["issue"][0]["code"], "not-supported"); + assert_eq!(outcome["issue"][0]["diagnostics"], "why"); + } } diff --git a/crates/persistence/src/core/storage.rs b/crates/persistence/src/core/storage.rs index c65e347c0..9c68d381b 100644 --- a/crates/persistence/src/core/storage.rs +++ b/crates/persistence/src/core/storage.rs @@ -927,9 +927,13 @@ pub enum ConditionalUpdateResult { /// Result of a conditional delete operation. #[derive(Debug, Clone)] +#[allow(clippy::large_enum_variant)] pub enum ConditionalDeleteResult { - /// Resource was deleted. - Deleted, + /// Resource was deleted. Carries the pre-delete snapshot so callers can + /// name the entity they removed — an audit event, a secondary-index sync, + /// or a bundle entry response all need the id that the criteria resolved + /// to, and nothing else on this path has it. + Deleted(StoredResource), /// No resource matched the condition. NoMatch, /// Multiple resources matched (error condition). @@ -1119,7 +1123,13 @@ mod tests { #[test] fn test_conditional_delete_result_variants() { - let _deleted = ConditionalDeleteResult::Deleted; + let _deleted = ConditionalDeleteResult::Deleted(StoredResource::new( + "Patient", + "p1", + crate::tenant::TenantId::new("t"), + serde_json::json!({"resourceType": "Patient", "id": "p1"}), + FhirVersion::default(), + )); let _no_match = ConditionalDeleteResult::NoMatch; let _multiple = ConditionalDeleteResult::MultipleMatches(5); } diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index 0544fad4d..cea4a1c1c 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -1081,6 +1081,89 @@ async fn mongodb_integration_transaction_bundle_mixed_operations_and_idempotent_ assert_eq!(idempotent_result.entries[0].status, 204); } +#[tokio::test] +async fn mongodb_integration_transaction_if_none_exist_match_resolves_urn_references() { + let Some(backend) = create_backend("if_none_exist_urn").await else { + eprintln!( + "Skipping mongodb_integration_transaction_if_none_exist_match_resolves_urn_references (requires Docker or HFS_TEST_MONGODB_URL)" + ); + return; + }; + + let tenant = create_tenant("tenant-if-none-exist-urn"); + + let existing = backend + .create( + &tenant, + "Patient", + json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org/mrn", "value": "MRN-URN-1"}], + "name": [{"family": "AlreadyThere"}] + }), + FhirVersion::default(), + ) + .await + .unwrap(); + + let entries = vec![ + BundleEntry { + method: BundleMethod::Post, + url: "Patient".to_string(), + resource: Some(json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org/mrn", "value": "MRN-URN-1"}], + "name": [{"family": "Duplicate"}] + })), + if_match: None, + 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()), + }, + BundleEntry { + method: BundleMethod::Post, + url: "Observation".to_string(), + resource: Some(json!({ + "resourceType": "Observation", + "status": "final", + "code": {"text": "test"}, + "subject": {"reference": "urn:uuid:patient"} + })), + if_match: None, + if_none_match: None, + if_none_exist: None, + full_url: Some("urn:uuid:observation".to_string()), + }, + ]; + + let Some(result) = process_transaction_or_skip( + &backend, + &tenant, + entries, + "mongodb_integration_transaction_if_none_exist_match_resolves_urn_references", + ) + .await + else { + return; + }; + + assert_eq!( + result.entries[0].status, 200, + "the match is answered, not duplicated" + ); + assert_eq!(result.entries[1].status, 201); + + let observation = result.entries[1] + .resource + .as_ref() + .expect("created observation is echoed"); + assert_eq!( + observation["subject"]["reference"], + json!(format!("Patient/{}", existing.id())), + "a urn:uuid reference to a matched ifNoneExist entry must resolve to the match" + ); +} + #[tokio::test] async fn mongodb_integration_transaction_bundle_conditional_headers() { let Some(backend) = create_backend("bundle_conditional_headers").await else { @@ -1129,6 +1212,13 @@ async fn mongodb_integration_transaction_bundle_conditional_headers() { return; }; assert_eq!(second_create.entries[0].status, 200); + // A matched `ifNoneExist` names the match in `location`, exactly as a + // fresh create names the row it wrote; that is what the transaction's + // fullUrl → id map is built from (#511). + assert_eq!( + second_create.entries[0].location, first_create.entries[0].location, + "the 200 entry must point at the resource the 201 entry created" + ); backend .create( @@ -2573,7 +2663,7 @@ async fn mongodb_integration_conditional_update_delete_and_no_match() { ) .await .unwrap(); - assert!(matches!(deleted, ConditionalDeleteResult::Deleted)); + assert!(matches!(deleted, ConditionalDeleteResult::Deleted(_))); let no_match = backend .conditional_delete( diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 2e1059bb5..f49daf959 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -4567,6 +4567,186 @@ mod postgres_integration { } } + fn if_none_exist_entry(family: &str, full_url: &str) -> helios_persistence::core::BundleEntry { + use helios_persistence::core::{BundleEntry, BundleMethod}; + BundleEntry { + method: BundleMethod::Post, + url: "Patient".to_string(), + resource: Some(json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org/mrn", "value": "MRN-TX-COND-1"}], + "name": [{"family": family}] + })), + if_match: None, + 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()), + } + } + + /// `ifNoneExist` is resolved inside the transaction (#511): the same bundle + /// twice answers 201 then 200, the 200 names the match, and one row exists. + /// Two entries with the same criteria in one bundle see each other, since + /// buffered creates are flushed before the search. + #[tokio::test] + async fn postgres_integration_transaction_if_none_exist_is_idempotent() { + use helios_persistence::core::BundleProvider; + + let backend = create_backend().await; + let tenant = create_tenant("tx-if-none-exist"); + + let first = backend + .process_transaction( + &tenant, + vec![if_none_exist_entry("First", "urn:uuid:first")], + FhirVersion::default(), + ) + .await + .unwrap(); + assert_eq!(first.entries[0].status, 201); + + let second = backend + .process_transaction( + &tenant, + vec![if_none_exist_entry("Second", "urn:uuid:second")], + FhirVersion::default(), + ) + .await + .unwrap(); + assert_eq!( + second.entries[0].status, 200, + "the match is answered, not duplicated" + ); + assert_eq!(second.entries[0].location, first.entries[0].location); + assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 1); + + let tenant = create_tenant("tx-if-none-exist-same-bundle"); + let both = backend + .process_transaction( + &tenant, + vec![ + if_none_exist_entry("First", "urn:uuid:first"), + if_none_exist_entry("Second", "urn:uuid:second"), + ], + FhirVersion::default(), + ) + .await + .unwrap(); + assert_eq!(both.entries[0].status, 201); + assert_eq!(both.entries[1].status, 200); + assert_eq!(both.entries[1].location, both.entries[0].location); + assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 1); + } + + /// A `urn:uuid` reference to a matched `ifNoneExist` entry resolves to the + /// match (R4 §3.1.0.11.2); several matches fail the entry with 412 and roll + /// the bundle back. + #[tokio::test] + async fn postgres_integration_transaction_if_none_exist_resolves_references_and_rejects_ambiguity() + { + use helios_persistence::core::{BundleEntry, BundleMethod, BundleProvider}; + use helios_persistence::error::TransactionError; + + let backend = create_backend().await; + let tenant = create_tenant("tx-if-none-exist-urn"); + + let existing = backend + .create( + &tenant, + "Patient", + json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org/mrn", "value": "MRN-TX-COND-1"}], + "name": [{"family": "AlreadyThere"}] + }), + FhirVersion::default(), + ) + .await + .unwrap(); + + let result = backend + .process_transaction( + &tenant, + vec![ + if_none_exist_entry("Duplicate", "urn:uuid:patient"), + BundleEntry { + method: BundleMethod::Post, + url: "Observation".to_string(), + resource: Some(json!({ + "resourceType": "Observation", + "status": "final", + "code": {"text": "test"}, + "subject": {"reference": "urn:uuid:patient"} + })), + if_match: None, + if_none_match: None, + if_none_exist: None, + full_url: Some("urn:uuid:observation".to_string()), + }, + ], + FhirVersion::default(), + ) + .await + .unwrap(); + assert_eq!(result.entries[0].status, 200); + assert_eq!(result.entries[1].status, 201); + let observation = result.entries[1].resource.as_ref().expect("created"); + assert_eq!( + observation["subject"]["reference"], + json!(format!("Patient/{}", existing.id())) + ); + + // A second identical patient makes the criteria ambiguous. + backend + .create( + &tenant, + "Patient", + json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org/mrn", "value": "MRN-TX-COND-1"}], + "name": [{"family": "Twin"}] + }), + FhirVersion::default(), + ) + .await + .unwrap(); + let before = backend.count(&tenant, Some("Patient")).await.unwrap(); + + let err = backend + .process_transaction( + &tenant, + vec![ + BundleEntry { + method: BundleMethod::Post, + url: "Patient".to_string(), + resource: Some( + json!({"resourceType": "Patient", "name": [{"family": "Plain"}]}), + ), + if_match: None, + if_none_match: None, + if_none_exist: None, + full_url: None, + }, + if_none_exist_entry("Ambiguous", "urn:uuid:ambiguous"), + ], + FhirVersion::default(), + ) + .await + .expect_err("an ambiguous ifNoneExist must fail the bundle"); + match err { + TransactionError::BundleError { index, message } => { + assert_eq!(index, 1); + assert!(message.contains("412"), "{message}"); + } + other => panic!("unexpected error: {other:?}"), + } + assert_eq!( + backend.count(&tenant, Some("Patient")).await.unwrap(), + before, + "the plain create in entry 0 must have been rolled back" + ); + } + #[tokio::test] async fn postgres_integration_conditional_delete() { use helios_persistence::core::{ @@ -4602,7 +4782,7 @@ mod postgres_integration { .unwrap(); assert!( - matches!(result, ConditionalDeleteResult::Deleted), + matches!(result, ConditionalDeleteResult::Deleted(_)), "Conditional delete should find and delete resource" ); diff --git a/crates/persistence/tests/sqlite_tests.rs b/crates/persistence/tests/sqlite_tests.rs index fe2053122..72ec97847 100644 --- a/crates/persistence/tests/sqlite_tests.rs +++ b/crates/persistence/tests/sqlite_tests.rs @@ -2811,7 +2811,7 @@ async fn test_conditional_delete_with_identifier() { "identifier": [{"system": "http://hospital.org/mrn", "value": "MRN-DELETE-1"}], "name": [{"family": "ToDelete"}] }); - backend + let created = backend .create(&tenant, "Patient", patient, FhirVersion::default()) .await .unwrap(); @@ -2826,9 +2826,13 @@ async fn test_conditional_delete_with_identifier() { .await .unwrap(); - assert!( - matches!(result, ConditionalDeleteResult::Deleted), - "Conditional delete should find and delete resource" + let ConditionalDeleteResult::Deleted(deleted) = result else { + panic!("Conditional delete should find and delete resource, got {result:?}"); + }; + assert_eq!( + deleted.id(), + created.id(), + "the carried snapshot must name the row the criteria resolved to" ); // Verify deletion by searching - should not find diff --git a/crates/persistence/tests/transactions/bundle_tests.rs b/crates/persistence/tests/transactions/bundle_tests.rs index 136847d43..eaec4554c 100644 --- a/crates/persistence/tests/transactions/bundle_tests.rs +++ b/crates/persistence/tests/transactions/bundle_tests.rs @@ -10,7 +10,7 @@ use helios_persistence::core::{BundleEntry, BundleMethod, BundleProvider, Resour use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; #[cfg(feature = "sqlite")] -use helios_persistence::backends::sqlite::SqliteBackend; +use helios_persistence::backends::sqlite::{SqliteBackend, SqliteBackendConfig}; #[cfg(feature = "sqlite")] fn create_sqlite_backend() -> SqliteBackend { @@ -19,6 +19,44 @@ fn create_sqlite_backend() -> SqliteBackend { backend } +/// An in-memory backend that also loads the spec `SearchParameter`s from the +/// workspace `data/` directory. `in_memory()` indexes only the embedded minimal +/// set, which does not include `identifier`, so conditional criteria on it +/// would silently match nothing. +#[cfg(feature = "sqlite")] +fn create_sqlite_backend_with_spec_params() -> SqliteBackend { + let data_dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(|p| p.parent()) + .map(|p| p.join("data")) + .expect("workspace root"); + let config = SqliteBackendConfig { + data_dir: Some(data_dir), + ..Default::default() + }; + let backend = + SqliteBackend::with_config(":memory:", config).expect("Failed to create SQLite backend"); + backend.init_schema().expect("Failed to initialize schema"); + backend +} + +#[cfg(feature = "sqlite")] +fn if_none_exist_entry(family: &str, full_url: &str) -> BundleEntry { + BundleEntry { + method: BundleMethod::Post, + url: "Patient".to_string(), + resource: Some(json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": family}] + })), + if_match: None, + if_none_match: None, + if_none_exist: Some("identifier=http://example.org|12345".to_string()), + full_url: Some(full_url.to_string()), + } +} + fn create_tenant() -> TenantContext { TenantContext::new( TenantId::new("test-tenant"), @@ -348,66 +386,220 @@ async fn test_bundle_internal_references() { /// Test bundle with conditional create (if-none-exist). /// -/// Ported to the current bundle API for structure, but `#[ignore]`d: the -/// transaction bundle path does not implement `if-none-exist` conditional -/// creates — a POST always creates a new resource — so the "should not create -/// a duplicate" assertions do not hold. Preserved for the #306 follow-up. +/// The transaction executor resolves `ifNoneExist` on the transaction's own +/// connection (#511); before that a POST always created, and this test was +/// `#[ignore]`d for the #306 follow-up. #[cfg(feature = "sqlite")] #[tokio::test] -#[ignore = "#306 follow-up: if-none-exist conditional create not implemented in transaction bundle API"] async fn test_bundle_conditional_create() { - let backend = create_sqlite_backend(); + let backend = create_sqlite_backend_with_spec_params(); let tenant = create_tenant(); // First bundle - should create - let bundle1 = vec![BundleEntry { - method: BundleMethod::Post, - url: "Patient".to_string(), - resource: Some(json!({ - "resourceType": "Patient", - "identifier": [{"system": "http://example.org", "value": "12345"}], - "name": [{"family": "Conditional"}] - })), - if_match: None, - if_none_match: None, - if_none_exist: Some("identifier=http://example.org|12345".to_string()), - full_url: Some("urn:uuid:conditional".to_string()), - }]; - let result1 = backend - .process_transaction(&tenant, bundle1, FhirVersion::default()) + .process_transaction( + &tenant, + vec![if_none_exist_entry("Conditional", "urn:uuid:conditional")], + FhirVersion::default(), + ) .await .unwrap(); assert_eq!(result1.entries[0].status, 201); // Second bundle with same condition - should return existing - let bundle2 = vec![BundleEntry { - method: BundleMethod::Post, - url: "Patient".to_string(), - resource: Some(json!({ - "resourceType": "Patient", - "identifier": [{"system": "http://example.org", "value": "12345"}], - "name": [{"family": "ShouldNotCreate"}] - })), - if_match: None, - if_none_match: None, - if_none_exist: Some("identifier=http://example.org|12345".to_string()), - full_url: Some("urn:uuid:conditional".to_string()), - }]; - let result2 = backend - .process_transaction(&tenant, bundle2, FhirVersion::default()) + .process_transaction( + &tenant, + vec![if_none_exist_entry( + "ShouldNotCreate", + "urn:uuid:conditional", + )], + FhirVersion::default(), + ) .await .unwrap(); - // Should not create duplicate - assert_ne!(result2.entries[0].status, 201); + assert_eq!( + result2.entries[0].status, 200, + "the match is answered, not duplicated" + ); + assert_eq!( + result2.entries[0].location, result1.entries[0].location, + "the 200 entry must name the resource the 201 entry created" + ); + let echoed = result2.entries[0].resource.as_ref().expect("match echoed"); + assert_eq!(echoed["name"][0]["family"], "Conditional"); // Only one patient should exist let count = backend.count(&tenant, Some("Patient")).await.unwrap(); assert_eq!(count, 1); } +/// A `urn:uuid` reference to a matched `ifNoneExist` entry resolves to the +/// match (R4 §3.1.0.11.2), which needs the 200 entry's `location`. +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_bundle_if_none_exist_match_resolves_urn_references() { + let backend = create_sqlite_backend_with_spec_params(); + let tenant = create_tenant(); + + let existing = backend + .create( + &tenant, + "Patient", + json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": "AlreadyThere"}] + }), + FhirVersion::default(), + ) + .await + .unwrap(); + + let entries = vec![ + if_none_exist_entry("Duplicate", "urn:uuid:patient"), + BundleEntry { + method: BundleMethod::Post, + url: "Observation".to_string(), + resource: Some(json!({ + "resourceType": "Observation", + "status": "final", + "code": {"text": "test"}, + "subject": {"reference": "urn:uuid:patient"} + })), + if_match: None, + if_none_match: None, + if_none_exist: None, + full_url: Some("urn:uuid:observation".to_string()), + }, + ]; + + let result = backend + .process_transaction(&tenant, entries, FhirVersion::default()) + .await + .unwrap(); + + assert_eq!(result.entries[0].status, 200); + assert_eq!(result.entries[1].status, 201); + let observation = result.entries[1].resource.as_ref().expect("created"); + assert_eq!( + observation["subject"]["reference"], + json!(format!("Patient/{}", existing.id())) + ); + assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 1); +} + +/// Criteria that match several resources fail the entry with 412 and roll the +/// whole bundle back, including entries that already succeeded. +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_bundle_if_none_exist_multiple_matches_rolls_back() { + let backend = create_sqlite_backend_with_spec_params(); + let tenant = create_tenant(); + + for family in ["One", "Two"] { + backend + .create( + &tenant, + "Patient", + json!({ + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": family}] + }), + FhirVersion::default(), + ) + .await + .unwrap(); + } + + let entries = vec![ + BundleEntry { + method: BundleMethod::Post, + url: "Patient".to_string(), + resource: Some(json!({"resourceType": "Patient", "name": [{"family": "Plain"}]})), + if_match: None, + if_none_match: None, + if_none_exist: None, + full_url: None, + }, + if_none_exist_entry("Ambiguous", "urn:uuid:ambiguous"), + ]; + + let err = backend + .process_transaction(&tenant, entries, FhirVersion::default()) + .await + .expect_err("an ambiguous ifNoneExist must fail the bundle"); + match err { + helios_persistence::error::TransactionError::BundleError { index, message } => { + assert_eq!(index, 1); + assert!(message.contains("412"), "{message}"); + } + other => panic!("unexpected error: {other:?}"), + } + + assert_eq!( + backend.count(&tenant, Some("Patient")).await.unwrap(), + 2, + "the plain create in entry 0 must have been rolled back" + ); +} + +/// Two entries with the same criteria in one bundle: the second sees the row +/// the first wrote, because the search runs on the transaction's connection. +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_bundle_if_none_exist_same_criteria_twice_in_one_bundle() { + let backend = create_sqlite_backend_with_spec_params(); + let tenant = create_tenant(); + + let result = backend + .process_transaction( + &tenant, + vec![ + if_none_exist_entry("First", "urn:uuid:first"), + if_none_exist_entry("Second", "urn:uuid:second"), + ], + FhirVersion::default(), + ) + .await + .unwrap(); + + assert_eq!(result.entries[0].status, 201); + assert_eq!(result.entries[1].status, 200); + assert_eq!(result.entries[1].location, result.entries[0].location); + assert_eq!(backend.count(&tenant, Some("Patient")).await.unwrap(), 1); +} + +/// With search offloaded to a secondary backend the local index is empty, so +/// the executor refuses the entry rather than creating the duplicate an +/// always-empty match set would allow. +#[cfg(feature = "sqlite")] +#[tokio::test] +async fn test_bundle_if_none_exist_is_refused_when_search_is_offloaded() { + let mut backend = create_sqlite_backend_with_spec_params(); + backend.set_search_offloaded(true); + let tenant = create_tenant(); + + let err = backend + .process_transaction( + &tenant, + vec![if_none_exist_entry("Offloaded", "urn:uuid:offloaded")], + FhirVersion::default(), + ) + .await + .expect_err("ifNoneExist must be refused, not silently ignored"); + 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); +} + /// Test bundle with conditional update (if-match). #[cfg(feature = "sqlite")] #[tokio::test] diff --git a/crates/rest/README.md b/crates/rest/README.md index af1eabe89..acefdc27b 100644 --- a/crates/rest/README.md +++ b/crates/rest/README.md @@ -545,27 +545,46 @@ and `HEAD` are refused as described under Current Limitations. list RFC 9110 §13.1.1 defines: satisfied when any supplied entity-tag matches. - `ifNoneMatch` — **parsed and ignored.** `parse_bundle_entry` populates `BundleEntry.if_none_match`; no handler or backend reads it. -- `ifNoneExist` — **MongoDB transactions only.** MongoDB resolves it inside the - bundle's session. The batch path never reads it, and SQLite/PostgreSQL ignore it - in a transaction and create a duplicate. Tracked by #511. +- `ifNoneExist` — **supported** on `POST` entries, in both `batch` and + `transaction` bundles, on every backend that implements `ConditionalStorage` + (SQLite, PostgreSQL, MongoDB; S3's implementation is a stub and answers `501` + per entry). The value is passed to storage verbatim, as the `If-None-Exist` + header is. No match creates (`201`); one match answers `200` with the existing + resource and its `location`, so a `urn:uuid` reference to that entry resolves to + the match; several matches answer `412 multiple-matches`. In a transaction the + criteria are resolved inside the open transaction, so two entries with the same + criteria in one bundle yield one resource. When search is offloaded to a + secondary backend (composite SQLite/PostgreSQL + Elasticsearch) the local index + is empty, so a transaction `ifNoneExist` entry is refused with `501` and the + bundle fails at that entry rather than creating a duplicate. Conditional interactions expressed in the entry URL (`PUT [type]?[criteria]`, -`DELETE [type]?[criteria]`) are **not resolved**: - -- In a `batch`, such an entry is refused per-entry with `400`; nothing is written. -- In a `transaction`, any non-`GET` entry whose URL carries a query string - declines the whole bundle with `400 not-supported` before anything executes, - because the backends parse entry URLs query-blind and would otherwise commit - the criteria as part of the resource type or the id. +`DELETE [type]?[criteria]`): + +- In a `batch`, they are **resolved** with the status mapping the resource + endpoints use. `PUT`: one match updates (`200`, `location` = `[type]/[id]`), no + match creates (`201`), several matches `412`. `DELETE`: deleted or no match + `204`, several matches `412` (`/metadata` elects `conditionalDelete: "single"`). + The criteria are percent-decoded like a request URL's query, with repeated + parameters kept. A bundle carrying a conditional entry runs its entries + serially, because the backends resolve criteria as read-then-write rather than + 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. Note that `/metadata` advertises `conditionalCreate`, `conditionalUpdate` and -`conditionalDelete` for every resource type. That is accurate for the resource -endpoints and **not** for bundle entries; reconciling the two is #511. +`conditionalDelete` for every resource type regardless of backend; gating it per +backend is #514. ### Current Limitations The following FHIR transaction features are not yet implemented: -- **Conditional interactions in bundle entries** - `[type]?[criteria]` URLs are refused rather than resolved (#511) +- **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 c4769b5b2..2d89293fd 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -17,8 +17,9 @@ use helios_audit::{AuditAction, AuditCorrelation, AuditEventBuilder}; use helios_auth::{FhirOperation, Principal, SmartScopePolicy}; use helios_fhir::FhirVersion; use helios_persistence::core::{ - BundleEntry, BundleEntryResult, BundleMethod, BundleProvider, IncludeProvider, ResourceStorage, - RevincludeProvider, SearchProvider, bundle_if_match_gate, + BundleEntry, BundleEntryResult, BundleMethod, BundleProvider, ConditionalCreateResult, + ConditionalDeleteResult, ConditionalStorage, ConditionalUpdateResult, IncludeProvider, + ResourceStorage, RevincludeProvider, SearchProvider, bundle_if_match_gate, }; use helios_persistence::error::{ResourceError, StorageError, TransactionError}; use serde_json::Value; @@ -66,6 +67,7 @@ where + IncludeProvider + RevincludeProvider + BundleProvider + + ConditionalStorage + Send + Sync, { @@ -182,19 +184,36 @@ where // load-bearing — but the scan and the write still agree, which is the // invariant this is keyed for. // + // A conditional entry (`PUT/DELETE [type]?[criteria]`, or `POST` with + // `ifNoneExist`) is a read-then-write inside the backend, not a + // compare-and-swap. Two such entries racing in one bundle can both resolve + // their criteria against the same pre-bundle state and both write — two + // `ifNoneExist` creates with the same identifier would yield two + // resources, which is precisely what the client asked the server to + // prevent. Serialize the bundle when any entry is conditional (#511). + // // NOTE: extend this scan in lockstep with any new cross-entry // `state.validation()` mutation added to `process_batch_entry`. - let writes_conformance = entries.iter().any(|entry| { - entry - .get("request") - .and_then(|request| { - let method = parse_entry_method(request).ok()?; - let url = request.get("url").and_then(Value::as_str)?; - parse_bundle_request_url(&method, url).ok() - }) - .is_some_and(|(resource_type, _)| resource_type == "StructureDefinition") + let needs_serial = entries.iter().any(|entry| { + let Some(request) = entry.get("request") else { + return false; + }; + if request.get("ifNoneExist").and_then(Value::as_str).is_some() { + return true; + } + let Ok(method) = parse_entry_method(request) else { + return false; + }; + let Some(url) = request.get("url").and_then(Value::as_str) else { + return false; + }; + let Ok((resource_type, id)) = parse_bundle_request_url(&method, url) else { + return false; + }; + resource_type == "StructureDefinition" + || (!matches!(method, BundleMethod::Get) && conditional_criteria(url, &id).is_some()) }); - if writes_conformance { + if needs_serial { return 1; } @@ -260,7 +279,13 @@ async fn process_batch( principal: Option<&Principal>, ) -> RestResult where - S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, + S: ResourceStorage + + SearchProvider + + IncludeProvider + + RevincludeProvider + + ConditionalStorage + + Send + + Sync, { debug!( tenant = %tenant.tenant_id(), @@ -305,8 +330,17 @@ where let results: Vec<(usize, Value)> = stream::iter(0..entries_ref.len()) .map(|index| async move { let entry = &entries_ref[index]; - let result = - process_batch_entry(state, tenant, fhir_version, entry, index, principal).await; + let mut audit_target = None; + let result = process_batch_entry( + state, + tenant, + fhir_version, + entry, + index, + principal, + &mut audit_target, + ) + .await; // Audit is emitted inside the entry future rather than after // collection. `emit_batch_entry_audit` hands off to a detached @@ -320,6 +354,7 @@ where state, entry, &result, + audit_target.as_ref(), principal, None, Some(&correlation_details), @@ -427,10 +462,12 @@ where // that work lands on an untouched dispatch path instead of // merging against a refusal it is about to replace. // - // `ifNoneExist` is left alone too — MongoDB resolves it inside - // the session, so refusing it here would remove a working, - // atomic feature. Resolving URL criteria within a transaction's - // atomic scope is #511. + // `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('?') { @@ -770,6 +807,11 @@ where } /// Processes a single batch entry, returning a structured BundleEntryResult. +/// +/// `audit_target` is an out-parameter for the one case where neither the +/// request URL nor the response body names the entity the entry acted on: a +/// conditional DELETE answers 204 with no body, and its URL carries criteria +/// rather than an id. `emit_entry_audit` reads it after everything else. async fn process_batch_entry( state: &AppState, tenant: &TenantExtractor, @@ -777,9 +819,16 @@ async fn process_batch_entry( entry: &Value, index: usize, principal: Option<&Principal>, + audit_target: &mut Option, ) -> BundleEntryResult where - S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, + S: ResourceStorage + + SearchProvider + + IncludeProvider + + RevincludeProvider + + ConditionalStorage + + Send + + Sync, { let request = match entry.get("request") { Some(r) => r, @@ -800,6 +849,7 @@ where }; let url = request.get("url").and_then(|v| v.as_str()).unwrap_or(""); let if_match = request.get("ifMatch").and_then(|v| v.as_str()); + let if_none_exist = request.get("ifNoneExist").and_then(|v| v.as_str()); // Parse the URL to extract resource type and ID let (resource_type, id) = match parse_bundle_request_url(&method, url) { @@ -828,33 +878,50 @@ where } } - // A query on a type-level URL is FHIR conditional criteria, and this path - // cannot resolve one — that needs a search, which is #511. Refuse it - // explicitly rather than dispatch something else: before #503 the criteria - // rode along in `resource_type`, so a conditional PUT reached - // `create_or_update` with an empty id and wrote a row no search can address. - // - // GET is exempt. A query there is a search rather than a condition, and - // executing it is #478's deliverable; leaving the arm untouched keeps this - // fix off that diff. - // - // Since #502 the predicate is enum-typed, matching its transaction twin. As - // a raw `method != "GET"` this was the third case-sensitive comparison in - // the file: a lowercase `get` on a search URL failed it and was refused as a - // conditional interaction. Such an entry is now refused at the seam and - // never reaches here. `{method}` below renders the canonical spelling rather - // than echoing raw client bytes. - if !matches!(method, BundleMethod::Get) - && let Some(criteria) = conditional_criteria(url, &id) - { + // A query on a type-level URL is FHIR conditional criteria (#511). It is + // percent-decoded here, once, so the backend receives exactly what the + // resource endpoints hand it: axum's `Query` decodes for them, and no + // backend decodes for itself. Repeated keys survive, which the endpoints' + // `HashMap` round-trip loses (FHIR AND semantics). GET is exempt — a query + // there is a search, executed below. + let criteria = if matches!(method, BundleMethod::Get) { + None + } else { + conditional_criteria(url, &id).map(normalize_criteria) + }; + + if let Some(criteria) = criteria.as_deref() { + // FHIR defines no `POST [type]?[criteria]`; a conditional create is + // expressed through `request.ifNoneExist`. Refuse rather than guess. + if matches!(method, BundleMethod::Post) { + return create_error_result( + 400, + &format!( + "Entry {index}: POST {url} carries criteria, but a conditional \ + create is expressed through request.ifNoneExist, not the URL. \ + Nothing was written." + ), + ); + } + if criteria.is_empty() { + // `Patient?&` decodes to nothing. Empty criteria would match every + // resource of the type on a literal reading; no conditional + // interaction means that. + return create_error_result( + 400, + &format!("Entry {index}: {method} {url} carries no usable criteria"), + ); + } + } + + // `ifMatch` names a version of one instance; a conditional entry names no + // instance until the server resolves it. FHIR gives the pairing no meaning. + if if_match.is_some() && (criteria.is_some() || if_none_exist.is_some()) { return create_error_result( 400, &format!( - "Conditional interactions are not supported in Bundle entries \ - (entry {index}: {method} {url}). Criteria were not applied and \ - nothing was written. Address the instance directly, or perform \ - the conditional interaction against the resource endpoint. \ - Criteria: {criteria}" + "Entry {index}: ifMatch cannot be combined with a conditional \ + interaction ({method} {url}); address the instance directly" ), ); } @@ -921,19 +988,56 @@ where return create_error_result(422, &validation_failure_message(&e)); } + // Conditional create. The criteria are passed verbatim, as the + // resource endpoint passes its `If-None-Exist` header and as the + // transaction executors pass the same field: it is a query string + // by definition, not a URL component to decode. + if let Some(criteria) = if_none_exist { + return match state + .storage() + .conditional_create( + tenant.context(), + &resource_type, + resource, + criteria, + fhir_version, + ) + .await + { + Ok(ConditionalCreateResult::Created(stored)) => { + record_stored_profile(state, tenant, fhir_version, &stored); + BundleEntryResult::created(stored) + } + // The match is answered as the resource endpoint answers + // it (200, no write) — with the match's location, which the + // transaction executors also set through + // `bundle_if_none_exist_gate`. + Ok(ConditionalCreateResult::Exists(stored)) => { + let location = stored.versioned_url(); + let mut result = BundleEntryResult::ok(stored); + result.location = Some(location); + result + } + Ok(ConditionalCreateResult::MultipleMatches(count)) => { + entry_failure(RestError::MultipleMatches { + operation: "create".to_string(), + count, + }) + } + Err(e) => { + let (status, message) = entry_error(e); + create_error_result(status, &message) + } + }; + } + match state .storage() .create(tenant.context(), &resource_type, resource, fhir_version) .await { Ok(stored) => { - if resource_type == "StructureDefinition" { - state.validation().upsert_stored_profile( - tenant.tenant_id(), - fhir_version, - stored.content(), - ); - } + record_stored_profile(state, tenant, fhir_version, &stored); BundleEntryResult::created(stored) } Err(e) => { @@ -957,6 +1061,59 @@ where return entry_failure(error); } + // Conditional update, mirroring `conditional_update_handler`: + // upsert, so no match creates (201) and one match updates (200). + if let Some(criteria) = criteria.as_deref() { + if let Err(e) = state + .validation() + .check_write(tenant.tenant_id(), fhir_version, &resource_type, &resource) + .await + { + return create_error_result(422, &validation_failure_message(&e)); + } + + return match state + .storage() + .conditional_update( + tenant.context(), + &resource_type, + resource, + criteria, + true, + fhir_version, + ) + .await + { + Ok(ConditionalUpdateResult::Updated(stored)) => { + record_stored_profile(state, tenant, fhir_version, &stored); + let location = format!("{}/{}", stored.resource_type(), stored.id()); + let mut result = BundleEntryResult::ok(stored); + result.location = Some(location); + result + } + Ok(ConditionalUpdateResult::Created(stored)) => { + record_stored_profile(state, tenant, fhir_version, &stored); + BundleEntryResult::created(stored) + } + // Unreachable with upsert, kept so the match stays + // exhaustive over the trait's contract. + Ok(ConditionalUpdateResult::NoMatch) => entry_failure(RestError::NotFound { + resource_type: resource_type.clone(), + id: "conditional".to_string(), + }), + Ok(ConditionalUpdateResult::MultipleMatches(count)) => { + entry_failure(RestError::MultipleMatches { + operation: "update".to_string(), + count, + }) + } + Err(e) => { + let (status, message) = entry_error(e); + create_error_result(status, &message) + } + }; + } + // `PUT Patient` names no instance to update. Left to fall through it // reaches `create_or_update` with an empty id, and that writes a row // rather than rejecting: the backend inserts `"id": ""` into the @@ -1000,13 +1157,7 @@ where .await { Ok((stored, created)) => { - if resource_type == "StructureDefinition" { - state.validation().upsert_stored_profile( - tenant.tenant_id(), - fhir_version, - stored.content(), - ); - } + record_stored_profile(state, tenant, fhir_version, &stored); if created { BundleEntryResult::created(stored) } else { @@ -1027,6 +1178,33 @@ where return entry_failure(error); } + // Conditional delete, mirroring `conditional_delete_handler`: no + // match is a success (R4 §3.1.0.7.1), several matches are 412 + // because `/metadata` elects `conditionalDelete: "single"`. + if let Some(criteria) = criteria.as_deref() { + return match state + .storage() + .conditional_delete(tenant.context(), &resource_type, criteria) + .await + { + Ok(ConditionalDeleteResult::Deleted(deleted)) => { + *audit_target = Some(AuditTarget::from_stored(&deleted)); + BundleEntryResult::deleted() + } + Ok(ConditionalDeleteResult::NoMatch) => BundleEntryResult::deleted(), + Ok(ConditionalDeleteResult::MultipleMatches(count)) => { + entry_failure(RestError::MultipleMatches { + operation: "delete".to_string(), + count, + }) + } + Err(e) => { + let (status, message) = entry_error(e); + create_error_result(status, &message) + } + }; + } + // Mirror of the PUT guard above. FHIR defines no unconditional // type-level delete, and an empty id would otherwise target the // empty-id row a pre-#503 conditional PUT could have written. @@ -1082,6 +1260,62 @@ where /// Applies the type and immutability gates shared by batch and transaction /// mutations. The caller decides whether the error belongs to one batch entry /// or rejects the whole transaction. +/// Folds a written StructureDefinition into the tenant profile registry so +/// later entries' `check_write` resolve against it. Every write arm calls this; +/// `batch_concurrency` serializes the bundle when one of them will. +fn record_stored_profile( + state: &AppState, + tenant: &TenantExtractor, + fhir_version: FhirVersion, + stored: &helios_persistence::types::StoredResource, +) where + S: ResourceStorage + Send + Sync, +{ + if stored.resource_type() == "StructureDefinition" { + state.validation().upsert_stored_profile( + tenant.tenant_id(), + fhir_version, + stored.content(), + ); + } +} + +/// The entity a batch entry acted on, when neither its URL nor its response +/// body says: a conditional DELETE's 204 has no body and its URL carries +/// criteria, not an id. +struct AuditTarget { + resource_type: String, + id: String, + patient_reference: Option, +} + +impl AuditTarget { + fn from_stored(stored: &helios_persistence::types::StoredResource) -> Self { + Self { + resource_type: stored.resource_type().to_string(), + id: stored.id().to_string(), + patient_reference: extract_patient_from_resource( + stored.resource_type(), + stored.content(), + ), + } + } +} + +/// Percent-decodes a bundle entry's conditional criteria into the `k=v&k=v` +/// form `ConditionalStorage` takes, keeping repeated keys and their order. +/// +/// A decoded value that itself contains `&` or `=` cannot survive the re-join; +/// the resource endpoints share that limit, since they re-join axum's decoded +/// pairs the same way (`conditional_update_handler`). +fn normalize_criteria(raw: &str) -> String { + crate::extractors::query_pairs::parse_query_pairs(Some(raw)) + .into_iter() + .map(|(key, value)| format!("{key}={value}")) + .collect::>() + .join("&") +} + fn admit_bundle_mutation( method: &BundleMethod, resource_type: &str, @@ -1137,6 +1371,7 @@ fn emit_batch_entry_audit( state: &AppState, entry: &Value, result: &BundleEntryResult, + audit_target: Option<&AuditTarget>, principal: Option<&Principal>, rollback_reason: Option<&str>, correlation: Option<&EntryAuditCorrelation>, @@ -1156,6 +1391,7 @@ fn emit_batch_entry_audit( url, request_resource, result, + audit_target, principal, rollback_reason, correlation, @@ -1179,6 +1415,7 @@ fn emit_transaction_entry_audit( &entry.url, entry.resource.as_ref(), result, + None, principal, rollback_reason, correlation, @@ -1193,6 +1430,7 @@ fn emit_entry_audit( url: &str, request_resource: Option<&Value>, result: &BundleEntryResult, + audit_target: Option<&AuditTarget>, principal: Option<&Principal>, rollback_reason: Option<&str>, correlation: Option<&EntryAuditCorrelation>, @@ -1228,6 +1466,13 @@ fn emit_entry_audit( } } + // An explicit target wins: it exists precisely because the URL and the + // body name nothing (conditional DELETE). + if let Some(target) = audit_target { + resource_type = target.resource_type.clone(); + resource_id = Some(target.id.clone()); + } + let patient_ref = result .resource .as_ref() @@ -1270,7 +1515,9 @@ fn emit_entry_audit( .detail("entry-index", correlation.entry_index.to_string()); } - if let Some(patient_ref) = patient_ref { + if let Some(patient_ref) = + patient_ref.or_else(|| audit_target.and_then(|t| t.patient_reference.clone())) + { builder = builder.patient(patient_ref); } if let Some(principal) = principal { @@ -1327,8 +1574,8 @@ fn extract_outcome_description(outcome: Option<&Value>) -> Option { /// `[type]/?[criteria]` form that `http.html` prints for conditional delete both /// reduce to the type alone instead of producing an empty id. /// -/// The query itself is deliberately not returned. Callers refuse conditional -/// entries via [`conditional_criteria`]; resolving them is #511. +/// The query itself is deliberately not returned. Callers recover conditional +/// criteria via [`conditional_criteria`] and resolve them separately (#511). fn parse_request_url(url: &str) -> Result<(String, String), String> { let path = url.split_once('?').map_or(url, |(path, _)| path); let mut segments = path.split('/').filter(|segment| !segment.is_empty()); @@ -2469,6 +2716,7 @@ mod tests { &result_1, None, None, + None, Some(&correlation_0), ); emit_batch_entry_audit( @@ -2477,6 +2725,7 @@ mod tests { &result_2, None, None, + None, Some(&correlation_1), ); @@ -2532,6 +2781,26 @@ mod tests { reverse_of: Option, in_flight: AtomicUsize, peak_in_flight: AtomicUsize, + /// What every `ConditionalStorage` call answers with (#511). The + /// default panics, so a test that reaches conditional storage without + /// scripting it fails loudly rather than exercising a stub. + conditional_reply: ConditionalReply, + /// Every `ConditionalStorage` call, as `(operation, resource type, + /// criteria exactly as received)`. + conditional_calls: std::sync::Mutex>, + } + + /// The scripted outcome of a conditional call on [`DelayStorage`]. + #[derive(Clone, Copy)] + enum ConditionalReply { + Unscripted, + Created, + Updated, + Exists, + NoMatch, + Deleted, + MultipleMatches(usize), + Unsupported, } impl DelayStorage { @@ -2542,9 +2811,55 @@ mod tests { reverse_of: None, in_flight: AtomicUsize::new(0), peak_in_flight: AtomicUsize::new(0), + conditional_reply: ConditionalReply::Unscripted, + conditional_calls: std::sync::Mutex::new(Vec::new()), + } + } + + fn conditional(reply: ConditionalReply) -> Self { + Self { + conditional_reply: reply, + ..Self::new(8, 0) } } + fn conditional_calls(&self) -> Vec<(&'static str, String, String)> { + self.conditional_calls.lock().unwrap().clone() + } + + fn record_conditional(&self, op: &'static str, resource_type: &str, criteria: &str) { + self.conditional_calls.lock().unwrap().push(( + op, + resource_type.to_string(), + criteria.to_string(), + )); + } + + /// The resource a scripted reply hands back: the one the criteria + /// "matched", under a fixed id so tests can assert locations. + fn existing(tenant: &TenantContext, resource_type: &str) -> StoredResource { + StoredResource::new( + resource_type, + "existing", + tenant.tenant_id().clone(), + serde_json::json!({ + "resourceType": resource_type, + "id": "existing", + "name": [{"family": "Existing"}] + }), + FhirVersion::default(), + ) + } + + fn unsupported(capability: &str) -> helios_persistence::error::StorageError { + helios_persistence::error::StorageError::Backend( + helios_persistence::error::BackendError::UnsupportedCapability { + backend_name: "delay".to_string(), + capability: capability.to_string(), + }, + ) + } + fn reversing(concurrency: usize, delay_ms: u64, total: usize) -> Self { Self { reverse_of: Some(total), @@ -2700,6 +3015,102 @@ mod tests { } } + // #511 widened the bound to `ConditionalStorage`. Each call records what + // reached storage and answers with the scripted reply, so tests pin both + // the criteria the batch arm hands over and the status each outcome maps + // to, without a search index. + #[async_trait] + impl ConditionalStorage for DelayStorage { + async fn conditional_create( + &self, + tenant: &TenantContext, + resource_type: &str, + resource: Value, + search_params: &str, + fhir_version: FhirVersion, + ) -> StorageResult { + self.record_conditional("create", resource_type, search_params); + match self.conditional_reply { + ConditionalReply::Created => { + Ok(ConditionalCreateResult::Created(StoredResource::new( + resource_type, + "created", + tenant.tenant_id().clone(), + resource, + fhir_version, + ))) + } + ConditionalReply::Exists => Ok(ConditionalCreateResult::Exists(Self::existing( + tenant, + resource_type, + ))), + ConditionalReply::MultipleMatches(n) => { + Ok(ConditionalCreateResult::MultipleMatches(n)) + } + ConditionalReply::Unsupported => Err(Self::unsupported("conditional_create")), + _ => panic!("conditional_create is not scripted for this test"), + } + } + + async fn conditional_update( + &self, + tenant: &TenantContext, + resource_type: &str, + resource: Value, + search_params: &str, + upsert: bool, + fhir_version: FhirVersion, + ) -> StorageResult { + assert!( + upsert, + "the batch arm mirrors the resource endpoint: upsert" + ); + self.record_conditional("update", resource_type, search_params); + match self.conditional_reply { + ConditionalReply::Updated => Ok(ConditionalUpdateResult::Updated(Self::existing( + tenant, + resource_type, + ))), + ConditionalReply::Created => { + Ok(ConditionalUpdateResult::Created(StoredResource::new( + resource_type, + "created", + tenant.tenant_id().clone(), + resource, + fhir_version, + ))) + } + ConditionalReply::NoMatch => Ok(ConditionalUpdateResult::NoMatch), + ConditionalReply::MultipleMatches(n) => { + Ok(ConditionalUpdateResult::MultipleMatches(n)) + } + ConditionalReply::Unsupported => Err(Self::unsupported("conditional_update")), + _ => panic!("conditional_update is not scripted for this test"), + } + } + + async fn conditional_delete( + &self, + tenant: &TenantContext, + resource_type: &str, + search_params: &str, + ) -> StorageResult { + self.record_conditional("delete", resource_type, search_params); + match self.conditional_reply { + ConditionalReply::Deleted => Ok(ConditionalDeleteResult::Deleted(Self::existing( + tenant, + resource_type, + ))), + ConditionalReply::NoMatch => Ok(ConditionalDeleteResult::NoMatch), + ConditionalReply::MultipleMatches(n) => { + Ok(ConditionalDeleteResult::MultipleMatches(n)) + } + ConditionalReply::Unsupported => Err(Self::unsupported("conditional_delete")), + _ => panic!("conditional_delete is not scripted for this test"), + } + } + } + /// A batch Bundle of `count` GET entries, targeting `Patient/p0..p{count}`. fn get_bundle(count: usize) -> Value { let entries: Vec = (0..count) @@ -2722,7 +3133,13 @@ mod tests { principal: Option<&Principal>, ) -> Value where - S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, + S: ResourceStorage + + SearchProvider + + IncludeProvider + + RevincludeProvider + + ConditionalStorage + + Send + + Sync, { let tenant = TenantExtractor::new("test-tenant", crate::tenant::TenantSource::Default); let response = process_batch( @@ -2878,11 +3295,8 @@ mod tests { })]; assert_eq!(batch_concurrency(&state, &url_only), 1); - // Since #503 the parse strips the query, so a conditional conformance - // URL is caught here where it silently was not before. Such an entry is - // refused before it writes, which makes this clamp conservative — but - // the scan and the write must not disagree, which is what it is keyed - // for. + // A conditional conformance write is caught twice over: as a + // StructureDefinition write and as a conditional entry (#511). let conditional = [serde_json::json!({ "request": { "method": "PUT", "url": "StructureDefinition?url=http://example.org/sd" }, "resource": { "resourceType": "StructureDefinition" } @@ -3152,11 +3566,13 @@ mod tests { /// A conditional write is refused per-entry and never reaches storage. /// - /// `DelayStorage::create_or_update` and `::delete` are `unimplemented!()`, - /// so this panics rather than merely failing if a refusal is ever moved - /// after dispatch. + /// What is still refused after #511: criteria on a POST (FHIR expresses a + /// conditional create through `ifNoneExist`), and `ifMatch` paired with any + /// conditional interaction. `DelayStorage`'s conditional reply is + /// unscripted, so this panics rather than merely failing if a refusal is + /// ever moved after dispatch. #[tokio::test] - async fn conditional_write_entries_are_refused_before_they_reach_storage() { + async fn conditional_entries_that_fhir_leaves_undefined_are_refused_before_storage() { let state = state_with(DelayStorage::new(8, 0)); let bundle = serde_json::json!({ @@ -3164,12 +3580,35 @@ mod tests { "type": "batch", "entry": [ { - "request": { "method": "PUT", "url": "Patient?identifier=http://example.org|1" }, + "request": { "method": "POST", "url": "Patient?identifier=x" }, "resource": { "resourceType": "Patient" } }, - { "request": { "method": "DELETE", "url": "Patient?identifier=x" } }, { - "request": { "method": "POST", "url": "Patient?identifier=x" }, + "request": { + "method": "PUT", + "url": "Patient?identifier=x", + "ifMatch": "W/\"1\"" + }, + "resource": { "resourceType": "Patient" } + }, + { + "request": { + "method": "DELETE", + "url": "Patient?identifier=x", + "ifMatch": "W/\"1\"" + } + }, + { + "request": { + "method": "POST", + "url": "Patient", + "ifNoneExist": "identifier=x", + "ifMatch": "W/\"1\"" + }, + "resource": { "resourceType": "Patient" } + }, + { + "request": { "method": "PUT", "url": "Patient?&" }, "resource": { "resourceType": "Patient" } }, ] @@ -3177,7 +3616,7 @@ mod tests { let response = run_batch(&state, &bundle, None).await; let entries = response["entry"].as_array().unwrap(); - assert_eq!(entries.len(), 3); + assert_eq!(entries.len(), 5); for (index, entry) in entries.iter().enumerate() { assert_eq!( entry["response"]["status"], "400 Bad Request", @@ -3185,6 +3624,210 @@ mod tests { ); } assert_eq!(state.storage().peak(), 0, "no entry may reach storage"); + assert!(state.storage().conditional_calls().is_empty()); + } + + /// A conditional PUT hands the backend percent-decoded criteria with + /// repeated keys intact, and maps each `ConditionalUpdateResult` the way + /// `conditional_update_handler` maps it (#511). + #[tokio::test] + async fn conditional_put_decodes_criteria_and_maps_update_results() { + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { + "method": "PUT", + "url": "Patient?identifier=http%3A%2F%2Fexample.org%7C123&identifier=x" + }, + "resource": { "resourceType": "Patient" } + }] + }); + + let state = state_with(DelayStorage::conditional(ConditionalReply::Updated)); + let response = run_batch(&state, &bundle, None).await; + assert_eq!( + state.storage().conditional_calls(), + vec![( + "update", + "Patient".to_string(), + "identifier=http://example.org|123&identifier=x".to_string() + )] + ); + let entry = &response["entry"][0]; + assert_eq!(entry["response"]["status"], "200 OK", "{entry}"); + assert_eq!(entry["response"]["location"], "Patient/existing"); + assert_eq!(entry["resource"]["id"], "existing"); + + let state = state_with(DelayStorage::conditional(ConditionalReply::Created)); + let response = run_batch(&state, &bundle, None).await; + let entry = &response["entry"][0]; + assert_eq!(entry["response"]["status"], "201 Created", "{entry}"); + assert_eq!(entry["response"]["location"], "Patient/created/_history/1"); + + let state = state_with(DelayStorage::conditional( + ConditionalReply::MultipleMatches(2), + )); + let response = run_batch(&state, &bundle, None).await; + let entry = &response["entry"][0]; + assert_eq!( + entry["response"]["status"], "412 Precondition Failed", + "{entry}" + ); + assert!(entry["resource"].is_null()); + assert!( + entry["response"]["outcome"] + .to_string() + .contains("matched 2"), + "{entry}" + ); + } + + /// A conditional DELETE answers 204 with no body for both a deletion and + /// no match, and 412 for several matches (#511). + #[tokio::test] + async fn conditional_delete_maps_results() { + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ "request": { "method": "DELETE", "url": "Patient?identifier=x" } }] + }); + + for reply in [ConditionalReply::Deleted, ConditionalReply::NoMatch] { + let state = state_with(DelayStorage::conditional(reply)); + let response = run_batch(&state, &bundle, None).await; + let entry = &response["entry"][0]; + assert_eq!(entry["response"]["status"], "204 No Content", "{entry}"); + assert!( + entry.get("resource").is_none(), + "a 204 carries no body: {entry}" + ); + assert_eq!( + state.storage().conditional_calls(), + vec![("delete", "Patient".to_string(), "identifier=x".to_string())] + ); + } + + let state = state_with(DelayStorage::conditional( + ConditionalReply::MultipleMatches(3), + )); + let response = run_batch(&state, &bundle, None).await; + assert_eq!( + response["entry"][0]["response"]["status"], "412 Precondition Failed", + "{}", + response["entry"][0] + ); + } + + /// `ifNoneExist` reaches storage verbatim — it is a query string by + /// definition, as the resource endpoint's `If-None-Exist` header is — and a + /// match answers 200 with the match's location (#511). + #[tokio::test] + async fn post_with_if_none_exist_is_passed_verbatim_and_maps_create_results() { + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { + "method": "POST", + "url": "Patient", + "ifNoneExist": "identifier=http%3A%2F%2Fexample.org|1" + }, + "resource": { "resourceType": "Patient" } + }] + }); + + let state = state_with(DelayStorage::conditional(ConditionalReply::Exists)); + let response = run_batch(&state, &bundle, None).await; + assert_eq!( + state.storage().conditional_calls(), + vec![( + "create", + "Patient".to_string(), + "identifier=http%3A%2F%2Fexample.org|1".to_string() + )] + ); + let entry = &response["entry"][0]; + assert_eq!(entry["response"]["status"], "200 OK", "{entry}"); + assert_eq!(entry["response"]["location"], "Patient/existing/_history/1"); + + let state = state_with(DelayStorage::conditional(ConditionalReply::Created)); + let response = run_batch(&state, &bundle, None).await; + assert_eq!(response["entry"][0]["response"]["status"], "201 Created"); + + let state = state_with(DelayStorage::conditional( + ConditionalReply::MultipleMatches(2), + )); + let response = run_batch(&state, &bundle, None).await; + assert_eq!( + response["entry"][0]["response"]["status"], + "412 Precondition Failed" + ); + } + + /// A backend whose `ConditionalStorage` is a stub (S3) answers 501 per + /// entry, through the same error funnel every other storage error takes. + #[tokio::test] + async fn unsupported_conditional_storage_is_reported_as_501_per_entry() { + let state = state_with(DelayStorage::conditional(ConditionalReply::Unsupported)); + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "request": { "method": "PUT", "url": "Patient?identifier=x" }, + "resource": { "resourceType": "Patient" } + }, + { "request": { "method": "DELETE", "url": "Patient?identifier=x" } }, + { + "request": { "method": "POST", "url": "Patient", "ifNoneExist": "identifier=x" }, + "resource": { "resourceType": "Patient" } + }, + ] + }); + + let response = run_batch(&state, &bundle, None).await; + for (index, entry) in response["entry"].as_array().unwrap().iter().enumerate() { + assert_eq!( + entry["response"]["status"], "501 Not Implemented", + "entry {index}: {entry}" + ); + } + } + + /// Conditional entries are read-then-write in the backend, so a bundle + /// carrying one runs serially (#511). + #[test] + fn batch_concurrency_is_one_when_an_entry_is_conditional() { + let state = state_with(DelayStorage::new(32, 0)); + + let conditional_put = [serde_json::json!({ + "request": { "method": "PUT", "url": "Patient?identifier=x" }, + "resource": { "resourceType": "Patient" } + })]; + assert_eq!(batch_concurrency(&state, &conditional_put), 1); + + let conditional_delete = [serde_json::json!({ + "request": { "method": "DELETE", "url": "Patient?identifier=x" } + })]; + assert_eq!(batch_concurrency(&state, &conditional_delete), 1); + + let if_none_exist = [serde_json::json!({ + "request": { "method": "POST", "url": "Patient", "ifNoneExist": "identifier=x" }, + "resource": { "resourceType": "Patient" } + })]; + assert_eq!(batch_concurrency(&state, &if_none_exist), 1); + + // A GET with a query is a search, not a condition; an instance URL + // with a control parameter is not conditional either. + let not_conditional = [ + serde_json::json!({ "request": { "method": "GET", "url": "Patient?name=x" } }), + serde_json::json!({ "request": { "method": "GET", "url": "Patient/p1?_format=json" } }), + ]; + assert_eq!( + batch_concurrency(&state, ¬_conditional), + batch_concurrency(&state, &[]) + ); } /// A type-level URL with no criteria names no instance. Left to fall diff --git a/crates/rest/src/handlers/delete.rs b/crates/rest/src/handlers/delete.rs index e490d9e56..48569bdc7 100644 --- a/crates/rest/src/handlers/delete.rs +++ b/crates/rest/src/handlers/delete.rs @@ -273,12 +273,27 @@ where use helios_persistence::core::ConditionalDeleteResult; match result { - ConditionalDeleteResult::Deleted => { + ConditionalDeleteResult::Deleted(deleted) => { debug!( resource_type = %resource_type, + id = %deleted.id(), "Resource conditionally deleted" ); - Ok(StatusCode::NO_CONTENT.into_response()) + // Name the entity in the audit trail, as the instance delete does. + // Before the result carried the snapshot, a delete-by-criteria + // produced an AuditEvent with no entity at all. + let mut response = StatusCode::NO_CONTENT.into_response(); + response + .extensions_mut() + .insert(helios_audit::AuditResponseContext { + resource_type: Some(resource_type.clone()), + resource_id: Some(deleted.id().to_string()), + patient_reference: super::extract_patient_from_resource( + &resource_type, + deleted.content(), + ), + }); + Ok(response) } ConditionalDeleteResult::NoMatch => { // Per FHIR spec, no match on conditional delete is success diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index 192dd4c44..35eea1564 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -1220,15 +1220,19 @@ mod conditional_references { } // ============================================================================= -// Conditional Entry Tests (#503) +// Conditional Entry Tests (#503, #511) // ============================================================================= -/// Conditional interactions expressed in an entry URL (`[type]?[criteria]`) are -/// refused rather than resolved, and — the point of #503 — nothing is written. +/// Conditional interactions in batch entries — `PUT`/`DELETE [type]?[criteria]` +/// and `POST` with `ifNoneExist` — resolve against storage with the status +/// mapping the resource endpoints use (#511), and `ifNoneExist` resolves inside +/// a transaction. /// -/// Before the fix the criteria rode along inside the parsed resource type, so a +/// Before #503 the criteria rode along inside the parsed resource type, so a /// conditional `PUT`/`DELETE` addressed storage with a type like -/// `Patient?identifier=http:` and an empty id. Resolving these is #511. +/// `Patient?identifier=http:` and an empty id; the type-level guards below keep +/// that row from ever being written. URL criteria inside a transaction remain +/// declined whole. mod conditional_entries { use super::*; @@ -1253,68 +1257,373 @@ mod conditional_entries { .expect("count failed") } + async fn seed_patient_with_identifier(backend: &SqliteBackend, id: &str, family: &str) { + backend + .create( + &test_tenant(), + "Patient", + json!({ + "resourceType": "Patient", + "id": id, + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": family}] + }), + FhirVersion::R4, + ) + .await + .expect("Failed to seed patient"); + } + + fn conditional_put(url: &str, family: &str) -> Value { + json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "PUT", "url": url }, + "resource": { + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": family}] + } + }] + }) + } + + fn if_none_exist_post(criteria: &str, family: &str, full_url: Option<&str>) -> Value { + let mut entry = json!({ + "request": { "method": "POST", "url": "Patient", "ifNoneExist": criteria }, + "resource": { + "resourceType": "Patient", + "identifier": [{"system": "http://example.org", "value": "12345"}], + "name": [{"family": family}] + } + }); + if let Some(full_url) = full_url { + entry["fullUrl"] = json!(full_url); + } + entry + } + + async fn family_of(backend: &SqliteBackend, id: &str) -> String { + backend + .read(&test_tenant(), "Patient", id) + .await + .expect("read failed") + .expect("patient exists") + .content()["name"][0]["family"] + .as_str() + .expect("family") + .to_string() + } + + // ── PUT [type]?[criteria] ──────────────────────────────────────────────── + #[tokio::test] - async fn conditional_put_is_refused_and_writes_nothing() { + async fn conditional_put_creates_when_nothing_matches() { let (server, backend) = create_test_server().await; - seed_patient(&backend, "p1", "Nguyen").await; let before = patient_count(&backend).await; let body = post_batch( &server, - json!({ - "resourceType": "Bundle", - "type": "batch", - "entry": [{ - "request": { - "method": "PUT", - "url": "Patient?identifier=http://example.org|12345" - }, - "resource": { "resourceType": "Patient", "name": [{"family": "Conditional"}] } - }] - }), + conditional_put("Patient?identifier=http://example.org|12345", "Conditional"), ) .await; - assert_eq!(body["entry"][0]["response"]["status"], "400 Bad Request"); + 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!(entry["resource"]["name"][0]["family"], "Conditional"); + assert_eq!(patient_count(&backend).await, before + 1); + } + + #[tokio::test] + async fn 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, + conditional_put("Patient?identifier=http://example.org|12345", "Updated"), + ) + .await; + + let entry = &body["entry"][0]; + assert_eq!(entry["response"]["status"], "200 OK", "{entry}"); + assert_eq!(entry["response"]["location"], "Patient/p1"); + assert_eq!(entry["resource"]["id"], "p1"); + assert_eq!(family_of(&backend, "p1").await, "Updated"); assert_eq!( patient_count(&backend).await, before, - "a refused conditional PUT must not create a resource" + "an update creates nothing" + ); + } + + #[tokio::test] + async fn conditional_put_with_several_matches_is_412_and_writes_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 body = post_batch( + &server, + conditional_put("Patient?identifier=http://example.org|12345", "Ambiguous"), + ) + .await; + + let entry = &body["entry"][0]; + assert_eq!( + entry["response"]["status"], "412 Precondition Failed", + "{entry}" + ); + // Entry failures render through `create_error_result`, which carries + // the message in `details.text` (the issue-code refinement is #516). + assert!( + entry["response"]["outcome"]["issue"][0]["details"]["text"] + .as_str() + .is_some_and(|t| t.contains("matched 2")), + "{entry}" ); + assert_eq!(patient_count(&backend).await, before); + assert_eq!(family_of(&backend, "p1").await, "One"); + assert_eq!(family_of(&backend, "p2").await, "Two"); + } + + /// Bundle entry URLs arrive percent-encoded; the criteria are decoded + /// before the backend sees them, as a request URL's query would be. + #[tokio::test] + async fn percent_encoded_criteria_match_the_decoded_identifier() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let body = post_batch( + &server, + conditional_put( + "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"); } #[tokio::test] - async fn conditional_delete_is_refused_and_deletes_nothing() { + async fn if_match_on_a_conditional_entry_is_400_and_writes_nothing() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + + let mut bundle = conditional_put("Patient?identifier=http://example.org|12345", "Stale"); + bundle["entry"][0]["request"]["ifMatch"] = json!("W/\"1\""); + let body = post_batch(&server, bundle).await; + + assert_eq!( + body["entry"][0]["response"]["status"], "400 Bad Request", + "{body}" + ); + assert_eq!(family_of(&backend, "p1").await, "Nguyen"); + } + + // ── DELETE [type]?[criteria] ───────────────────────────────────────────── + + fn conditional_delete(url: &str) -> Value { + json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ "request": { "method": "DELETE", "url": url } }] + }) + } + + #[tokio::test] + async fn conditional_delete_removes_the_single_match() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "Nguyen").await; + seed_patient(&backend, "p2", "Bystander").await; + + let body = post_batch( + &server, + conditional_delete("Patient?identifier=http://example.org|12345"), + ) + .await; + + let entry = &body["entry"][0]; + assert_eq!(entry["response"]["status"], "204 No Content", "{entry}"); + assert!( + entry.get("resource").is_none(), + "a 204 carries no body: {entry}" + ); + assert!( + !matches!( + backend.read(&test_tenant(), "Patient", "p1").await, + Ok(Some(_)) + ), + "the match must be gone" + ); + assert_eq!(family_of(&backend, "p2").await, "Bystander"); + } + + #[tokio::test] + async fn conditional_delete_with_no_match_is_204() { let (server, backend) = create_test_server().await; seed_patient(&backend, "p1", "Nguyen").await; let before = patient_count(&backend).await; let body = post_batch( &server, + conditional_delete("Patient?identifier=http://example.org|nobody"), + ) + .await; + + assert_eq!( + body["entry"][0]["response"]["status"], "204 No Content", + "{body}" + ); + assert_eq!(patient_count(&backend).await, before); + } + + #[tokio::test] + async fn 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 body = post_batch( + &server, + conditional_delete("Patient?identifier=http://example.org|12345"), + ) + .await; + + assert_eq!( + body["entry"][0]["response"]["status"], "412 Precondition Failed", + "{body}" + ); + assert_eq!(patient_count(&backend).await, before); + } + + // ── POST + ifNoneExist ─────────────────────────────────────────────────── + + #[tokio::test] + async fn if_none_exist_creates_then_answers_the_match() { + let (server, backend) = create_test_server().await; + let bundle = |family: &str| { json!({ "resourceType": "Bundle", "type": "batch", - "entry": [{ - "request": { "method": "DELETE", "url": "Patient?name=Nguyen" } - }] + "entry": [if_none_exist_post("identifier=http://example.org|12345", family, None)] + }) + }; + + let first = post_batch(&server, bundle("First")).await; + let first_entry = &first["entry"][0]; + assert_eq!( + first_entry["response"]["status"], "201 Created", + "{first_entry}" + ); + let created_location = first_entry["response"]["location"] + .as_str() + .expect("location") + .to_string(); + + let second = post_batch(&server, bundle("Second")).await; + let second_entry = &second["entry"][0]; + assert_eq!( + second_entry["response"]["status"], "200 OK", + "{second_entry}" + ); + assert_eq!( + second_entry["response"]["location"], created_location, + "the match is named, exactly as the create was" + ); + assert_eq!(second_entry["resource"]["name"][0]["family"], "First"); + assert_eq!(patient_count(&backend).await, 1); + } + + #[tokio::test] + async fn if_none_exist_with_several_matches_is_412() { + let (server, backend) = create_test_server().await; + seed_patient_with_identifier(&backend, "p1", "One").await; + seed_patient_with_identifier(&backend, "p2", "Two").await; + + let body = post_batch( + &server, + json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [if_none_exist_post("identifier=http://example.org|12345", "Third", None)] }), ) .await; - assert_eq!(body["entry"][0]["response"]["status"], "400 Bad Request"); assert_eq!( - patient_count(&backend).await, - before, - "a refused conditional DELETE must not remove a resource" + body["entry"][0]["response"]["status"], "412 Precondition Failed", + "{body}" ); - assert!( - backend - .read(&test_tenant(), "Patient", "p1") - .await - .expect("read failed") - .is_some(), - "the seeded patient must survive" + assert_eq!(patient_count(&backend).await, 2); + } + + /// The transaction executor resolves `ifNoneExist` inside the transaction: + /// the same transaction twice creates once, and on the replay a `urn:uuid` + /// reference to the matched entry resolves to the match. + #[tokio::test] + async fn transaction_if_none_exist_is_idempotent_and_resolves_references() { + let (server, backend) = create_test_server().await; + let bundle = json!({ + "resourceType": "Bundle", + "type": "transaction", + "entry": [ + if_none_exist_post( + "identifier=http://example.org|12345", + "Once", + Some("urn:uuid:patient") + ), + { + "fullUrl": "urn:uuid:observation", + "request": { "method": "POST", "url": "Observation" }, + "resource": { + "resourceType": "Observation", + "status": "final", + "code": {"text": "test"}, + "subject": {"reference": "urn:uuid:patient"} + } + } + ] + }); + + let first = post_batch(&server, bundle.clone()).await; + assert_eq!( + first["entry"][0]["response"]["status"], "201 Created", + "{first}" + ); + let patient_id = first["entry"][0]["resource"]["id"] + .as_str() + .expect("created patient id") + .to_string(); + assert_eq!( + first["entry"][1]["resource"]["subject"]["reference"], + json!(format!("Patient/{patient_id}")) + ); + + let second = post_batch(&server, bundle).await; + assert_eq!( + second["entry"][0]["response"]["status"], "200 OK", + "{second}" + ); + assert_eq!(second["entry"][0]["resource"]["id"], json!(patient_id)); + assert_eq!( + second["entry"][1]["resource"]["subject"]["reference"], + json!(format!("Patient/{patient_id}")), + "a urn:uuid reference to the matched entry resolves to the match" ); + assert_eq!(patient_count(&backend).await, 1); } /// The corruption #503 closes: `create_or_update` with an empty id inserts