From 45ce2a6f22b6b4942ac619f8814480fb77d588ba Mon Sep 17 00:00:00 2001 From: angela-helios Date: Sat, 1 Aug 2026 01:52:06 -0400 Subject: [PATCH] fix(rest): execute search-style GET bundle entries as searches Bundle GET entries were unconditionally parsed as Type/id reads, so a search entry like 'Patient?name=x' became a read of resource type 'Patient?name=x' -- a 404 in batch bundles and a 400 rejecting the whole bundle in transactions -- even though the spec allows any read OR search URL in a GET entry. The URL parser now strips the query string before extracting type/id, and a GET entry addressing a type ('Patient?name=x', or bare 'Patient') runs through the same search pipeline as the HTTP search endpoint (terminology expansion, list/chain resolution, includes), embedding the searchset Bundle as the entry resource. Instance reads ('Patient/123') are untouched. In transactions the spec orders GETs after all writes, and a search cannot run inside the storage transaction, so search entries execute against the just-committed state -- which also makes them see the bundle's own writes. Their queries are validated up front so a malformed search still rejects the whole bundle before anything executes; a post-commit execution failure surfaces as that entry's own error outcome rather than a misleading whole-bundle failure for writes that did commit. Closes #478 --- crates/rest/src/handlers/batch.rs | 147 +++++++++++++++++++++-- crates/rest/src/handlers/search.rs | 37 ++++-- crates/rest/tests/batch_conformance.rs | 154 +++++++++++++++++++++++++ 3 files changed, 322 insertions(+), 16 deletions(-) diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index 7f60b86d0..d92dee3d9 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -15,7 +15,8 @@ use helios_audit::{AuditAction, AuditCorrelation, AuditEventBuilder}; use helios_auth::{FhirOperation, Principal, SmartScopePolicy}; use helios_fhir::FhirVersion; use helios_persistence::core::{ - BundleEntry, BundleEntryResult, BundleMethod, BundleProvider, ResourceStorage, + BundleEntry, BundleEntryResult, BundleMethod, BundleProvider, IncludeProvider, ResourceStorage, + RevincludeProvider, SearchProvider, }; use helios_persistence::error::{StorageError, TransactionError}; use serde_json::Value; @@ -57,7 +58,13 @@ pub async fn batch_handler( request: Request, ) -> RestResult where - S: ResourceStorage + BundleProvider + Send + Sync, + S: ResourceStorage + + SearchProvider + + IncludeProvider + + RevincludeProvider + + BundleProvider + + Send + + Sync, { // Extract the Principal from request extensions (set by auth middleware). // If present, per-entry scope checks will be enforced. @@ -141,7 +148,7 @@ async fn process_batch( principal: Option<&Principal>, ) -> RestResult where - S: ResourceStorage + Send + Sync, + S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, { debug!( tenant = %tenant.tenant_id(), @@ -204,7 +211,13 @@ async fn process_transaction( principal: Option<&Principal>, ) -> RestResult where - S: ResourceStorage + BundleProvider + Send + Sync, + S: ResourceStorage + + SearchProvider + + IncludeProvider + + RevincludeProvider + + BundleProvider + + Send + + Sync, { debug!( tenant = %tenant.tenant_id(), @@ -254,6 +267,35 @@ where } } + // GET search entries (`Patient?name=x`, bare `Patient`) cannot run inside + // the storage transaction; the spec orders GETs after all writes, so they + // execute against the just-committed state instead (#478). Their queries + // are still validated up front, where a malformed search can reject the + // whole bundle before anything executes. + let (search_entries, remaining): (Vec<_>, Vec<_>) = indexed_entries.into_iter().partition( + |(_, entry, _): &(usize, BundleEntry, Option)| { + matches!(entry.method, BundleMethod::Get) + && parse_search_entry_url(&entry.url).is_some() + }, + ); + let mut indexed_entries = remaining; + for (index, entry, _) in &search_entries { + let (search_type, pairs) = + parse_search_entry_url(&entry.url).expect("partitioned on is_some"); + let reg = state.storage().search_param_registry(tenant.context()); + let registry = reg.read(); + crate::extractors::build_search_query_from_pairs(&search_type, &pairs, ®istry).map_err( + |e| RestError::BadRequest { + message: format!( + "Entry {}: invalid search '{}': {}", + index, + entry.url, + e.client_response().2 + ), + }, + )?; + } + // Write-path validation: transactions are atomic, so any invalid write // entry rejects the whole bundle before anything executes. for (index, entry, _) in &indexed_entries { @@ -311,6 +353,33 @@ where } } + // GET searches run against the committed state (see above). A + // failure here cannot roll the transaction back, so it surfaces + // as that entry's own error outcome rather than a misleading + // whole-bundle failure for writes that did commit. + let mut search_results: Vec<(usize, BundleEntry, BundleEntryResult)> = + Vec::with_capacity(search_entries.len()); + for (index, entry, _) in &search_entries { + let (search_type, pairs) = + parse_search_entry_url(&entry.url).expect("partitioned on is_some"); + let result = match crate::handlers::search::execute_search_bundle( + state, + &tenant, + &search_type, + pairs, + false, + ) + .await + { + Ok(bundle) => searchset_result(bundle), + Err(e) => { + let (status, _, details) = e.client_response(); + create_error_result(status.as_u16(), &details) + } + }; + search_results.push((*index, entry.clone(), result)); + } + // Reorder results back to original entry order let mut ordered_results: Vec<(usize, &BundleEntry, &BundleEntryResult)> = indexed_entries @@ -318,6 +387,9 @@ where .zip(bundle_result.entries.iter()) .map(|((orig_idx, entry, _), result)| (*orig_idx, entry, result)) .collect(); + for (orig_idx, entry, result) in &search_results { + ordered_results.push((*orig_idx, entry, result)); + } ordered_results.sort_by_key(|(idx, _, _)| *idx); for (orig_idx, entry, result) in &ordered_results { @@ -360,7 +432,7 @@ where let (_, _, rollback_reason) = transaction_error_response_parts(&e); let rollback_result = create_error_result(500, &format!("Transaction rolled back: {rollback_reason}")); - for (orig_idx, entry, _) in &indexed_entries { + for (orig_idx, entry, _) in indexed_entries.iter().chain(&search_entries) { let correlation_details = EntryAuditCorrelation::from_bundle(&correlation, *orig_idx); emit_transaction_entry_audit( @@ -388,7 +460,7 @@ async fn process_batch_entry( principal: Option<&Principal>, ) -> BundleEntryResult where - S: ResourceStorage + Send + Sync, + S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, { let request = match entry.get("request") { Some(r) => r, @@ -430,7 +502,26 @@ where match method { "GET" => { - // Read operation + // A GET entry is either a search (`Patient?name=x`, bare + // `Patient`) or an instance read (`Patient/123`), per the spec's + // "read or search" wording for bundle GETs (#478). + if let Some((search_type, pairs)) = parse_search_entry_url(url) { + return match crate::handlers::search::execute_search_bundle( + state, + tenant, + &search_type, + pairs, + false, + ) + .await + { + Ok(bundle) => searchset_result(bundle), + Err(e) => { + let (status, _, details) = e.client_response(); + create_error_result(status.as_u16(), &details) + } + }; + } match state .storage() .read(tenant.context(), &resource_type, &id) @@ -758,7 +849,10 @@ fn extract_outcome_description(outcome: Option<&Value>) -> Option { /// Parses a request URL to extract resource type and optional ID. fn parse_request_url(url: &str) -> Result<(String, String), String> { - let parts: Vec<&str> = url.trim_start_matches('/').split('/').collect(); + // A query string is not part of the type/id path (`Patient?name=x` is a + // search on Patient, not a read of a resource named `Patient?name=x`). + let path = url.split('?').next().unwrap_or(url); + let parts: Vec<&str> = path.trim_start_matches('/').split('/').collect(); match parts.len() { 0 => Err("Empty URL".to_string()), @@ -771,6 +865,43 @@ fn parse_request_url(url: &str) -> Result<(String, String), String> { } } +/// Interprets a bundle-entry GET url as a type-level search, if it is one. +/// +/// Per the FHIR spec, a GET entry may carry any read OR search URL +/// (`Patient?name=x`, or bare `Patient` for an unfiltered type search). +/// Returns the resource type and the parsed query pairs, or `None` when the +/// url addresses a specific instance (`Patient/123`) and should be a read. +fn parse_search_entry_url(url: &str) -> Option<(String, Vec<(String, String)>)> { + let (path, query) = match url.split_once('?') { + Some((p, q)) => (p, Some(q)), + None => (url, None), + }; + let parts: Vec<&str> = path + .trim_start_matches('/') + .split('/') + .filter(|s| !s.is_empty()) + .collect(); + match parts.as_slice() { + [resource_type] => Some(( + resource_type.to_string(), + crate::extractors::query_pairs::parse_query_pairs(query), + )), + _ => None, + } +} + +/// Builds the entry result embedding a searchset Bundle (bundle GET search). +fn searchset_result(bundle: Value) -> BundleEntryResult { + BundleEntryResult { + status: 200, + location: None, + etag: None, + last_modified: None, + resource: Some(bundle), + outcome: None, + } +} + /// Creates an error BundleEntryResult. /// Flatten an enforce-mode validation failure into a per-entry message /// (batch entry outcomes are message-based). diff --git a/crates/rest/src/handlers/search.rs b/crates/rest/src/handlers/search.rs index b3d156a62..56e2b2dd3 100644 --- a/crates/rest/src/handlers/search.rs +++ b/crates/rest/src/handlers/search.rs @@ -156,6 +156,29 @@ async fn execute_search( format: FhirFormat, strict: bool, ) -> RestResult +where + S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, +{ + let bundle_json = execute_search_bundle(state, &tenant, resource_type, pairs, strict).await?; + format_resource_response(StatusCode::OK, HeaderMap::new(), &bundle_json, format).map_err(|_| { + RestError::InternalError { + message: "Failed to serialize response".to_string(), + } + }) +} + +/// Executes a type-level search and returns the searchset Bundle as JSON. +/// +/// The HTTP search handlers wrap this in content negotiation; bundle +/// processing (`GET [type]?params` entries in batch/transaction Bundles, +/// #478) embeds the returned Bundle as an entry resource. +pub(crate) async fn execute_search_bundle( + state: &AppState, + tenant: &TenantExtractor, + resource_type: &str, + pairs: Vec<(String, String)>, + strict: bool, +) -> RestResult where S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, { @@ -390,14 +413,12 @@ where // Get FHIR version from config for subsetting let fhir_version = state.config().default_fhir_version; - let bundle_json = - bundle_to_json_with_subsetting(bundle, summary_mode, elements.as_deref(), fhir_version); - - format_resource_response(StatusCode::OK, HeaderMap::new(), &bundle_json, format).map_err(|_| { - RestError::InternalError { - message: "Failed to serialize response".to_string(), - } - }) + Ok(bundle_to_json_with_subsetting( + bundle, + summary_mode, + elements.as_deref(), + fhir_version, + )) } /// Executes a system-level search across all resource types. diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index b40964311..c8084ed1f 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -809,3 +809,157 @@ mod transaction_errors { response.assert_status(StatusCode::BAD_REQUEST); } } + +// ============================================================================= +// GET Search Entry Tests (#478) +// ============================================================================= + +mod search_entries { + use super::*; + + #[tokio::test] + async fn test_batch_get_search_entry_returns_searchset() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + seed_patient(&backend, "p2", "Smith").await; + + let bundle = json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "GET", "url": "Patient?family=Nguyen" } + }] + }); + + let body = post_batch(&server, bundle).await; + let entry = &body["entry"][0]; + + assert_eq!(entry["response"]["status"].as_str().unwrap(), "200 OK"); + let searchset = &entry["resource"]; + assert_eq!(searchset["resourceType"].as_str().unwrap(), "Bundle"); + assert_eq!(searchset["type"].as_str().unwrap(), "searchset"); + assert_eq!(searchset["entry"].as_array().unwrap().len(), 1); + assert_eq!( + searchset["entry"][0]["resource"]["name"][0]["family"] + .as_str() + .unwrap(), + "Nguyen" + ); + } + + #[tokio::test] + async fn test_batch_get_bare_type_is_a_search() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + seed_patient(&backend, "p2", "Smith").await; + + let bundle = json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "GET", "url": "Patient" } + }] + }); + + let body = post_batch(&server, bundle).await; + let searchset = &body["entry"][0]["resource"]; + + assert_eq!(searchset["type"].as_str().unwrap(), "searchset"); + assert_eq!(searchset["entry"].as_array().unwrap().len(), 2); + } + + #[tokio::test] + async fn test_batch_mixes_search_and_read_entries() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + + let bundle = json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { "request": { "method": "GET", "url": "Patient/p1" } }, + { "request": { "method": "GET", "url": "Patient?family=Nguyen" } } + ] + }); + + let body = post_batch(&server, bundle).await; + + let read = &body["entry"][0]; + assert_eq!(read["response"]["status"].as_str().unwrap(), "200 OK"); + assert_eq!( + read["resource"]["resourceType"].as_str().unwrap(), + "Patient" + ); + + let search = &body["entry"][1]; + assert_eq!(search["response"]["status"].as_str().unwrap(), "200 OK"); + assert_eq!(search["resource"]["type"].as_str().unwrap(), "searchset"); + } + + #[tokio::test] + async fn test_transaction_get_search_sees_the_bundles_own_writes() { + let (server, _backend) = create_test_server().await; + + let bundle = json!({ + "resourceType": "Bundle", + "type": "transaction", + "entry": [ + { + "resource": { + "resourceType": "Patient", + "name": [{"family": "Tran"}] + }, + "request": { "method": "POST", "url": "Patient" } + }, + { "request": { "method": "GET", "url": "Patient?family=Tran" } } + ] + }); + + let body = post_batch(&server, bundle).await; + assert_eq!(body["type"].as_str().unwrap(), "transaction-response"); + + let created = &body["entry"][0]; + assert_eq!( + created["response"]["status"].as_str().unwrap(), + "201 Created" + ); + + let search = &body["entry"][1]; + assert_eq!(search["response"]["status"].as_str().unwrap(), "200 OK"); + let searchset = &search["resource"]; + assert_eq!(searchset["type"].as_str().unwrap(), "searchset"); + assert_eq!( + searchset["entry"].as_array().unwrap().len(), + 1, + "the search runs after the writes and must see the created patient" + ); + assert_eq!( + searchset["entry"][0]["resource"]["name"][0]["family"] + .as_str() + .unwrap(), + "Tran" + ); + } + + #[tokio::test] + async fn test_transaction_get_by_id_still_reads_in_transaction() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + + let bundle = json!({ + "resourceType": "Bundle", + "type": "transaction", + "entry": [{ + "request": { "method": "GET", "url": "Patient/p1" } + }] + }); + + let body = post_batch(&server, bundle).await; + let entry = &body["entry"][0]; + assert_eq!(entry["response"]["status"].as_str().unwrap(), "200 OK"); + assert_eq!( + entry["resource"]["resourceType"].as_str().unwrap(), + "Patient" + ); + } +}