faces: surface UNIQUE constraint as 409, not 500
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) <noreply@anthropic.com>
This commit is contained in:
93
src/faces.rs
93
src/faces.rs
@@ -1516,12 +1516,14 @@ async fn create_person_handler<D: FaceDao>(
|
|||||||
Ok(p) => HttpResponse::Created().json(p),
|
Ok(p) => HttpResponse::Created().json(p),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
// SQLite UNIQUE(name COLLATE NOCASE) → 409 Conflict so the UI
|
// SQLite UNIQUE(name COLLATE NOCASE) → 409 Conflict so the UI
|
||||||
// can show "name already exists" without parsing.
|
// can show "name already exists" without parsing. Use {:#} to
|
||||||
let msg = format!("{}", e);
|
// include the source chain — anyhow's plain Display only shows
|
||||||
if msg.to_lowercase().contains("unique") {
|
// 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")
|
HttpResponse::Conflict().body("person name already exists")
|
||||||
} else {
|
} else {
|
||||||
HttpResponse::InternalServerError().body(msg)
|
HttpResponse::InternalServerError().body(format!("{:#}", e))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1559,11 +1561,10 @@ async fn update_person_handler<D: FaceDao>(
|
|||||||
match dao.update_person(&span_context, path.into_inner(), &body) {
|
match dao.update_person(&span_context, path.into_inner(), &body) {
|
||||||
Ok(p) => HttpResponse::Ok().json(p),
|
Ok(p) => HttpResponse::Ok().json(p),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
let msg = format!("{}", e);
|
if is_unique_violation(&e) {
|
||||||
if msg.to_lowercase().contains("unique") {
|
|
||||||
HttpResponse::Conflict().body("person name already exists")
|
HttpResponse::Conflict().body("person name already exists")
|
||||||
} else {
|
} else {
|
||||||
HttpResponse::InternalServerError().body(msg)
|
HttpResponse::InternalServerError().body(format!("{:#}", e))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1605,7 +1606,7 @@ async fn merge_persons_handler<D: FaceDao>(
|
|||||||
match dao.merge_persons(&span_context, src, body.into) {
|
match dao.merge_persons(&span_context, src, body.into) {
|
||||||
Ok(p) => HttpResponse::Ok().json(p),
|
Ok(p) => HttpResponse::Ok().json(p),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
let msg = format!("{}", e);
|
let msg = format!("{:#}", e);
|
||||||
if msg.contains("itself") {
|
if msg.contains("itself") {
|
||||||
HttpResponse::BadRequest().body(msg)
|
HttpResponse::BadRequest().body(msg)
|
||||||
} else {
|
} else {
|
||||||
@@ -1681,6 +1682,27 @@ fn crop_image_to_bbox(
|
|||||||
Ok(out.into_inner())
|
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::<DieselError>().is_some_and(|de| {
|
||||||
|
matches!(
|
||||||
|
de,
|
||||||
|
DieselError::DatabaseError(DatabaseErrorKind::UniqueViolation, _)
|
||||||
|
)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
// ── Tests ───────────────────────────────────────────────────────────────────
|
// ── Tests ───────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
@@ -1696,6 +1718,61 @@ mod tests {
|
|||||||
opentelemetry::Context::current()
|
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]
|
#[test]
|
||||||
fn person_crud_roundtrip() {
|
fn person_crud_roundtrip() {
|
||||||
let mut dao = fresh_dao();
|
let mut dao = fresh_dao();
|
||||||
|
|||||||
Reference in New Issue
Block a user