fix(rest): render batch entry failures through the single-resource error mapping - #516
fix(rest): render batch entry failures through the single-resource error mapping#516aacruzgon wants to merge 5 commits into
Conversation
cc5962c to
076dbc4
Compare
94863c6 to
c2bf9a1
Compare
c2bf9a1 to
5605ed9
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
5605ed9 to
6891b70
Compare
6891b70 to
a162abc
Compare
a162abc to
01929ed
Compare
smunini
left a comment
There was a problem hiding this comment.
Review
The core refactor is genuinely good work. RestError::client_outcome() as the single funnel is the right fix, not the issue's suggested "add an issue-code argument" — the let (status, _code, message) discard in entry_error was the actual defect, and deleting it corrects five call sites by construction. The 403 processing → 403 forbidden row matters most (forbidden is-a security; processing is not an ancestor of it in any version). Threading real codes also surfaced two latent bugs the PR fixes: status_text had no arm for 413/429/503/504, and extract_outcome_description never read diagnostics, so a failed ifMatch produced an AuditEvent with no outcomeDesc at all. The disable-runs proving the tests non-vacuous are the right discipline.
However, I'd hold the merge on one finding — and it's in the #481 commit this branch carries, not in the #504 work.
Verification I ran
cargo fmt --all --check→ clean (verified on55f6fd3a1, not onmain).cargo test -p helios-rest --all-features→ 971 passed, 1 failed. The one failure is environmental:sof_conformance_mongodb_tests::test_sof_v2_conformance_in_db_mongodbpanics withSocketNotFoundError("/var/run/docker.sock")— no Docker daemon locally. Every new batch/issue-code and search-entry test passes.
Blocking: bundle GET search entries are authorized with the READ scope, not SEARCH
bundle_method_to_fhir_operation (crates/rest/src/handlers/batch.rs:1797) maps every BundleMethod::Get to FhirOperation::Read, and both scope gates use it — the transaction arm at :459 and the batch arm at :809.
Before the 01929ed56 commit this was harmless: a type-level GET Patient?family=X entry was a read of an empty id and answered 404. It now executes a real type-level search.
crates/auth/src/policy/mod.rs:31mapsRead→SmartPermissions::READandSearch→SEARCH, and these are distinct bits — SMART v2.rdoes not imply.s(crates/auth/src/scope/permissions.rs:41).crates/rest/src/middleware/auth.rs:416classifiesGET /Patient?...(one path segment) asFhirOperation::Search.middleware/auth.rsexplicitly defers bundle authorization to the handler ("the same 'defer to handler' pattern batch and $export already use"), so this per-entry check is the only gate.
Concrete scenario: a SMART token with user/Patient.r (read, no search) is denied at GET [base]/Patient?family=X, but gets the full searchset by wrapping that exact URL in a batch entry. Scope escalation. The inverse also breaks — a .s-only token gets a spurious 403 on a bundle search entry.
This is precisely the batch-vs-single divergence class this PR stack exists to close, so it seems in-scope to fix here rather than defer. No existing test asserts that a bundle search entry is gated on the same permission as the equivalent HTTP request.
Should fix
parse_search_entry_url accepts any single path segment as a resource type (batch.rs:1498). The match arm is [resource_type] => Some(...) with no validity check, and nothing on the search path validates the type either. GET metadata (an entry form the spec sanctions), GET $export, or a typo'd GET Patinet now run a search on that string and return 200 OK with an empty searchset. Previously they answered 404. A false 200 is strictly worse for a client — it can't distinguish "nothing matched" from "no such endpoint".
The README limitation added by commit 4 is falsified by commit 5 of the same PR. crates/rest/README.md:579 still reads:
Bare type-level
GET-GET Patientin a batch entry is read as an instance read with an empty id and answers404 not-found… Executing it as a search is #478
01929ed56 implements exactly that. The PR body acknowledges the claim "is simply false here now", but the line wasn't removed.
Prefer: handling=strict is ignored for bundle search entries. Both execute_search_bundle call sites (batch.rs:600, :862) hardcode strict = false, while batch_handler already has the parsed PreferHeader in hand. GET Patient?famly=X (typo) in a strict-handling bundle returns 200 OK with an unfiltered searchset; the identical GET [base]/Patient?famly=X with the same header returns 400. New divergence in the same family this PR closes.
Audit events for search entries name the wrong entity type. emit_entry_audit derives resource_type from the entry URL, then unconditionally overrides it from result.resource["resourceType"] (batch.rs:1190). A searchset serializes as "resourceType": "Bundle", so an AuditEvent for GET Patient?family=X inside a Bundle records the accessed type as Bundle instead of Patient. extract_patient_from_resource("Bundle", …) also returns None, so the patient-compartment linkage is lost. Relevant given the BALP profiles.
Nits / discussion
batch.rs:522— the transaction search pre-validation doesmap_err(|e| RestError::BadRequest { message: format!("… {}", e.client_response().2) }), discarding the status and issue code the inner error computed and re-coding everything as400 invalid. That is the exact_code-discard pattern this PR deletes in five other places.batch.rs:600— a search entry failing after the transaction commits surfaces as a per-entry outcome inside a200 OKtransaction-response. Defensible (the writes already committed; there's no rollback available) and the code comment argues it well, but a client checking only the HTTP status — the normal transaction contract — silently drops the failure. Worth stating in the README's limitations rather than only in a comment.batch.rs:1854—return=minimalomitsentry.resource, which now strips searchset bodies. Pre-existing for instance reads, but writes + searches in one bundle is exactly the case where a client sendsreturn=minimal.- The search-param registry read lock is re-acquired per entry inside the transaction pre-validation loop; it could be hoisted.
- Untested edge: a transaction whose entries are all searches passes an empty entry list to
execute_bundleafter the partition. No test covers it.
Process
Two scope observations, offered as a question rather than a request:
- The PR body is admirably honest that this branch carries #481 — but a PR titled
fix(rest): render batch entry failures through the single-resource error mappingcontaining a+393/-16behavioral feature, which is also where the one blocking finding lives, is hard to review as a unit. That the search commit's two_-discard sites became compile errors is a nice validation of the refactor, but it doesn't require shipping both together. Would splitting #481 back out be feasible? - Commit
55f6fd3a1"style: rustfmt" reformatscrates/subscriptions/*andcrates/ui/*, unrelated to either PR. Those files already passrustfmt 1.9.0-stableonmain, so this looks like output from a different rustfmt version — worth pinning down before it churns back the other way.
Cherry-picked from the HeliosSoftware#501..HeliosSoftware#504 stack (PR HeliosSoftware#481, commit 32b3766) onto main, so HeliosSoftware#478's fix ships without waiting for the issue-code refinement chain (HeliosSoftware#516/HeliosSoftware#518) to clear review. The conflict resolution, recorded because it is more than textual: - Search entries partition out of indexed_entries *before* HeliosSoftware#459's conditional-reference resolution, which runs on the write entries only — a GET entry's query string is a search, not a conditional reference. - entry_failure lands as a single seam in main's current style: the status/details pair from client_response rendered through create_error_result. HeliosSoftware#516 upgrades exactly this function to the full OperationOutcome issue-code mapping when it lands; its scope is untouched. - The rollback fan-out keeps main's message-based result and gains this commit's .chain(&search_entries) — every entry of a failed bundle owes the audit trail a record, searches included. - execute_search_bundle keeps main's ignored-params outcome (HeliosSoftware#460-era lenient-handling reporting) and returns the bundle JSON; the HTTP wrapper formats. Verified live: batch and transaction bundles with GET Patient?family=X entries return 200 searchset entries, and a transaction's search sees the bundle's own committed writes. Full helios-rest suite green (34 binaries; batch_conformance 59 including this commit's five). Closes HeliosSoftware#478 Co-authored-by: Angela Valdez <angela@heliossoftware.com>
55f6fd3 to
f24eeba
Compare
|
@smunini — on whether #635 landing makes this PR unnecessary: the two target different defects that happen to sit on the same path. PR #635 (angela-helios) fixes what a bundle PR #516 (mine) fixes how a bundle entry failure is described. Every batch entry error was built by a helper that hardcoded the OperationOutcome issue code to Where they touch. Both edit the batch handler, and her PR had to introduce |
f0a25a6 to
6ac2971
Compare
…ror mapping Every error a batch entry can produce was built by one helper that hardcoded its issue code, so a scope denial, a missing resource, a malformed entry and an unsupported method all reached the client as `"code": "processing"`, distinguishable only by `response.status` and free-text English. `OperationOutcome.issue.code` is bound `required` to `http://hl7.org/fhir/ValueSet/issue-type` in all four bundled versions, and the ElementDefinition adds an unqualified SHALL: "The system that creates an OperationOutcome SHALL choose the most applicable code from the IssueType value set". Nineteen call sites emitting one code is the absence of a choice. The fix is not an issue-code argument on `create_error_result`. The defect is that `create_error_result` existed at all: a second renderer of failure-to-OperationOutcome, written beside the one `IntoResponse` already uses. Giving it an argument keeps two renderers agreeing by hand — the arrangement #502 died of. So the second renderer is deleted. `RestError::client_outcome` becomes the only place a `RestError` becomes an OperationOutcome; `IntoResponse` renders the pair as an HTTP body and `handlers::batch` as a `Bundle.entry.response.outcome`. Each call site now constructs the same `RestError` its transaction twin already constructs, so the two arms cannot report different codes for one failure because there is no second table to disagree with. `entry_error` goes with it — it called `client_response()`, bound the correct code to `_code` and discarded it, after which the wrapper stamped `processing` over the result. Deleting that one underscore corrects five sites by construction and makes `client_response`'s own doc comment ("shared by IntoResponse and the batch/transaction handler so both sanitize identically") true for the first time. `EntryMethodRefusal::status()` is deleted too. #515 made the arms agree on the refusal status by writing the number twice — once there, once implicitly in `into_rest_error`'s choice of variant — and pinning the copies with a test. There is now one function, so the status and the code are decided once. The same move gives HEAD one message instead of two: `into_rest_error` computed a message and discarded it on that arm, so the batch arm printed guidance the transaction arm never showed. It now rides in `resource_type`, which `MethodNotAllowed` renders, and both arms print it. `EntryParseError::Malformed(String)` becomes `MissingRequest`/`MissingUrl`, moving its two strings into helpers both arms call — an absent `request.url` was caught by `parse_bundle_entry` on one arm and by `unwrap_or("")` on the other, with different text for the same input. Two new `RestError` variants, both children of `BadRequest`'s `invalid` in the `issue-type` hierarchy rather than alternatives to it: `MissingElement` (400 + `required`) for an absent mandatory element, and `InvalidElementValue` (400 + `value`) for one present and unusable. The crate could say neither before. `MissingElement` is used only where the SD gives `min=1` — `request` (mandatory by bdl-3 in R4/R4B and transitively by bdl-3c in R5/R6), `request.method` (1..1), `request.url` (1..1) — and deliberately NOT for an absent `Bundle.entry.resource`, which is 0..1 with only R5/R6's bdl-3c requiring it: a call site serving four versions must not assert a rule two of them lack. The invariant keys stay in doc comments and off the wire, because `bdl-3` does not exist in R5 or R6. On the element this writes into. `Bundle.entry.response.outcome` carries a comment, byte-identical in all four bundled versions and generated into the model at `crates/fhir/src/r4.rs:10011`: "This outcome is not used for error responses in batch/transaction, only for hints and warnings. In a batch operation, the error will be in Bundle.entry.response". Four things about it. It is a `comment`, not an invariant — no `bdl-*` constrains that element. HFS has placed error outcomes there since before this stack, pinned by `test_batch_error_outcome_in_response_not_resource`, so this commit changes the code and not the placement. "The error will be in Bundle.entry.response" holds: `response.status` still carries it and `outcome` is a sibling under the same `response`, not a substitute. And the SHALL applies to any OperationOutcome the system creates — if HFS creates one here it must code it correctly regardless of whether it was obliged to create one. Tests: a twelve-row table over the mapping, a cross-arm test asserting both arms agree on status, code AND message for every method refusal, four assertion-only extensions to the existing #515/#512 refusal tests, and a new integration file asserting the code on the wire for every reachable class plus byte-identical parity with `GET [base]/Patient/ghost`. Verified non-vacuous. With `entry_failure` reverted to a hardcoded `processing` outcome while every call site keeps its new argument, 8 unit tests and 3 integration tests fail — `left: String("processing"), right: "forbidden"`, `right: "not-found"`, `right: "required"`, `right: "exception"`. The refusal tests keep their teeth for free: `DelayStorage`'s write methods are `unimplemented!()` and `peak() == 0` is still asserted, so a refusal moved after dispatch panics rather than merely reporting a different code. Closes #504
…tch entry
`check_write` returns `RestError::ValidationFailed { outcome }` carrying a
fully-formed multi-issue OperationOutcome: one issue per validator finding,
each with a code computed precisely (`Required` -> `required`,
`FixedValue`/`PatternValue`/`PrimitiveValue` -> `value`, `FhirpathConstraint`
-> `invariant`, `TerminologyBinding` -> `code-invalid`,
`UnknownSchema`/`UnknownProfile` -> `not-supported`, and twelve structural
kinds -> `structure`), its own severity, and an `expression` giving the
FHIRPath location of the element that failed.
The batch arm flattened all of it. `validation_failure_message` walked
`issue[].details.text`, joined the strings with "; ", and handed one sentence
to a wrapper that stamped `processing` over it. N coded, located issues became
one uncoded, unlocated issue.
The transaction arm never did this. It propagates the same error from the same
call with a bare `?`, reaching the branch whose comment already states the
rule: "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." So an identical bundle
carrying an identical invalid resource returned typed codes and FHIRPath
expressions as a `transaction` and one English sentence as a `batch`, decided
purely by `Bundle.type`. This is not a new capability; it is the batch arm
being brought into line with a decision this crate documented and then applied
to two of its three write paths.
`validation_failure_message` is deleted with no replacement — the flattening
was its entire job. The interception lives in `RestError::client_outcome`,
above `client_response`, and that placement is load-bearing rather than
stylistic: `client_response`'s own `ValidationFailed` arm returns `(422,
"processing", "Resource validation failed")`, so any design that routes the 422
through the code table reproduces this defect with a shorter message. Putting
the special case in the funnel means no future caller can re-flatten it.
Nothing in helios-persistence changes. `BundleEntryResult.outcome` has always
been `Option<Value>` and `BundleEntryResult::error` has always taken an
arbitrary `Value`, so `validation_failure_message`'s doc-comment premise —
"batch entry outcomes are message-based" — was false at the type level, and
that false premise was the bug.
Known amplification, named rather than mitigated: `validation_outcome` is
uncapped, so an entry with N findings now carries N issues where it carried
one. That is exact parity with `POST [base]/[type]`, which has had the same
exposure since enforce mode shipped. Any bound belongs in `validation_outcome`,
covering both surfaces at once; a batch-only cap would re-create the very
divergence this commit closes.
Tests: `the_two_surfaces_report_the_same_validation_issues` posts the same
invalid resource to `POST /Patient` and as a one-entry batch and asserts the
two `(code, expression)` sets are equal, plus the literal pin that the set
contains `("structure", "Patient.bogusElement")` — without the pin, a
regression flattening *both* surfaces would satisfy the equality vacuously.
It is also the first coverage of the batch half of
`enforce_mode_rejects_invalid_writes_with_outcome`, which pinned the
single-resource half and left the batch half asserting only a status. A unit
test adds the ordering guarantee: `DelayStorage::create` is `unimplemented!()`
and `peak() == 0`, so a validation failure moved after dispatch panics rather
than quietly writing.
Verified non-vacuous, and independently of the previous commit's disable run.
With `validation_failure_message` restored at both sites:
left: [("processing", "")]
right: [("structure", "Patient.bogusElement")]
The two disable runs isolate different mechanisms: reverting `entry_failure`
to a hardcoded `processing` while keeping the `ValidationFailed`
short-circuit leaves this test green (7 passed), and restoring the flattener
fails only this one.
Refs #504
…iagnostics Two one-line inconsistencies that threading the issue codes exposes, fixed here rather than folded into the mechanism so each is reviewable on its own. `status_text` has no arm for 413, 429, 503 or 504, all four of which a batch entry can return with a correct issue code: `BackendError::PoolExhausted`/`Unavailable`/`ConnectionFailed` map to 503 `transient` and `BackendError::Timeout` to 504 `timeout`. An entry hitting an exhausted pool rendered `"status": "503 Unknown"` beside `"code": "transient"`, which is incoherent once the code is right. Nothing noticed while every entry carried `processing`: `test_status_text_covers_known_and_unknown_codes` enumerates the mapped codes and asserts 418 and "" fall through, but never checked that a code an entry can actually produce has a phrase. It does now. `extract_outcome_description` reads only `issue[0].details.text`, but the one batch entry outcome #504 deliberately does not touch — the 412 from `helios_persistence::core::preconditions::precondition_failed_entry` — writes its text into `diagnostics`. So a failed `ifMatch` produced an AuditEvent with no `outcomeDesc` at all. A `diagnostics` fallback closes that with no wire change; `details.text` still wins when both are present. The underlying shape split stands: eighteen outcomes on `details.text`, one on `diagnostics`. Unifying it means editing helios-persistence and re-baselining `batch_if_match.rs`, and is named as a non-goal rather than smuggled in here. That the 412 gate's own tests pass untouched — `batch_if_match.rs:152` still asserts `conflict` — is the evidence #504 did not over-reach into a code that was already correct. Tests: the `status_text` table gains the four codes plus a loop asserting no mapped code falls through; the audit fallback is asserted directly against `precondition_failed_entry`'s outcome. Verified non-vacuous — reverting the four arms fails with `left: "Unknown", right: "Service Unavailable"`, and removing the `.or_else` fails with `left: None, right: Some("stale tag")`. Refs #504
The Error Handling section shows a single-issue OperationOutcome with `"code": "not-found"` and says nothing about bundle entries, which until now could not produce that code. Adds a "Per-entry outcomes" subsection stating the contract the code now enforces: 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; and an entry that fails enforce-mode write validation carries the validator's own multi-issue outcome, with per-issue codes, severities and `expression` locations, exactly as `POST [base]/[type]` does. Includes the full table of codes a batch entry can emit, and notes that `required` and `value` are children of `invalid` so a reader can see why three 400s carry three codes. Two Current Limitations bullets gain their codes — conditional interactions are `400 not-supported` in both arms (the same status *and* code, not merely the same status), and HEAD is `405 not-supported`. Two bullets are added for things this work does not fix, so a reader cannot infer they were closed. A transaction entry that fails after dispatch is still collapsed to `400 processing` with the real status stringified into the message, because the backends discard the entry result at their `status >= 400` guard and return `TransactionError::BundleError`, which carries neither status nor code. And a bare type-level `GET Patient` in a batch entry is read as an instance read with an empty id, answering `404 not-found` with the message "Resource Patient/ not found" — a wart on the arm #478 is about to rewrite as a search, left there rather than patched around. Tests: none — documentation only. The CI-skip marker this repo uses on docs-only pushes is deliberately omitted: this commit is the tip of a branch carrying code commits, and GitHub reads that directive from the HEAD commit of the push, so it would suppress CI for the whole PR. Refs #504
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
6ac2971 to
b5b15fd
Compare
Summary
Every error a batch entry could produce was built by one helper that hardcoded its OperationOutcome
issue code, so a scope denial, a missing resource, a malformed entry and an unsupported method all
reached the client as
code: "processing"— distinguishable only byresponse.statusand free-textEnglish.
OperationOutcome.issue.codeis bound required tohttp://hl7.org/fhir/ValueSet/issue-typein all four bundled versions, and the ElementDefinition adds an unqualified SHALL: "The system that
creates an OperationOutcome SHALL choose the most applicable code from the IssueType value set."
Nineteen call sites emitting one code is the absence of a choice.
The issue proposes an issue-code argument on
create_error_result. That treats the symptom. Thedefect is that
create_error_resultexisted at all: a second renderer of failure-to-OperationOutcome,written beside the one
IntoResponsealready uses.entry_error(batch.rs:1533) is the proof —— it computed the correct code and bound it to
_code, after which the wrapper stampedprocessingover the result. And
client_response's own doc comment already claims the parity that was missing:"the single source of truth for how a
RestErroris surfaced to callers, shared byIntoResponseand the batch/transaction handler so both sanitize identically." That sentence was false.
So the second renderer is deleted rather than parameterised. Giving it an argument keeps two mappings
agreeing by hand — the arrangement #502 died of.
Rebased onto main after #635
#481 (search-style
GETentries) reachedmainthrough #635 on 2026-08-21, so this branch nolonger carries it: the search commit and the unrelated rustfmt commit were dropped, and the four
#504 commits were re-applied on current
main. Diff is now 6 files, all incrates/rest.Reconciliations folded into the commits:
RestError::MultiIssue(main's request-level 400 pass-through) moved intoclient_outcomebeside
ValidationFailed, soIntoResponsestill has no renderer of its own.EntryParseErrorgainedMalformedUrlfor main'scanonical_bundle_mutation_urlcheck,rendered as
InvalidElementValue— the batch arm's existing mapping for the same parser error.BadRequestfromclient_response().2; it now keeps the error's own variant with the entry named in front.GET" README limitation is removed; fix(rest): execute search-style GET bundle entries as searches #635 implements it.#518's single commit (three tests pinning the issue codes on the two search-entry failure sites) is
folded in as the fifth commit, since its only reason to be a separate PR was the #481 stacking. #518
is closed.
cargo test -p helios-rest --no-fail-fast: 1289 passed, 0 failed across 41 binaries, plus #518'sthree on top. CI's clippy
line scoped to
helios-rest: clean.Rebased again onto main after #511
#511 (conditional interactions in batch Bundle entries) reached
mainon 2026-09-02 and lands in theone expression this PR's first commit rewrites, so the five commits were re-applied on current
main.#511 removed the failure this PR was recoding. The blanket refusal of URL criteria — the
not-supportedrow in the table below — is gone: aPUT [type]?[criteria]orDELETE [type]?[criteria]entry is now resolved against the backend. What survives is the narrower set #511still declines, and those needed codes that had never been chosen:
POSTentry's URLvaluePatient?&)valueifMatchon a conditional entryinvalidNot
not-supportedfor any of them, and the reasoning is the reason this PR exists.not-supportedisfor a spec-defined interaction the server declines — which is what the old blanket refusal was, and it
invited the client to retry elsewhere. FHIR defines no
POST [type]?[criteria]at all, andPatient?&decodes to nothing: those are urls whose value is unusable, so
value. TheifMatchpairing isdifferent again — both elements are individually well-formed and the fault is the combination, so
invalid, the parent, is as precise as the fault allows. Picking a child code where none applies wouldbe the same defect as picking
processingfor everything.Reconciliations folded into the commits:
entry_failurewas a deliberate placeholder — its doc comment names this PR: "thestacked issue-code refinement (fix(rest): render batch entry failures through the single-resource error mapping #516) upgrades this to the full
OperationOutcomemapping; until itlands, entries keep main's message-based outcomes." Commit 1 replaces its body with the
client_outcomefunnel, so every call site batch/transaction: conditional interactions are refused rather than resolved — wire bundle entries to ConditionalStorage #511 already routed through it is corrected for free.entry_error(e)+create_error_result(status, &message)pairs (the threeConditional*Resulterror arms and the conditional-updatecheck_write) becomeentry_storage_failure(e)andentry_failure(e). As predicted, deletingcreate_error_resultmadethese a compile error rather than a silent reintroduction of
processing.conditional_entries_that_fhir_leaves_undefined_are_refused_before_storageis batch/transaction: conditional interactions are refused rather than resolved — wire bundle entries to ConditionalStorage #511's,renamed by batch/transaction: conditional interactions are refused rather than resolved — wire bundle entries to ConditionalStorage #511 for exactly this narrowing; its five entries now assert
["value", "invalid", "invalid", "invalid", "value"]per entry rather than one code for all five.PUT Patient?identifier=xrow is no longer a failure at all under batch/transaction: conditional interactions are refused rather than resolved — wire bundle entries to ConditionalStorage #511, soit is replaced by the two rows that still fail: criteria on a
POST, andifMatchon a conditionalentry.
issue-code table above it is this PR's.
cargo test -p helios-rest --all-features --no-fail-fast: 1389 passed, 0 failed across 41 binaries.cargo fmt --all --check: clean. CI's clippy line (ci.yml:552, with its eight-Aflags) scoped tohelios-rest: clean, exit 0.Rebased onto current main (
fa33d5ddc)The branch had fallen 41 commits behind. Re-applied on current
main; no reconciliations wereneeded this time. None of the 41 commits touch this PR's surface — the only
crates/restchangesamong them are
src/config.rs(+17) and two new integration files (tests/sof_export.rs,tests/transaction_bundle_search_parameter.rs), and the five files this PR edits are untouched onthe main side.
Verified mechanical rather than asserted:
git diff <old-merge-base> <old-head>andgit diff main <new-head>are byte-identical (2114 lines each). The change set that was reviewedis the change set that is now on top of
main— nothing was dropped, absorbed, or re-resolved.Rebased onto current main (
0cdbb00af)The branch had fallen 69 commits behind, and the stack it belonged to is gone: #512 and #515 merged,
#518 closed once its commit was folded in here as the fifth commit. Nothing sits above or below this
PR any more — it is a single branch on
main, so this rebase is a plain replay rather than a cascade.Re-applied on current
main; no conflicts and no reconciliations. None of the 69 commits touchthis PR's surface — the
crates/restchanges among them aresrc/config.rs(+7), sevensingle-resource handlers picking up
content_with_meta(create,history,patch,read,subscription_event,update,vread), andtests/search_integration.rs(+90). The six files thisPR edits are untouched on the main side.
One near-miss worth naming, because its commit message says "batch/transaction responses": main's
90b85296dalso editspersistence::core::transaction, but only the twoBundleEntryResultconstructors that carry a stored resource —
resource: Some(...),outcome: None. This PR writesoutcomeon entries that have no resource. The two changes are disjoint by construction, not by luck.Verified mechanical rather than asserted:
git diff <old-merge-base> <old-head>andgit diff main <new-head>are byte-identical (2114 lines each). The change set that was reviewedis the change set that is now on top of
main— nothing was dropped, absorbed, or re-resolved.cargo fmt --all --check: clean. The suite recorded below was not re-run locally against thisbase; CI is the gate for it.
Changes
One funnel.
RestError::client_outcome()is now the only place aRestErrorbecomes anOperationOutcome.
IntoResponserenders the pair as an HTTP body;handlers::batchrenders it as aBundle.entry.response.outcome. Neither builds its own. Four functions deleted —create_error_result,entry_error,validation_failure_message,EntryMethodRefusal::status()—two added. Each call site constructs the same
RestErrorits transaction twin already constructs,so the arms cannot report different codes because there is no second table to disagree with.
Two new
RestErrorvariants, children ofBadRequest'sinvalidin theissue-typehierarchyrather than alternatives to it:
MissingElement(400 +required) andInvalidElementValue(400 +
value). The crate could say neither before.MissingElementis used only where the SD givesmin=1; deliberately not for an absentBundle.entry.resource, which is 0..1 with only R5/R6'sbdl-3crequiring it — a call site serving four versions must not assert a rule two of them lack.Invariant keys stay in doc comments and off the wire, because
bdl-3does not exist in R5 or R6.requestabsentprocessingrequiredrequest.methodabsentprocessingrequiredrequest.methodnot an http-verb codeprocessingvaluerequest.urlabsentprocessingrequiredrequest.urlnames nothingprocessingvalueresourceabsent on POST/PUTprocessinginvalidprocessingvaluePOSTentry's URLprocessingvalueprocessingvalueifMatchon a conditional entryprocessinginvalidprocessingforbiddenprocessingnot-foundprocessingnot-supportedprocessingprocessingnot-supportedprocessingclient_responsecomputedifMatchfailedconflictThe 403 is the sharpest row:
forbiddenis a child ofsecurity, andprocessingis not an ancestorof it in any version, so a client filtering
code is-a securityto trigger re-auth got a falsenegative. Four other emitters in this codebase already say
forbiddenfor the identical denial.The 422 is the lossiest case.
check_writereturns a fully-formed multi-issue outcome — per-issuecode, severity and
expression(the FHIRPath location). The batch arm joinedissue[].details.textwith
"; "and re-wrapped it underprocessing. The transaction arm never did: it propagates with abare
?, reaching the branch whose comment already states the rule. So an identical bundle returnedtyped codes and FHIRPath expressions as a
transactionand one English sentence as abatch, decidedpurely by
Bundle.type. The interception now lives inclient_outcomeaboveclient_response,which is load-bearing:
client_response's ownValidationFailedarm returns(422, "processing", "Resource validation failed"), so routing the 422 through the code table would reproduce the defectwith a shorter message.
Two exposed inconsistencies (commit 4):
status_texthad no arm for 413/429/503/504, all reachable,so an exhausted pool rendered
"503 Unknown"beside a correcttransient; andextract_outcome_descriptionread onlydetails.text, while the 412 gate writesdiagnostics— so afailed
ifMatchproduced an AuditEvent with nooutcomeDescat all.On the element this writes into
Bundle.entry.response.outcomecarries a comment, byte-identical in all four bundled versions andgenerated into the model (
crates/fhir/src/r4.rs:10011):Four things. It is a
comment, not an invariant — nobdl-*constrains that element, verified byenumerating every Bundle constraint in all four versions. HFS has placed error outcomes there since
before this stack, pinned by
test_batch_error_outcome_in_response_not_resource, so this changes thecode and not the placement. "The error will be in
Bundle.entry.response" holds:response.statusstill carries it and
outcomeis a sibling under the sameresponse. And the SHALL applies to anyOperationOutcome the system creates — if HFS creates one here it must code it correctly regardless of
whether it was obliged to create one.
Testing
Six net-new tests, four compile-forced rewrites, five assertion-only extensions.
Verified non-vacuous, with the two disable runs isolated from each other:
entry_failurereverted to a hardcodedprocessingoutcome,ValidationFailedshort-circuit kept: 8 unit + 3 integration tests fail, e.g.
left: String("processing"), right: "forbidden"/"not-found"/"required"/"exception". The 422 leg stays green (7 passed),which is what makes the two runs independent.
validation_failure_messagerestored at both sites: exactly one test fails,left: [("processing", "")],right: [("structure", "Patient.bogusElement")].DelayStorage's writes areunimplemented!()andpeak() == 0is still asserted, so a refusal moved after dispatch panics rather than reporting adifferent code.
batch_if_match.rs:152passing untouched is the evidence of restraint — the 412'sconflictwasalready correct and was deliberately not routed through the funnel.
Clippy: CI's invocation (
ci.yml:497, which carries-A collapsible_ifand seven sibling allow-flags) passes clean — exit 0, zero errors. Worth correcting explicitly, because this PR previously repeated the parent PRs' claim that the gate "still exits 101 on pre-existing lints tracked in #513": that is true of the barecargo clippy --all-targets --all-features -- -D warnings, but not of what CI runs. All 42crates/restwarnings and all six files that fail the bare form sit inside the allowed lint families, so the Linting job was never going to fail. Separately measured:crates/restlint set is 42 → 42, zero introduced, zero removed against the base with forced recompilation.Notes
"Resource not found"→
"Resource Patient/ghost not found", which is what makes byte-identical parity with the resourceendpoint possible), HEAD (both arms now print the guidance the batch arm printed and the transaction
arm dropped), and PATCH (
NotImplementedwraps its feature string). Every other message isbyte-identical, including all of fix(rest): parse bundle entry URLs without their query string #512's. Nothing anywhere asserts on entry prose.
processing— a 403, 404 and 405 are indistinguishable without parsing English #504 fixes uniformly"; fix(rest): parse bundle entry methods through one shared matcher #515promised "batch: every per-entry error carries OperationOutcome code
processing— a 403, 404 and 405 are indistinguishable without parsing English #504's stated fix, and landing it here would take that PR's mechanism early."that
a_transaction_get_with_a_query_is_not_declined_by_the_query_guardcannot fail: its thirdconjunct reads
issue[0].diagnostics, whichRestErrornever emits, so the negated assert holdseven when the guard does fire. That test belongs to fix(rest): parse bundle entry URLs without their query string #512's
f7a87938c, so the one-field fix landedthere as a follow-up commit instead of here — blame stays with the PR that owns the test, and fix(rest): parse bundle entry URLs without their query string #512
stays a fast-forward with its in-flight review intact. fix(rest): parse bundle entry methods through one shared matcher #515 and this branch were restacked on top.
fix(rest): execute search-style GET bundle entries as searches #635"), and batch/transaction: conditional interactions are refused rather than resolved — wire bundle entries to ConditionalStorage #511 through the section after it — the second one landed in the exact expression
commit 1 rewrites, and the compile error predicted below is what caught it.
It inserts a search branch immediately above the GET arm's
refusal at
:751— the one expression this PR changes on that arm — and adds two call sites withthe
_code-discard, one of them on the transaction arm. That second one falsifies "no transactionper-entry outcome is reachable", which is true of the current tree only. Both become
entry_failure(e)on rebase, and deletingcreate_error_resultmakes that a compile errorrather than a silent reintroduction of
processing. fix(transaction): resolve conditional references before execution #467 lands afterstatus_text, which commit 4also edits. Integration coverage went in a new file so batch: every per-entry error carries OperationOutcome code
processing— a 403, 404 and 405 are indistinguishable without parsing English #504 is not a fourth claimant onbatch_conformance.rs's append anchor.validation_outcomeis uncapped, so a 422 entry now carries N issues whereit carried one. Exact parity with
POST [base]/[type], unbounded since enforce mode shipped. Any capbelongs there, covering both surfaces; a batch-only cap would re-create the divergence this closes.
collapsed to
400 processingbyTransactionError::BundleError, which carries neither status norcode — the backends discard the entry result at their
>= 400guard.Both halves of this bullet were narrowed once fix(rest): execute search-style GET bundle entries as searches #481 landed on this branch. It no longer holds for
search entries: fix(rest): execute search-style GET bundle entries as searches #481's search loop bypasses the backend executor, so a failed search surfaces as that
entry's own outcome — the first reachable per-entry outcome on the transaction arm, which test(rest): pin the issue codes on #478's two search-entry failure paths #518 asserts.
And the bare type-level
GET Patient→"Resource Patient/ not found"claim is simply false here now;that arm is removed by the top commit.
client_responseto returnIssueType. That enum lacksForbidden,Exception,Timeout,ThrottledandTooLong, all of whichclient_responsereturnstoday, so the typed path would start by admitting the type is incomplete. Separate change.
harnesses hardcode
FhirVersion::R4in seeding. The new file follows the same convention as the fileit mirrors; the lib itself has 38 pre-existing errors in that configuration, identical before and
after.
Closes #504