diff --git a/Cargo.lock b/Cargo.lock index 3e21ba974..9bc9e5e1f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3584,6 +3584,7 @@ dependencies = [ "mime", "p256 0.13.2", "p384", + "parking_lot", "rand 0.8.6", "regex", "reqwest", diff --git a/crates/rest/Cargo.toml b/crates/rest/Cargo.toml index 1bdf82695..f8ff40032 100644 --- a/crates/rest/Cargo.toml +++ b/crates/rest/Cargo.toml @@ -133,6 +133,12 @@ helios-observability = { path = "../observability" } # Key generation for the JWE round-trip tests rand = "0.8" +# `SearchProvider::search_param_registry` returns a `parking_lot::RwLock`, and +# the batch unit tests' mock storage has to satisfy that trait since #478 +# widened `process_batch`'s bound. Test-only: the crate's own code reaches the +# registry through `helios-persistence` and never names the lock type. +parking_lot = "0.12" + # Temp files tempfile = "3" diff --git a/crates/rest/README.md b/crates/rest/README.md index 856bd5fbf..93c09bb1e 100644 --- a/crates/rest/README.md +++ b/crates/rest/README.md @@ -508,6 +508,39 @@ version — so a lowercase `"post"` is invalid instance data and is refused with `400`, not silently accepted. `GET`, `POST`, `PUT` and `DELETE` dispatch; `PATCH` and `HEAD` are refused as described under Current Limitations. +### Per-entry outcomes + +A failed entry's `response.outcome` carries the **same issue code the equivalent +single-resource request would return**, because both are rendered by one mapping +(`RestError::client_outcome`). A batch `GET Patient/missing` and +`GET [base]/Patient/missing` produce byte-identical OperationOutcomes. + +| Entry failure | Status | `issue.code` | +|---|---|---| +| `request` absent | 400 | `required` | +| `request.method` absent | 400 | `required` | +| `request.method` not an `http-verb` code | 400 | `value` | +| `request.url` absent | 400 | `required` | +| `request.url` names nothing | 400 | `value` | +| `resource` absent on `POST`/`PUT` | 400 | `invalid` | +| `PUT`/`DELETE` URL names no instance | 400 | `value` | +| conditional criteria in the URL | 400 | `not-supported` | +| insufficient scope | 403 | `forbidden` | +| target not found | 404 | `not-found` | +| `HEAD` | 405 | `not-supported` | +| `ifMatch` precondition failed | 412 | `conflict` | +| write validation failed | 422 | the validator's own issues | +| `PATCH` | 501 | `not-supported` | +| storage error | per class | `deleted`, `conflict`, `multiple-matches`, `transient`, `timeout`, `exception`, … | + +An entry that fails enforce-mode write validation carries the validator's **full +multi-issue outcome** — per-issue `code`, `severity` and `expression` (the +FHIRPath location of the failing element) — exactly as `POST [base]/[type]` does. + +`required` and `value` are both children of `invalid` in the `issue-type` +hierarchy, and `invalid` is used where the distinction between an absent element +and an unusable value does not apply. + ### Conditional Operations in Bundles - `ifMatch` — **supported.** ETag for optimistic locking on `PUT` **and `DELETE`** @@ -522,7 +555,8 @@ and `HEAD` are refused as described under Current Limitations. 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 `batch`, such an entry is refused per-entry with `400 not-supported`; + nothing is written. Both arms agree on the status **and** the issue code. - 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 @@ -537,10 +571,12 @@ endpoints and **not** for bundle entries; reconciling the two is #511. The following FHIR transaction features are not yet implemented: - **Conditional interactions in bundle entries** - `[type]?[criteria]` URLs are refused rather than resolved (#511) - **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 +- **PATCH method** - PATCH operations in bundles return `501 not-supported`, in both `batch` (per entry) and `transaction` (whole bundle). Send the patch to the instance endpoint instead +- **HEAD entries** - refused with `405 not-supported`. `HEAD` is a legal `http-verb` code and is served on the instance-read route, but not inside a Bundle - **Prefer header** - `return=minimal` and `return=OperationOutcome` not honored - **Duplicate detection** - Same resource appearing twice in a transaction is not detected +- **Transaction entry failures after dispatch** - a transaction entry that fails once the backend is executing it is still collapsed to `400 processing`, with the real status stringified into the message (`Entry failed with status 404`). The backends discard the entry result at their `status >= 400` guard and return `TransactionError::BundleError`, which carries neither. Tracked separately; the per-entry codes above are the `batch` arm and the transaction refusals raised *before* dispatch +- **Bare type-level `GET`** - `GET Patient` in a batch entry is read as an instance read with an empty id and answers `404 not-found` with the message `Resource Patient/ not found`. Executing it as a search is #478 ## HTTP Headers @@ -574,6 +610,11 @@ All errors are returned as FHIR OperationOutcome resources: } ``` +The same shape is used for a failed Bundle entry, placed at +`Bundle.entry.response.outcome` — see [Per-entry outcomes](#per-entry-outcomes). +Both are built by `RestError::client_outcome`, so an error is described +identically wherever it surfaces. + ## Testing Tests use a JSON-driven specification format: diff --git a/crates/rest/src/error.rs b/crates/rest/src/error.rs index 75c371603..02c623527 100644 --- a/crates/rest/src/error.rs +++ b/crates/rest/src/error.rs @@ -39,6 +39,15 @@ //! spec-defined parameters/features that the server explicitly refuses; //! [`RestError::NotImplemented`] (501 + `not-supported`) signals work that //! has not yet been wired up. +//! +//! Three variants share `400` and differ only in how precisely they classify +//! the fault, using the `issue-type` hierarchy rather than three shades of the +//! same code: [`RestError::MissingElement`] (`required`) for an absent +//! mandatory element, [`RestError::InvalidElementValue`] (`value`) for one +//! that is present but unusable, and [`RestError::BadRequest`] (`invalid`, +//! their common parent) for everything else. Prefer a child where the +//! distinction is real — `OperationOutcome.issue.code` is bound `required` to +//! `issue-type` and its ElementDefinition asks for the most applicable code. use axum::{ Json, @@ -120,6 +129,45 @@ pub enum RestError { message: String, }, + /// A mandatory element is absent (HTTP 400 + `required`). + /// + /// Split from [`RestError::BadRequest`] so the outcome can carry + /// `required` — "A required element is missing." — rather than its is-a + /// parent `invalid`. Nothing in this crate could say `required` before: + /// every handler reported an absent element as `BadRequest`, and the + /// Bundle arms were the first place the distinction mattered enough to + /// notice (#504). + /// + /// Use it only where the StructureDefinition gives `min=1` or a named + /// invariant makes the element mandatory — `Bundle.entry.request` + /// (`bdl-3` in R4/R4B, transitively `bdl-3c` in R5/R6), + /// `Bundle.entry.request.method` (1..1) and `Bundle.entry.request.url` + /// (1..1). It is deliberately **not** used for an absent + /// `Bundle.entry.resource`: that element is 0..1 and only R5/R6's + /// `bdl-3c` requires it for POST/PUT/PATCH, so claiming `required` there + /// would assert a rule R4 and R4B do not have. + MissingElement { + /// Message naming the absent element and the entry it belongs to. + message: String, + }, + + /// An element is present but its value cannot be used (HTTP 400 + `value`). + /// + /// Split from [`RestError::BadRequest`] for the same reason as + /// [`RestError::MissingElement`], one level down the other branch: + /// `value` — "An element or header value is invalid." — is a child of + /// `invalid`, and `OperationOutcome.issue.code`'s ElementDefinition + /// requires the most applicable code rather than an ancestor that happens + /// to be true. + /// + /// The distinction against `MissingElement` is absent-versus-unusable, and + /// it is the one a client acts on differently: a missing element is added, + /// an invalid value is corrected. + InvalidElementValue { + /// Message naming the element and why its value cannot be used. + message: String, + }, + /// Unsupported media type (HTTP 415). UnsupportedMediaType { /// The unsupported content type. @@ -284,6 +332,12 @@ impl fmt::Display for RestError { RestError::BadRequest { message } => { write!(f, "Bad request: {}", message) } + RestError::MissingElement { message } => { + write!(f, "Missing element: {}", message) + } + RestError::InvalidElementValue { message } => { + write!(f, "Invalid element value: {}", message) + } RestError::UnsupportedMediaType { content_type } => { write!(f, "Unsupported media type: {}", content_type) } @@ -393,6 +447,16 @@ impl RestError { RestError::BadRequest { message } => { (StatusCode::BAD_REQUEST, "invalid", message.clone()) } + // `required` and `value` are both children of `invalid` in the + // `issue-type` hierarchy (verified identical in R4/R4B/R5/R6 at + // `crates/fhir-gen/resources/*/valuesets.json`), so these two + // refine `BadRequest` rather than contradicting it. + RestError::MissingElement { message } => { + (StatusCode::BAD_REQUEST, "required", message.clone()) + } + RestError::InvalidElementValue { message } => { + (StatusCode::BAD_REQUEST, "value", message.clone()) + } RestError::UnsupportedMediaType { content_type } => ( StatusCode::UNSUPPORTED_MEDIA_TYPE, "not-supported", @@ -406,6 +470,11 @@ impl RestError { "processing", message.clone(), ), + // Unreachable from either renderer since #504: both go through + // [`Self::client_outcome`], which intercepts `ValidationFailed` + // above this table and surfaces the validator's own multi-issue + // outcome. Kept so the match stays exhaustive, and because a + // caller wanting only a summary line is still entitled to one. RestError::ValidationFailed { .. } => ( StatusCode::UNPROCESSABLE_ENTITY, "processing", @@ -487,18 +556,43 @@ impl RestError { ), } } + + /// The client-facing `(status, OperationOutcome)` for this error. + /// + /// **This is the only place a [`RestError`] becomes an OperationOutcome.** + /// [`IntoResponse`] renders the pair as an HTTP response body and + /// `handlers::batch` renders it as a `Bundle.entry.response.outcome`; + /// neither builds its own. That is the point. Before #504 the batch + /// handler had a second renderer that hardcoded `"code": "processing"`, + /// so the identical failure carried `forbidden` at `GET [base]/Patient/1` + /// and `processing` for the same read inside a Bundle entry — and + /// [`Self::client_response`]'s promise one doc comment above, that it is + /// "shared by `IntoResponse` and the batch/transaction handler so both + /// sanitize identically", was not true. + /// + /// `ValidationFailed` is surfaced verbatim rather than collapsed, because + /// it already carries a fully-formed multi-issue outcome from the + /// write-path validator — per-issue `code`, `severity` and `expression`. + /// The interception has to happen **here** rather than at any call site: + /// [`Self::client_response`]'s own `ValidationFailed` arm returns + /// `(422, "processing", "Resource validation failed")`, so a caller that + /// reached the code table first would re-flatten it. + pub(crate) fn client_outcome(&self) -> (StatusCode, serde_json::Value) { + if let RestError::ValidationFailed { outcome } = self { + return (StatusCode::UNPROCESSABLE_ENTITY, outcome.clone()); + } + let (status, code, details) = self.client_response(); + (status, create_operation_outcome("error", code, &details)) + } } impl IntoResponse for RestError { fn into_response(self) -> Response { - // ValidationFailed carries a fully-formed OperationOutcome (potentially - // many issues from the write-path validator); surface it verbatim - // rather than collapsing it to the generic single-issue shape. - if let RestError::ValidationFailed { outcome } = &self { - return (StatusCode::UNPROCESSABLE_ENTITY, Json(outcome.clone())).into_response(); - } - let (status, code, details) = self.client_response(); - let operation_outcome = create_operation_outcome("error", code, &details); + // Both the HTTP body and a Bundle entry's `response.outcome` are + // rendered by `client_outcome`, so the two cannot describe the same + // failure differently (#504). It also carries the `ValidationFailed` + // pass-through that used to live here. + let (status, operation_outcome) = self.client_outcome(); // Unauthorized additionally carries a Bearer challenge in the // WWW-Authenticate header. @@ -557,7 +651,11 @@ impl IntoResponse for RestError { /// * `severity` - The issue severity (fatal, error, warning, information) /// * `code` - The FHIR issue code /// * `details` - Human-readable details -fn create_operation_outcome(severity: &str, code: &str, details: &str) -> serde_json::Value { +pub(crate) fn create_operation_outcome( + severity: &str, + code: &str, + details: &str, +) -> serde_json::Value { serde_json::json!({ "resourceType": "OperationOutcome", "issue": [{ diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index 0d04758ec..d1319732d 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -17,14 +17,14 @@ 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, - bundle_if_match_gate, + BundleEntry, BundleEntryResult, BundleMethod, BundleProvider, IncludeProvider, ResourceStorage, + RevincludeProvider, SearchProvider, bundle_if_match_gate, }; use helios_persistence::error::{ResourceError, StorageError, TransactionError}; use serde_json::Value; use tracing::{debug, error, warn}; -use crate::error::{RestError, RestResult}; +use crate::error::{RestError, RestResult, create_operation_outcome}; use crate::extractors::{FhirVersionExtractor, TenantExtractor}; use crate::handlers::extract_patient_from_resource; use crate::middleware::prefer::PreferHeader; @@ -60,7 +60,13 @@ pub async fn batch_handler( request: Request, ) -> RestResult where - S: ResourceStorage + BundleProvider + helios_persistence::core::SearchProvider + 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. @@ -251,7 +257,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(), @@ -371,7 +377,13 @@ async fn process_transaction( principal: Option<&Principal>, ) -> RestResult where - S: ResourceStorage + BundleProvider + helios_persistence::core::SearchProvider + Send + Sync, + S: ResourceStorage + + SearchProvider + + IncludeProvider + + RevincludeProvider + + BundleProvider + + Send + + Sync, { debug!( tenant = %tenant.tenant_id(), @@ -435,7 +447,11 @@ where // Transactions are atomic so any denied entry rejects the whole bundle. if let Some(principal) = principal { let (resource_type, _) = parse_request_url(&bundle_entry.url).map_err(|e| { - RestError::BadRequest { + // `value`, not its parent `invalid`: `request.url` is + // present and its value cannot be used. Its batch twin + // makes the same choice, so the arms classify it + // identically (#504). + RestError::InvalidElementValue { message: format!("Entry {}: {}", index, e), } })?; @@ -480,8 +496,41 @@ where // fail the bundle (#459). They used to be stored verbatim — unsearchable // and unresolvable. References to entries created by this same bundle use // `fullUrl`s, which the storage layer resolves during processing. + // + // This runs on the full entry set, before the GET search entries are + // partitioned out below: those carry no `resource`, so they contribute no + // conditional references either way. resolve_conditional_references(state, &tenant, &mut indexed_entries).await?; + // 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 { @@ -492,7 +541,7 @@ where continue; }; let (resource_type, _) = - parse_request_url(&entry.url).map_err(|e| RestError::BadRequest { + parse_request_url(&entry.url).map_err(|e| RestError::InvalidElementValue { message: format!("Entry {}: {}", index, e), })?; state @@ -539,6 +588,35 @@ 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), + // The second of #481's two code-discarding call sites, and + // the more consequential one: this loop bypasses the + // backend executor, so it is the first *reachable* + // per-entry outcome on the transaction arm. Rendered + // through the funnel like every other entry failure. + Err(e) => entry_failure(e), + }; + search_results.push((*index, entry.clone(), result)); + } + // Reorder results back to original entry order let mut ordered_results: Vec<(usize, &BundleEntry, &BundleEntryResult)> = indexed_entries @@ -546,6 +624,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 { @@ -585,10 +666,25 @@ where // rolled-back/internal transaction error never reaches the client // response, the audit trail, or the entry outcome. The raw detail is // preserved server-side by the `error!` log below. - 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 { + // + // The status and the code come from the same triple as the reason, + // so this synthetic per-entry result cannot contradict the + // whole-bundle response it accompanies — a rollback is `transient` + // and retryable, where the hardcoded `processing` it carried meant + // "there is no point resubmitting the same content unchanged". + // Audit-only: the client gets `transaction_error_to_response(e)` + // below, so nothing here is observable on the wire (#504). + let (rollback_status, rollback_code, rollback_reason) = + transaction_error_response_parts(&e); + let rollback_result = BundleEntryResult::error( + rollback_status.as_u16(), + create_operation_outcome( + "error", + rollback_code, + &format!("Transaction rolled back: {rollback_reason}"), + ), + ); + for (orig_idx, entry, _) in indexed_entries.iter().chain(&search_entries) { let correlation_details = EntryAuditCorrelation::from_bundle(&correlation, *orig_idx); emit_transaction_entry_audit( @@ -648,10 +744,7 @@ where // precondition rather than a storage error — the same mapping // `handlers::update` and the backends' own batch arms make. Err(StorageError::Resource(ResourceError::Gone { .. })) => None, - Err(e) => { - let (status, message) = entry_error(e); - return Some(create_error_result(status, &message)); - } + Err(e) => return Some(entry_storage_failure(e)), }; bundle_if_match_gate(if_match, current.as_ref().map(|r| r.version_id())) @@ -667,12 +760,12 @@ 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, None => { - return create_error_result(400, &format!("Entry {} missing request", index)); + return entry_failure(missing_request(index)); } }; @@ -683,17 +776,25 @@ where let method = match parse_entry_method(request) { Ok(method) => method, Err(refusal) => { - return create_error_result(refusal.status(), &refusal.message(index)); + return entry_failure(refusal.into_rest_error(index)); } }; - let url = request.get("url").and_then(|v| v.as_str()).unwrap_or(""); + // An absent `request.url` is a cardinality violation (1..1), reported as + // `required` — distinct from a url that is present and unusable, which + // `parse_request_url` refuses below as `value`. Splitting them also closes + // a divergence: an absent url was caught by `parse_bundle_entry` on the + // transaction arm and, one line later, by `unwrap_or("")` here, so the two + // arms described the same entry differently (#504). + let Some(url) = request.get("url").and_then(|v| v.as_str()) else { + return entry_failure(missing_url(index)); + }; let if_match = request.get("ifMatch").and_then(|v| v.as_str()); // Parse the URL to extract resource type and ID let (resource_type, id) = match parse_request_url(url) { Ok(parsed) => parsed, Err(e) => { - return create_error_result(400, &e); + return entry_failure(RestError::InvalidElementValue { message: e }); } }; @@ -706,13 +807,16 @@ where // authorized as a read and then executed as whatever it was. let operation = bundle_method_to_fhir_operation(&method); if SmartScopePolicy::check(principal, &resource_type, operation).is_err() { - return create_error_result( - 403, - &format!( + // `forbidden` is a child of `security`; `processing` is not an + // ancestor of it in any supported version, so a client filtering + // `code is-a security` to trigger re-auth or re-consent saw a + // false negative on this denial and only this one (#504). + return entry_failure(RestError::Forbidden { + message: format!( "Insufficient scope for {} on {} (batch entry {})", operation, resource_type, index ), - ); + }); } } @@ -735,20 +839,44 @@ where if !matches!(method, BundleMethod::Get) && let Some(criteria) = conditional_criteria(url, &id) { - return create_error_result( - 400, - &format!( + // `NotSupported` passes `feature` through verbatim, so the message + // survives byte-for-byte while the code moves to `not-supported` — the + // pair the transaction twin already returns for the same entry. + return entry_failure(RestError::NotSupported { + feature: 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}" ), - ); + }); } match method { BundleMethod::Get => { + // 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), + // Rendered through the funnel like every other entry + // failure. #481 wrote this as `let (status, _, details) = + // e.client_response()` — the same code-discard #504 + // deleted everywhere else, which would have made a search + // entry the one path still answering `processing`. + Err(e) => entry_failure(e), + }; + } // Read operation match state .storage() @@ -756,11 +884,16 @@ where .await { Ok(Some(stored)) => BundleEntryResult::ok(stored), - Ok(None) => create_error_result(404, "Resource not found"), - Err(e) => { - let (status, message) = entry_error(e); - create_error_result(status, &message) - } + // The one expression this PR changes on the arm #478/#481 is + // rewriting. `not-found` is the only issue-type code whose + // definition names HTTP 404, and all three backends already + // emit it for the byte-identical condition inside their own + // transaction executors — this entry was the outlier. + Ok(None) => entry_failure(RestError::NotFound { + resource_type: resource_type.clone(), + id: id.clone(), + }), + Err(e) => entry_storage_failure(e), } } BundleMethod::Post => { @@ -768,17 +901,31 @@ where let resource = match entry.get("resource") { Some(r) => r.clone(), None => { - return create_error_result(400, "POST entry missing resource"); + // `invalid`, not `required`: `Bundle.entry.resource` is + // 0..1, and only R5/R6's bdl-3c makes it mandatory for a + // POST/PUT/PATCH entry. R4 and R4B have no equivalent — + // bdl-5 is satisfied by a request-only entry — so a single + // call site serving four versions must not claim a rule + // half of them do not have. + return entry_failure(RestError::BadRequest { + message: "POST entry missing resource".to_string(), + }); } }; // Write-path validation (per-entry outcome in batch semantics). + // + // The error carries the validator's own multi-issue outcome — + // per-issue code, severity and `expression` — and + // `client_outcome` surfaces it verbatim. It used to be flattened + // to a joined string of `details.text` and re-wrapped under + // `processing`, which is the lossiest case in #504. 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 entry_failure(e); } match state @@ -796,10 +943,7 @@ where } BundleEntryResult::created(stored) } - Err(e) => { - let (status, message) = entry_error(e); - create_error_result(status, &message) - } + Err(e) => entry_storage_failure(e), } } BundleMethod::Put => { @@ -807,7 +951,11 @@ where let resource = match entry.get("resource") { Some(r) => r.clone(), None => { - return create_error_result(400, "PUT entry missing resource"); + // See the POST arm: `invalid` rather than `required`, + // because R4 and R4B do not require the element. + return entry_failure(RestError::BadRequest { + message: "PUT entry missing resource".to_string(), + }); } }; @@ -818,10 +966,13 @@ where // an absent id, not an empty one. Every later such entry then reads // that row back and overwrites it (#503). if id.is_empty() { - return create_error_result( - 400, - "PUT entry request.url must address an instance ('[type]/[id]')", - ); + // `value`: the element is present and its value cannot address + // what the method needs. Its sibling guard on DELETE makes the + // same choice. + return entry_failure(RestError::InvalidElementValue { + message: "PUT entry request.url must address an instance ('[type]/[id]')" + .to_string(), + }); } // Ahead of validation, because every backend evaluates `ifMatch` @@ -839,7 +990,7 @@ where .check_write(tenant.tenant_id(), fhir_version, &resource_type, &resource) .await { - return create_error_result(422, &validation_failure_message(&e)); + return entry_failure(e); } match state @@ -870,10 +1021,11 @@ where result } } - Err(e) => { - let (status, message) = entry_error(e); - create_error_result(status, &message) - } + // Also closes a divergence internal to the batch arm: an + // optimistic-lock failure surfacing here now answers 412 + + // `conflict`, the pair the `ifMatch` gate above already + // answers, where it used to answer 412 + `processing`. + Err(e) => entry_storage_failure(e), } } BundleMethod::Delete => { @@ -881,10 +1033,10 @@ where // type-level delete, and an empty id would otherwise target the // empty-id row a pre-#503 conditional PUT could have written. if id.is_empty() { - return create_error_result( - 400, - "DELETE entry request.url must address an instance ('[type]/[id]')", - ); + return entry_failure(RestError::InvalidElementValue { + message: "DELETE entry request.url must address an instance ('[type]/[id]')" + .to_string(), + }); } // Honour `ifMatch` on DELETE: a client asking to delete only the @@ -902,10 +1054,7 @@ where .await { Ok(()) => BundleEntryResult::deleted(), - Err(e) => { - let (status, message) = entry_error(e); - create_error_result(status, &message) - } + Err(e) => entry_storage_failure(e), } } // Declined rather than dispatched, matching the transaction arm and all @@ -914,14 +1063,17 @@ where // derives the patch format entirely from it, so there is nothing here // to dispatch on; R4 designates FHIRPath Patch as the bundle format and // `apply_patch` does not implement it. Tracked by #502's follow-up. - BundleMethod::Patch => create_error_result( - 501, - &format!( - "Entry {index}: PATCH is not implemented in Bundle entries, so \ - nothing was applied. Send the patch to the instance endpoint \ - (PATCH [base]/[type]/[id])." + // + // `NotImplemented` wraps its `feature` as "Feature '…' is not + // implemented", so this message changes shape while keeping its + // guidance. The arm label stays: an audit trail wants to know which + // arm refused, and the transaction twin identifies itself the same way. + BundleMethod::Patch => entry_failure(RestError::NotImplemented { + feature: format!( + "PATCH in a Bundle entry (batch entry {index}); nothing was applied — \ + send the patch to the instance endpoint instead, PATCH [base]/[type]/[id]" ), - ), + }), // No catch-all: the match is exhaustive over `BundleMethod`, so adding a // variant is a compile error here rather than a silent 405. Codes // outside the value set never reach this point — `parse_entry_method` @@ -1118,14 +1270,20 @@ fn bundle_method_to_http_method(method: &BundleMethod) -> &'static str { } } +/// Reads the first issue's text for an audit event's `outcomeDesc`. +/// +/// Falls back to `diagnostics`. The one batch entry outcome #504 deliberately +/// leaves alone — the 412 from +/// [`helios_persistence::core::preconditions::precondition_failed_entry`] — +/// writes its text there rather than to `details.text`, so a failed `ifMatch` +/// produced an AuditEvent with no description at all. fn extract_outcome_description(outcome: Option<&Value>) -> Option { - outcome - .and_then(|value| value.get("issue")) - .and_then(|issues| issues.as_array()) - .and_then(|issues| issues.first()) - .and_then(|issue| issue.get("details")) + let issue = outcome?.get("issue")?.as_array()?.first()?; + issue + .get("details") .and_then(|details| details.get("text")) .and_then(|text| text.as_str()) + .or_else(|| issue.get("diagnostics").and_then(Value::as_str)) .map(ToString::to_string) } @@ -1198,42 +1356,81 @@ enum EntryMethodRefusal { } impl EntryMethodRefusal { - fn status(&self) -> u16 { - match self { - Self::Missing | Self::NotCanonical(_) => 400, - Self::Head => 405, - } - } - - fn message(&self, index: usize) -> String { - match self { - Self::Missing => format!("Entry {index}: request.method is required"), - Self::NotCanonical(raw) => format!( - "Entry {index}: '{raw}' is not an http-verb code. \ - Bundle.entry.request.method is a code with a required binding to \ - http://hl7.org/fhir/ValueSet/http-verb, and FHIR codes are \ - case-sensitive — use GET, POST, PUT, PATCH or DELETE." - ), - Self::Head => format!( - "Entry {index}: HEAD is not supported in Bundle entries. Use GET, \ - or send HEAD to the instance endpoint directly." - ), - } - } - - /// Renders the refusal for the transaction arm, where it fails the bundle. + /// Renders the refusal as the error **both** arms report. + /// + /// #515 gave this type a `status()` beside this function and pinned the two + /// with a test — agreement by hand. There is now one function, so the + /// status *and* the issue code are decided once, by the [`RestError`] this + /// produces: the batch arm wraps it with [`entry_failure`] and the + /// transaction arm returns it as the whole-bundle error (#504). + /// + /// The per-variant text was previously a separate `message()`, whose `Head` + /// arm this function computed and then discarded — which is why the batch + /// arm printed HEAD guidance the transaction arm never showed. fn into_rest_error(self, index: usize) -> RestError { - let message = self.message(index); match self { + // 405 + `not-supported`. HEAD is a legal `http-verb` code, so the + // entry is well-formed instance data and it is the interaction the + // server does not implement inside a Bundle. The guidance rides in + // `resource_type` because `MethodNotAllowed` renders + // "Method {method} not allowed on {resource_type}" and has no + // detail slot — that is how both arms come to print it. Self::Head => RestError::MethodNotAllowed { method: "HEAD".to_string(), - resource_type: format!("a Bundle entry (entry {index})"), + resource_type: format!( + "a Bundle entry (entry {index}) — use GET, or send HEAD to the \ + instance endpoint directly" + ), + }, + // 400 + `required`: `Bundle.entry.request.method` is 1..1 in every + // supported version, so an absent code is a cardinality violation + // rather than an unusable value. + Self::Missing => RestError::MissingElement { + message: format!("Entry {index}: request.method is required"), + }, + // 400 + `value`: the element is present and its value fails the + // required binding. Deliberately not `code-invalid`, which names + // that mechanism precisely but is a **child of `processing`** — + // emitting it would move a malformed-instance failure back into the + // branch #504 exists to escape. + Self::NotCanonical(raw) => RestError::InvalidElementValue { + message: format!( + "Entry {index}: '{raw}' is not an http-verb code. \ + Bundle.entry.request.method is a code with a required binding to \ + http://hl7.org/fhir/ValueSet/http-verb, and FHIR codes are \ + case-sensitive — use GET, POST, PUT, PATCH or DELETE." + ), }, - Self::Missing | Self::NotCanonical(_) => RestError::BadRequest { message }, } } } +/// `Bundle.entry.request` is absent. +/// +/// Shared by both arms so the batch entry outcome and the whole-bundle +/// transaction error carry one message and one code. `request` is 0..1 in the +/// StructureDefinition, but it is mandatory for these bundle types — `bdl-3` in +/// R4/R4B, and transitively `bdl-3c` in R5/R6, which requires +/// `request.method.exists()`. The invariant key stays out of the message: the +/// handler serves all four versions and `bdl-3` does not exist in R5 or R6. +fn missing_request(index: usize) -> RestError { + RestError::MissingElement { + message: format!( + "Entry {index}: request is required — a batch or transaction entry must carry it." + ), + } +} + +/// `Bundle.entry.request.url` is absent. +/// +/// Distinct from a url that is present but names nothing (`""`, `"/"`, +/// `"?identifier=x"`), which [`parse_request_url`] refuses as `value`. +fn missing_url(index: usize) -> RestError { + RestError::MissingElement { + message: format!("Entry {index}: request.url is required (it is 1..1)."), + } +} + /// Parses a bundle entry's `request.method` into a [`BundleMethod`]. /// /// **This is the only `&str` -> `BundleMethod` table in this crate.** Both the @@ -1274,63 +1471,103 @@ fn parse_entry_method(request: &Value) -> Result RestError { match self { Self::Method(refusal) => refusal.into_rest_error(index), - Self::Malformed(message) => RestError::BadRequest { - message: format!("Entry {}: {}", index, message), - }, + // Both arms now reach the same helper, so an absent element is + // described once rather than by whichever arm noticed it first. + Self::MissingRequest => missing_request(index), + Self::MissingUrl => missing_url(index), } } } +/// 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). -fn validation_failure_message(error: &RestError) -> String { - if let RestError::ValidationFailed { outcome } = error { - let details: Vec = outcome - .get("issue") - .and_then(|i| i.as_array()) - .map(|issues| { - issues - .iter() - .filter_map(|issue| { - issue - .get("details") - .and_then(|d| d.get("text")) - .and_then(|t| t.as_str()) - .map(str::to_string) - }) - .collect() - }) - .unwrap_or_default(); - if !details.is_empty() { - return format!("Validation failed: {}", details.join("; ")); - } - } - format!("Validation failed: {error}") +/// Renders a failed Bundle entry. +/// +/// **Replaces `create_error_result`,** which hardcoded `"code": "processing"` +/// across nineteen call sites, so a scope denial, a missing resource, a +/// malformed entry and an unsupported method were distinguishable only by +/// `response.status` and free-text English (#504). +/// +/// The status and the OperationOutcome both come from +/// [`RestError::client_outcome`] — the same function `impl IntoResponse for +/// RestError` uses — so a per-entry outcome and the single-resource response +/// for the identical failure are produced by one mapping rather than by two +/// kept in step by review. That is what makes the two describable as the same +/// error rather than two errors that happen to agree. +fn entry_failure(err: RestError) -> BundleEntryResult { + let (status, outcome) = err.client_outcome(); + BundleEntryResult::error(status.as_u16(), outcome) } -fn create_error_result(status: u16, message: &str) -> BundleEntryResult { - let outcome = serde_json::json!({ - "resourceType": "OperationOutcome", - "issue": [{ - "severity": "error", - "code": "processing", - "details": { - "text": message - } - }] - }); - BundleEntryResult::error(status, outcome) +/// Renders a storage error as a failed Bundle entry. +/// +/// **Replaces `entry_error`,** which called `client_response()`, bound the +/// correct FHIR issue code to `_code` and discarded it, after which +/// `create_error_result` stamped `processing` over the result. Deleting that +/// one underscore-binding corrects five call sites by construction. +/// +/// The sanitizing behaviour is unchanged and is now strictly stronger: the code +/// comes from the same sanitized triple as the message, so it cannot classify +/// an error more specifically than the message is permitted to describe it. +fn entry_storage_failure(err: StorageError) -> BundleEntryResult { + entry_failure(RestError::from(err)) } /// Returns HTTP status text for a status code. +/// +/// Every status [`RestError::client_response`] can produce for an entry has an +/// arm here; anything else renders as `" Unknown"`. The 413/429/503/504 +/// arms were missing while every entry error carried `processing`, so nothing +/// noticed — an entry hitting an exhausted pool rendered `"503 Unknown"` +/// beside a correct `transient` code once the codes were threaded (#504). fn status_text(code: &str) -> &'static str { match code { "200" => "OK", @@ -1345,10 +1582,14 @@ fn status_text(code: &str) -> &'static str { "409" => "Conflict", "410" => "Gone", "412" => "Precondition Failed", + "413" => "Payload Too Large", "415" => "Unsupported Media Type", "422" => "Unprocessable Entity", + "429" => "Too Many Requests", "500" => "Internal Server Error", "501" => "Not Implemented", + "503" => "Service Unavailable", + "504" => "Gateway Timeout", _ => "Unknown", } } @@ -1502,7 +1743,7 @@ fn rewrite_conditional_references( fn parse_bundle_entry(entry: &Value) -> Result<(BundleEntry, Option), EntryParseError> { let request = entry .get("request") - .ok_or_else(|| EntryParseError::Malformed("Entry missing 'request'".to_string()))?; + .ok_or(EntryParseError::MissingRequest)?; // Was an independently-written `to_uppercase()` ladder — the second of the // two matchers #502 is about. It no longer case-folds: `request.method` is a @@ -1515,7 +1756,7 @@ fn parse_bundle_entry(entry: &Value) -> Result<(BundleEntry, Option), En let url = request .get("url") .and_then(|v| v.as_str()) - .ok_or_else(|| EntryParseError::Malformed("Entry request missing 'url'".to_string()))? + .ok_or(EntryParseError::MissingUrl)? .to_string(); let resource = entry.get("resource").cloned(); @@ -1675,17 +1916,6 @@ fn build_full_url(result: &BundleEntryResult, base_url: &str) -> Option None } -/// Derives a sanitized `(status, message)` for a batch/transaction entry -/// OperationOutcome from a storage error. -/// -/// Reuses [`RestError`]'s client-facing mapping so backend/internal detail is -/// never leaked to callers (and is logged server-side) while safe classes keep -/// their specific, actionable message and correct HTTP status. -fn entry_error(err: StorageError) -> (u16, String) { - let (status, _code, message) = RestError::from(err).client_response(); - (status.as_u16(), message) -} - /// Computes the sanitized `(status, issue code, message)` for a failed /// transaction. /// @@ -1762,20 +1992,14 @@ fn transaction_error_response_parts(err: &TransactionError) -> (StatusCode, &'st } /// Converts a TransactionError to an HTTP response with OperationOutcome. +/// +/// The twin of [`RestError::client_outcome`] for the one error type that is not +/// a [`RestError`]: it builds the outcome through the same +/// `create_operation_outcome` every other error in this crate uses, rather than +/// carrying its own `json!` literal. fn transaction_error_to_response(err: TransactionError) -> RestResult { let (status_code, issue_code, message) = transaction_error_response_parts(&err); - - let outcome = serde_json::json!({ - "resourceType": "OperationOutcome", - "issue": [{ - "severity": "error", - "code": issue_code, - "details": { - "text": message - } - }] - }); - + let outcome = create_operation_outcome("error", issue_code, &message); Ok((status_code, Json(outcome)).into_response()) } @@ -1894,8 +2118,16 @@ mod tests { details } + /// The first issue of an entry result's outcome. + fn entry_issue(result: &BundleEntryResult) -> &Value { + &result + .outcome + .as_ref() + .expect("a failed entry carries an outcome")["issue"][0] + } + #[test] - fn test_entry_error_sanitizes_backend_detail() { + fn entry_storage_failure_sanitizes_backend_detail() { // A backend/internal storage error whose Display embeds sensitive DB // detail (table/column names, SQL fragments) must be collapsed to the // generic client message with a 5xx status. @@ -1906,8 +2138,17 @@ mod tests { message: raw_detail.to_string(), }); - let (status, message) = entry_error(err); - assert_eq!(status, 500); + let result = entry_storage_failure(err); + assert_eq!(result.status, 500); + + let issue = entry_issue(&result); + // Threading the code strengthens this guarantee rather than diluting + // it: the code now comes from the same sanitized `client_response` + // triple as the message, so it cannot classify the error more + // specifically than the message is permitted to describe it (#504). + assert_eq!(issue["code"], "exception"); + + let message = issue["details"]["text"].as_str().unwrap(); assert!( !message.contains("resources"), "entry outcome leaked raw backend detail: {message}" @@ -1923,7 +2164,7 @@ mod tests { } #[test] - fn test_entry_error_preserves_not_found() { + fn entry_storage_failure_preserves_not_found() { // Safe error classes keep their specific message and correct status. use helios_persistence::error::ResourceError; @@ -1932,8 +2173,12 @@ mod tests { id: "123".to_string(), }); - let (status, message) = entry_error(err); - assert_eq!(status, 404); + let result = entry_storage_failure(err); + assert_eq!(result.status, 404); + + let issue = entry_issue(&result); + assert_eq!(issue["code"], "not-found"); + let message = issue["details"]["text"].as_str().unwrap(); assert!(message.contains("Patient/123"), "message was: {message}"); } @@ -2023,6 +2268,146 @@ mod tests { assert!(msg.contains("serializable"), "isolation message: {msg}"); } + /// A failed entry carries the issue code the single-resource endpoint + /// would return for the identical `RestError`. + /// + /// This is #504's whole claim in one table. Every row is a `RestError` the + /// batch arm now constructs, and the pair asserted is the one + /// [`RestError::client_outcome`] produces — the same function + /// `impl IntoResponse for RestError` uses to build an HTTP body. + #[test] + fn entry_failure_renders_the_single_resource_mapping() { + let cases: Vec<(RestError, u16, &str)> = vec![ + ( + RestError::MissingElement { + message: "m".to_string(), + }, + 400, + "required", + ), + ( + RestError::InvalidElementValue { + message: "m".to_string(), + }, + 400, + "value", + ), + ( + RestError::BadRequest { + message: "m".to_string(), + }, + 400, + "invalid", + ), + ( + RestError::NotSupported { + feature: "m".to_string(), + }, + 400, + "not-supported", + ), + ( + RestError::Forbidden { + message: "m".to_string(), + }, + 403, + "forbidden", + ), + ( + RestError::NotFound { + resource_type: "Patient".to_string(), + id: "ghost".to_string(), + }, + 404, + "not-found", + ), + ( + RestError::MethodNotAllowed { + method: "HEAD".to_string(), + resource_type: "a Bundle entry".to_string(), + }, + 405, + "not-supported", + ), + ( + RestError::Gone { + resource_type: "Patient".to_string(), + id: "p1".to_string(), + }, + 410, + "deleted", + ), + ( + RestError::PreconditionFailed { + message: "m".to_string(), + }, + 412, + "conflict", + ), + ( + RestError::NotImplemented { + feature: "m".to_string(), + }, + 501, + "not-supported", + ), + ( + RestError::ServiceUnavailable { + message: "m".to_string(), + }, + 503, + "transient", + ), + ( + RestError::InternalError { + message: "m".to_string(), + }, + 500, + "exception", + ), + ]; + + for (err, status, code) in cases { + let label = format!("{err:?}"); + let result = entry_failure(err); + assert_eq!(result.status, status, "{label}"); + assert!(result.resource.is_none(), "{label}"); + + let issue = entry_issue(&result); + assert_eq!(issue["code"], code, "{label}"); + assert_eq!(issue["severity"], "error", "{label}"); + assert!( + issue["details"]["text"].is_string(), + "{label} carried no details.text" + ); + } + } + + /// A failed `ifMatch` must still produce an audit description. + /// + /// The 412 gate is the one entry outcome #504 leaves alone, and it writes + /// its text to `diagnostics` rather than `details.text` — so before the + /// fallback below, `outcomeDesc` was absent for exactly that case. + #[test] + fn extract_outcome_description_reads_the_gates_diagnostics() { + let gate = helios_persistence::core::preconditions::precondition_failed_entry("stale tag"); + assert_eq!( + extract_outcome_description(gate.outcome.as_ref()), + Some("stale tag".to_string()), + "the 412 gate writes to `diagnostics`" + ); + + // `details.text` still wins, and still works on its own. + let both = serde_json::json!({ + "issue": [{ "details": { "text": "text wins" }, "diagnostics": "ignored" }] + }); + assert_eq!( + extract_outcome_description(Some(&both)), + Some("text wins".to_string()) + ); + assert_eq!(extract_outcome_description(None), None); + } + #[test] fn test_status_text_covers_known_and_unknown_codes() { // The batch response builder renders a reason phrase per entry status; the @@ -2040,16 +2425,28 @@ mod tests { ("409", "Conflict"), ("410", "Gone"), ("412", "Precondition Failed"), + // Reachable, and unmapped until #504. `BackendError::PoolExhausted` + // / `Unavailable` / `ConnectionFailed` reach an entry as 503 and + // `Timeout` as 504, so an entry hitting an exhausted pool rendered + // `"503 Unknown"` beside a correct `transient` code. + ("413", "Payload Too Large"), ("415", "Unsupported Media Type"), ("422", "Unprocessable Entity"), + ("429", "Too Many Requests"), ("500", "Internal Server Error"), ("501", "Not Implemented"), + ("503", "Service Unavailable"), + ("504", "Gateway Timeout"), ]; for (code, phrase) in known { assert_eq!(status_text(code), phrase, "reason phrase for {code}"); } // Any unmapped code falls through to the catch-all. assert_eq!(status_text("418"), "Unknown"); + // Every status a batch entry can now carry has a phrase. + for (code, _) in known { + assert_ne!(status_text(code), "Unknown", "unmapped entry status {code}"); + } assert_eq!(status_text(""), "Unknown"); } @@ -2287,6 +2684,62 @@ mod tests { } } + // #478's search-entry dispatch widened `process_batch`'s bound to + // `SearchProvider + IncludeProvider + RevincludeProvider`, so this mock has + // to satisfy them. Every method is `unimplemented!()`, which is the same + // lever the write methods above use: no unit test in this module drives a + // search entry, and one that started to would panic loudly rather than + // silently exercising a stub. + #[async_trait] + impl helios_persistence::core::SearchProvider for DelayStorage { + async fn search( + &self, + _tenant: &TenantContext, + _query: &helios_persistence::types::SearchQuery, + ) -> StorageResult { + unimplemented!() + } + + async fn search_count( + &self, + _tenant: &TenantContext, + _query: &helios_persistence::types::SearchQuery, + ) -> StorageResult { + unimplemented!() + } + + fn search_param_registry( + &self, + _tenant: &TenantContext, + ) -> Arc> { + unimplemented!() + } + } + + #[async_trait] + impl helios_persistence::core::IncludeProvider for DelayStorage { + async fn resolve_includes( + &self, + _tenant: &TenantContext, + _resources: &[StoredResource], + _includes: &[helios_persistence::types::IncludeDirective], + ) -> StorageResult> { + unimplemented!() + } + } + + #[async_trait] + impl helios_persistence::core::RevincludeProvider for DelayStorage { + async fn resolve_revincludes( + &self, + _tenant: &TenantContext, + _resources: &[StoredResource], + _revincludes: &[helios_persistence::types::IncludeDirective], + ) -> StorageResult> { + unimplemented!() + } + } + /// A batch Bundle of `count` GET entries, targeting `Patient/p0..p{count}`. fn get_bundle(count: usize) -> Value { let entries: Vec = (0..count) @@ -2309,7 +2762,7 @@ mod tests { principal: Option<&Principal>, ) -> Value where - S: ResourceStorage + Send + Sync, + S: ResourceStorage + SearchProvider + IncludeProvider + RevincludeProvider + Send + Sync, { let tenant = TenantExtractor::new("test-tenant", crate::tenant::TenantSource::Default); let response = process_batch( @@ -2333,6 +2786,65 @@ mod tests { AppState::new(Arc::new(storage), crate::config::ServerConfig::default()) } + /// Like [`state_with`], but with write-path validation in `enforce` mode. + /// `ServerConfig::default()`'s validation mode is `off`, so no other test + /// in this module can reach the 422 arm. + fn enforcing_state_with(storage: DelayStorage) -> AppState { + AppState::new( + Arc::new(storage), + crate::config::ServerConfig { + validation: crate::config::ValidationConfig { + mode: "enforce".to_string(), + ..Default::default() + }, + ..crate::config::ServerConfig::default() + }, + ) + } + + /// A validation failure carries the validator's own issues, and is refused + /// before the entry reaches storage. + /// + /// The wire-level parity with `POST [base]/Patient` is asserted by + /// `the_two_surfaces_report_the_same_validation_issues` in + /// `tests/validation_enforcement_tests.rs`; what this adds is the ordering + /// guarantee. `DelayStorage::create` is `unimplemented!()`, so a validation + /// failure moved after dispatch panics here rather than quietly writing, + /// and `peak() == 0` proves not even a read occurred. + #[tokio::test] + async fn a_validation_failure_carries_the_validators_own_issues() { + let state = enforcing_state_with(DelayStorage::new(8, 0)); + + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "POST", "url": "Patient" }, + "resource": { "resourceType": "Patient", "bogusElement": true } + }] + }); + + let response = run_batch(&state, &bundle, None).await; + let entry = &response["entry"][0]["response"]; + assert_eq!(entry["status"], "422 Unprocessable Entity", "{response}"); + + let issues = entry["outcome"]["issue"] + .as_array() + .expect("the validator's issue array"); + assert!( + issues.iter().any(|i| { + i["code"] == "structure" && i["expression"][0] == "Patient.bogusElement" + }), + "the entry must carry the validator's coded, located issues: {entry}" + ); + + assert_eq!( + state.storage().peak(), + 0, + "the entry must not reach storage" + ); + } + /// Response entry *i* must answer request entry *i*, even when entry *i* /// finishes last. /// @@ -2580,7 +3092,16 @@ mod tests { // HEAD *is* served on the instance-read route. let head = serde_json::json!({ "method": "HEAD", "url": "Patient/p1" }); assert_eq!(parse_entry_method(&head), Err(EntryMethodRefusal::Head)); - assert_eq!(EntryMethodRefusal::Head.status(), 405); + // Read through the only remaining source of the status since #504 + // deleted `EntryMethodRefusal::status()`, which held a second copy of + // it beside `into_rest_error`'s choice of variant. + assert_eq!( + EntryMethodRefusal::Head + .into_rest_error(0) + .client_response() + .0, + StatusCode::METHOD_NOT_ALLOWED + ); // Case-folded spellings are invalid instance data, not valid entries a // strict server wrongly rejects — this is the premise #502 inverted. @@ -2593,8 +3114,11 @@ mod tests { ); } assert_eq!( - EntryMethodRefusal::NotCanonical("post".to_string()).status(), - 400 + EntryMethodRefusal::NotCanonical("post".to_string()) + .into_rest_error(0) + .client_response() + .0, + StatusCode::BAD_REQUEST ); // Absent or non-string is distinguishable from a bogus code. It used to @@ -2610,26 +3134,62 @@ mod tests { "request: {request}" ); } - assert_eq!(EntryMethodRefusal::Missing.status(), 400); + assert_eq!( + EntryMethodRefusal::Missing + .into_rest_error(0) + .client_response() + .0, + StatusCode::BAD_REQUEST + ); } - /// The refusal keeps its status across the transaction boundary. Flattening - /// it to a bare 400 there would re-create #502's divergence in a new place: - /// HEAD would be 405 per-entry in a batch and 400 for the whole bundle. + /// A refusal is rendered identically by both arms — status, issue code and + /// message. + /// + /// #515 pinned only the status, and did it by asserting the `RestError` + /// *variant* as a proxy for the pair. That assertion survives #504 + /// unchanged while saying nothing about the code, so it is replaced rather + /// than kept: the batch arm's per-entry outcome and the transaction arm's + /// whole-bundle error must now agree on all three, which is strictly + /// stronger than what it replaces. #[test] - fn a_method_refusal_keeps_its_status_on_the_transaction_path() { - let head = EntryMethodRefusal::Head.into_rest_error(3); - assert!( - matches!(head, RestError::MethodNotAllowed { .. }), - "HEAD must stay 405, got {head:?}" - ); + fn a_method_refusal_renders_identically_on_both_arms() { + // (refusal, status, issue code) + let cases = [ + ( + EntryMethodRefusal::Head, + StatusCode::METHOD_NOT_ALLOWED, + "not-supported", + ), + ( + EntryMethodRefusal::Missing, + StatusCode::BAD_REQUEST, + "required", + ), + ( + EntryMethodRefusal::NotCanonical("post".to_string()), + StatusCode::BAD_REQUEST, + "value", + ), + ]; - let lowercase = EntryMethodRefusal::NotCanonical("post".to_string()).into_rest_error(0); - assert!(matches!(lowercase, RestError::BadRequest { .. })); - assert!(matches!( - EntryMethodRefusal::Missing.into_rest_error(0), - RestError::BadRequest { .. } - )); + for (refusal, status, code) in cases { + // What the transaction arm returns as the whole-bundle error… + let (tx_status, tx_code, tx_message) = + refusal.clone().into_rest_error(7).client_response(); + // …and what the batch arm records for that entry. + let entry = entry_failure(refusal.clone().into_rest_error(7)); + let issue = entry_issue(&entry); + + assert_eq!(tx_status, status, "{refusal:?}"); + assert_eq!(tx_code, code, "{refusal:?}"); + assert_eq!(entry.status, status.as_u16(), "{refusal:?}"); + assert_eq!(issue["code"], code, "{refusal:?}"); + assert_eq!( + issue["details"]["text"], tx_message, + "the two arms must print one sentence for {refusal:?}" + ); + } } /// The transaction matcher no longer case-folds. `to_uppercase()` was the @@ -2697,6 +3257,22 @@ mod tests { "400 Bad Request", ] ); + // The four refusals were indistinguishable below the status line until + // #504 — every one carried `processing`. PATCH and HEAD are capability + // gaps, a lowercase verb is an unusable value, and an absent method is + // a missing element. + let codes: Vec<&str> = entries + .iter() + .map(|e| { + e["response"]["outcome"]["issue"][0]["code"] + .as_str() + .unwrap() + }) + .collect(); + assert_eq!( + codes, + vec!["not-supported", "not-supported", "value", "required"] + ); assert_eq!(state.storage().peak(), 0, "no entry may reach storage"); } @@ -2733,6 +3309,13 @@ mod tests { entry["response"]["status"], "400 Bad Request", "entry {index}: {entry}" ); + // A conditional interaction is a named FHIR capability this server + // does not have (#511) — the entry itself is valid. Same code the + // transaction arm returns for the same entry. + assert_eq!( + entry["response"]["outcome"]["issue"][0]["code"], "not-supported", + "entry {index}: {entry}" + ); } assert_eq!(state.storage().peak(), 0, "no entry may reach storage"); } @@ -2764,6 +3347,11 @@ mod tests { entry["response"]["status"], "400 Bad Request", "entry {index}: {entry}" ); + // `request.url` is present; its value cannot address an instance. + assert_eq!( + entry["response"]["outcome"]["issue"][0]["code"], "value", + "entry {index}: {entry}" + ); } assert_eq!(state.storage().peak(), 0, "no entry may reach storage"); } @@ -2835,6 +3423,14 @@ mod tests { status.starts_with("403"), "entry {i} (Observation) must be denied: {status}" ); + // `forbidden` is a child of `security`, and `processing` — what + // this denial carried until #504 — is not an ancestor of it in + // any supported version. A client filtering `code is-a + // security` to trigger re-auth saw a false negative here. + assert_eq!( + entry["response"]["outcome"]["issue"][0]["code"], "forbidden", + "entry {i}: {entry}" + ); } } } diff --git a/crates/rest/src/handlers/search.rs b/crates/rest/src/handlers/search.rs index d7adc31e9..29622bc26 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, { @@ -423,15 +446,15 @@ where // after subsetting so `_elements`/`_summary` don't strip the outcome. // `_summary=count` returns only `Bundle.total`, so there is no entry list to // attach it to. + // + // This sits below the `execute_search`/`execute_search_bundle` split so a + // search dispatched as a bundle entry reports its ignored parameters the + // same way the HTTP surface does. if !ignored_params.is_empty() && summary_mode != Some(SummaryMode::Count) { append_ignored_params_outcome(&mut bundle_json, &ignored_params); } - format_resource_response(StatusCode::OK, HeaderMap::new(), &bundle_json, format).map_err(|_| { - RestError::InternalError { - message: "Failed to serialize response".to_string(), - } - }) + Ok(bundle_json) } /// Appends a `search.mode = outcome` entry to a searchset bundle reporting the diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index f5568b441..8bdc96d8d 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -1325,8 +1325,14 @@ mod entry_methods { /// whether it arrives per-entry in a batch or as a whole-bundle transaction /// failure. Flattening the refusal at the transaction boundary would have /// made this 400 and re-created the divergence in a new place. + /// + /// They now agree on the issue code and the message too. #502 closed the + /// status half and named the rest as #504's: the batch entry said + /// `processing` while the transaction body said `not-supported`, and the + /// batch arm printed HEAD guidance the transaction arm dropped, because + /// `into_rest_error` computed a message and discarded it on that arm. #[tokio::test] - async fn the_two_arms_agree_on_the_refusal_status() { + async fn the_two_arms_agree_on_the_refusal_status_and_code() { let (server, _backend) = create_test_server().await; let batch = post_batch( @@ -1340,6 +1346,7 @@ mod entry_methods { batch["entry"][0]["response"]["status"], "405 Method Not Allowed" ); + let batch_issue = &batch["entry"][0]["response"]["outcome"]["issue"][0]; let transaction = post_bundle( &server, @@ -1351,5 +1358,177 @@ mod entry_methods { ) .await; transaction.assert_status(StatusCode::METHOD_NOT_ALLOWED); + let transaction_body: Value = transaction.json(); + let transaction_issue = &transaction_body["issue"][0]; + + assert_eq!(batch_issue["code"], transaction_issue["code"]); + assert_eq!( + batch_issue["details"]["text"], transaction_issue["details"]["text"], + "one refusal, one sentence" + ); + + // Pinned literally: equality alone is satisfied by both arms being + // wrong in the same way. + assert_eq!(batch_issue["code"], "not-supported"); + assert!( + batch_issue["details"]["text"] + .as_str() + .is_some_and(|t| t.contains("send HEAD to the instance endpoint")), + "both arms must keep the guidance: {batch_issue}" + ); + } +} + +// ============================================================================= +// 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" + ); } } diff --git a/crates/rest/tests/batch_entry_issue_codes.rs b/crates/rest/tests/batch_entry_issue_codes.rs new file mode 100644 index 000000000..6d5bab2ea --- /dev/null +++ b/crates/rest/tests/batch_entry_issue_codes.rs @@ -0,0 +1,254 @@ +//! Per-entry OperationOutcome issue codes on the batch path (#504). +//! +//! Every error a batch entry could produce was built by one helper that +//! hardcoded `"code": "processing"`, so a scope denial, a missing resource, a +//! malformed entry and an unsupported method were distinguishable only by +//! `response.status` and free-text English. These tests assert the code **on +//! the wire**, which the unit tests in `handlers::batch` cannot: they drive +//! `run_batch` directly and so never exercise the router or +//! `bundle_entry_result_to_json`'s placement of `outcome` under +//! `response.outcome`. +//! +//! A separate file rather than a module appended to `batch_conformance.rs`: +//! that file's tail already has several claimants across the open stack, and +//! the harness below is ~40 lines it duplicates twice internally anyway. + +use std::path::PathBuf; +use std::sync::Arc; + +use axum::http::{HeaderName, HeaderValue}; +use axum_test::TestServer; +use helios_fhir::FhirVersion; +use helios_persistence::backends::sqlite::{SqliteBackend, SqliteBackendConfig}; +use helios_persistence::core::ResourceStorage; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; +use helios_rest::ServerConfig; +use helios_rest::config::{MultitenancyConfig, TenantRoutingMode}; +use serde_json::{Value, json}; + +const X_TENANT_ID: HeaderName = HeaderName::from_static("x-tenant-id"); +const CONTENT_TYPE: HeaderName = HeaderName::from_static("content-type"); + +async fn create_test_server() -> (TestServer, Arc) { + let data_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(|p| p.parent()) + .map(|p| p.join("data")) + .unwrap_or_else(|| PathBuf::from("data")); + + let backend_config = SqliteBackendConfig { + data_dir: Some(data_dir), + ..Default::default() + }; + let backend = SqliteBackend::with_config(":memory:", backend_config) + .expect("Failed to create SQLite backend"); + backend.init_schema().expect("Failed to init schema"); + let backend = Arc::new(backend); + + let config = ServerConfig { + multitenancy: MultitenancyConfig { + routing_mode: TenantRoutingMode::HeaderOnly, + ..Default::default() + }, + base_url: "http://localhost:8080".to_string(), + default_tenant: "test-tenant".to_string(), + ..ServerConfig::for_testing() + }; + + let state = helios_rest::AppState::new(Arc::clone(&backend), config); + let app = helios_rest::routing::fhir_routes::create_routes(state); + let server = TestServer::new(app).expect("Failed to create test server"); + + (server, backend) +} + +fn test_tenant() -> TenantContext { + TenantContext::new( + TenantId::new("test-tenant"), + TenantPermissions::full_access(), + ) +} + +async fn seed_patient(backend: &SqliteBackend, id: &str) { + let patient = json!({ "resourceType": "Patient", "id": id, "active": true }); + backend + .create(&test_tenant(), "Patient", patient, FhirVersion::R4) + .await + .expect("Failed to seed patient"); +} + +async fn post_batch(server: &TestServer, entries: Vec) -> Value { + let response = server + .post("/") + .add_header(X_TENANT_ID, HeaderValue::from_static("test-tenant")) + .add_header( + CONTENT_TYPE, + HeaderValue::from_static("application/fhir+json"), + ) + .json(&json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": entries, + })) + .await; + response.assert_status_ok(); + response.json() +} + +/// Each failure class reaches the client with its own issue code. +/// +/// One bundle, one entry per reachable class. Before #504 the `code` column +/// below read `processing` for every row, so a client could only tell these +/// apart by parsing `details.text`. +#[tokio::test] +async fn a_batch_entry_reports_the_issue_code_for_its_failure() { + let (server, _backend) = create_test_server().await; + + // (entry, expected status prefix, expected issue code) + let cases: Vec<(Value, &str, &str)> = vec![ + ( + json!({ "request": { "method": "GET", "url": "Patient/ghost" } }), + "404", + "not-found", + ), + ( + // A lowercase verb is invalid instance data: `request.method` is a + // code with a required binding to `http-verb` (#502). + json!({ + "request": { "method": "post", "url": "Patient" }, + "resource": { "resourceType": "Patient" } + }), + "400", + "value", + ), + ( + json!({ "resource": { "resourceType": "Patient" } }), + "400", + "required", + ), + ( + json!({ "request": { "url": "Patient/p1" } }), + "400", + "required", + ), + ( + json!({ + "request": { "method": "PUT", "url": "Patient?identifier=x" }, + "resource": { "resourceType": "Patient" } + }), + "400", + "not-supported", + ), + ( + // Carries a `resource` deliberately: without one this is refused by + // the missing-resource guard first and would never reach the + // `id.is_empty()` guard this row names. + json!({ + "request": { "method": "PUT", "url": "Patient" }, + "resource": { "resourceType": "Patient" } + }), + "400", + "value", + ), + ( + json!({ "request": { "method": "HEAD", "url": "Patient/p1" } }), + "405", + "not-supported", + ), + ( + json!({ + "request": { "method": "PATCH", "url": "Patient/p1" }, + "resource": { "resourceType": "Patient" } + }), + "501", + "not-supported", + ), + ]; + + let entries: Vec = cases.iter().map(|(entry, _, _)| entry.clone()).collect(); + let body = post_batch(&server, entries).await; + let responses = body["entry"].as_array().expect("entry array"); + assert_eq!(responses.len(), cases.len()); + + for (index, (request, status, code)) in cases.iter().enumerate() { + let entry = &responses[index]; + let response = &entry["response"]; + + assert!( + response["status"] + .as_str() + .unwrap_or_default() + .starts_with(status), + "entry {index} ({request}) status: {response}" + ); + assert_eq!( + response["outcome"]["issue"][0]["code"], *code, + "entry {index} ({request}) issue code: {response}" + ); + // #504 changes the code, not the placement: the outcome stays under + // `response.outcome` and never becomes the entry resource. + assert_eq!( + response["outcome"]["resourceType"], "OperationOutcome", + "entry {index}: {response}" + ); + assert!( + entry.get("resource").is_none(), + "entry {index} must not carry the outcome as its resource: {entry}" + ); + } +} + +/// #504's stated impact, as an executable claim: the same failure, described +/// identically, whether it arrives at the resource endpoint or inside a Bundle +/// entry. +/// +/// Both bodies are now produced by `RestError::client_outcome`, so this is a +/// property of the code rather than a coincidence two mappings happen to share. +/// Before #504 the entry said `processing` with the text "Resource not found" +/// while the endpoint said `not-found` with "Resource Patient/ghost not found" +/// — a different code *and* different prose for one condition. +#[tokio::test] +async fn a_missing_resource_is_described_identically_by_both_surfaces() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1").await; + + let single: Value = server + .get("/Patient/ghost") + .add_header(X_TENANT_ID, HeaderValue::from_static("test-tenant")) + .await + .json(); + + let batch = post_batch( + &server, + vec![json!({ "request": { "method": "GET", "url": "Patient/ghost" } })], + ) + .await; + let response = &batch["entry"][0]["response"]; + + assert!( + response["status"] + .as_str() + .unwrap_or_default() + .starts_with("404"), + "entry status: {response}" + ); + + // Field by field. `details.text` is included deliberately: an issue code + // that agrees while the prose diverges is half a fix. + let endpoint_issue = &single["issue"][0]; + let entry_issue = &response["outcome"]["issue"][0]; + assert_eq!(entry_issue["severity"], endpoint_issue["severity"]); + assert_eq!(entry_issue["code"], endpoint_issue["code"]); + assert_eq!( + entry_issue["details"]["text"], + endpoint_issue["details"]["text"] + ); + + // Pinned literally, not just to each other: a regression that made *both* + // surfaces say `processing` would satisfy every assertion above. + assert_eq!(entry_issue["code"], "not-found"); + assert_eq!( + entry_issue["details"]["text"], + "Resource Patient/ghost not found" + ); +} diff --git a/crates/rest/tests/validation_enforcement_tests.rs b/crates/rest/tests/validation_enforcement_tests.rs index 38101978f..fc4d71e7c 100644 --- a/crates/rest/tests/validation_enforcement_tests.rs +++ b/crates/rest/tests/validation_enforcement_tests.rs @@ -139,6 +139,96 @@ async fn enforce_mode_rejects_invalid_batch_entries_individually() { ); } +/// The write validator's own issues reach a batch entry, not a flattened +/// sentence. +/// +/// `check_write` returns a fully-formed multi-issue OperationOutcome — one +/// issue per finding, each with its own code, severity and `expression` giving +/// the FHIRPath location of the element that failed. The batch arm used to walk +/// `issue[].details.text`, join the strings with `"; "`, and hand one sentence +/// to a wrapper that stamped `processing` over it: N coded, located issues +/// became one uncoded, unlocated issue. +/// +/// The transaction arm never did that — it propagates the same error from the +/// same call with a bare `?`, reaching `IntoResponse`'s pass-through. So an +/// identical resource returned typed codes and FHIRPath expressions as a +/// `transaction` and one English sentence as a `batch`, decided purely by +/// `Bundle.type` (#504). +/// +/// Complements `enforce_mode_rejects_invalid_writes_with_outcome` above, which +/// pinned the single-resource half and left the batch half asserting only a +/// status. +#[tokio::test] +async fn the_two_surfaces_report_the_same_validation_issues() { + let server = create_test_server("enforce").await; + + /// Every `(code, expression)` pair in an outcome, sorted. + fn issue_keys(outcome: &Value) -> Vec<(String, String)> { + let mut keys: Vec<(String, String)> = outcome["issue"] + .as_array() + .expect("issue array") + .iter() + .map(|i| { + ( + i["code"].as_str().unwrap_or_default().to_string(), + i["expression"][0].as_str().unwrap_or_default().to_string(), + ) + }) + .collect(); + keys.sort(); + keys + } + + let single: Value = server + .post("/Patient") + .json(&invalid_patient()) + .await + .json(); + + let batch: Value = server + .post("/") + .json(&json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "POST", "url": "Patient" }, + "resource": invalid_patient() + }] + })) + .await + .json(); + let entry = &batch["entry"][0]["response"]; + + assert!( + entry["status"] + .as_str() + .unwrap_or_default() + .starts_with("422"), + "entry status: {batch:#}" + ); + + let entry_outcome = &entry["outcome"]; + assert_eq!( + entry_outcome["resourceType"], "OperationOutcome", + "{batch:#}" + ); + + let single_keys = issue_keys(&single); + let entry_keys = issue_keys(entry_outcome); + assert_eq!( + entry_keys, single_keys, + "a batch entry must report the issues the resource endpoint reports\n\ + single: {single:#}\nentry: {entry_outcome:#}" + ); + + // Pinned literally, not just to each other: a regression that flattened + // *both* surfaces would satisfy the equality above vacuously. + assert!( + entry_keys.contains(&("structure".to_string(), "Patient.bogusElement".to_string())), + "the structural issue must survive with its FHIRPath location: {entry_outcome:#}" + ); +} + #[tokio::test] async fn stored_profile_registers_and_validates() { let server = create_test_server("off").await;