From efb6a9a5c2f7b5d1b2ce11d49e62e32806c5682b Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Thu, 6 Aug 2026 11:05:13 -0400 Subject: [PATCH] test(rest): pin the issue codes on #478's two search-entry failure paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The search-entry dispatch added two failure sites, one per bundle arm, and covered neither: all five of its tests assert `200 OK`. Both sites were written as `let (status, _, details) = e.client_response(); create_error_result( status.as_u16(), &details)` — the same code-discard #504 removed from every other call site. The rebase migrated them to `entry_failure`; this pins that so they cannot drift back. Three tests. A batch search entry failing on `_query` (400 `invalid`) and on `:not-in` (501 `not-supported`); the same failure inside a `transaction`; and parity with `GET [base]/Patient?_query=…`, asserting severity, code and `details.text` all agree plus a literal pin, since `processing == processing` would satisfy the equalities on its own. The transaction case is worth naming. #504 stated that no per-entry outcome is reachable on the transaction arm, because the backends discard an entry result at their `status >= 400` guard and return `TransactionError::BundleError`. That was true of the tree #504 landed on and is false here: this search loop bypasses the backend executor and surfaces the failure as that entry's own outcome — deliberately, since a search failure cannot roll back writes that already committed. So this is the first reachable per-entry outcome on that arm, and the first place its issue code is asserted. Verified non-vacuous: with both sites reverted to their original code-discard, all three fail — `left: String("processing"), right: "invalid"`. Also corrects a README bullet #504 added. It documented `GET Patient` in a batch entry as an instance read answering `404 not-found` with the message `Resource Patient/ not found`, and named executing it as a search as future work. That work is the commit below this one, so the bullet is now false and is removed rather than left to mislead. The per-entry code table gains a row for search-entry failures. Tests: 1117 pass. Refs #478, #504 --- crates/rest/README.md | 5 +- crates/rest/tests/batch_entry_issue_codes.rs | 141 +++++++++++++++++++ 2 files changed, 144 insertions(+), 2 deletions(-) diff --git a/crates/rest/README.md b/crates/rest/README.md index 795d88277..6a2f0227f 100644 --- a/crates/rest/README.md +++ b/crates/rest/README.md @@ -513,7 +513,8 @@ and `HEAD` are refused as described under Current Limitations. 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. +`GET [base]/Patient/missing` produce byte-identical OperationOutcomes, and so do a failed +search entry and the same search at `GET [base]/[type]?…`. | Entry failure | Status | `issue.code` | |---|---|---| @@ -527,6 +528,7 @@ single-resource request would return**, because both are rendered by one mapping | conditional criteria in the URL | 400 | `not-supported` | | insufficient scope | 403 | `forbidden` | | target not found | 404 | `not-found` | +| search entry failed (`_query`, `:not-in`, …) | per class | `invalid`, `not-supported`, … | | `HEAD` | 405 | `not-supported` | | `ifMatch` precondition failed | 412 | `conflict` | | write validation failed | 422 | the validator's own issues | @@ -576,7 +578,6 @@ The following FHIR transaction features are not yet implemented: - **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 diff --git a/crates/rest/tests/batch_entry_issue_codes.rs b/crates/rest/tests/batch_entry_issue_codes.rs index 6d5bab2ea..f5283813e 100644 --- a/crates/rest/tests/batch_entry_issue_codes.rs +++ b/crates/rest/tests/batch_entry_issue_codes.rs @@ -252,3 +252,144 @@ async fn a_missing_resource_is_described_identically_by_both_surfaces() { "Resource Patient/ghost not found" ); } + +// ============================================================================= +// Search entries (#478) +// ============================================================================= +// +// #478 added two failure sites — one per arm — and covered neither: all five of +// its tests are happy-path `200 OK`. Both were written with the same +// `let (status, _, details) = e.client_response()` code-discard #504 removed +// everywhere else, so without these a search entry would have been the one path +// left answering `processing`. + +/// Post `bundle` and return the parsed body, asserting HTTP 200. +async fn post_bundle_ok(server: &TestServer, bundle: Value) -> 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(&bundle) + .await; + response.assert_status_ok(); + response.json() +} + +/// A search entry that fails carries the issue code for *why* it failed. +#[tokio::test] +async fn a_failed_search_entry_reports_its_issue_code() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1").await; + + // (entry url, expected status prefix, expected issue code) + let cases: Vec<(&str, &str, &str)> = vec![ + // `_query` is a known-but-unimplemented control parameter. + ("Patient?_query=byName", "400", "invalid"), + // `:not-in` needs negated value-set filtering no backend implements. + ( + "Patient?code:not-in=http://example.org/vs", + "501", + "not-supported", + ), + ]; + + let body = post_batch( + &server, + cases + .iter() + .map(|(url, _, _)| json!({ "request": { "method": "GET", "url": url } })) + .collect(), + ) + .await; + + for (index, (url, status, code)) in cases.iter().enumerate() { + let response = &body["entry"][index]["response"]; + assert!( + response["status"] + .as_str() + .unwrap_or_default() + .starts_with(status), + "entry {index} ({url}) status: {response}" + ); + assert_eq!( + response["outcome"]["issue"][0]["code"], *code, + "entry {index} ({url}) issue code: {response}" + ); + } +} + +/// The transaction arm's search loop is the **first reachable** per-entry +/// outcome on that arm. +/// +/// #504 could accurately say none existed: the backends discard an entry result +/// at their `status >= 400` guard, so a transaction never surfaced a per-entry +/// outcome. #478's search loop bypasses the backend executor entirely and +/// surfaces the failure as that entry's own outcome — deliberately, since a +/// search failure cannot roll back writes that already committed. So this is a +/// per-entry code on the transaction arm, and it must not be `processing`. +#[tokio::test] +async fn a_failed_transaction_search_entry_reports_its_issue_code() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1").await; + + let body = post_bundle_ok( + &server, + json!({ + "resourceType": "Bundle", + "type": "transaction", + "entry": [{ + "request": { "method": "GET", "url": "Patient?_query=byName" } + }] + }), + ) + .await; + + assert_eq!(body["type"], "transaction-response", "{body}"); + let response = &body["entry"][0]["response"]; + assert!( + response["status"] + .as_str() + .unwrap_or_default() + .starts_with("400"), + "entry status: {response}" + ); + assert_eq!( + response["outcome"]["issue"][0]["code"], "invalid", + "{response}" + ); +} + +/// The same bad search, described identically by both surfaces — the parity +/// claim extended to the arm #478 added. +#[tokio::test] +async fn a_failed_search_is_described_identically_by_both_surfaces() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1").await; + + let single: Value = server + .get("/Patient?_query=byName") + .add_header(X_TENANT_ID, HeaderValue::from_static("test-tenant")) + .await + .json(); + + let batch = post_batch( + &server, + vec![json!({ "request": { "method": "GET", "url": "Patient?_query=byName" } })], + ) + .await; + let entry_issue = &batch["entry"][0]["response"]["outcome"]["issue"][0]; + let endpoint_issue = &single["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: `processing == processing` would satisfy the equalities. + assert_eq!(entry_issue["code"], "invalid"); +}