diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index 55e27a9375..94350ab12d 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -86,6 +86,19 @@ enum PendingSearchParameterChange { Delete, } +/// The version a Bundle writes over a stored document: an update over the live +/// document, or a restore over a tombstone (`deleted`, #2145). +/// See [`MongoBackend::write_next_version_in_bundle_transaction`]. +struct NextVersion<'a> { + resource_type: &'a str, + id: &'a str, + stored: &'a Document, + expected_version: &'a str, + deleted: bool, + fhir_version: helios_fhir::FhirVersion, + change: PendingSearchParameterChange, +} + /// What one attempt at a Bundle transaction has staged: the SearchParameter /// changes for after its commit, for a Bundle that defers its search rows /// (#1975) the resources it left without them, and the creates not yet sent to @@ -5301,6 +5314,31 @@ impl MongoBackend { let mut resource_with_id = resource; resource_with_id["id"] = serde_json::json!(id); + // A deleted id is restored, as the same PUT outside a + // Bundle is; only an id never stored creates (#2145). + if let Some(deleted) = self + .read_deleted_in_bundle_transaction( + db, + session, + tenant, + &resource_type, + &id, + ) + .await? + { + let restored = self + .restore_resource_in_bundle_transaction( + db, + session, + tenant, + &deleted, + resource_with_id, + writes, + ) + .await?; + return Ok(BundleEntryResult::created(restored)); + } + let created = self.create_resource_in_bundle_transaction( index, tenant, @@ -5568,7 +5606,6 @@ impl MongoBackend { writes: &mut BundleWrites, ) -> StorageResult { let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); - let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); let tenant_id = tenant.tenant_id().as_str(); let resource_type = current.resource_type(); let id = current.id(); @@ -5608,7 +5645,124 @@ impl MongoBackend { )); } - let new_version = next_version(current.version_id())?; + self.write_next_version_in_bundle_transaction( + db, + session, + tenant, + NextVersion { + resource_type, + id, + stored: &existing_doc, + expected_version: current.version_id(), + deleted: false, + fhir_version: current.fhir_version(), + change: PendingSearchParameterChange::Update, + }, + resource, + writes, + ) + .await + } + + /// The tombstone of a soft-deleted resource, read inside the Bundle's + /// session; `None` when the id is live or was never stored. + async fn read_deleted_in_bundle_transaction( + &self, + db: &mongodb::Database, + session: &mut ClientSession, + tenant: &TenantContext, + resource_type: &str, + id: &str, + ) -> StorageResult> { + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + resources + .find_one(doc! { + "tenant_id": tenant.tenant_id().as_str(), + "resource_type": resource_type, + "id": id, + "is_deleted": true, + }) + .session(&mut *session) + .await + .map_err(|e| internal_driver_error("Failed to read deleted resource in transaction", e)) + } + + /// A `PUT` to the id of a soft-deleted resource restores it inside the + /// Bundle's session and continues its version chain, as the same `PUT` + /// outside a Bundle does ([`Self::restore_attempt`], FHIR + /// http.html#delete; #2145). `deleted` is the tombstone + /// ([`Self::read_deleted_in_bundle_transaction`]). + /// + /// The tombstone keeps its document under the unique, non-partial + /// `idx_resources_identity`, so a create over it would be refused as + /// [`ResourceError::AlreadyExists`]; that refusal was the Bundle's answer + /// before this restore existed. + async fn restore_resource_in_bundle_transaction( + &self, + db: &mongodb::Database, + session: &mut ClientSession, + tenant: &TenantContext, + deleted: &Document, + resource: Value, + writes: &mut BundleWrites, + ) -> StorageResult { + let field = |name: &str| { + deleted.get_str(name).map_err(|e| { + internal_error(format!( + "Missing {name} of deleted resource in transaction: {e}" + )) + }) + }; + // The restore keeps the FHIR version the resource was stored under. + let fhir_version = + FhirVersion::from_storage(deleted.get_str("fhir_version").unwrap_or("4.0")) + .unwrap_or_else(helios_fhir::FhirVersion::default_enabled); + + self.write_next_version_in_bundle_transaction( + db, + session, + tenant, + NextVersion { + resource_type: field("resource_type")?, + id: field("id")?, + stored: deleted, + expected_version: field("version_id")?, + deleted: true, + fhir_version, + change: PendingSearchParameterChange::Create, + }, + resource, + writes, + ) + .await + } + + /// Writes the version after `expected_version` over the stored document + /// (live, or a tombstone when `deleted`) inside the Bundle's session: the + /// compare-and-swap on the current document, its history row and its + /// search rows. A lost compare-and-swap is a `VersionConflict`. + async fn write_next_version_in_bundle_transaction( + &self, + db: &mongodb::Database, + session: &mut ClientSession, + tenant: &TenantContext, + next: NextVersion<'_>, + resource: Value, + writes: &mut BundleWrites, + ) -> StorageResult { + let NextVersion { + resource_type, + id, + stored: existing_doc, + expected_version, + deleted, + fhir_version, + change, + } = next; + let resources = db.collection::(MongoBackend::RESOURCES_COLLECTION); + let history = db.collection::(MongoBackend::RESOURCE_HISTORY_COLLECTION); + let tenant_id = tenant.tenant_id().as_str(); + let new_version = next_version(expected_version)?; let mut resource = resource; ensure_resource_identity(resource_type, id, &mut resource); let search_documents = self.search_index_documents(tenant_id, resource_type, id, &resource); @@ -5616,7 +5770,6 @@ impl MongoBackend { let now = Utc::now(); let now_bson = chrono_to_bson(now); - let fhir_version = current.fhir_version(); let fhir_version_str = fhir_version.as_mime_param().to_string(); let mut set = doc! { @@ -5637,8 +5790,8 @@ impl MongoBackend { "tenant_id": tenant_id, "resource_type": resource_type, "id": id, - "version_id": current.version_id(), - "is_deleted": false, + "version_id": expected_version, + "is_deleted": deleted, }, search_documents.resource_update(doc! { "$set": set }), ) @@ -5651,13 +5804,13 @@ impl MongoBackend { ConcurrencyError::VersionConflict { resource_type: resource_type.to_string(), id: id.to_string(), - expected_version: current.version_id().to_string(), + expected_version: expected_version.to_string(), actual_version: "unknown".to_string(), }, )); } - let created_at = extract_created_at(&existing_doc, now); + let created_at = extract_created_at(existing_doc, now); history .insert_one(doc! { @@ -5688,9 +5841,7 @@ impl MongoBackend { .await?; if resource_type == "SearchParameter" { - writes - .search_parameter_changes - .push(PendingSearchParameterChange::Update); + writes.search_parameter_changes.push(change); } Ok(StoredResource::from_storage( diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index 483a70eae3..f14073b5f9 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -5221,9 +5221,18 @@ impl PostgresBackend { Ok(BundleEntryResult::updated(updated)) } None => { - // Create new resource with specified ID let mut resource_with_id = resource; resource_with_id["id"] = serde_json::json!(id); + // A deleted id is restored, as the same PUT outside a + // Bundle is; only an id never stored creates (#2145). + if let Some(deleted) = tx + .read_including_deleted(&resource_type, &id) + .await? + .filter(|r| r.is_deleted()) + { + let restored = tx.restore(&deleted, resource_with_id).await?; + return Ok(BundleEntryResult::created(restored)); + } let created = tx.create(&resource_type, resource_with_id).await?; Ok(BundleEntryResult::created(created)) } diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index e96c593699..a12118374b 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -3971,9 +3971,18 @@ impl SqliteBackend { Ok(BundleEntryResult::updated(updated)) } None => { - // Create new resource with specified ID let mut resource_with_id = resource; resource_with_id["id"] = serde_json::json!(id); + // A deleted id is restored, as the same PUT outside a + // Bundle is; only an id never stored creates (#2145). + if let Some(deleted) = tx + .read_including_deleted(&resource_type, &id) + .await? + .filter(|r| r.is_deleted()) + { + let restored = tx.restore(&deleted, resource_with_id).await?; + return Ok(BundleEntryResult::created(restored)); + } let created = tx.create(&resource_type, resource_with_id).await?; Ok(BundleEntryResult::created(created)) } diff --git a/crates/persistence/tests/mongodb/bundle_create_chunks.rs b/crates/persistence/tests/mongodb/bundle_create_chunks.rs index 5bb8fc1864..673c5cd0b0 100644 --- a/crates/persistence/tests/mongodb/bundle_create_chunks.rs +++ b/crates/persistence/tests/mongodb/bundle_create_chunks.rs @@ -870,11 +870,12 @@ async fn an_earlier_duplicate_is_reported_before_a_later_invalid_entry() { } } -/// A PUT to the id of a deleted resource creates, and fails as the id is taken. -/// This pins the answer the Bundle path has always given; a PUT outside a Bundle -/// restores the resource instead. +/// A PUT to the id of a deleted resource restores it, as the same PUT outside a +/// Bundle does, instead of failing as the id is taken (#2145): the entry answers +/// 201, the version chain continues (create 1, delete 2, restore 3) on the +/// tombstone's document, and the restored body is searchable. #[tokio::test] -async fn a_put_that_creates_over_a_deleted_id_fails_as_before() { +async fn a_put_over_a_deleted_id_restores_it() { for mode in MODES { let name = format!("put_over_deleted_{}", mode.label()); let Some(backend) = backend_or_skip(&name, mode).await else { @@ -882,7 +883,7 @@ async fn a_put_that_creates_over_a_deleted_id_fails_as_before() { }; let tenant = create_tenant("chunks-reuse"); seed_patient(&backend, &tenant, "dup-put", true).await; - let before = store_counts(&backend, tenant.tenant_id().as_str()).await; + let [resources, history, ..] = store_counts(&backend, tenant.tenant_id().as_str()).await; let mut entries = patient_posts(6); entries[2] = BundleEntry { @@ -891,15 +892,40 @@ async fn a_put_that_creates_over_a_deleted_id_fails_as_before() { resource: Some(json!({ "resourceType": "Patient", "id": "dup-put", - "name": [{"family": "Put"}], + "name": [{"family": "RestoredByPut"}], })), ..Default::default() }; - let Some(failure) = assert_bundle_error(&backend, &tenant, entries, before, &name).await + let Some(result) = process_transaction_or_skip(&backend, &tenant, entries, &name).await else { return; }; - assert_eq!(failure, (2, already_exists("dup-put")), "{name}"); + assert_eq!(result.entries[2].status, 201, "{name}: the PUT restores"); + + let restored = backend + .read(&tenant, "Patient", "dup-put") + .await + .expect("the restored resource reads") + .expect("the restored resource is live"); + assert_eq!(restored.version_id(), "3", "{name}: the chain continues"); + assert_eq!( + restored.content()["name"][0]["family"], + "RestoredByPut", + "{name}" + ); + + // Five POSTs add five documents; the restore reuses the tombstone's. + // Every entry adds one history version. + let [resources_after, history_after, ..] = + store_counts(&backend, tenant.tenant_id().as_str()).await; + assert_eq!(resources_after, resources + 5, "{name}: resources"); + assert_eq!(history_after, history + 6, "{name}: history"); + + assert_eq!( + patients_by_family(&backend, &tenant, "RestoredByPut").await, + vec!["dup-put".to_string()], + "{name}: the restored body is searchable" + ); } } diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index c6b21abc00..947f72bf91 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -24152,6 +24152,10 @@ mongodb_bundle_identity_test!( mongodb_integration_changed_conditional_target_after_a_written_reference_rolls_back, changed_conditional_target_after_a_written_reference_rolls_back ); +mongodb_bundle_identity_test!( + mongodb_integration_put_over_a_deleted_id_restores_it, + put_over_a_deleted_id_restores_it +); #[path = "mongodb/missing_index.rs"] mod missing_index; diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index ad4bf82aef..df21cc3adf 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -31657,6 +31657,10 @@ mod postgres_integration { postgres_integration_changed_conditional_target_after_a_written_reference_rolls_back, changed_conditional_target_after_a_written_reference_rolls_back ); + pg_bundle_identity_test!( + postgres_integration_put_over_a_deleted_id_restores_it, + put_over_a_deleted_id_restores_it + ); pg_if_match_test!( postgres_integration_multi_valued_if_match_matches_any_member, diff --git a/crates/persistence/tests/transactions/bundle_identity_suite.rs b/crates/persistence/tests/transactions/bundle_identity_suite.rs index ff1d96aa3e..e3b104df3b 100644 --- a/crates/persistence/tests/transactions/bundle_identity_suite.rs +++ b/crates/persistence/tests/transactions/bundle_identity_suite.rs @@ -266,6 +266,54 @@ pub async fn two_puts_on_one_id_roll_back(backend: &B, tenant assert_eq!(count(backend, tenant, "Patient").await, 0); } +/// A PUT to the id of a soft-deleted resource restores it, as the same PUT +/// outside a Bundle does (FHIR http.html#delete): the entry answers 201, the +/// version chain continues (create 1, delete 2, restore 3) and the resource +/// counts as live again. Every backend used to create instead and collide with +/// the tombstone's row, failing the whole Bundle as "already exists" (#2145). +pub async fn put_over_a_deleted_id_restores_it( + backend: &B, + tenant: &TenantContext, +) { + backend + .create( + tenant, + "Patient", + json!({"resourceType": "Patient", "id": "restored", "gender": "male"}), + FhirVersion::default(), + ) + .await + .expect("seed"); + backend + .delete(tenant, "Patient", "restored") + .await + .expect("soft delete"); + + let result = backend + .process_transaction( + tenant, + vec![put( + "restored", + json!({"resourceType": "Patient", "id": "restored", "gender": "female"}), + None, + )], + FhirVersion::default(), + ) + .await + .expect("the PUT restores the deleted id"); + assert_eq!(result.entries[0].status, 201); + assert_eq!(result.entries[0].effect.live_count_delta(), 1); + + let restored = backend + .read(tenant, "Patient", "restored") + .await + .expect("read") + .expect("the restored resource is live"); + assert_eq!(restored.version_id(), "3", "the version chain continues"); + assert_eq!(restored.content()["gender"], "female"); + assert_eq!(count(backend, tenant, "Patient").await, 1); +} + /// A conditional DELETE that matched nothing answers 204 but reports that /// nothing was deleted, so no live count moves (#1078). pub async fn conditional_delete_without_a_match_reports_not_found( diff --git a/crates/persistence/tests/transactions/bundle_tests.rs b/crates/persistence/tests/transactions/bundle_tests.rs index a7e2defb7c..a4ae4ffa60 100644 --- a/crates/persistence/tests/transactions/bundle_tests.rs +++ b/crates/persistence/tests/transactions/bundle_tests.rs @@ -2133,3 +2133,4 @@ sqlite_bundle_identity_test!(put_then_patch_on_one_id_rolls_back); sqlite_bundle_identity_test!(two_puts_on_one_id_roll_back); sqlite_bundle_identity_test!(conditional_delete_without_a_match_reports_not_found); sqlite_bundle_identity_test!(changed_conditional_target_after_a_written_reference_rolls_back); +sqlite_bundle_identity_test!(put_over_a_deleted_id_restores_it);