From 26d0a22174ce5df20a55611967955928e7f612c4 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 5 Aug 2026 17:12:32 -0400 Subject: [PATCH 1/5] fix(rest): parse bundle entry URLs without their query string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parse_request_url` split a batch entry's `request.url` on `/` and nothing else, so every FHIR conditional interaction was dispatched against a resource type containing its own criteria. `Patient?identifier=http://example.org|12345` parsed as the type `Patient?identifier=http:` with an empty id, the `//` inside the value supplying the extra segment. Storage was then addressed with that type: `create` wrote a row under it, and `create_or_update` was called with an empty id. Split the query off before the path, and drop empty segments so a leading `/` and the `[type]/?[criteria]` form both reduce to the type alone. The signature is unchanged, so four of the five call sites — the concurrency scan, both transaction sites and the audit path — are repaired without edits. The `0 => Err("Empty URL")` arm was unreachable, since `str::split` always yields at least one element. An absent `request.url` therefore parsed as the resource type "" and POST created a row under it. That arm is now reachable. Stripping the query alone would be a regression, not a fix. With the criteria gone, `PUT Patient?identifier=x` reaches `create_or_update` with an empty id and SQLite writes the row rather than rejecting it: the backend inserts `"id": ""` before delegating to `create`, whose id fallback fires on an absent id, not an empty one, so every later such entry reads that row back and overwrites it. Conditional entries are therefore refused per-entry with a 400, as are `PUT`/`DELETE` entries that name no instance. GET is untouched, leaving search-style entries to #478. Resolving conditional criteria is #511. The concurrency scan's `StructureDefinition` carve-out now also matches `StructureDefinition?url=...`, where it silently did not before. Such entries are refused before they write, so that clamp is conservative rather than load-bearing, but the scan and the write still agree. Tests: 6 unit tests over the parser and the criteria predicate, and 4 integration tests against SQLite asserting nothing is written. Verified non-vacuous — with the empty-id guard disabled, `PUT Patient` returns 201 Created and the test fails. Refs #503 --- crates/rest/src/handlers/batch.rs | 278 ++++++++++++++++++++++++- crates/rest/tests/batch_conformance.rs | 143 +++++++++++++ 2 files changed, 410 insertions(+), 11 deletions(-) diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index e78fcba7d..5285f371f 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -168,7 +168,12 @@ where // that rely on it. // // Keyed off `request.url` through the same `parse_request_url` the side - // effect itself keys off, so the scan and the write cannot disagree. + // effect itself keys off, so the scan and the write cannot disagree. Since + // #503 that parse strips the query, so `StructureDefinition?url=…` matches + // here where it did not before. Such an entry is refused as conditional + // before it writes, which makes the clamp conservative rather than + // load-bearing — but the scan and the write still agree, which is the + // invariant this is keyed for. // // NOTE: extend this scan in lockstep with any new cross-entry // `state.validation()` mutation added to `process_batch_entry`. @@ -650,6 +655,30 @@ where } } + // A query on a type-level URL is FHIR conditional criteria, and this path + // cannot resolve one — that needs a search, which is #511. Refuse it + // explicitly rather than dispatch something else: before #503 the criteria + // rode along in `resource_type`, so a conditional PUT reached + // `create_or_update` with an empty id and wrote a row no search can address. + // + // GET is exempt. A query there is a search rather than a condition, and + // executing it is #478's deliverable; leaving the arm untouched keeps this + // fix off that diff. + if method != "GET" + && let Some(criteria) = conditional_criteria(url, &id) + { + return create_error_result( + 400, + &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 { "GET" => { // Read operation @@ -714,6 +743,19 @@ where } }; + // `PUT Patient` names no instance to update. Left to fall through it + // reaches `create_or_update` with an empty id, and that writes a row + // rather than rejecting: the backend inserts `"id": ""` into the + // resource before delegating to `create`, whose id fallback fires on + // 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]')", + ); + } + // Ahead of validation, because every backend evaluates `ifMatch` // first: a stale precondition carrying an invalid body is a 412, // not a 422. @@ -767,6 +809,16 @@ where } } "DELETE" => { + // Mirror of the PUT guard above. FHIR defines no unconditional + // 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]')", + ); + } + // Honour `ifMatch` on DELETE: a client asking to delete only the // version it reviewed must not destroy a concurrent amendment. if let Some(failure) = @@ -996,18 +1048,54 @@ fn extract_outcome_description(outcome: Option<&Value>) -> Option { } /// Parses a request URL to extract resource type and optional ID. +/// +/// The query string is split off **before** the path is parsed. FHIR conditional +/// criteria routinely contain `/` — the spec's own transaction example carries +/// `Patient?identifier=http:/example.org/fhir/ids|456456` — so splitting the raw +/// URL on `/` first folds the criteria into the resource type, and the caller +/// then addresses storage with a type like `Patient?identifier=http:` (#503). +/// +/// Empty segments are dropped rather than yielded, so a leading `/` and the +/// `[type]/?[criteria]` form that `http.html` prints for conditional delete both +/// reduce to the type alone instead of producing an empty id. +/// +/// The query itself is deliberately not returned. Callers refuse conditional +/// entries via [`conditional_criteria`]; resolving them is #511. fn parse_request_url(url: &str) -> Result<(String, String), String> { - let parts: Vec<&str> = url.trim_start_matches('/').split('/').collect(); + let path = url.split_once('?').map_or(url, |(path, _)| path); + let mut segments = path.split('/').filter(|segment| !segment.is_empty()); + + // Unlike the previous `Vec`-and-`match` shape, this arm is reachable: an + // absent or empty `request.url` used to parse as the resource type `""`, + // which the POST arm then created a row under. + let resource_type = segments + .next() + .ok_or_else(|| "Entry request.url is empty".to_string())?; + + // `Patient/123/_history/1` addresses `Patient/123`; anything past the id + // qualifies that address rather than extending it. + Ok(( + resource_type.to_string(), + segments.next().unwrap_or_default().to_string(), + )) +} - match parts.len() { - 0 => Err("Empty URL".to_string()), - 1 => Ok((parts[0].to_string(), String::new())), - 2 => Ok((parts[0].to_string(), parts[1].to_string())), - _ => { - // Handle URLs like Patient/123/_history/1 - Ok((parts[0].to_string(), parts[1].to_string())) - } - } +/// Returns the conditional criteria an entry URL carries, if any. +/// +/// A query on a **type-level** URL (`Patient?identifier=x`) is FHIR conditional +/// criteria. A query on an **instance** URL (`Patient/123?_format=json`) is a +/// control parameter — the entry addresses a known resource either way — so it +/// is not reported here. +/// +/// A bare `Patient?` carries no criteria and is not conditional; treating it as +/// one would match every resource of the type. +fn conditional_criteria<'a>(url: &'a str, id: &str) -> Option<&'a str> { + if !id.is_empty() { + return None; + } + url.split_once('?') + .map(|(_, query)| query) + .filter(|query| !query.is_empty()) } /// Creates an error BundleEntryResult. @@ -2193,6 +2281,17 @@ mod tests { })]; assert_eq!(batch_concurrency(&state, &url_only), 1); + // Since #503 the parse strips the query, so a conditional conformance + // URL is caught here where it silently was not before. Such an entry is + // refused before it writes, which makes this clamp conservative — but + // the scan and the write must not disagree, which is what it is keyed + // for. + let conditional = [serde_json::json!({ + "request": { "method": "PUT", "url": "StructureDefinition?url=http://example.org/sd" }, + "resource": { "resourceType": "StructureDefinition" } + })]; + assert_eq!(batch_concurrency(&state, &conditional), 1); + // A bundle with no conformance writes resolves normally. Compared // against the empty bundle rather than a literal, so this test stays // about the carve-out; the cap itself is pinned by @@ -2207,6 +2306,163 @@ mod tests { assert!(batch_concurrency(&state, &data_only) > 1); } + /// The query is split off before the path, so conditional criteria never + /// ride along in the resource type (#503). + #[test] + fn parse_request_url_splits_the_query_off_before_the_path() { + // The shape from the issue: the `//` inside the criteria produced an + // empty path segment, and the criteria became the resource type. + assert_eq!( + parse_request_url("Patient?identifier=http://example.org|12345").unwrap(), + ("Patient".to_string(), String::new()) + ); + // The spec's own transaction example carries `/` inside its criteria. + assert_eq!( + parse_request_url("Patient?identifier=http:/example.org/fhir/ids|456456").unwrap(), + ("Patient".to_string(), String::new()) + ); + // `[type]/?[criteria]` is the form `http.html` prints for conditional + // delete; the empty segment must not become an id. + assert_eq!( + parse_request_url("Patient/?identifier=x").unwrap(), + ("Patient".to_string(), String::new()) + ); + // A query on an instance URL qualifies the request; it is not the id. + assert_eq!( + parse_request_url("Patient/p1?_format=json").unwrap(), + ("Patient".to_string(), "p1".to_string()) + ); + } + + /// Shapes that already resolved keep resolving identically. + #[test] + fn parse_request_url_still_addresses_types_instances_and_history() { + for (url, expected_type, expected_id) in [ + ("Patient", "Patient", ""), + ("Patient/p1", "Patient", "p1"), + ("/Patient/p1", "Patient", "p1"), + ("Patient/p1/_history/2", "Patient", "p1"), + ] { + assert_eq!( + parse_request_url(url).unwrap(), + (expected_type.to_string(), expected_id.to_string()), + "url: {url}" + ); + } + } + + /// The empty-URL arm used to be unreachable — `str::split` always yields at + /// least one element, so an absent `request.url` parsed as the resource type + /// `""` and the POST arm created a row under it. + #[test] + fn parse_request_url_rejects_a_url_with_no_resource_type() { + for url in ["", "/", "?identifier=x"] { + assert!(parse_request_url(url).is_err(), "url: {url}"); + } + } + + #[test] + fn conditional_criteria_only_fires_on_a_type_level_url() { + assert_eq!( + conditional_criteria("Patient?identifier=x", ""), + Some("identifier=x") + ); + // An instance URL already addresses its target. + assert_eq!(conditional_criteria("Patient/p1?_format=json", "p1"), None); + // Nothing to condition on. A bare `Patient?` in particular must not be + // read as criteria — that would match every Patient. + assert_eq!(conditional_criteria("Patient", ""), None); + assert_eq!(conditional_criteria("Patient?", ""), None); + } + + /// A conditional write is refused per-entry and never reaches storage. + /// + /// `DelayStorage::create_or_update` and `::delete` are `unimplemented!()`, + /// so this panics rather than merely failing if a refusal is ever moved + /// after dispatch. + #[tokio::test] + async fn conditional_write_entries_are_refused_before_they_reach_storage() { + let state = state_with(DelayStorage::new(8, 0)); + + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "request": { "method": "PUT", "url": "Patient?identifier=http://example.org|1" }, + "resource": { "resourceType": "Patient" } + }, + { "request": { "method": "DELETE", "url": "Patient?identifier=x" } }, + { + "request": { "method": "POST", "url": "Patient?identifier=x" }, + "resource": { "resourceType": "Patient" } + }, + ] + }); + + let response = run_batch(&state, &bundle, None).await; + let entries = response["entry"].as_array().unwrap(); + assert_eq!(entries.len(), 3); + for (index, entry) in entries.iter().enumerate() { + assert_eq!( + entry["response"]["status"], "400 Bad Request", + "entry {index}: {entry}" + ); + } + assert_eq!(state.storage().peak(), 0, "no entry may reach storage"); + } + + /// A type-level URL with no criteria names no instance. Left to fall + /// through, `PUT Patient` reached `create_or_update` with an empty id, and + /// the backend wrote a row rather than rejecting (#503). + #[tokio::test] + async fn type_level_writes_without_an_id_are_refused() { + let state = state_with(DelayStorage::new(8, 0)); + + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "request": { "method": "PUT", "url": "Patient" }, + "resource": { "resourceType": "Patient" } + }, + { "request": { "method": "DELETE", "url": "Patient" } }, + ] + }); + + let response = run_batch(&state, &bundle, None).await; + let entries = response["entry"].as_array().unwrap(); + assert_eq!(entries.len(), 2); + for (index, entry) in entries.iter().enumerate() { + assert_eq!( + entry["response"]["status"], "400 Bad Request", + "entry {index}: {entry}" + ); + } + assert_eq!(state.storage().peak(), 0, "no entry may reach storage"); + } + + /// An instance-addressed entry carrying a control parameter still resolves: + /// the query is dropped, not treated as criteria. + #[tokio::test] + async fn an_instance_url_with_a_query_still_addresses_its_instance() { + let state = state_with(DelayStorage::new(8, 0)); + + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { "request": { "method": "GET", "url": "Patient/p1?_format=json" } }, + ] + }); + + let response = run_batch(&state, &bundle, None).await; + let entry = &response["entry"][0]; + assert_eq!(entry["response"]["status"], "200 OK", "{entry}"); + assert_eq!(entry["resource"]["id"], "p1"); + } + /// Scope enforcement stays per-entry when entries run concurrently: denied /// entries become 403 response entries and permitted ones still succeed. /// diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index bb83bb706..21b3dce53 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -908,3 +908,146 @@ mod conditional_references { response.assert_status(StatusCode::BAD_REQUEST); } } + +// ============================================================================= +// Conditional Entry Tests (#503) +// ============================================================================= + +/// Conditional interactions expressed in an entry URL (`[type]?[criteria]`) are +/// refused rather than resolved, and — the point of #503 — nothing is written. +/// +/// Before the fix the criteria rode along inside the parsed resource type, so a +/// conditional `PUT`/`DELETE` addressed storage with a type like +/// `Patient?identifier=http:` and an empty id. Resolving these is #511. +mod conditional_entries { + use super::*; + + async fn patient_count(backend: &SqliteBackend) -> u64 { + backend + .count(&test_tenant(), Some("Patient")) + .await + .expect("count failed") + } + + #[tokio::test] + async fn conditional_put_is_refused_and_writes_nothing() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + let before = patient_count(&backend).await; + + let body = post_batch( + &server, + json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { + "method": "PUT", + "url": "Patient?identifier=http://example.org|12345" + }, + "resource": { "resourceType": "Patient", "name": [{"family": "Conditional"}] } + }] + }), + ) + .await; + + assert_eq!(body["entry"][0]["response"]["status"], "400 Bad Request"); + assert_eq!( + patient_count(&backend).await, + before, + "a refused conditional PUT must not create a resource" + ); + } + + #[tokio::test] + async fn conditional_delete_is_refused_and_deletes_nothing() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + let before = patient_count(&backend).await; + + let body = post_batch( + &server, + json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "DELETE", "url": "Patient?name=Nguyen" } + }] + }), + ) + .await; + + assert_eq!(body["entry"][0]["response"]["status"], "400 Bad Request"); + assert_eq!( + patient_count(&backend).await, + before, + "a refused conditional DELETE must not remove a resource" + ); + assert!( + backend + .read(&test_tenant(), "Patient", "p1") + .await + .expect("read failed") + .is_some(), + "the seeded patient must survive" + ); + } + + /// The corruption #503 closes: `create_or_update` with an empty id inserts + /// `"id": ""` into the resource and delegates to `create`, whose id fallback + /// fires on an absent id rather than an empty one — so the row is written, + /// and every later type-level PUT reads it back and overwrites it. + #[tokio::test] + async fn a_type_level_put_never_writes_an_empty_id_row() { + let (server, backend) = create_test_server().await; + let before = patient_count(&backend).await; + + let body = post_batch( + &server, + json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "PUT", "url": "Patient" }, + "resource": { "resourceType": "Patient", "name": [{"family": "NoId"}] } + }] + }), + ) + .await; + + assert_eq!(body["entry"][0]["response"]["status"], "400 Bad Request"); + assert_eq!(patient_count(&backend).await, before); + assert!( + backend + .read(&test_tenant(), "Patient", "") + .await + .ok() + .flatten() + .is_none(), + "no resource may be stored under the empty id" + ); + } + + /// An instance URL carrying a control parameter still addresses its + /// instance — the query is dropped, not read as criteria. + #[tokio::test] + async fn an_instance_url_with_a_query_still_resolves() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + + let body = post_batch( + &server, + json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [{ + "request": { "method": "GET", "url": "Patient/p1?_format=json" } + }] + }), + ) + .await; + + assert_eq!(body["entry"][0]["response"]["status"], "200 OK"); + assert_eq!(body["entry"][0]["resource"]["id"], "p1"); + } +} From 861225ee78c808d14c26a61db44ad8dac5792e95 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 5 Aug 2026 17:13:17 -0400 Subject: [PATCH 2/5] fix(rest): decline transaction entries whose URL carries a query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundle entry URLs reach the backends unparsed, and every backend's `parse_url` splits on `/` alone and takes the last two segments — sqlite, postgres and mongodb carry byte-equivalent copies, none of which strips a query. So a query lands in storage as part of the resource type or the id, and the transaction PUT arm commits it without validating either: - `PUT Patient?identifier=http://example.org|12345` commits a row typed `Patient?identifier=http:` with the id `example.org|12345`. - `PUT Patient/123?_format=json` commits one whose id is `123?_format=json`. - `PUT Patient?name=peter` yields a single segment, so `parse_url` errors and the whole bundle rolls back citing the URL format rather than the criteria. This corrects issue #503's claim that the transaction path is unaffected because the backends resolve conditionals themselves. They do not; only MongoDB's `ifNoneExist` path does. Decline any non-GET entry whose URL carries a query, raised before `process_transaction` runs so the bundle is declined intact and nothing is applied. `RestError::NotSupported` (400 + `not-supported`) is the documented variant for a spec-defined feature the server refuses by design, as opposed to `NotImplemented`, and it matches R4's repeated SHOULD for unsupported conditional interactions. Two exemptions, both deliberate. GET entries stay on their existing path so search-style entries remain #478's deliverable. `ifNoneExist` is untouched because MongoDB already resolves it inside the bundle's session — refusing it here would remove a working, atomic, covered feature. SQLite and Postgres silently ignore it instead, which wants a backend capability predicate rather than a REST-layer constant; that and resolving URL criteria within a transaction's atomic scope are #511. Tests: an integration test asserting a declined bundle applies none of its entries, including a sibling create that would otherwise have landed, and one pinning that GET entries are not caught by this guard. Refs #503 --- crates/rest/src/handlers/batch.rs | 34 ++++++++++ crates/rest/tests/batch_conformance.rs | 87 ++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index 5285f371f..8090ee63f 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -392,6 +392,40 @@ where for (index, entry) in json_entries.iter().enumerate() { match parse_bundle_entry(entry) { Ok((bundle_entry, full_url)) => { + // Entry URLs reach the backends unparsed, and every backend's + // `parse_url` splits on `/` alone and takes the last two + // segments — sqlite, postgres and mongodb carry byte-equivalent + // copies. A query string therefore lands in storage as part of + // the resource type or the id: `PUT Patient?identifier=http://…` + // commits a row typed `Patient?identifier=http:`, and + // `PUT Patient/123?_format=json` commits one whose id is + // `123?_format=json`. `PUT Patient?name=peter` yields a single + // segment and fails the whole bundle with a message about the + // URL format instead. Decline here, before anything executes, so + // the bundle is declined intact (#503). + // + // GET is exempt: those URLs are searches, resolved by the REST + // layer rather than by a backend `parse_url` (#478). + // `ifNoneExist` is left alone too — MongoDB resolves it inside + // the session, so refusing it here would remove a working, + // atomic feature. Resolving URL criteria within a transaction's + // atomic scope is #511. + if !matches!(bundle_entry.method, BundleMethod::Get) + && bundle_entry.url.contains('?') + { + return Err(RestError::NotSupported { + feature: format!( + "Transaction entry {} ({} {}) carries a query string. This \ + server cannot resolve one inside a transaction's atomic \ + scope, so no entries were applied. Submit it in a batch \ + Bundle, or address the instance directly.", + index, + bundle_method_to_http_method(&bundle_entry.method), + bundle_entry.url + ), + }); + } + // Enforce per-entry scope authorization for transactions. // Transactions are atomic so any denied entry rejects the whole bundle. if let Some(principal) = principal { diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index 21b3dce53..633266475 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -922,6 +922,20 @@ mod conditional_references { mod conditional_entries { use super::*; + /// Posts a bundle and returns the raw response without asserting on status, + /// so declined transactions can be inspected. + async fn post_bundle(server: &TestServer, bundle: Value) -> axum_test::TestResponse { + 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 + } + async fn patient_count(backend: &SqliteBackend) -> u64 { backend .count(&test_tenant(), Some("Patient")) @@ -1050,4 +1064,77 @@ mod conditional_entries { assert_eq!(body["entry"][0]["response"]["status"], "200 OK"); assert_eq!(body["entry"][0]["resource"]["id"], "p1"); } + + /// A transaction carrying a query-bearing non-GET entry is declined whole, + /// before anything executes — so the sibling create in the same bundle must + /// not have landed. Backends parse entry URLs query-blind, so letting it + /// through commits the criteria as part of the resource type or the id. + #[tokio::test] + async fn a_transaction_with_a_conditional_url_is_declined_intact() { + let (server, backend) = create_test_server().await; + let before = patient_count(&backend).await; + + let response = post_bundle( + &server, + json!({ + "resourceType": "Bundle", + "type": "transaction", + "entry": [ + { + "request": { "method": "POST", "url": "Patient" }, + "resource": { "resourceType": "Patient", "name": [{"family": "Sibling"}] } + }, + { + "request": { + "method": "PUT", + "url": "Patient?identifier=http://example.org|12345" + }, + "resource": { "resourceType": "Patient" } + } + ] + }), + ) + .await; + + response.assert_status(StatusCode::BAD_REQUEST); + let body: Value = response.json(); + assert_eq!(body["resourceType"], "OperationOutcome"); + assert_eq!(body["issue"][0]["code"], "not-supported"); + assert_eq!( + patient_count(&backend).await, + before, + "the bundle must be declined before any entry is applied" + ); + } + + /// GET entries are left to #478: a transaction search URL is not declined + /// by the query guard, so that work lands on an untouched arm. + #[tokio::test] + async fn a_transaction_get_with_a_query_is_not_declined_by_the_query_guard() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + + let response = post_bundle( + &server, + json!({ + "resourceType": "Bundle", + "type": "transaction", + "entry": [{ + "request": { "method": "GET", "url": "Patient?name=Nguyen" } + }] + }), + ) + .await; + + let body: Value = response.json(); + let declined_by_the_guard = body["resourceType"] == "OperationOutcome" + && body["issue"][0]["code"] == "not-supported" + && body["issue"][0]["diagnostics"] + .as_str() + .is_some_and(|d| d.contains("carries a query string")); + assert!( + !declined_by_the_guard, + "GET entries must stay on #478's path, not this guard: {body}" + ); + } } From c0ea6524bc44e30e20af238c4d8741088eeec3b3 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 5 Aug 2026 17:13:31 -0400 Subject: [PATCH 3/5] docs(rest): correct the bundle conditional-header claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Conditional Operations in Bundles" section advertised support the server does not have. `ifNoneMatch` is populated by `parse_bundle_entry` and read by no handler or backend. `ifNoneExist` is honoured only by MongoDB's transaction path — the batch path never reads it, and SQLite and Postgres ignore it in a transaction and create a duplicate. State what is true per header, document that URL-borne conditional interactions are refused (per-entry in a batch, whole-bundle in a transaction), and note that `/metadata` advertises `conditionalCreate`, `conditionalUpdate` and `conditionalDelete` for every resource type — accurate for the resource endpoints, not for bundle entries. Reconciling those is #511, which is also added to Current Limitations. The PATCH bullet is knowingly left alone: it claims 501 while the batch path returns 405 and only the transaction path returns 501. That belongs to #502, which owns the behaviour. Tests: none — documentation only. Refs #503 --- crates/rest/README.md | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/crates/rest/README.md b/crates/rest/README.md index 56c2928e6..e3a018a50 100644 --- a/crates/rest/README.md +++ b/crates/rest/README.md @@ -501,16 +501,32 @@ curl -X POST http://localhost:8080/ \ ### Conditional Operations in Bundles -Bundle entries support conditional headers: -- `ifMatch` - ETag for optimistic locking on `PUT` **and `DELETE`** entries, in - both `batch` and `transaction` bundles. Parsed as the comma-separated list - RFC 9110 §13.1.1 defines: satisfied when any supplied entity-tag matches. -- `ifNoneMatch` - Prevent overwrites (`*` for conditional create) -- `ifNoneExist` - Search query for conditional create +- `ifMatch` — **supported.** ETag for optimistic locking on `PUT` **and `DELETE`** + entries, in both `batch` and `transaction` bundles. Parsed as the comma-separated + list RFC 9110 §13.1.1 defines: satisfied when any supplied entity-tag matches. +- `ifNoneMatch` — **parsed and ignored.** `parse_bundle_entry` populates + `BundleEntry.if_none_match`; no handler or backend reads it. +- `ifNoneExist` — **MongoDB transactions only.** MongoDB resolves it inside the + bundle's session. The batch path never reads it, and SQLite/PostgreSQL ignore it + in a transaction and create a duplicate. Tracked by #511. + +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 `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 + the criteria as part of the resource type or the id. + +Note that `/metadata` advertises `conditionalCreate`, `conditionalUpdate` and +`conditionalDelete` for every resource type. That is accurate for the resource +endpoints and **not** for bundle entries; reconciling the two is #511. ### Current Limitations 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 - **Prefer header** - `return=minimal` and `return=OperationOutcome` not honored From 263ca74b2e56e64ca2cc34cab83e60e9ceed30c9 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 5 Aug 2026 18:41:57 -0400 Subject: [PATCH 4/5] docs(rest): correct the GET-exemption rationale in the transaction guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment justified exempting GET entries from the query guard by saying those URLs are "resolved by the REST layer rather than by a backend `parse_url`". That describes a state this branch does not have: nothing in `process_transaction` resolves a GET entry, and `parse_search_entry_url` does not exist here — it arrives with #478/PR #481, which is still open. A query-bearing GET entry still reaches the backend's `parse_url` and still fails there. The exemption itself is correct, for a different reason: it keeps this guard off the dispatch arm #478 is rewriting, so that work lands on an untouched path rather than merging against a refusal it is about to replace. Say that instead. Comment only; no behaviour change. `a_transaction_get_with_a_query_is_not_declined_by_the_query_guard` already pins the exemption, and it asserts only that the guard does not catch the entry — not that the entry succeeds. Tests: unchanged and re-run — 20 unit tests in handlers::batch pass. Refs #503 --- crates/rest/src/handlers/batch.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index 8090ee63f..708153432 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -404,8 +404,13 @@ where // URL format instead. Decline here, before anything executes, so // the bundle is declined intact (#503). // - // GET is exempt: those URLs are searches, resolved by the REST - // layer rather than by a backend `parse_url` (#478). + // GET is exempt — but not because this path resolves searches. + // It does not: a GET entry still reaches the backend's + // `parse_url`, and a query-bearing one still fails there. The + // exemption keeps this guard off the arm #478 is rewriting, so + // that work lands on an untouched dispatch path instead of + // merging against a refusal it is about to replace. + // // `ifNoneExist` is left alone too — MongoDB resolves it inside // the session, so refusing it here would remove a working, // atomic feature. Resolving URL criteria within a transaction's From 6cf3c53bd1c933a1c7dac8aa9b5b9a7d41dc203c Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 5 Aug 2026 22:34:19 -0400 Subject: [PATCH 5/5] test(rest): assert the query-guard refusal on the field the server actually writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `a_transaction_get_with_a_query_is_not_declined_by_the_query_guard`, added two commits ago, cannot fail. Its predicate is a three-way conjunction whose third clause reads `body["issue"][0]["diagnostics"]`, but the guard it tests raises `RestError::NotSupported`, which renders through `create_operation_outcome` (error.rs:560-571) into `details.text`. `grep -n diagnostics crates/rest/src/error.rs` returns nothing: `RestError` never emits that field. So `declined_by_the_guard` is permanently `false`, and `assert!(!declined_by_the_guard)` would hold even if a GET entry *were* declined by the guard — which is the one thing the test exists to catch. The sibling assertion three tests earlier reads `body["issue"][0]["code"]` and passes, confirming the shape. One field path: `["diagnostics"]` becomes `["details"]["text"]`. Verified non-vacuous in the only way available for a negation. With the GET exemption at batch.rs:418 removed so the guard does fire on a GET entry, the corrected test fails: GET entries must stay on #478's path, not this guard: {"resourceType": "OperationOutcome","issue":[{"severity":"error","code":"not-supported", "details":{"text":"Transaction entry 0 (GET Patient?name=Nguyen) carries a query string. ..."}}]} while the original, against that same broken guard, still reports `test result: ok. 1 passed`. Found while grounding #504. Refs #503 --- crates/rest/tests/batch_conformance.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index 633266475..cea895aac 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -1127,9 +1127,16 @@ mod conditional_entries { .await; let body: Value = response.json(); + // `details.text`, not `diagnostics`: the guard raises + // `RestError::NotSupported`, and `create_operation_outcome` writes only + // `details.text` — `RestError` never renders a `diagnostics` field. As + // written against `diagnostics` the third conjunct was unsatisfiable, + // so `declined_by_the_guard` was permanently false and the negated + // assert below held even if a GET entry *were* declined by the guard, + // which is the one thing this test exists to catch. let declined_by_the_guard = body["resourceType"] == "OperationOutcome" && body["issue"][0]["code"] == "not-supported" - && body["issue"][0]["diagnostics"] + && body["issue"][0]["details"]["text"] .as_str() .is_some_and(|d| d.contains("carries a query string")); assert!(