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();