From d17522aa853ee9443998a8865425e2b03498a730 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 5 Aug 2026 18:53:34 -0400 Subject: [PATCH 1/2] fix(rest): parse bundle entry methods through one shared matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/rest/src/handlers/batch.rs | 360 ++++++++++++++++++++++--- crates/rest/tests/batch_conformance.rs | 208 ++++++++++++++ 2 files changed, 531 insertions(+), 37 deletions(-) diff --git a/crates/rest/src/handlers/batch.rs b/crates/rest/src/handlers/batch.rs index 708153432..0d04758ec 100644 --- a/crates/rest/src/handlers/batch.rs +++ b/crates/rest/src/handlers/batch.rs @@ -449,13 +449,27 @@ where }, )?; } + // Decline PATCH before anything executes, at the same 501 the + // batch arm returns and all three backends already return from + // inside the transaction. Today such a bundle executes its + // earlier entries, hits the backend's 501, rolls back, and + // surfaces as a generic "Transaction failed at entry N" — the + // status the client sees never mentions PATCH. Raised here, the + // bundle is declined intact and says why. + if matches!(bundle_entry.method, BundleMethod::Patch) { + return Err(RestError::NotImplemented { + feature: format!("PATCH in a Bundle entry (transaction entry {index})"), + }); + } + indexed_entries.push((index, bundle_entry, full_url)); } Err(e) => { - // For transactions, any parse error fails the whole bundle - return Err(RestError::BadRequest { - message: format!("Entry {}: {}", index, e), - }); + // For transactions, any parse error fails the whole bundle. + // Rendered through the error itself rather than flattened to a + // 400: a HEAD entry is 405 here exactly as it is per-entry in a + // batch, which is the agreement #502 asks for. + return Err(e.into_rest_error(index)); } } } @@ -662,7 +676,16 @@ where } }; - let method = request.get("method").and_then(|v| v.as_str()).unwrap_or(""); + // Resolved through the shared seam, so this arm and the transaction arm + // accept exactly the same set of codes and refuse the rest with the same + // status (#502). It runs before the URL parse below, so an entry that is + // wrong in both ways now reports the method rather than the URL. + let method = match parse_entry_method(request) { + Ok(method) => method, + Err(refusal) => { + return create_error_result(refusal.status(), &refusal.message(index)); + } + }; let url = request.get("url").and_then(|v| v.as_str()).unwrap_or(""); let if_match = request.get("ifMatch").and_then(|v| v.as_str()); @@ -676,13 +699,12 @@ where // Enforce per-entry scope authorization if let Some(principal) = principal { - let operation = match method { - "GET" => FhirOperation::Read, - "POST" => FhirOperation::Create, - "PUT" | "PATCH" => FhirOperation::Update, - "DELETE" => FhirOperation::Delete, - _ => FhirOperation::Read, // will be caught by unsupported method below - }; + // The same enum-typed table the transaction arm uses. The raw-string + // copy this replaced ended in `_ => FhirOperation::Read`, which was only + // safe while an unsupported method was caught further down — with the + // catch-all gone, a method that slipped through would have been + // authorized as a read and then executed as whatever it was. + let operation = bundle_method_to_fhir_operation(&method); if SmartScopePolicy::check(principal, &resource_type, operation).is_err() { return create_error_result( 403, @@ -703,7 +725,14 @@ where // 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" + // + // Since #502 the predicate is enum-typed, matching its transaction twin. As + // a raw `method != "GET"` this was the third case-sensitive comparison in + // the file: a lowercase `get` on a search URL failed it and was refused as a + // conditional interaction. Such an entry is now refused at the seam and + // never reaches here. `{method}` below renders the canonical spelling rather + // than echoing raw client bytes. + if !matches!(method, BundleMethod::Get) && let Some(criteria) = conditional_criteria(url, &id) { return create_error_result( @@ -719,7 +748,7 @@ where } match method { - "GET" => { + BundleMethod::Get => { // Read operation match state .storage() @@ -734,7 +763,7 @@ where } } } - "POST" => { + BundleMethod::Post => { // Create operation let resource = match entry.get("resource") { Some(r) => r.clone(), @@ -773,7 +802,7 @@ where } } } - "PUT" => { + BundleMethod::Put => { // Update operation let resource = match entry.get("resource") { Some(r) => r.clone(), @@ -847,7 +876,7 @@ where } } } - "DELETE" => { + BundleMethod::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. @@ -879,10 +908,24 @@ where } } } - _ => { - warn!(method = method, "Unsupported batch method"); - create_error_result(405, &format!("Unsupported method: {}", method)) - } + // Declined rather than dispatched, matching the transaction arm and all + // three backends, which already return 501 for a bundle PATCH. A + // bundle entry carries no Content-Type, and `parse_patch_format` + // derives the patch format entirely from it, so there is nothing here + // to dispatch on; R4 designates FHIRPath Patch as the bundle format and + // `apply_patch` does not implement it. Tracked by #502's follow-up. + BundleMethod::Patch => create_error_result( + 501, + &format!( + "Entry {index}: PATCH is not implemented in Bundle entries, so \ + nothing was applied. Send the patch to the instance endpoint \ + (PATCH [base]/[type]/[id])." + ), + ), + // No catch-all: the match is exhaustive over `BundleMethod`, so adding a + // variant is a compile error here rather than a silent 405. Codes + // outside the value set never reach this point — `parse_entry_method` + // refuses them at the top of this function. } } @@ -1137,6 +1180,114 @@ fn conditional_criteria<'a>(url: &'a str, id: &str) -> Option<&'a str> { .filter(|query| !query.is_empty()) } +/// Why a bundle entry's `request.method` was refused. +/// +/// The refusal carries its own status so the batch and transaction arms cannot +/// disagree about it. Batch renders it as a per-entry response and transaction +/// as the whole-bundle error, but the status is decided once, here — which is +/// the divergence #502 is about. +#[derive(Debug, Clone, PartialEq, Eq)] +enum EntryMethodRefusal { + /// `request.method` is absent, or is not a JSON string. + Missing, + /// Present, but not an `http-verb` code. Carries the raw spelling so the + /// message can show the client exactly what was sent. + NotCanonical(String), + /// `HEAD` — a legal `http-verb` code this server does not accept in a Bundle. + Head, +} + +impl EntryMethodRefusal { + fn status(&self) -> u16 { + match self { + Self::Missing | Self::NotCanonical(_) => 400, + Self::Head => 405, + } + } + + fn message(&self, index: usize) -> String { + match self { + Self::Missing => format!("Entry {index}: request.method is required"), + Self::NotCanonical(raw) => format!( + "Entry {index}: '{raw}' is not an http-verb code. \ + Bundle.entry.request.method is a code with a required binding to \ + http://hl7.org/fhir/ValueSet/http-verb, and FHIR codes are \ + case-sensitive — use GET, POST, PUT, PATCH or DELETE." + ), + Self::Head => format!( + "Entry {index}: HEAD is not supported in Bundle entries. Use GET, \ + or send HEAD to the instance endpoint directly." + ), + } + } + + /// Renders the refusal for the transaction arm, where it fails the bundle. + fn into_rest_error(self, index: usize) -> RestError { + let message = self.message(index); + match self { + Self::Head => RestError::MethodNotAllowed { + method: "HEAD".to_string(), + resource_type: format!("a Bundle entry (entry {index})"), + }, + Self::Missing | Self::NotCanonical(_) => RestError::BadRequest { message }, + } + } +} + +/// Parses a bundle entry's `request.method` into a [`BundleMethod`]. +/// +/// **This is the only `&str` -> `BundleMethod` table in this crate.** Both the +/// batch and the transaction arm go through it, which is the point: they used +/// to carry two independently-written matchers that disagreed, so the same +/// Bundle succeeded as a `transaction` and failed as a `batch` (#502). +/// +/// The match is deliberately **case-sensitive**. `Bundle.entry.request.method` +/// is a `code` with a *required* binding to `http://hl7.org/fhir/ValueSet/http-verb`, +/// whose concepts are `caseSensitive: true` and uppercase in every FHIR version +/// this server supports. A lowercase `"post"` is therefore invalid instance +/// data, not a valid entry a strict server wrongly rejects — so the previous +/// `to_uppercase()` on the transaction path was the non-conformant matcher, and +/// removing it is the fix rather than copying it across. +fn parse_entry_method(request: &Value) -> Result { + let Some(raw) = request.get("method").and_then(Value::as_str) else { + return Err(EntryMethodRefusal::Missing); + }; + + match raw { + "GET" => Ok(BundleMethod::Get), + "POST" => Ok(BundleMethod::Post), + "PUT" => Ok(BundleMethod::Put), + "PATCH" => Ok(BundleMethod::Patch), + "DELETE" => Ok(BundleMethod::Delete), + // A legal code, but one no bundle arm implements. HEAD *is* served on + // the instance-read route; it is Bundle entries it is refused in. + "HEAD" => Err(EntryMethodRefusal::Head), + _ => Err(EntryMethodRefusal::NotCanonical(raw.to_string())), + } +} + +/// Why a bundle entry could not be parsed at all. +/// +/// Split from a bare `String` so the method refusal keeps its status across the +/// transaction boundary; flattening it there is what would re-create #502's +/// divergence in a new place. +#[derive(Debug)] +enum EntryParseError { + Method(EntryMethodRefusal), + Malformed(String), +} + +impl EntryParseError { + fn into_rest_error(self, index: usize) -> RestError { + match self { + Self::Method(refusal) => refusal.into_rest_error(index), + Self::Malformed(message) => RestError::BadRequest { + message: format!("Entry {}: {}", index, message), + }, + } + } +} + /// Creates an error BundleEntryResult. /// Flatten an enforce-mode validation failure into a per-entry message /// (batch entry outcomes are message-based). @@ -1348,29 +1499,23 @@ fn rewrite_conditional_references( } } -fn parse_bundle_entry(entry: &Value) -> Result<(BundleEntry, Option), String> { +fn parse_bundle_entry(entry: &Value) -> Result<(BundleEntry, Option), EntryParseError> { let request = entry .get("request") - .ok_or_else(|| "Entry missing 'request'".to_string())?; + .ok_or_else(|| EntryParseError::Malformed("Entry missing 'request'".to_string()))?; - let method_str = request - .get("method") - .and_then(|v| v.as_str()) - .ok_or_else(|| "Entry request missing 'method'".to_string())?; - - let method = match method_str.to_uppercase().as_str() { - "GET" => BundleMethod::Get, - "POST" => BundleMethod::Post, - "PUT" => BundleMethod::Put, - "PATCH" => BundleMethod::Patch, - "DELETE" => BundleMethod::Delete, - _ => return Err(format!("Unsupported method: {}", method_str)), - }; + // Was an independently-written `to_uppercase()` ladder — the second of the + // two matchers #502 is about. It no longer case-folds: `request.method` is a + // `code` with a required binding, and folding it was the only thing standing + // between invalid instance data and a real write. The refusal keeps its + // status across this boundary so the whole-bundle error the caller raises + // agrees with the per-entry result the batch arm would produce. + let method = parse_entry_method(request).map_err(EntryParseError::Method)?; let url = request .get("url") .and_then(|v| v.as_str()) - .ok_or_else(|| "Entry request missing 'url'".to_string())? + .ok_or_else(|| EntryParseError::Malformed("Entry request missing 'url'".to_string()))? .to_string(); let resource = entry.get("resource").cloned(); @@ -2414,6 +2559,147 @@ mod tests { assert_eq!(conditional_criteria("Patient?", ""), None); } + /// `request.method` is a `code` with a required binding to `http-verb`, + /// whose concepts are case-sensitive and uppercase. Only those five codes + /// dispatch; everything else is refused, and the refusal carries the status + /// both bundle arms will use (#502). + #[test] + fn parse_entry_method_accepts_only_the_canonical_http_verb_codes() { + for (raw, expected) in [ + ("GET", BundleMethod::Get), + ("POST", BundleMethod::Post), + ("PUT", BundleMethod::Put), + ("PATCH", BundleMethod::Patch), + ("DELETE", BundleMethod::Delete), + ] { + let request = serde_json::json!({ "method": raw, "url": "Patient" }); + assert_eq!(parse_entry_method(&request), Ok(expected), "raw: {raw}"); + } + + // A legal http-verb code this server does not accept inside a Bundle. + // HEAD *is* served on the instance-read route. + let head = serde_json::json!({ "method": "HEAD", "url": "Patient/p1" }); + assert_eq!(parse_entry_method(&head), Err(EntryMethodRefusal::Head)); + assert_eq!(EntryMethodRefusal::Head.status(), 405); + + // Case-folded spellings are invalid instance data, not valid entries a + // strict server wrongly rejects — this is the premise #502 inverted. + for raw in ["post", "Post", "get", "Patch", "delete", "FOO", ""] { + let request = serde_json::json!({ "method": raw, "url": "Patient" }); + assert_eq!( + parse_entry_method(&request), + Err(EntryMethodRefusal::NotCanonical(raw.to_string())), + "raw: {raw}" + ); + } + assert_eq!( + EntryMethodRefusal::NotCanonical("post".to_string()).status(), + 400 + ); + + // Absent or non-string is distinguishable from a bogus code. It used to + // read as `""` via `unwrap_or("")`, yielding "Unsupported method: ". + for request in [ + serde_json::json!({ "url": "Patient" }), + serde_json::json!({ "method": 42, "url": "Patient" }), + serde_json::json!({ "method": null, "url": "Patient" }), + ] { + assert_eq!( + parse_entry_method(&request), + Err(EntryMethodRefusal::Missing), + "request: {request}" + ); + } + assert_eq!(EntryMethodRefusal::Missing.status(), 400); + } + + /// The refusal keeps its status across the transaction boundary. Flattening + /// it to a bare 400 there would re-create #502's divergence in a new place: + /// HEAD would be 405 per-entry in a batch and 400 for the whole bundle. + #[test] + fn a_method_refusal_keeps_its_status_on_the_transaction_path() { + let head = EntryMethodRefusal::Head.into_rest_error(3); + assert!( + matches!(head, RestError::MethodNotAllowed { .. }), + "HEAD must stay 405, got {head:?}" + ); + + let lowercase = EntryMethodRefusal::NotCanonical("post".to_string()).into_rest_error(0); + assert!(matches!(lowercase, RestError::BadRequest { .. })); + assert!(matches!( + EntryMethodRefusal::Missing.into_rest_error(0), + RestError::BadRequest { .. } + )); + } + + /// The transaction matcher no longer case-folds. `to_uppercase()` was the + /// only gate between an invalid `code` and a real write. + #[test] + fn the_transaction_matcher_no_longer_accepts_a_lowercase_method() { + let entry = serde_json::json!({ + "request": { "method": "post", "url": "Patient" }, + "resource": { "resourceType": "Patient" } + }); + let err = parse_bundle_entry(&entry).expect_err("must be refused"); + assert!(matches!( + err, + EntryParseError::Method(EntryMethodRefusal::NotCanonical(_)) + )); + + // The canonical spelling still parses. + let ok = serde_json::json!({ + "request": { "method": "POST", "url": "Patient" }, + "resource": { "resourceType": "Patient" } + }); + assert_eq!( + parse_bundle_entry(&ok).unwrap().0.method, + BundleMethod::Post + ); + } + + /// Refused methods are answered per-entry and never dispatch. + /// + /// `DelayStorage`'s write methods are `unimplemented!()`, so a refusal moved + /// after dispatch panics rather than silently writing. + #[tokio::test] + async fn refused_methods_are_answered_per_entry_and_never_reach_storage() { + let state = state_with(DelayStorage::new(8, 0)); + + let bundle = serde_json::json!({ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "request": { "method": "PATCH", "url": "Patient/p1" }, + "resource": { "resourceType": "Patient" } + }, + { "request": { "method": "HEAD", "url": "Patient/p1" } }, + { + "request": { "method": "post", "url": "Patient" }, + "resource": { "resourceType": "Patient" } + }, + { "request": { "url": "Patient/p1" } }, + ] + }); + + let response = run_batch(&state, &bundle, None).await; + let entries = response["entry"].as_array().unwrap(); + let statuses: Vec<&str> = entries + .iter() + .map(|e| e["response"]["status"].as_str().unwrap()) + .collect(); + assert_eq!( + statuses, + vec![ + "501 Not Implemented", + "405 Method Not Allowed", + "400 Bad Request", + "400 Bad Request", + ] + ); + assert_eq!(state.storage().peak(), 0, "no entry may reach storage"); + } + /// A conditional write is refused per-entry and never reaches storage. /// /// `DelayStorage::create_or_update` and `::delete` are `unimplemented!()`, diff --git a/crates/rest/tests/batch_conformance.rs b/crates/rest/tests/batch_conformance.rs index cea895aac..f5568b441 100644 --- a/crates/rest/tests/batch_conformance.rs +++ b/crates/rest/tests/batch_conformance.rs @@ -1145,3 +1145,211 @@ mod conditional_entries { ); } } + +// ============================================================================= +// Entry Method Tests (#502) +// ============================================================================= + +/// The batch and transaction arms parse `request.method` through one shared +/// matcher, so they accept exactly the same codes and refuse the rest with the +/// same status. +/// +/// `Bundle.entry.request.method` is a `code` with a required binding to +/// `http-verb`, whose concepts are case-sensitive and uppercase — so a lowercase +/// verb is invalid instance data, and the transaction arm's old `to_uppercase()` +/// was the non-conformant matcher rather than batch being wrongly strict. +mod entry_methods { + use super::*; + + 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")) + .await + .expect("count failed") + } + + fn batch_of(entries: Vec) -> Value { + json!({ "resourceType": "Bundle", "type": "batch", "entry": entries }) + } + + /// PATCH is declined at 501 — the status all three backends already return + /// from inside a transaction, and the one both READMEs already claimed. + #[tokio::test] + async fn batch_patch_is_declined_at_501_and_changes_nothing() { + let (server, backend) = create_test_server().await; + seed_patient(&backend, "p1", "Nguyen").await; + + let body = post_batch( + &server, + batch_of(vec![json!({ + "request": { "method": "PATCH", "url": "Patient/p1" }, + "resource": { "resourceType": "Patient", "name": [{"family": "Patched"}] } + })]), + ) + .await; + + assert_eq!( + body["entry"][0]["response"]["status"], + "501 Not Implemented" + ); + let stored = backend + .read(&test_tenant(), "Patient", "p1") + .await + .expect("read failed") + .expect("patient must survive"); + assert_eq!(stored.content()["name"][0]["family"], "Nguyen"); + } + + /// HEAD is a legal http-verb code this server does not accept in a Bundle. + #[tokio::test] + async fn batch_head_is_refused_at_405() { + let (server, _backend) = create_test_server().await; + + let body = post_batch( + &server, + batch_of(vec![json!({ + "request": { "method": "HEAD", "url": "Patient/p1" } + })]), + ) + .await; + + assert_eq!( + body["entry"][0]["response"]["status"], + "405 Method Not Allowed" + ); + } + + #[tokio::test] + async fn batch_refuses_a_lowercase_verb_and_a_missing_one() { + let (server, backend) = create_test_server().await; + let before = patient_count(&backend).await; + + let body = post_batch( + &server, + batch_of(vec![ + json!({ + "request": { "method": "post", "url": "Patient" }, + "resource": { "resourceType": "Patient" } + }), + json!({ "request": { "url": "Patient/p1" } }), + ]), + ) + .await; + + assert_eq!(body["entry"][0]["response"]["status"], "400 Bad Request"); + assert_eq!(body["entry"][1]["response"]["status"], "400 Bad Request"); + assert_eq!(patient_count(&backend).await, before); + } + + /// **The regression test for #502.** On the old code this entry created a + /// Patient: the transaction matcher upper-cased `"post"` and dispatched it, + /// while the same Bundle 405'd as a batch. + #[tokio::test] + async fn a_transaction_lowercase_verb_no_longer_writes() { + 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": "Lowercase"}] } + }] + }), + ) + .await; + + response.assert_status(StatusCode::BAD_REQUEST); + assert_eq!( + patient_count(&backend).await, + before, + "a lowercase verb must not create a resource" + ); + } + + /// A PATCH transaction is declined before anything executes, so a sibling + /// create in the same bundle must not have landed. + #[tokio::test] + async fn a_transaction_patch_is_declined_intact_at_501() { + 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": "PATCH", "url": "Patient/p1" }, + "resource": { "resourceType": "Patient" } + } + ] + }), + ) + .await; + + response.assert_status(StatusCode::NOT_IMPLEMENTED); + let body: Value = response.json(); + assert_eq!(body["resourceType"], "OperationOutcome"); + assert_eq!(body["issue"][0]["code"], "not-supported"); + assert!( + body["issue"][0]["details"]["text"] + .as_str() + .is_some_and(|t| t.contains("PATCH")), + "the outcome must name PATCH: {body}" + ); + assert_eq!(patient_count(&backend).await, before); + } + + /// The two arms agree on status, which is what #502 asks for: HEAD is 405 + /// whether it arrives per-entry in a batch or as a whole-bundle transaction + /// failure. Flattening the refusal at the transaction boundary would have + /// made this 400 and re-created the divergence in a new place. + #[tokio::test] + async fn the_two_arms_agree_on_the_refusal_status() { + let (server, _backend) = create_test_server().await; + + let batch = post_batch( + &server, + batch_of(vec![json!({ + "request": { "method": "HEAD", "url": "Patient/p1" } + })]), + ) + .await; + assert_eq!( + batch["entry"][0]["response"]["status"], + "405 Method Not Allowed" + ); + + let transaction = post_bundle( + &server, + json!({ + "resourceType": "Bundle", + "type": "transaction", + "entry": [{ "request": { "method": "HEAD", "url": "Patient/p1" } }] + }), + ) + .await; + transaction.assert_status(StatusCode::METHOD_NOT_ALLOWED); + } +} From 21e497cd976071b79a369c1e91d2cf56bebfca44 Mon Sep 17 00:00:00 2001 From: Alan Cruz Date: Wed, 5 Aug 2026 18:53:45 -0400 Subject: [PATCH 2/2] docs(rest): document entry-method matching and the PATCH/HEAD refusals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/rest/README.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/rest/README.md b/crates/rest/README.md index afdaa9603..eff21c6b9 100644 --- a/crates/rest/README.md +++ b/crates/rest/README.md @@ -499,6 +499,15 @@ curl -X POST http://localhost:8080/ \ }' ``` +### Entry Methods + +`Bundle.entry.request.method` is matched **case-sensitively** in both bundle types. +It is a `code` with a required binding to `http://hl7.org/fhir/ValueSet/http-verb`, +whose concepts are `caseSensitive: true` and uppercase in every supported FHIR +version — so a lowercase `"post"` is invalid instance data and is refused with +`400`, not silently accepted. `GET`, `POST`, `PUT` and `DELETE` dispatch; `PATCH` +and `HEAD` are refused as described under Current Limitations. + ### Conditional Operations in Bundles - `ifMatch` — **supported.** ETag for optimistic locking on `PUT` **and `DELETE`** @@ -528,7 +537,8 @@ endpoints and **not** for bundle entries; reconciling the two is #511. 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 +- **PATCH method** - PATCH operations in bundles return 501 Not Implemented, in both `batch` (per entry) and `transaction` (whole bundle). Send the patch to the instance endpoint instead +- **HEAD entries** - refused with 405. `HEAD` is a legal `http-verb` code and is served on the instance-read route, but not inside a Bundle - **Prefer header** - `return=minimal` and `return=OperationOutcome` not honored - **Duplicate detection** - Same resource appearing twice in a transaction is not detected