fix(rest): execute search-style GET bundle entries as searches

[angela-helios]

Closes #478

The bug

Bundle GET entries were unconditionally parsed as Type/id reads. GET Patient?name=Nguyen became a read of resource type Patient?name=Nguyen — a 404 in batch bundles, and a 400 rejecting the whole bundle in transactions — even though the spec's processing rules allow any read or search URL in a GET entry (this is what blocked the #476 screenshot bundle and got the bug filed).

The fix

• parse_request_url strips the query string before extracting type/id, so scope checks and reads see the real resource type.
• A GET entry addressing a type (Patient?name=x, or bare Patient) now runs through the same search pipeline as the HTTP endpoint (execute_search_bundle, factored out of the search handler): terminology expansion, _list/chain resolution, _include, paging clamps. The resulting searchset Bundle is embedded as the entry's resource with 200 OK. Instance reads (Patient/123) are untouched.
• Transactions: the spec orders GETs after all writes, and a search cannot run inside the storage transaction, so search entries execute against the just-committed state — which also makes them see the bundle's own writes (tested: POST a patient + GET Patient?family=Tran in one transaction finds it). Their queries are validated up front, so a malformed search still rejects the whole bundle before anything executes; a post-commit execution failure surfaces as that entry's own error outcome rather than a misleading whole-bundle failure for writes that did commit. GET-by-id keeps running inside the storage transaction as before.

Tests

New search_entries conformance tests: batch search entry returns a searchset, bare-type GET is a search, batch mixing read + search entries, transaction search seeing the bundle's own writes, transaction GET-by-id unchanged. Full helios-rest suite green (33 test binaries, incl. batch_conformance 44, search_integration 101, rest_conformance 84); clippy clean.

[codecov]

Codecov Report

❌ Patch coverage is 81.30081% with 23 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/rest/src/handlers/batch.rs	81.00%	19 Missing ⚠️
crates/rest/src/handlers/search.rs	82.60%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[aacruzgon]

Flagging a data-corruption interaction found while implementing #503 (PR #512) — it lands squarely
on this PR's parse_request_url change, and it is not visible from this diff alone.

What this PR does

// A query string is not part of the type/id path (`Patient?name=x` is a
// search on Patient, not a read of a resource named `Patient?name=x`).
let path = url.split('?').next().unwrap_or(url);

That is correct, and it is exactly what #503 needs. The problem is what happens to the write
arms once the criteria are gone — this PR adds parse_search_entry_url for the GET path but does
not touch the "PUT" or "DELETE" arms.

The mechanism

With the query stripped, PUT Patient?identifier=x parses to ("Patient", "") and falls through to
state.storage().create_or_update(tenant, "Patient", "", …). That does not reject:

• sqlite/storage.rs:186-214 — on Ok(None) it does obj.insert("id", Value::String(id.to_string()))
  and calls create.
• sqlite/storage.rs:95-99 — create does
  resource.get("id").and_then(as_str).map(String::from).unwrap_or_else(|| Uuid::new_v4()). The UUID
  fallback fires on None, not on Some("").

So it writes a row whose id is the empty string, into the real Patient table. The next conditional
PUT of that type reads Some(current) and overwrites it. DELETE Patient?identifier=x then
targets that same empty-id row.

Why it matters that it is this PR

Before the strip, the corruption is quarantined: the criteria ride along in the resource type, so the
row lands under Patient?identifier=http: — invisible to every legitimate search and to the type
routes. After the strip, it lands in Patient, where search can see it and where every conditional
PUT of that type collides on one row. Same bug, materially worse blast radius.

Verified empirically: with the empty-id guard disabled, a batch PUT Patient returns
201 Created, not a 4xx.

Options

1. Add the guard here — if id.is_empty() { return create_error_result(400, …) } at the top of
   the "PUT" and "DELETE" arms. About 12 lines, keeps this PR self-contained and mergeable in any
   order.

   Correction (edited after posting): those two guards alone are not sufficient.
   POST Patient?identifier=x post-strip parses to ("Patient", ""), and the "POST" arm never
   reads id — it goes straight to check_write and then
   state.storage().create(tenant, "Patient", resource, …). So a conditional create becomes a
   real, unconditional create with the dedup criteria silently discarded, reporting 201.
   Pre-strip that entry landed under the bogus type Patient?identifier=x; post-strip it is a
   silent duplicate in the real table. An id.is_empty() test cannot catch it, because an empty
   id is legitimate for POST — the check has to key on the URL carrying a query instead. fix(rest): parse bundle entry URLs without their query string #512
   handles this by refusing any non-GET entry whose type-level URL carries criteria.

2. Sequence it — land fix(rest): parse bundle entry URLs without their query string #512 first, after which this PR's strip becomes a no-op on rebase (fix(rest): parse bundle entry URLs without their query string #512
   rewrites parse_request_url to split_once('?') plus empty-segment filtering, and adds the
   guards). If this PR lands first, fix(rest): parse bundle entry URLs without their query string #512 should follow immediately.

Either is fine; option 2 needs no change here. Happy to do whichever you prefer — just flagging so
it is a decision rather than a surprise.

Context

• batch: parse_request_url never strips the query string, so every conditional entry is dispatched against a resource type containing the query #503 — the parser defect
• PR fix(rest): parse bundle entry URLs without their query string #512 — the fix, stacked on batch: entries are processed sequentially and BundleProvider::process_batch is dead on every backend — a batch over ~90 entries times out on S3 #501 → transaction: s3-elasticsearch advertises and accepts transaction Bundles it cannot honour — a timed-out bundle commits 466 of 473 entries behind a 408 #489. Also carries the empty-id guards and a transaction-path
  refusal, and deliberately does not touch the GET arm so this PR's search work lands on an
  untouched dispatch path.
• batch/transaction: conditional interactions are refused rather than resolved — wire bundle entries to ConditionalStorage #511 — resolving conditional interactions properly (this PR and fix(rest): parse bundle entry URLs without their query string #512 both only make them fail
  honestly).

[aacruzgon]

Rebased onto the batch stack

Heads-up @angela-helios — I force-pushed this branch and retargeted its base from main to fix/504-batch-entry-issue-codes (#516). Your original is preserved at backup/481-original-pre-restack (65f127a10) if you want to diff against it.

Why: this PR and the #501→#489→#503→#502→#504 stack both touch process_batch_entry, and they conflict. Rather than leave it for whoever merged second, we took it — but the reconciliation needs entry_failure, which only exists after #516, so the only shape that keeps both diffs scoped was to put this branch on top of that stack. Your diff is unchanged in substance and still reads as 4 files.

Resolution applied (5 hunks in batch.rs, 1 keep-both append in batch_conformance.rs):

• Both new failure sites now render through entry_failure. They were written as let (status, _, details) = e.client_response(); create_error_result(status.as_u16(), &details) — the code-discard batch: every per-entry error carries OperationOutcome code processing — a 403, 404 and 405 are indistinguishable without parsing English #504 deleted everywhere else. Left as-is they'd have made search entries the one path still answering processing.
• parse_request_url's query strip is dropped as redundant — batch: parse_request_url never strips the query string, so every conditional entry is dispatched against a resource type containing the query #503 landed the same fix, with the empty-id write guards that make it safe. fix(rest): parse bundle entry URLs without their query string #512 called this: "if this lands first, fix(rest): execute search-style GET bundle entries as searches #481's strip becomes a no-op on rebase."
• The GET arm keys off BundleMethod::Get rather than the raw "GET" string, since batch: PATCH entries return 405 and methods are case-sensitive — the batch and transaction method matchers disagree #502 replaced the string matcher with an exhaustive enum match.
• The rollback fan-out keeps batch: every per-entry error carries OperationOutcome code processing — a 403, 404 and 405 are indistinguishable without parsing English #504's status/code threading and gains your .chain(&search_entries).
• DelayStorage gains SearchProvider/IncludeProvider/RevincludeProvider and run_batch's bound widens to match process_batch's — fix(rest): run batch bundle entries with backend-bounded concurrency #505 added a unit-test harness that drives process_batch directly with a mock, so widening the bound here means the mock has to satisfy it. All unimplemented!(), matching how the mock's write methods already work. That adds parking_lot as a dev-dependency.

1114 tests pass (1109 on the base + your 5).

One gap worth flagging: your five tests are all happy-path 200 OK, so neither new failure site had coverage. #518 stacks on this and pins the issue codes on both — including the transaction-arm one, which turns out to be the first reachable per-entry outcome on that arm.

Nothing here changes what this PR does; shout if you'd rather own the resolution yourself and I'll hand it back.