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 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); + } +}