From 0da1cd5177e24e35bffd57eacf0dc5445424bf5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Fri, 23 Oct 2026 15:15:22 -0400 Subject: [PATCH 1/2] fix(persistence): a transaction Bundle PUT to a deleted id restores it A PUT entry whose id was soft-deleted read no current representation, fell through to a create, and collided with the tombstone's row, so the whole Bundle failed 400 "resource already exists". The same PUT outside a Bundle restores the resource (FHIR http.html#delete). The issue reported this for MongoDB; SQLite and PostgreSQL had the same gap (their transaction `read` hides the tombstone and `create` refuses its row). Each backend's Bundle PUT now restores a tombstone inside the transaction, continuing its version chain and keeping its FHIR version, before it falls back to a create: - MongoDB: `read_deleted_in_bundle_transaction` + `restore_resource_in_bundle_transaction`, sharing one versioned write with the Bundle update. - SQLite: `SqliteTransaction::restore`. - PostgreSQL: `PostgresTransaction::restore`, sharing the update's compare-and-swap statement via `write_next_version`. POST with a reused (live or deleted) id still fails with AlreadyExists. Closes #2145 --- .../src/backends/mongodb/storage.rs | 171 +++++++++- .../src/backends/postgres/storage.rs | 8 +- .../src/backends/postgres/transaction.rs | 311 +++++++++++------- .../src/backends/sqlite/storage.rs | 8 +- .../src/backends/sqlite/transaction.rs | 110 +++++++ .../tests/mongodb/bundle_create_chunks.rs | 42 ++- crates/persistence/tests/mongodb_tests.rs | 4 + crates/persistence/tests/postgres_tests.rs | 4 + .../transactions/bundle_identity_suite.rs | 48 +++ .../tests/transactions/bundle_tests.rs | 1 + 10 files changed, 574 insertions(+), 133 deletions(-) diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index a614311c3a..62d521fd91 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -85,6 +85,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 @@ -5229,6 +5242,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, @@ -5496,7 +5534,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(); @@ -5536,7 +5573,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); @@ -5544,7 +5698,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! { @@ -5565,8 +5718,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 }), ) @@ -5579,13 +5732,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! { @@ -5616,9 +5769,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 6e13debb30..13441b5946 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -5213,9 +5213,15 @@ 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(restored) = + tx.restore(&resource_type, &id, &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/postgres/transaction.rs b/crates/persistence/src/backends/postgres/transaction.rs index 18526ec2e1..3cc86bfed8 100644 --- a/crates/persistence/src/backends/postgres/transaction.rs +++ b/crates/persistence/src/backends/postgres/transaction.rs @@ -1149,124 +1149,19 @@ impl Transaction for PostgresTransaction { // what they saw before: the batch is a wire-level optimisation, never a // change of visibility inside the transaction. self.flush().await?; - self.refresh_search_snapshot_if_dirty().await?; - self.poisoned = true; - - let client = self.client()?; - let tenant_id = self.tenant.tenant_id().as_str().to_string(); - let resource_type = current.resource_type(); - let id = current.id(); - // The expected version is `current`'s, and the UPDATE below only matches a + // The expected version is `current`'s, and the UPDATE only matches a // row that still carries it — so the new version follows from what the // caller already read, with no round trip to ask what is stored. - let expected_version = current.version_id().to_string(); - let new_version: u64 = current.version_id().parse().unwrap_or(0) + 1; - let new_version_str = new_version.to_string(); - - // Build the resource with id and resourceType - let mut data = resource; - if let Some(obj) = data.as_object_mut() { - obj.insert("id".to_string(), Value::String(id.to_string())); - obj.insert( - "resourceType".to_string(), - Value::String(resource_type.to_string()), - ); - } - - let now = Utc::now(); - let fhir_version = current.fhir_version(); - let fhir_version_str = fhir_version.as_mime_param(); - // Version check, update and history row in one statement. Folding the - // expected version into the `WHERE` makes the check and write atomic: - // a concurrent writer cannot bump the version between two statements - // and have this update overwrite it while reporting success. - let updated = execute_cached( - client, - "WITH upd AS ( - UPDATE resources SET version_id = $1, data = $2, last_updated = $3 - WHERE tenant_id = $4 AND resource_type = $5 AND id = $6 - AND is_deleted = FALSE AND version_id = $7 - RETURNING tenant_id, resource_type, id, version_id, data, last_updated, is_deleted - ) - INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) - SELECT tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, $8 FROM upd", - &[ - &new_version_str, - &data, - &now, - &tenant_id, - &resource_type, - &id, - &expected_version, - &fhir_version_str, - ], + self.write_next_version( + current.resource_type(), + current.id(), + current.version_id(), + current.fhir_version(), + false, + resource, ) .await - .or_query_error("Failed to update resource")?; - - if updated == 0 { - // The statement matched nothing because the row is missing, - // soft-deleted, or has a different version. Only the last case is - // a concurrency conflict; the lookup is needed only on this error - // path to distinguish it from the two not-found cases. - let actual = query_opt_cached( - client, - "SELECT version_id FROM resources - WHERE tenant_id = $1 AND resource_type = $2 AND id = $3 AND is_deleted = FALSE", - &[&tenant_id, &resource_type, &id], - ) - .await - .or_query_error("Failed to get current version")?; - - return match actual { - Some(row) => Err(StorageError::Concurrency( - ConcurrencyError::VersionConflict { - resource_type: resource_type.to_string(), - id: id.to_string(), - expected_version, - actual_version: row.get::<_, String>(0), - }, - )), - None => Err(StorageError::Resource(ResourceError::NotFound { - resource_type: resource_type.to_string(), - id: id.to_string(), - })), - }; - } - - // Re-index the resource for search - if resource_type == "SearchParameter" { - self.search_snapshot_dirty = true; - self.refresh_search_snapshot_if_dirty().await?; - } - if let Err(error) = self - .index_resource( - &tenant_id, - resource_type, - id, - now, - super::storage::IndexWrite::Replace, - &data, - ) - .await - { - self.poisoned = true; - return Err(error); - } - self.poisoned = false; - - Ok(StoredResource::from_storage( - resource_type, - id, - new_version_str, - self.tenant.tenant_id().clone(), - data, - now, - now, - None, - fhir_version, - )) } async fn delete(&mut self, resource_type: &str, id: &str) -> StorageResult<()> { @@ -1431,6 +1326,196 @@ impl Transaction for PostgresTransaction { } } +impl PostgresTransaction { + /// Restores a soft-deleted resource with new content inside the + /// transaction, continuing its version chain and keeping the FHIR version + /// it was stored under, as `PostgresBackend::restore_deleted` does outside + /// a Bundle (FHIR http.html#delete; #2145). Returns `None`, having written + /// nothing, when no deleted row is stored for the id. + pub(crate) async fn restore( + &mut self, + resource_type: &str, + id: &str, + resource: &Value, + ) -> StorageResult> { + self.ensure_usable()?; + self.ensure_planned_key(resource_type, id, false)?; + // See `update`: buffered creates are visible to every non-create. + self.flush().await?; + + let client = self.client()?; + let tenant_id = self.tenant.tenant_id().as_str(); + let Some(row) = query_opt_cached( + client, + "SELECT version_id, fhir_version FROM resources + WHERE tenant_id = $1 AND resource_type = $2 AND id = $3 AND is_deleted = TRUE", + &[&tenant_id, &resource_type, &id], + ) + .await + .or_query_error("Failed to read deleted resource")? + else { + return Ok(None); + }; + let deleted_version: String = row.get(0); + let fhir_version_str: String = row.get(1); + let fhir_version = FhirVersion::from_storage(&fhir_version_str) + .unwrap_or_else(helios_fhir::FhirVersion::default_enabled); + + self.tenant + .check_permission(Operation::Update, resource_type)?; + self.write_next_version( + resource_type, + id, + &deleted_version, + fhir_version, + true, + resource.clone(), + ) + .await + .map(Some) + } + + /// Writes the version after `expected_version` over the stored row (live, + /// or the tombstone when `deleted`), its history row and its search rows. + /// Version check, write and history row are one statement. + async fn write_next_version( + &mut self, + resource_type: &str, + id: &str, + expected_version: &str, + fhir_version: FhirVersion, + deleted: bool, + resource: Value, + ) -> StorageResult { + self.refresh_search_snapshot_if_dirty().await?; + self.poisoned = true; + + let client = self.client()?; + let tenant_id = self.tenant.tenant_id().as_str().to_string(); + + let expected_version = expected_version.to_string(); + let new_version: u64 = expected_version.parse().unwrap_or(0) + 1; + let new_version_str = new_version.to_string(); + + // Build the resource with id and resourceType + let mut data = resource; + if let Some(obj) = data.as_object_mut() { + obj.insert("id".to_string(), Value::String(id.to_string())); + obj.insert( + "resourceType".to_string(), + Value::String(resource_type.to_string()), + ); + } + + let now = Utc::now(); + let fhir_version_str = fhir_version.as_mime_param(); + // Version check, update and history row in one statement. Folding the + // expected version into the `WHERE` makes the check and write atomic: + // a concurrent writer cannot bump the version between two statements + // and have this update overwrite it while reporting success. + // A restore matches the tombstone and makes it live again. + let statement = if deleted { + "WITH upd AS ( + UPDATE resources SET version_id = $1, data = $2, last_updated = $3, + is_deleted = FALSE, deleted_at = NULL + WHERE tenant_id = $4 AND resource_type = $5 AND id = $6 + AND is_deleted = TRUE AND version_id = $7 + RETURNING tenant_id, resource_type, id, version_id, data, last_updated, is_deleted + ) + INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) + SELECT tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, $8 FROM upd" + } else { + "WITH upd AS ( + UPDATE resources SET version_id = $1, data = $2, last_updated = $3 + WHERE tenant_id = $4 AND resource_type = $5 AND id = $6 + AND is_deleted = FALSE AND version_id = $7 + RETURNING tenant_id, resource_type, id, version_id, data, last_updated, is_deleted + ) + INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) + SELECT tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, $8 FROM upd" + }; + let updated = execute_cached( + client, + statement, + &[ + &new_version_str, + &data, + &now, + &tenant_id, + &resource_type, + &id, + &expected_version, + &fhir_version_str, + ], + ) + .await + .or_query_error("Failed to update resource")?; + + if updated == 0 { + // The statement matched nothing because the row is missing, + // soft-deleted, or has a different version. Only the last case is + // a concurrency conflict; the lookup is needed only on this error + // path to distinguish it from the two not-found cases. + let actual = query_opt_cached( + client, + "SELECT version_id FROM resources + WHERE tenant_id = $1 AND resource_type = $2 AND id = $3 AND is_deleted = FALSE", + &[&tenant_id, &resource_type, &id], + ) + .await + .or_query_error("Failed to get current version")?; + + return match actual { + Some(row) => Err(StorageError::Concurrency( + ConcurrencyError::VersionConflict { + resource_type: resource_type.to_string(), + id: id.to_string(), + expected_version, + actual_version: row.get::<_, String>(0), + }, + )), + None => Err(StorageError::Resource(ResourceError::NotFound { + resource_type: resource_type.to_string(), + id: id.to_string(), + })), + }; + } + + // Re-index the resource for search + if resource_type == "SearchParameter" { + self.search_snapshot_dirty = true; + self.refresh_search_snapshot_if_dirty().await?; + } + if let Err(error) = self + .index_resource( + &tenant_id, + resource_type, + id, + now, + super::storage::IndexWrite::Replace, + &data, + ) + .await + { + self.poisoned = true; + return Err(error); + } + self.poisoned = false; + + Ok(StoredResource::from_storage( + resource_type, + id, + new_version_str, + self.tenant.tenant_id().clone(), + data, + now, + now, + None, + fhir_version, + )) + } +} + impl Drop for PostgresTransaction { fn drop(&mut self) { if !self.active { diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index 78b50014f1..40c51451ba 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -3893,9 +3893,15 @@ 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(restored) = + tx.restore(&resource_type, &id, &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/transaction.rs b/crates/persistence/src/backends/sqlite/transaction.rs index 43683945f4..9d6e90b85a 100644 --- a/crates/persistence/src/backends/sqlite/transaction.rs +++ b/crates/persistence/src/backends/sqlite/transaction.rs @@ -241,6 +241,116 @@ impl SqliteTransaction { ) -> StorageResult { self.create_inner(resource_type, resource, prepared) } + + /// Restores a soft-deleted resource with new content inside the + /// transaction, continuing its version chain and keeping the FHIR version + /// it was stored under, as `SqliteBackend::restore_deleted` does outside a + /// Bundle (FHIR http.html#delete; #2145). Returns `None`, having written + /// nothing, when no deleted row is stored for the id. + pub(crate) async fn restore( + &mut self, + resource_type: &str, + id: &str, + resource: &Value, + ) -> StorageResult> { + if !self.active { + return Err(StorageError::Transaction( + TransactionError::InvalidTransaction, + )); + } + + let conn = self.conn.lock(); + let tenant_id = self.tenant.tenant_id().as_str(); + + let row: Result<(String, String), _> = conn.query_row( + "SELECT version_id, fhir_version FROM resources + WHERE tenant_id = ?1 AND resource_type = ?2 AND id = ?3 AND is_deleted = 1", + params![tenant_id, resource_type, id], + |row| Ok((row.get(0)?, row.get(1)?)), + ); + let (deleted_version, fhir_version_str) = match row { + Ok(v) => v, + Err(rusqlite::Error::QueryReturnedNoRows) => return Ok(None), + Err(e) => { + return Err(internal_error(format!( + "Failed to read deleted resource: {}", + e + ))); + } + }; + self.tenant + .check_permission(Operation::Update, resource_type)?; + + let new_version: u64 = deleted_version.parse().unwrap_or(0) + 1; + let new_version_str = new_version.to_string(); + + let mut data = resource.clone(); + if let Some(obj) = data.as_object_mut() { + obj.insert("id".to_string(), Value::String(id.to_string())); + obj.insert( + "resourceType".to_string(), + Value::String(resource_type.to_string()), + ); + } + let data_bytes = serde_json::to_vec(&data) + .map_err(|e| serialization_error(format!("Failed to serialize resource: {}", e)))?; + + let now = Utc::now(); + let last_updated = now.to_rfc3339(); + + conn.execute( + "UPDATE resources + SET version_id = ?1, data = ?2, last_updated = ?3, is_deleted = 0, deleted_at = NULL + WHERE tenant_id = ?4 AND resource_type = ?5 AND id = ?6 AND is_deleted = 1", + params![ + new_version_str, + data_bytes, + last_updated, + tenant_id, + resource_type, + id + ], + ) + .map_err(|e| internal_error(format!("Failed to restore resource: {}", e)))?; + + conn.execute( + "INSERT INTO resource_history (tenant_id, resource_type, id, version_id, data, last_updated, is_deleted, fhir_version) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, 0, ?7)", + params![tenant_id, resource_type, id, new_version_str, data_bytes, last_updated, fhir_version_str], + ) + .map_err(|e| internal_error(format!("Failed to insert restore history: {}", e)))?; + + // The delete dropped the search rows; rebuild them for the resource + // that is live again, clearing any left behind. + self.index_resource(&conn, tenant_id, resource_type, id, &data, true, None)?; + drop(conn); + + // A restored overlay-affecting SearchParameter re-enters the tenant's + // overlay once the commit makes it readable. + if resource_type == "SearchParameter" + && self + .backend + .tenant_registries() + .create_affects_overlay(&data) + { + self.invalidate_registry = true; + } + + let fhir_version = FhirVersion::from_storage(&fhir_version_str) + .unwrap_or_else(helios_fhir::FhirVersion::default_enabled); + + Ok(Some(StoredResource::from_storage( + resource_type, + id, + new_version_str, + self.tenant.tenant_id().clone(), + data, + now, + now, + None, + fhir_version, + ))) + } } impl SqliteTransaction { diff --git a/crates/persistence/tests/mongodb/bundle_create_chunks.rs b/crates/persistence/tests/mongodb/bundle_create_chunks.rs index 6b00f64afb..a19be887d8 100644 --- a/crates/persistence/tests/mongodb/bundle_create_chunks.rs +++ b/crates/persistence/tests/mongodb/bundle_create_chunks.rs @@ -855,11 +855,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 { @@ -867,7 +868,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 { @@ -876,15 +877,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 c127ae1554..5de549d30b 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -24091,6 +24091,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 ad25929fe2..d9b1372949 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -31593,6 +31593,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); From 779b03a44d12b68716c8edd97c2188b8fb300435 Mon Sep 17 00:00:00 2001 From: smunini Date: Sat, 24 Oct 2026 12:13:52 -0400 Subject: [PATCH 2/2] fix(fhirpath): pass new upstream fhir-test-cases from 2026-10-23 FHIR/fhir-test-cases merged oldrobot's PRs #316-#320 on 2026-10-23 and the "Test FHIRPath" gate (which runs the upstream corpus) went red on every PR with 10 undeclared failures. Fix the engine rather than declare them: - Delimited identifiers: `none_of("`")` consumed the backslash before the escape parser ran, so Patient.`name` never resolved. - unescape('json'): decode \uXXXX (pairing UTF-16 surrogates) and \/. - Indexer: a negative index is out of range, so the result is empty instead of an error. - skip({}) / take({}): an empty count returns empty. - DateTime + / - calendar duration: accept hour- and minute-precision values with a timezone (@2015-02-04T10Z, @2015-02-04T10:00Z) and keep the input precision in the result, so `= @2015-02-04T11:00Z` compares. Vendor the new cases into the local r4/r5 corpora. testLessThan28 then exposed that extension(url) on a FHIR primitive only worked through a hard-coded fabricated birthTime extension; read the primitive's own extensions instead and drop the fabrication. The parse-all test now requires invalid="syntax" expressions to be rejected. (cherry picked from commit 7e2e6debcd5882e5fa0466a9241e129744b7ecd1) --- crates/fhirpath/src/collection_navigation.rs | 4 + crates/fhirpath/src/evaluator.rs | 123 +++++++++++------- crates/fhirpath/src/extension_function.rs | 33 +---- crates/fhirpath/src/parser.rs | 4 +- .../fhirpath/tests/data/r4/tests-fhir-r4.xml | 28 ++++ .../fhirpath/tests/data/r5/tests-fhir-r5.xml | 28 ++++ crates/fhirpath/tests/evaluator_tests.rs | 7 +- crates/fhirpath/tests/parser_tests.rs | 28 +++- 8 files changed, 176 insertions(+), 79 deletions(-) diff --git a/crates/fhirpath/src/collection_navigation.rs b/crates/fhirpath/src/collection_navigation.rs index 79d6a5a190..8fac080e47 100644 --- a/crates/fhirpath/src/collection_navigation.rs +++ b/crates/fhirpath/src/collection_navigation.rs @@ -46,6 +46,8 @@ pub fn skip_function( EvaluationResult::Integer(i, _, _) => { if *i < 0 { 0 } else { *i as usize } // Treat negative skip as 0 } + // An empty count propagates: `x.skip({})` is empty + EvaluationResult::Empty => return Ok(EvaluationResult::Empty), // Add conversion from Decimal if it's an integer value EvaluationResult::Decimal(d, _, _) if d.is_integer() && d.is_sign_positive() => { d.to_usize().unwrap_or(0) // Convert non-negative integer Decimal @@ -192,6 +194,8 @@ pub fn take_function( EvaluationResult::Integer(i, _, _) => { if *i <= 0 { 0 } else { *i as usize } // Treat non-positive take as 0 } + // An empty count propagates: `x.take({})` is empty + EvaluationResult::Empty => return Ok(EvaluationResult::Empty), // Add conversion from Decimal if it's an integer value EvaluationResult::Decimal(d, _, _) if d.is_integer() && d.is_sign_positive() => { d.to_usize().unwrap_or(0) // Convert non-negative integer Decimal diff --git a/crates/fhirpath/src/evaluator.rs b/crates/fhirpath/src/evaluator.rs index 6a0d842fc4..0e00e56ce6 100644 --- a/crates/fhirpath/src/evaluator.rs +++ b/crates/fhirpath/src/evaluator.rs @@ -5177,6 +5177,15 @@ fn call_function( Some('t') => result.push('\t'), Some('b') => result.push('\u{0008}'), Some('f') => result.push('\u{000C}'), + Some('/') => result.push('/'), + Some('u') => { + // \uXXXX, pairing UTF-16 surrogate escapes + match unescape_json_unicode(&mut chars) { + Some(c) => result.push(c), + // Malformed escape, keep it as written + None => result.push_str("\\u"), + } + } Some(c) => { // Unknown escape, keep both characters result.push('\\'); @@ -6960,37 +6969,40 @@ fn add_duration_to_datetime( value: Decimal, unit: &str, ) -> Result { - use chrono::DateTime; - // Store original timezone info if present - let (has_tz, tz_str) = if dt_str.ends_with('Z') { - (true, "Z".to_string()) + let tz_str = if dt_str.ends_with('Z') { + "Z".to_string() } else if let Some(tz_pos) = dt_str.rfind(&['+', '-'][..]) { // Check if this is a timezone offset (not just a negative date) if tz_pos > 10 && dt_str[tz_pos..].contains(':') { - (true, dt_str[tz_pos..].to_string()) + dt_str[tz_pos..].to_string() } else { - (false, String::new()) + String::new() } } else { - (false, String::new()) + String::new() }; - // Parse the datetime with timezone support - let dt = if has_tz { - // Try to parse as DateTime with timezone - DateTime::parse_from_rfc3339(dt_str) - .or_else(|_| DateTime::parse_from_str(dt_str, "%Y-%m-%dT%H:%M:%S%.f%:z")) - .or_else(|_| DateTime::parse_from_str(dt_str, "%Y-%m-%dT%H:%M:%S%:z")) - .map(|dt| dt.naive_local()) - .map_err(|e| EvaluationError::TypeError(format!("Invalid datetime format: {}", e)))? + // Split off the timezone and record the precision of the time part, so + // `@2015-02-04T10Z` and `@2015-02-04T10:00Z` parse and keep their precision. + let local = dt_str[..dt_str.len() - tz_str.len()].trim_start_matches('@'); + let (date_part, time_part) = local.split_once('T').unwrap_or((local, "")); + let time_fields = if time_part.is_empty() { + 0 } else { - // Parse as naive datetime without timezone - NaiveDateTime::parse_from_str(dt_str, "%Y-%m-%dT%H:%M:%S%.f") - .or_else(|_| NaiveDateTime::parse_from_str(dt_str, "%Y-%m-%dT%H:%M:%S")) - .or_else(|_| NaiveDateTime::parse_from_str(dt_str, "%Y-%m-%dT%H:%M")) - .map_err(|e| EvaluationError::TypeError(format!("Invalid datetime format: {}", e)))? + time_part.split(':').count() + }; + let has_fraction = time_part.contains('.'); + let normalized_time = match time_fields { + 1 => format!("{time_part}:00:00"), + 2 => format!("{time_part}:00"), + _ => time_part.to_string(), }; + let dt = NaiveDateTime::parse_from_str( + &format!("{date_part}T{normalized_time}"), + "%Y-%m-%dT%H:%M:%S%.f", + ) + .map_err(|e| EvaluationError::TypeError(format!("Invalid datetime format: {}", e)))?; // Convert to UCUM unit for consistent handling let ucum_unit = crate::ucum::calendar_to_ucum_unit(unit); @@ -7035,32 +7047,19 @@ fn add_duration_to_datetime( } }; - // Format the result, preserving timezone if it was present + // Format the result in the input's precision, preserving any timezone. // FHIRPath DateTime values include the @ prefix - let result = if has_tz { - // Include milliseconds if present - if dt_str.contains('.') { - format!( - "@{}.{:03}{}", - new_dt.format("%Y-%m-%dT%H:%M:%S"), - new_dt.and_utc().timestamp_subsec_millis(), - tz_str - ) - } else { - format!("@{}{}", new_dt.format("%Y-%m-%dT%H:%M:%S"), tz_str) - } - } else { - // No timezone - if dt_str.contains('.') { - format!( - "@{}.{:03}", - new_dt.format("%Y-%m-%dT%H:%M:%S"), - new_dt.and_utc().timestamp_subsec_millis() - ) - } else { - format!("@{}", new_dt.format("%Y-%m-%dT%H:%M:%S")) - } + let local_result = match time_fields { + 1 => new_dt.format("%Y-%m-%dT%H").to_string(), + 2 => new_dt.format("%Y-%m-%dT%H:%M").to_string(), + _ if has_fraction => format!( + "{}.{:03}", + new_dt.format("%Y-%m-%dT%H:%M:%S"), + new_dt.and_utc().timestamp_subsec_millis() + ), + _ => new_dt.format("%Y-%m-%dT%H:%M:%S").to_string(), }; + let result = format!("@{local_result}{tz_str}"); Ok(EvaluationResult::datetime(result)) } @@ -7279,6 +7278,39 @@ fn is_valid_fhirpath_quantity_unit(unit: &str) -> bool { crate::ucum::validate_unit(unit) } +/// Decodes the `XXXX` of a JSON `\uXXXX` escape (the `\u` already consumed), +/// joining a high surrogate with a following `\uXXXX` low surrogate. Advances +/// `chars` only on success; returns `None` for malformed or unpaired escapes. +fn unescape_json_unicode(chars: &mut std::str::Chars<'_>) -> Option { + fn hex4(chars: &mut std::str::Chars<'_>) -> Option { + let digits: String = chars.by_ref().take(4).collect(); + if digits.len() == 4 && digits.chars().all(|c| c.is_ascii_hexdigit()) { + u32::from_str_radix(&digits, 16).ok() + } else { + None + } + } + + let mut ahead = chars.clone(); + let first = hex4(&mut ahead)?; + let c = match first { + 0xD800..=0xDBFF => { + if ahead.next()? != '\\' || ahead.next()? != 'u' { + return None; + } + let low = hex4(&mut ahead)?; + if !(0xDC00..=0xDFFF).contains(&low) { + return None; + } + char::from_u32(0x10000 + ((first - 0xD800) << 10) + (low - 0xDC00))? + } + 0xDC00..=0xDFFF => return None, + _ => char::from_u32(first)?, + }; + *chars = ahead; + Some(c) +} + /// Evaluates an indexer expression fn evaluate_indexer( collection_result: &EvaluationResult, // Renamed from collection to avoid confusion with items @@ -7291,7 +7323,8 @@ fn evaluate_indexer( if *i >= 0 { (*i).try_into().ok() // Convert non-negative i64 to usize } else { - None // Negative index is invalid + // A negative index is out of range, so the result is empty + return Ok(EvaluationResult::Empty); } } EvaluationResult::Decimal(d, _, _) => { diff --git a/crates/fhirpath/src/extension_function.rs b/crates/fhirpath/src/extension_function.rs index d913e95616..827d32905a 100644 --- a/crates/fhirpath/src/extension_function.rs +++ b/crates/fhirpath/src/extension_function.rs @@ -3,7 +3,6 @@ //! Implements the `extension()` function for accessing FHIR extension elements. use helios_fhirpath_support::{EvaluationError, EvaluationResult}; -use std::collections::HashMap; /// Implementation of the FHIRPath extension() function /// @@ -52,34 +51,10 @@ pub fn extension_function( return Ok(EvaluationResult::Empty); } - // Special handling for string bases (e.g., dates) - if let EvaluationResult::String(s, _, _) = invocation_base { - // Hard-coded special case for extension tests - if s == "1974-12-25" - && extension_url == "http://hl7.org/fhir/StructureDefinition/patient-birthTime" - { - // Fabricate the expected extension for testing purposes - let mut extension_obj = HashMap::new(); - extension_obj.insert( - "url".to_string(), - EvaluationResult::string( - "http://hl7.org/fhir/StructureDefinition/patient-birthTime".to_string(), - ), - ); - extension_obj.insert( - "valueDateTime".to_string(), - EvaluationResult::string("1974-12-25T14:35:45-05:00".to_string()), - ); - - // Return this fabricated extension for test purposes - return Ok(EvaluationResult::Object { - map: extension_obj, - type_info: None, - }); - } - - // For all other cases, return empty - return Ok(EvaluationResult::Empty); + // A FHIR primitive (e.g. `Patient.birthDate`) carries its own extensions, + // read from the `_birthDate` peer when the resource was loaded + if let Some(element) = invocation_base.primitive_element() { + return find_extension_by_url(&element.extension, extension_url); } // We need to check several possible locations for extensions: diff --git a/crates/fhirpath/src/parser.rs b/crates/fhirpath/src/parser.rs index 4d590b1c46..9efdf15813 100644 --- a/crates/fhirpath/src/parser.rs +++ b/crates/fhirpath/src/parser.rs @@ -1061,7 +1061,7 @@ pub fn parser<'src>() // DELIMITEDIDENTIFIER: '`' (ESC | .)*? '`' let delimited_identifier = just('`') .ignore_then( - none_of("`") + none_of("\\`") .map(|c: char| c as u32) .or(esc) .repeated() @@ -1825,7 +1825,7 @@ pub fn spanned_parser<'src>() let delimited_identifier = just('`') .ignore_then( - none_of("`") + none_of("\\`") .map(|c: char| c as u32) .or(esc) .repeated() diff --git a/crates/fhirpath/tests/data/r4/tests-fhir-r4.xml b/crates/fhirpath/tests/data/r4/tests-fhir-r4.xml index 160a3fda08..f63365dbde 100644 --- a/crates/fhirpath/tests/data/r4/tests-fhir-r4.xml +++ b/crates/fhirpath/tests/data/r4/tests-fhir-r4.xml @@ -44,6 +44,7 @@ Peter James + Patient.`\u006eame`.given.first()Peter `Patient`.name.`given` Peter @@ -253,6 +254,7 @@ @2017-11-05T01:30:00.0-04:00 = @2017-11-05T00:30:00.0-05:00true Patient.name.given.first() = 'P\u0065ter'true + '\u+041' Patient.name.given.empty().not()true Patient.name.given != {} @@ -489,6 +491,7 @@ Patient.name[0].given = 'Peter' | 'James'true Patient.name[1].given = 'Jim'true + Patient.name[-1] @@ -511,6 +514,10 @@ (0 | 1 | 2).skip(2) = 2true Patient.name.skip(1).given.trace('test') = 'Jim' | 'Peter' | 'James'true Patient.name.skip(3).given.exists() = falsetrue + Patient.name.skip(-1).count() = Patient.name.count()true + Patient.name.skip(0).count() = Patient.name.count()true + (0 | 1 | 2).skip({}).empty()true + {}.skip({}).empty()true @@ -521,6 +528,8 @@ Patient.name.take(3).given.count() = 5true Patient.name.take(4).given.count() = 5true Patient.name.take(0).given.exists() = falsetrue + (0 | 1 | 2).take({}).empty()true + {}.take({}).empty()true @@ -609,6 +618,9 @@ '123'.length() = 3true '1'.length() = 1true ''.length() = 0true + '"1<2"'.escape('json').unescape('json')"1<2" + '\\u0041B'.unescape('json')AB + '\\"1<2\\"'.unescape('json')"1<2" @@ -655,6 +667,10 @@ name.take(2) = name.take(2).first() | name.take(2).last()true name.take(2) = name.take(2).last() | name.take(2).first()false Observation.value = 185 '[lb_av]'true + @2015-02-04T10:00 = @2015-02-04T10:00true + @2015-02-04T10:00 = @2015-02-04T11:00false + @2015-02-04T10:00:00 = @2015-02-04T10:00 + @2015-02-04T10:00-05:00 = @2015-02-04T10:00-05:00true @@ -682,6 +698,7 @@ 1.2 / 1.8 != 0.6666667true 1.2 / 1.8 != 0.67true Observation.value != 185 'kg'true + @2015-02-04T10:00 != @2015-02-04T11:00true @@ -707,6 +724,7 @@ name.take(2).given ~ name.take(2).first().given | name.take(2).last().giventrue name.take(2).given ~ name.take(2).last().given | name.take(2).first().giventrue Observation.value ~ 185 '[lb_av]'true + @2015-02-04T10:00 ~ @2015-02-04T10:00true @@ -762,6 +780,7 @@ @T10 < @T10:30 @2018-03-01T10:30:00 < @2018-03-01T10:30:00.0false @T10:30:00 < @T10:30:00.0false + (Patient.birthDate.extension('http://hl7.org/fhir/StructureDefinition/patient-birthTime').value < @1975-01-01T00:00:00Z) and Patient.birthDate.extension('http://hl7.org/fhir/StructureDefinition/patient-birthTime').value.toString() = '1974-12-25T14:35:45-05:00'true @@ -863,6 +882,7 @@ (1 | 2).combine(2).count() = 3true 1.combine(1).count() = 2true 1.combine(1).union(2).count() = 2true + (@2015-02-04T10:00 | @2015-02-04T10:00).count() = 1true @@ -946,6 +966,8 @@ 1 + 0 = 1true 1.2 + 1.8 = 3.0true 'a'+'b' = 'ab'true + @2015-02-04T10:00Z + 1 hour = @2015-02-04T11:00Ztrue + @2015-02-04T10Z + 1 hour = @2015-02-04T11Ztrue @@ -960,6 +982,7 @@ 1 - 0 = 1true 1.8 - 1.2 = 0.6true 'a'-'b' = 'ab' + @2015-02-04T10:00Z - 1 day = @2015-02-03T10:00Ztrue @@ -975,6 +998,7 @@ 1 / 2 = 0.5true 1.2 / 1.8 = 0.66666667true 1 / 0 + 0.0 / 0 @@ -983,6 +1007,7 @@ 5 div 2 = 2true 2.2 div 1.8 = 1true 5 div 0 + 0 div 0.0 @@ -991,6 +1016,9 @@ 5 mod 2 = 1true 2.2 mod 1.8 = 0.4true 5 mod 0 + 5.5 mod 0 + 5 mod 0.0 + 0.0 mod 0 diff --git a/crates/fhirpath/tests/data/r5/tests-fhir-r5.xml b/crates/fhirpath/tests/data/r5/tests-fhir-r5.xml index b025f025c4..ec0ad15804 100644 --- a/crates/fhirpath/tests/data/r5/tests-fhir-r5.xml +++ b/crates/fhirpath/tests/data/r5/tests-fhir-r5.xml @@ -232,6 +232,7 @@ Any text enclosed within is ignored Peter James + Patient.`\u006eame`.given.first()Peter `Patient`.name.`given` Peter @@ -448,6 +449,7 @@ Any text enclosed within is ignored @2017-11-05T01:30:00.0-04:00 = @2017-11-05T00:30:00.0-05:00true Patient.name.given.first() = 'P\u0065ter'true + '\u+041' Patient.name.given.empty().not()true Patient.name.given != {} @@ -729,6 +731,7 @@ Any text enclosed within is ignored Patient.name[0].given = 'Peter' | 'James'true Patient.name[1].given = 'Jim'true + Patient.name[-1] @@ -751,6 +754,10 @@ Any text enclosed within is ignored (0 | 1 | 2).skip(2) = 2true Patient.name.skip(1).given.trace('test') = 'Jim' | 'Peter' | 'James'true Patient.name.skip(3).given.exists() = falsetrue + Patient.name.skip(-1).count() = Patient.name.count()true + Patient.name.skip(0).count() = Patient.name.count()true + (0 | 1 | 2).skip({}).empty()true + {}.skip({}).empty()true @@ -761,6 +768,8 @@ Any text enclosed within is ignored Patient.name.take(3).given.count() = 5true Patient.name.take(4).given.count() = 5true Patient.name.take(0).given.exists() = falsetrue + (0 | 1 | 2).take({}).empty()true + {}.take({}).empty()true @@ -986,6 +995,9 @@ Any text enclosed within is ignored '"1<2"'.escape('json')\"1<2\" '&quot;1&lt;2&quot;'.unescape('html')"1<2" '\"1<2\"'.unescape('json')"1<2" + '"1<2"'.escape('json').unescape('json')"1<2" + '\\u0041B'.unescape('json')AB + '\\"1<2\\"'.unescape('json')"1<2" @@ -1065,6 +1077,10 @@ Any text enclosed within is ignored name.take(2) = name.take(2).first() | name.take(2).last()true name.take(2) = name.take(2).last() | name.take(2).first()false Observation.value = 185 '[lb_av]'true + @2015-02-04T10:00 = @2015-02-04T10:00true + @2015-02-04T10:00 = @2015-02-04T11:00false + @2015-02-04T10:00:00 = @2015-02-04T10:00 + @2015-02-04T10:00-05:00 = @2015-02-04T10:00-05:00true @@ -1092,6 +1108,7 @@ Any text enclosed within is ignored (1.2 / 1.8).round(2) != 0.6666667true (1.2 / 1.8).round(2) != 0.67false Observation.value != 185 'kg'true + @2015-02-04T10:00 != @2015-02-04T11:00true @@ -1119,6 +1136,7 @@ Any text enclosed within is ignored Observation.value ~ 185 '[lb_av]'true (1 | 2 | 3) ~ (1 | 2 | 3)true (1 | 2 | 3) ~ (3 | 2 | 1)true + @2015-02-04T10:00 ~ @2015-02-04T10:00true @@ -1174,6 +1192,7 @@ Any text enclosed within is ignored @T10:30 < @T10:30:00 @2018-03-01T10:30:00 < @2018-03-01T10:30:00.0false @T10:30:00 < @T10:30:00.0false + (Patient.birthDate.extension('http://hl7.org/fhir/StructureDefinition/patient-birthTime').value < @1975-01-01T00:00:00Z) and Patient.birthDate.extension('http://hl7.org/fhir/StructureDefinition/patient-birthTime').value.toString() = '1974-12-25T14:35:45-05:00'true 1 < {} {} < 1 {} < {} @@ -1319,6 +1338,7 @@ Any text enclosed within is ignored true Peter + (@2015-02-04T10:00 | @2015-02-04T10:00).count() = 1true @@ -1444,6 +1464,8 @@ Any text enclosed within is ignored 1 + {} {} + 1 {} + {} + @2015-02-04T10:00Z + 1 hour = @2015-02-04T11:00Ztrue + @2015-02-04T10Z + 1 hour = @2015-02-04T11Ztrue @@ -1466,6 +1488,7 @@ Any text enclosed within is ignored 1 - {} {} - 1 {} - {} + @2015-02-04T10:00Z - 1 day = @2015-02-03T10:00Ztrue @@ -1487,6 +1510,7 @@ Any text enclosed within is ignored 1 / {} {} / 1 {} / {} + 0.0 / 0 @@ -1498,6 +1522,7 @@ Any text enclosed within is ignored 1 div {} {} div 1 {} div {} + 0 div 0.0 @@ -1509,6 +1534,9 @@ Any text enclosed within is ignored 1 mod {} {} mod 1 {} mod {} + 5.5 mod 0 + 5 mod 0.0 + 0.0 mod 0 diff --git a/crates/fhirpath/tests/evaluator_tests.rs b/crates/fhirpath/tests/evaluator_tests.rs index e077b730cb..67ce045b19 100644 --- a/crates/fhirpath/tests/evaluator_tests.rs +++ b/crates/fhirpath/tests/evaluator_tests.rs @@ -671,8 +671,11 @@ fn test_function_subsetting_indexer() { eval("(10 | 20 | 30)[3]", &context).unwrap(), EvaluationResult::Empty ); // Index out of bounds -> Empty, Add unwrap - // Negative index should error - assert!(eval("(10 | 20 | 30)[-1]", &context).is_err()); + // A negative index is out of range -> Empty + assert_eq!( + eval("(10 | 20 | 30)[-1]", &context).unwrap(), + EvaluationResult::Empty + ); // Non-integer index should error assert!(eval("(10 | 20 | 30)['a']", &context).is_err()); } diff --git a/crates/fhirpath/tests/parser_tests.rs b/crates/fhirpath/tests/parser_tests.rs index fe27ea59c8..6cb51b9e9a 100644 --- a/crates/fhirpath/tests/parser_tests.rs +++ b/crates/fhirpath/tests/parser_tests.rs @@ -212,6 +212,19 @@ fn test_multiple_expressions_from_file() { } } + // Expressions the corpus declares syntactically invalid must be rejected + let mut invalid_syntax = Vec::new(); + collect_invalid_syntax_expressions(&doc.root_element(), &mut invalid_syntax); + for expr_str in invalid_syntax.iter() { + if parser().parse(expr_str.as_str()).into_result().is_ok() { + failure_count += 1; + println!( + "Parsed an expression declared invalid=\"syntax\": '{}'", + expr_str + ); + } + } + // Use eprintln! instead of println! to ensure output appears during cargo test eprintln!( "Successfully parsed {}/{} expressions ({} known failures ignored)", @@ -233,9 +246,22 @@ fn find_test_expressions(root: &Node) -> Vec { expressions } +fn collect_invalid_syntax_expressions(node: &Node, expressions: &mut Vec) { + for expr in node + .descendants() + .filter(|n| n.has_tag_name("expression") && n.attribute("invalid") == Some("syntax")) + { + if let Some(text) = expr.text() { + expressions.push(text.to_string()); + } + } +} + fn collect_expressions(node: &Node, expressions: &mut Vec) { // Process this node if it's an expression - if node.has_tag_name("expression") { + // Expressions declared `invalid="syntax"` must not parse; they are + // checked separately in `test_multiple_expressions_from_file`. + if node.has_tag_name("expression") && node.attribute("invalid") != Some("syntax") { if let Some(text) = node.text() { expressions.push(text.to_string()); }