From f985a0d65874ebae175219ee794398a761eba4d7 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Wed, 29 Apr 2026 18:44:10 +0000 Subject: [PATCH] faces: surface UNIQUE constraint as 409, not 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manual smoke test caught a bug: POST /persons with a duplicate name returned 500 with the body 'insert person Cameron' instead of the intended 409 Conflict. Root cause: the handler keyed on `format!("{}", e).contains("unique")`, but anyhow's plain Display only renders the *outermost* context ("insert person Cameron") and hides the diesel error nested below ('UNIQUE constraint failed: persons.name'). The string check was a false negative on every duplicate. Fix: walk the source chain and downcast for diesel::result::Error::DatabaseError(UniqueViolation, _) — exposed via a shared `is_unique_violation` helper used by both create_person_handler and update_person_handler. Error bodies for non-unique failures now use `{:#}` so the body actually carries the underlying cause when the user surfaces it. merge_persons_handler also moves to `{:#}` for richer error bodies; the "itself" check was already structural and unaffected. Regression test (faces::tests::is_unique_violation_walks_chain) pins both the bug shape ({} doesn't surface UNIQUE) and the fix (is_unique_violation correctly downcasts the chain), so a future refactor of error handling can't silently re-bury this. cargo test --lib: 171 / 0; fmt + clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/faces.rs | 93 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 85 insertions(+), 8 deletions(-) diff --git a/src/faces.rs b/src/faces.rs index ffd106e..8400f76 100644 --- a/src/faces.rs +++ b/src/faces.rs @@ -1516,12 +1516,14 @@ async fn create_person_handler( Ok(p) => HttpResponse::Created().json(p), Err(e) => { // SQLite UNIQUE(name COLLATE NOCASE) → 409 Conflict so the UI - // can show "name already exists" without parsing. - let msg = format!("{}", e); - if msg.to_lowercase().contains("unique") { + // can show "name already exists" without parsing. Use {:#} to + // include the source chain — anyhow's plain Display only shows + // the outermost context ("insert person ...") which hides the + // diesel "UNIQUE constraint failed" we're keying on. + if is_unique_violation(&e) { HttpResponse::Conflict().body("person name already exists") } else { - HttpResponse::InternalServerError().body(msg) + HttpResponse::InternalServerError().body(format!("{:#}", e)) } } } @@ -1559,11 +1561,10 @@ async fn update_person_handler( match dao.update_person(&span_context, path.into_inner(), &body) { Ok(p) => HttpResponse::Ok().json(p), Err(e) => { - let msg = format!("{}", e); - if msg.to_lowercase().contains("unique") { + if is_unique_violation(&e) { HttpResponse::Conflict().body("person name already exists") } else { - HttpResponse::InternalServerError().body(msg) + HttpResponse::InternalServerError().body(format!("{:#}", e)) } } } @@ -1605,7 +1606,7 @@ async fn merge_persons_handler( match dao.merge_persons(&span_context, src, body.into) { Ok(p) => HttpResponse::Ok().json(p), Err(e) => { - let msg = format!("{}", e); + let msg = format!("{:#}", e); if msg.contains("itself") { HttpResponse::BadRequest().body(msg) } else { @@ -1681,6 +1682,27 @@ fn crop_image_to_bbox( Ok(out.into_inner()) } +/// Returns true if `err` (or anything in its source chain) is a SQLite +/// `UNIQUE constraint failed`. Walks the chain so callers don't have to +/// know the wrapping order — anyhow `with_context` plus diesel's own +/// error layering buries the database error two levels deep. +/// +/// String matching on `format!("{:#}", e)` would also work but is +/// fragile (locale-dependent SQLite messages, false positives like +/// "uniquely identifies"). Downcasting to the actual diesel kind is +/// the contract-stable check. +fn is_unique_violation(err: &anyhow::Error) -> bool { + use diesel::result::{DatabaseErrorKind, Error as DieselError}; + err.chain().any(|cause| { + cause.downcast_ref::().is_some_and(|de| { + matches!( + de, + DieselError::DatabaseError(DatabaseErrorKind::UniqueViolation, _) + ) + }) + }) +} + // ── Tests ─────────────────────────────────────────────────────────────────── #[cfg(test)] @@ -1696,6 +1718,61 @@ mod tests { opentelemetry::Context::current() } + #[test] + fn is_unique_violation_walks_chain() { + // The bug we hit in manual testing: anyhow's plain Display only + // shows the outermost context ("insert person Cameron"), so a + // naive `format!("{}", e).contains("unique")` check misses the + // diesel UNIQUE error nested below. Downcasting the source chain + // is the stable contract. + let mut dao = fresh_dao(); + let _ = dao + .create_person( + &ctx(), + &CreatePersonReq { + name: "Cameron".into(), + notes: None, + entity_id: None, + }, + false, + ) + .expect("first insert"); + let dup_err = dao + .create_person( + &ctx(), + &CreatePersonReq { + name: "Cameron".into(), + notes: None, + entity_id: None, + }, + false, + ) + .expect_err("second insert must fail"); + + // Plain Display hides the UNIQUE — that's the bug we're guarding + // against. We don't assert a specific outer message; we just + // confirm string-matching at the top level is unreliable. + let plain = format!("{}", dup_err); + assert!( + !plain.to_lowercase().contains("unique"), + "if Display starts surfacing UNIQUE we can drop the helper, but \ + today it doesn't and the handler must downcast" + ); + + // Alt-Display walks the chain — useful for debug body content too. + let chained = format!("{:#}", dup_err); + assert!( + chained.to_uppercase().contains("UNIQUE"), + "chained display must surface the diesel error: {chained}" + ); + + // The contract-stable check the handler actually uses. + assert!( + is_unique_violation(&dup_err), + "is_unique_violation must downcast into the diesel chain" + ); + } + #[test] fn person_crud_roundtrip() { let mut dao = fresh_dao();