fix(rest): parse bundle entry methods through one shared matcher by aacruzgon · Pull Request #515 · HeliosSoftware/hfs

aacruzgon · 2026-08-05T23:22:23Z

Summary

The batch and transaction arms of POST [base] carried two independently-written matchers for
Bundle.entry.request.method, and they disagreed — so the same Bundle succeeded as a transaction
and failed as a batch. Both now go through parse_entry_method, the only &str → BundleMethod
table in the crate.

The issue's premise turned out to be inverted, and that changes the fix. #502 frames batch as
wrongly strict for rejecting a lowercase "post". But request.method is a code with a required
binding to http://hl7.org/fhir/ValueSet/http-verb, whose concepts are caseSensitive: true and
uppercase — verified identical across R4, R4B, R5 and R6 in this repo's own bundled spec data
(crates/fhir-gen/resources/*/valuesets.json), with the binding at crates/fhir/src/r4.rs:9743-9750.
A lowercase verb is therefore invalid instance data, and parse_bundle_entry's to_uppercase()
was the non-conformant matcher. The fix is to remove it, not to copy it to the batch arm.

That leniency was load-bearing: it was the only gate between an invalid code and a real write. A
transaction entry {"method": "post", "url": "Patient"} creates a Patient on current code.
a_transaction_lowercase_verb_no_longer_writes is the regression test.

Changes

One shared matcher. parse_entry_method(&Value) -> Result<BundleMethod, EntryMethodRefusal>,
called by process_batch_entry and parse_bundle_entry. Case-sensitive, with the spec basis in the
doc comment.

Refusals keep their status across the transaction boundary. EntryMethodRefusal carries the
status, and parse_bundle_entry's error type widens to EntryParseError so a method refusal is
rendered through into_rest_error rather than flattened. Flattening it to a bare 400 there — which
was the obvious implementation — would have made HEAD 405 per-entry in a batch and 400 for the
whole bundle: the same divergence #502 exists to close, re-created in a new place. Pinned by
the_two_arms_agree_on_the_refusal_status.

Per method:

	Before	After
`PATCH`	batch 405; transaction executed earlier entries, hit the backend 501, rolled back, surfaced as a generic "Transaction failed at entry N"	501 in both arms, declined before anything executes
`HEAD`	batch 405; transaction whole-bundle 400	405 in both arms
lowercase / bogus	batch 405; transaction dispatched it	400 in both arms
absent	`unwrap_or("")` → `Unsupported method:`	400, distinguishable from a bogus code

PATCH is declined rather than implemented because a bundle entry carries no Content-Type and
parse_patch_format (patch.rs:249) derives the format entirely from it — there is nothing to
dispatch on. R4 designates FHIRPath Patch as the bundle format, and apply_patch (patch.rs:295)
returns NotImplemented for it. 501 matches all three backends, which already return 501 from inside
a transaction (sqlite/storage.rs:3265, postgres/storage.rs:2929, mongodb/storage.rs:2837), and
it matches error.rs:38-41's written rule that 501 signals work not yet wired up.

Exhaustive dispatch. The match is now over BundleMethod with no catch-all, so adding a
variant is a compile error here rather than a silent 405. That let the duplicated raw scope table go:
its _ => FhirOperation::Read fallback was only safe because of the catch-all — without it, a
method slipping through would have been authorized as a read and then executed as whatever it was. It
also removes a warn! that logged unvalidated client input at warn level and echoed it into an
OperationOutcome.

#503's guard re-keyed to !matches!(method, BundleMethod::Get). As a raw method != "GET" it was
the third case-sensitive comparison in the file.

Testing

cargo test -p helios-rest   →  1103 passed, 0 failed
cargo fmt --all             →  clean

10 new tests. 4 unit: the verb table (five codes accepted; HEAD; seven case-folded spellings; absent /
non-string / null), refusal-status parity across the transaction boundary, the transaction matcher no
longer case-folding, and refusals never reaching storage — asserted against DelayStorage, whose
write methods are unimplemented!(), so a refusal moved after dispatch panics rather than silently
writing. 6 integration against SQLite, including the two named above.

No clippy lints in either changed file. Corrected: this previously said the CI-style
--all-features gate "still fails on this branch for the 12 pre-existing lints tracked in #513".
That is true of the bare cargo clippy --all-targets --all-features -- -D warnings, but CI runs
.github/workflows/ci.yml:497, which carries eight allow-flags covering exactly those lint families
— so CI's actual command passes clean, exit 0, verified on the current tip of this stack.

Notes

Issue codes are deliberately untouched. The new refusals carry processing like the other batch
call sites. Giving create_error_result an issue-code argument is 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. The arms agree on status, which is what batch: PATCH entries return 405 and methods are case-sensitive — the batch and transaction method matchers disagree #502 asks for;
batch: every per-entry error carries OperationOutcome code processing — a 403, 404 and 405 are indistinguishable without parsing English #504 closes the OperationOutcome half uniformly and is next in the stack.
The audit path is untouched. emit_entry_audit still does its own raw read. Consolidating it
would be tidier, but it closes no divergence — both fallbacks map to AuditAction::Execute, so no
input makes it disagree with the dispatch. Left for whichever PR next needs that function.
Behaviour change worth calling out: a transaction Bundle with a lowercase verb previously wrote
and now returns 400. That is the point of the fix, but it narrows a path that has shipped.
Conflict with fix(rest): execute search-style GET bundle entries as searches #481 (issue batch/transaction: search-style GET entries are mis-parsed as instance reads #478): it inserts its GET-search dispatch into the arm this PR
converts to BundleMethod::Get. One hunk in the same block, mechanical either way. Order does not
matter — whoever rebases resolves it.
HEAD could be implemented rather than refused, and it is cheaper than it looks: HEAD is already
routed at the instance endpoint (fhir_routes.rs:350), so a bundle arm would reach the same read
the GET arm does. The cost is a BundleMethod::Head variant plus arms in three backends' exhaustive
matches. Refusing is spec-sanctioned; implementing is a separate change.

Closes #502

codecov · 2026-08-06T03:30:59Z

Codecov Report

❌ Patch coverage is 97.81022% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/rest/src/handlers/batch.rs	97.81%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

…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

The batch and transaction arms carried two independently-written matchers for `Bundle.entry.request.method`, and they disagreed. `process_batch_entry` matched the raw string with arms for GET/POST/PUT/DELETE and a 405 catch-all; `parse_bundle_entry` matched `method_str.to_uppercase()` with a PATCH arm too. So the same Bundle succeeded as a `transaction` and failed as a `batch`. Both now go through `parse_entry_method`, the only `&str` -> `BundleMethod` table in the crate. The issue framed batch as wrongly strict, and that premise is inverted. `request.method` is a `code` with a *required* binding to `http://hl7.org/fhir/ValueSet/http-verb`, whose concepts are `caseSensitive: true` and uppercase — verified identical across R4, R4B, R5 and R6 in this repo's own bundled spec data. A lowercase `"post"` is invalid instance data, so `to_uppercase()` was the non-conformant matcher and the fix is to remove it, not to copy it across. It was also the only gate between an invalid code and a real write: a transaction `{"method":"post"}` entry creates a resource today. Refusals carry their status through `EntryMethodRefusal` rather than being flattened at the transaction boundary. Flattening is what would re-create this same divergence in a new place — HEAD would be 405 per-entry in a batch and 400 for the whole bundle. `parse_bundle_entry`'s error type widens to `EntryParseError` so a method refusal keeps its status while genuinely malformed entries stay 400. Per method: - **PATCH** is declined at 501 in both arms, matching all three backends, which already return 501 from inside a transaction. A bundle entry carries no Content-Type and `parse_patch_format` derives the format entirely from it, so there is nothing to dispatch on; R4 designates FHIRPath Patch as the bundle format and `apply_patch` does not implement it. Previously batch returned 405 and a transaction executed its earlier entries, hit the backend 501, rolled back, and surfaced as a generic "Transaction failed at entry N" that never mentioned PATCH. - **HEAD** is refused at 405 in both arms. It is a legal http-verb code, served on the instance-read route, but no bundle arm implements it. - Non-canonical and absent codes are refused at 400, and are now distinguishable: `unwrap_or("")` used to render an absent method as `Unsupported method: `. The dispatch is now an exhaustive match on `BundleMethod` with no catch-all, so adding a variant is a compile error here rather than a silent 405. That removes a `warn!` that logged unvalidated client input and echoed it into an OperationOutcome. It also lets the duplicated raw scope table go: its `_ => FhirOperation::Read` fallback was only safe while the catch-all existed — without it, a method slipping through would have been authorized as a read and executed as whatever it was. comparison it was the third case-sensitive one in the file; such entries are now refused at the seam and never reach it. Issue codes are deliberately left alone — the new refusals carry `processing` like the other batch call sites. Giving `create_error_result` an issue-code argument is #504's stated fix, and it belongs there rather than landing early here. Tests: 4 unit (the verb table, refusal-status parity across the boundary, the transaction matcher no longer case-folding, and refusals never reaching storage against a mock whose write methods are `unimplemented!()`), and 6 integration against SQLite — including `a_transaction_lowercase_verb_no_longer_writes`, which creates a Patient on the current code, and `the_two_arms_agree_on_the_refusal_status`. 1103 tests pass in helios-rest. Closes #502

Adds an "Entry Methods" section stating that `request.method` is matched case-sensitively in both bundle types, with the spec basis: a `code` with a required binding to `http-verb`, whose concepts are `caseSensitive: true` and uppercase in every supported version. Two Current Limitations bullets. The PATCH bullet needed no correction — it already claimed 501, which #503 flagged as false (batch returned 405) and left for this issue. Choosing 501 over 405 makes the existing claim true rather than requiring the doc to change, which was a concrete argument for that status. It gains a note that the 501 now applies to both arms. HEAD is newly documented as refused with 405, distinguishing it from the instance-read route where HEAD is served. 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. Do not name the literal token here either — the substring match does not care that it is quoted in prose. Refs #502

…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

aacruzgon changed the title ~~fix/502 batch method matching~~ fix(rest): parse bundle entry methods through one shared matcher Aug 5, 2026

aacruzgon force-pushed the fix/502-batch-method-matching branch from 477c15a to 0047f2d Compare August 5, 2026 23:29

aacruzgon requested a review from smunini August 6, 2026 01:30

aacruzgon force-pushed the fix/502-batch-method-matching branch from 0047f2d to ac33a9c Compare August 6, 2026 02:49

This was referenced Aug 6, 2026

fix(rest): render batch entry failures through the single-resource error mapping #516

Open

fix(rest): parse bundle entry URLs without their query string #512

Open

aacruzgon mentioned this pull request Aug 6, 2026

ci: CI-style clippy already fails on clippy 1.96 — Linting goes red repo-wide when the runner's stable updates #513

Open

aacruzgon force-pushed the fix/502-batch-method-matching branch from ac33a9c to 8e22016 Compare August 6, 2026 17:25

aacruzgon force-pushed the fix/502-batch-method-matching branch from 8e22016 to a61c331 Compare August 7, 2026 14:49

aacruzgon added 2 commits August 9, 2026 16:12

aacruzgon force-pushed the fix/502-batch-method-matching branch from a61c331 to b16671f Compare August 9, 2026 20:32

Navigation Menu

Uh oh!

fix(rest): parse bundle entry methods through one shared matcher - #515

fix(rest): parse bundle entry methods through one shared matcher#515
aacruzgon wants to merge 2 commits into
fix/503-batch-request-url-queryfrom
fix/502-batch-method-matching

aacruzgon commented Aug 5, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

aacruzgon commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Notes

Uh oh!

codecov Bot commented Aug 6, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

aacruzgon commented Aug 5, 2026 •

edited

Loading