Merge pull request 'One error response format: { error, code, details? } (step 1)' (#203) from feat/error-contract into master

Reviewed-on: #203

src/data/mod.rs

@@ -6,7 +6,6 @@ use anyhow::{Context, anyhow};
 use chrono::{DateTime, Utc};
 use log::error;
 
-use actix_web::error::ErrorUnauthorized;
 use actix_web::{Error, FromRequest, HttpRequest, dev, http::header};
 use futures::future::{Ready, err, ok};
 use jsonwebtoken::{Algorithm, DecodingKey, Validation, decode};
@@ -90,7 +89,11 @@ impl FromRequest for Claims {
             .map_or_else(
                 |e| {
                     error!("{}", e);
-                    err(ErrorUnauthorized("Bad token"))
+                    err(crate::error::ApiError::unauthorized(
+                        "unauthorized",
+                        "Your session has expired. Sign in again.",
+                    )
+                    .into())
                 },
                 ok,
             )

src/duplicates.rs

@@ -246,7 +246,9 @@ async fn list_exact_handler(
         match dao.list_duplicates_exact(library_id, include_resolved) {
             Ok(rows) => rows,
             Err(e) => {
-                return HttpResponse::InternalServerError().body(format!("{:?}", e));
+                return actix_web::ResponseError::error_response(
+                    &crate::error::ApiError::internal(e),
+                );
             }
         }
     };
@@ -287,7 +289,9 @@ async fn list_perceptual_handler(
         match dao.list_perceptual_candidates(library_id, include_resolved) {
             Ok(rows) => rows,
             Err(e) => {
-                return HttpResponse::InternalServerError().body(format!("{:?}", e));
+                return actix_web::ResponseError::error_response(
+                    &crate::error::ApiError::internal(e),
+                );
             }
         }
     };
@@ -325,7 +329,9 @@ async fn resolve_handler(
     {
         Ok(Some(row)) => row,
         Ok(None) => return HttpResponse::NotFound().body("survivor not found"),
-        Err(e) => return HttpResponse::InternalServerError().body(format!("{:?}", e)),
+        Err(e) => {
+            return actix_web::ResponseError::error_response(&crate::error::ApiError::internal(e));
+        }
     };
 
     // Survivor must not itself be soft-marked — otherwise the modal is
@@ -349,7 +355,9 @@ async fn resolve_handler(
                 continue;
             }
             Err(e) => {
-                return HttpResponse::InternalServerError().body(format!("{:?}", e));
+                return actix_web::ResponseError::error_response(
+                    &crate::error::ApiError::internal(e),
+                );
             }
         };
 
@@ -387,7 +395,7 @@ async fn resolve_handler(
             &survivor.content_hash,
             now,
         ) {
-            return HttpResponse::InternalServerError().body(format!("{:?}", e));
+            return actix_web::ResponseError::error_response(&crate::error::ApiError::internal(e));
         }
 
         resolved_count += 1;
@@ -408,7 +416,7 @@ async fn unresolve_handler(
 ) -> impl Responder {
     let mut dao = exif_dao.lock().expect("exif dao lock");
     if let Err(e) = dao.clear_duplicate_of(body.library_id, &body.rel_path) {
-        return HttpResponse::InternalServerError().body(format!("{:?}", e));
+        return actix_web::ResponseError::error_response(&crate::error::ApiError::internal(e));
     }
 
     drop(dao);
@@ -450,11 +458,19 @@ async fn list_folder_pairs_handler(
         let mut dao = exif_dao.lock().expect("exif dao lock");
         let dup_rows = match dao.list_duplicates_exact(library_id, include_resolved) {
             Ok(rows) => rows,
-            Err(e) => return HttpResponse::InternalServerError().body(format!("{:?}", e)),
+            Err(e) => {
+                return actix_web::ResponseError::error_response(
+                    &crate::error::ApiError::internal(e),
+                );
+            }
         };
         let all_paths = match dao.list_image_paths(library_id, include_resolved) {
             Ok(rows) => rows,
-            Err(e) => return HttpResponse::InternalServerError().body(format!("{:?}", e)),
+            Err(e) => {
+                return actix_web::ResponseError::error_response(
+                    &crate::error::ApiError::internal(e),
+                );
+            }
         };
         (dup_rows, all_paths)
     };

src/enhance/handlers.rs

@@ -24,27 +24,34 @@ use super::service::{EnhanceError, EnhanceService, SourceFile};
 use crate::data::Claims;
 use crate::database::ExifDao;
 use crate::database::enhance_dao::{EnhanceJobRow, STATUS_APPROVED, STATUS_READY};
+use crate::error::ApiError;
 use crate::libraries::{self, Library};
 use crate::state::AppState;
 use crate::utils::normalize_path;
 use crate::versions::handlers::build_response;
+use actix_web::ResponseError;
 
 type ExifData = web::Data<Mutex<Box<dyn ExifDao>>>;
 
+/// Enhance errors in the shared format (#202). The codes predate it and stay
+/// as they are: clients may already branch on them.
 fn error_response(e: &EnhanceError) -> HttpResponse {
-    let (mut builder, code) = match e {
-        EnhanceError::Disabled => (HttpResponse::ServiceUnavailable(), "disabled"),
-        EnhanceError::Unavailable => (HttpResponse::ServiceUnavailable(), "unavailable"),
-        EnhanceError::UnknownTemplate(_) => (HttpResponse::NotFound(), "unknown_template"),
-        EnhanceError::BadRequest(_) => (HttpResponse::BadRequest(), "bad_request"),
-        EnhanceError::Rejected(_) => (HttpResponse::UnprocessableEntity(), "rejected"),
-        EnhanceError::Expired => (HttpResponse::Gone(), "expired"),
-        EnhanceError::NotFound => (HttpResponse::NotFound(), "not_found"),
-        EnhanceError::Unplaceable(_) => (HttpResponse::BadGateway(), "unplaceable"),
-        EnhanceError::Upstream(_) => (HttpResponse::BadGateway(), "upstream"),
-        EnhanceError::Internal(_) => (HttpResponse::InternalServerError(), "internal"),
+    use actix_web::http::StatusCode;
+    let (status, code) = match e {
+        EnhanceError::Disabled => (StatusCode::SERVICE_UNAVAILABLE, "disabled"),
+        EnhanceError::Unavailable => (StatusCode::SERVICE_UNAVAILABLE, "unavailable"),
+        EnhanceError::UnknownTemplate(_) => (StatusCode::NOT_FOUND, "unknown_template"),
+        EnhanceError::BadRequest(_) => (StatusCode::BAD_REQUEST, "bad_request"),
+        EnhanceError::Rejected(_) => (StatusCode::UNPROCESSABLE_ENTITY, "rejected"),
+        EnhanceError::Expired => (StatusCode::GONE, "expired"),
+        EnhanceError::NotFound => (StatusCode::NOT_FOUND, "not_found"),
+        EnhanceError::Unplaceable(_) => (StatusCode::BAD_GATEWAY, "unplaceable"),
+        EnhanceError::Upstream(_) => (StatusCode::BAD_GATEWAY, "upstream"),
+        EnhanceError::Internal(_) => {
+            return ApiError::internal(e).error_response();
+        }
     };
-    builder.json(json!({ "error": e.message(), "code": code }))
+    ApiError::new(status, code, e.message()).error_response()
 }
 
 fn bad(msg: &str) -> HttpResponse {

src/error.rs

@@ -1,14 +1,318 @@
-use actix_web::{error::InternalError, http::StatusCode};
+//! The one error response format (#202):
+//!
+//! ```json
+//! { "error": "Tag name must not be empty", "code": "invalid_tag_name", "details": { } }
+//! ```
+//!
+//! - `error`: English, safe to show a user as is. Clients may translate known
+//!   codes into their own wording.
+//! - `code`: a stable snake_case id for clients to branch on. Never rename one
+//!   a client may depend on; add a new one instead.
+//! - `details` (optional): structured extras for the client.
+//!
+//! Handlers return `Result<_, ApiError>`. An unexpected failure (`anyhow`,
+//! diesel, I/O) converts into a 500 `internal` whose detail is logged and
+//! never sent: error text can hold SQL and library paths.
 
+use actix_web::error::{JsonPayloadError, PathError, QueryPayloadError};
+use actix_web::http::StatusCode;
+use actix_web::{HttpRequest, HttpResponse, ResponseError, web};
+use serde::{Deserialize, Serialize};
+use std::fmt;
+
+/// The JSON body of every error response.
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+pub struct ErrorBody {
+    pub error: String,
+    pub code: String,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub details: Option<serde_json::Value>,
+}
+
+/// An error response. Build one with a status constructor, e.g.
+/// `ApiError::bad_request("invalid_tag_name", "Tag name must not be empty")`.
+#[derive(Debug)]
+pub struct ApiError {
+    status: StatusCode,
+    code: &'static str,
+    message: String,
+    details: Option<serde_json::Value>,
+    /// Fields merged into the top level of the body, for responses that
+    /// carried them before this format existed. New code uses `details`.
+    legacy: Option<serde_json::Map<String, serde_json::Value>>,
+}
+
+pub type ApiResult<T> = Result<T, ApiError>;
+
+/// What a 500 says to the client; the real cause goes to the log.
+const INTERNAL_MESSAGE: &str = "Something went wrong on the server";
+
+impl ApiError {
+    pub fn new(status: StatusCode, code: &'static str, message: impl Into<String>) -> Self {
+        ApiError {
+            status,
+            code,
+            message: message.into(),
+            details: None,
+            legacy: None,
+        }
+    }
+
+    pub fn bad_request(code: &'static str, message: impl Into<String>) -> Self {
+        Self::new(StatusCode::BAD_REQUEST, code, message)
+    }
+
+    pub fn unauthorized(code: &'static str, message: impl Into<String>) -> Self {
+        Self::new(StatusCode::UNAUTHORIZED, code, message)
+    }
+
+    pub fn not_found(code: &'static str, message: impl Into<String>) -> Self {
+        Self::new(StatusCode::NOT_FOUND, code, message)
+    }
+
+    pub fn conflict(code: &'static str, message: impl Into<String>) -> Self {
+        Self::new(StatusCode::CONFLICT, code, message)
+    }
+
+    /// A 500 for an unexpected failure. `cause` is logged, never sent.
+    pub fn internal(cause: impl fmt::Debug) -> Self {
+        log::error!("Internal error: {:?}", cause);
+        Self::new(
+            StatusCode::INTERNAL_SERVER_ERROR,
+            "internal",
+            INTERNAL_MESSAGE,
+        )
+    }
+
+    pub fn with_details(mut self, details: impl Serialize) -> Self {
+        self.details = serde_json::to_value(details).ok();
+        self
+    }
+
+    /// Also put `key` at the top level of the body, where a client written
+    /// before this format reads it. The value is in `details` too.
+    pub fn with_legacy_field(mut self, key: &str, value: impl Serialize) -> Self {
+        let value = serde_json::to_value(value).unwrap_or_default();
+        let details = self
+            .details
+            .get_or_insert_with(|| serde_json::Value::Object(Default::default()));
+        if let Some(map) = details.as_object_mut() {
+            map.insert(key.to_string(), value.clone());
+        }
+        self.legacy
+            .get_or_insert_with(Default::default)
+            .insert(key.to_string(), value);
+        self
+    }
+
+    pub fn code(&self) -> &'static str {
+        self.code
+    }
+
+    pub fn body(&self) -> ErrorBody {
+        ErrorBody {
+            error: self.message.clone(),
+            code: self.code.to_string(),
+            details: self.details.clone(),
+        }
+    }
+}
+
+impl fmt::Display for ApiError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "{} {}: {}",
+            self.status.as_u16(),
+            self.code,
+            self.message
+        )
+    }
+}
+
+impl ResponseError for ApiError {
+    fn status_code(&self) -> StatusCode {
+        self.status
+    }
+
+    fn error_response(&self) -> HttpResponse {
+        let mut body = serde_json::to_value(self.body()).unwrap_or_default();
+        if let (Some(legacy), Some(map)) = (&self.legacy, body.as_object_mut()) {
+            for (k, v) in legacy {
+                map.entry(k.clone()).or_insert_with(|| v.clone());
+            }
+        }
+        HttpResponse::build(self.status).json(body)
+    }
+}
+
+impl From<anyhow::Error> for ApiError {
+    fn from(e: anyhow::Error) -> Self {
+        ApiError::internal(e)
+    }
+}
+
+impl From<diesel::result::Error> for ApiError {
+    fn from(e: diesel::result::Error) -> Self {
+        ApiError::internal(e)
+    }
+}
+
+/// Converts an `anyhow` result into an [`ApiResult`]: an error becomes a 500
+/// `internal` (logged, not sent). Prefer `?` with an `ApiResult` return type
+/// in new handlers.
 pub trait IntoHttpError<T> {
-    fn into_http_internal_err(self) -> Result<T, actix_web::Error>;
+    fn into_http_internal_err(self) -> ApiResult<T>;
 }
 
 impl<T> IntoHttpError<T> for Result<T, anyhow::Error> {
-    fn into_http_internal_err(self) -> Result<T, actix_web::Error> {
-        self.map_err(|e| {
-            log::error!("Map to err: {:?}", e);
-            InternalError::new(e, StatusCode::INTERNAL_SERVER_ERROR).into()
-        })
+    fn into_http_internal_err(self) -> ApiResult<T> {
+        self.map_err(ApiError::internal)
+    }
+}
+
+// ---- request-parsing failures ------------------------------------------
+//
+// actix answers these itself in plain text unless an extractor config says
+// otherwise. Register all three on the App (see `main.rs`).
+
+pub fn json_config() -> web::JsonConfig {
+    web::JsonConfig::default().error_handler(|err: JsonPayloadError, req: &HttpRequest| {
+        log::warn!("JSON body error on {} {}: {}", req.method(), req.uri(), err);
+        let api = match &err {
+            JsonPayloadError::OverflowKnownLength { .. } | JsonPayloadError::Overflow { .. } => {
+                ApiError::new(
+                    StatusCode::PAYLOAD_TOO_LARGE,
+                    "body_too_large",
+                    "The request is too large",
+                )
+            }
+            JsonPayloadError::ContentType => ApiError::bad_request(
+                "invalid_body",
+                "The request body must be JSON (Content-Type: application/json)",
+            ),
+            // serde's message names the field at fault, e.g. "missing field
+            // `file_names`"; it describes the request, not the server.
+            _ => ApiError::bad_request("invalid_body", format!("Invalid request body: {}", err)),
+        };
+        actix_web::error::InternalError::from_response(err, api.error_response()).into()
+    })
+}
+
+pub fn query_config() -> web::QueryConfig {
+    web::QueryConfig::default().error_handler(|err: QueryPayloadError, req: &HttpRequest| {
+        log::warn!("Query error on {} {}: {}", req.method(), req.uri(), err);
+        let api = ApiError::bad_request("invalid_query", format!("Invalid query: {}", err));
+        actix_web::error::InternalError::from_response(err, api.error_response()).into()
+    })
+}
+
+pub fn path_config() -> web::PathConfig {
+    web::PathConfig::default().error_handler(|err: PathError, req: &HttpRequest| {
+        log::warn!("Path error on {} {}: {}", req.method(), req.uri(), err);
+        let api = ApiError::bad_request("invalid_path", format!("Invalid URL: {}", err));
+        actix_web::error::InternalError::from_response(err, api.error_response()).into()
+    })
+}
+
+/// The App's default service: an unknown route answers in the same format.
+pub async fn unknown_route() -> HttpResponse {
+    ApiError::not_found("unknown_route", "No such endpoint").error_response()
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use actix_web::body::MessageBody;
+    use actix_web::{App, test as awtest};
+
+    fn body_of(resp: HttpResponse) -> serde_json::Value {
+        let bytes = resp.into_body().try_into_bytes().unwrap();
+        serde_json::from_slice(&bytes).unwrap()
+    }
+
+    #[test]
+    fn renders_error_and_code() {
+        let resp = ApiError::bad_request("invalid_tag_name", "Tag name must not be empty")
+            .error_response();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+        assert_eq!(
+            body_of(resp),
+            serde_json::json!({ "error": "Tag name must not be empty", "code": "invalid_tag_name" })
+        );
+    }
+
+    #[test]
+    fn details_and_legacy_fields() {
+        let resp = ApiError::conflict("tag_name_taken", "Taken")
+            .with_legacy_field("existing_tag", serde_json::json!({ "id": 3 }))
+            .error_response();
+        assert_eq!(
+            body_of(resp),
+            serde_json::json!({
+                "error": "Taken",
+                "code": "tag_name_taken",
+                "details": { "existing_tag": { "id": 3 } },
+                "existing_tag": { "id": 3 },
+            })
+        );
+    }
+
+    #[test]
+    fn internal_errors_never_send_their_cause() {
+        let cause = anyhow::anyhow!("SELECT * FROM image_exif WHERE rel_path = '/secret/a.jpg'");
+        let resp = ApiError::from(cause).error_response();
+        assert_eq!(resp.status(), StatusCode::INTERNAL_SERVER_ERROR);
+        let body = body_of(resp);
+        assert_eq!(body["code"], "internal");
+        assert!(!body.to_string().contains("secret"), "{body}");
+    }
+
+    #[derive(Deserialize)]
+    struct Q {
+        #[allow(dead_code)]
+        n: i32,
+    }
+
+    #[actix_rt::test]
+    async fn request_parsing_failures_use_the_format() {
+        let app = awtest::init_service(
+            App::new()
+                .app_data(json_config())
+                .app_data(query_config())
+                .app_data(path_config())
+                .route("/j", web::post().to(|_: web::Json<Q>| async { "ok" }))
+                .route("/q", web::get().to(|_: web::Query<Q>| async { "ok" }))
+                .route("/p/{n}", web::get().to(|_: web::Path<i32>| async { "ok" }))
+                .default_service(web::to(unknown_route)),
+        )
+        .await;
+
+        for (req, status, code) in [
+            (
+                awtest::TestRequest::post()
+                    .uri("/j")
+                    .set_json(serde_json::json!({})),
+                400,
+                "invalid_body",
+            ),
+            (
+                awtest::TestRequest::get().uri("/q?n=x"),
+                400,
+                "invalid_query",
+            ),
+            (awtest::TestRequest::get().uri("/p/x"), 400, "invalid_path"),
+            (
+                awtest::TestRequest::get().uri("/nope"),
+                404,
+                "unknown_route",
+            ),
+        ] {
+            let resp = awtest::call_service(&app, req.to_request()).await;
+            assert_eq!(resp.status().as_u16(), status);
+            let body: ErrorBody = awtest::read_body_json(resp).await;
+            assert_eq!(body.code, code);
+            assert!(!body.error.is_empty());
+        }
     }
 }

src/files.rs

@@ -578,7 +578,7 @@ pub async fn list_photos<TagD: TagDao, FS: FileSystemAccess>(
             })
         })
         .into_http_internal_err()
-        .unwrap_or_else(|e| e.error_response());
+        .unwrap_or_else(|e| actix_web::ResponseError::error_response(&e));
     }
 
     // In scoped mode `libraries_to_scan` has one entry (the selected library);

src/main.rs

@@ -12,7 +12,7 @@ use std::sync::{Arc, Mutex};
 use actix_cors::Cors;
 use actix_governor::{Governor, GovernorConfigBuilder};
 use actix_multipart as mp;
-use actix_web::{App, HttpResponse, HttpServer, middleware, web};
+use actix_web::{App, HttpServer, middleware, web};
 use diesel::sqlite::Sqlite;
 use std::error::Error;
 
@@ -444,18 +444,12 @@ fn main() -> std::io::Result<()> {
                     app_data.face_client.clone(),
                 ))
                 .app_data(mp::form::MultipartFormConfig::default().total_limit(1024 * 1024 * 1024)) // 1GB upload limit
-                .app_data(web::JsonConfig::default().error_handler(|err, req| {
-                    let detail = err.to_string();
-                    log::warn!(
-                        "JSON parse error on {} {}: {}",
-                        req.method(),
-                        req.uri(),
-                        detail
-                    );
-                    let response =
-                        HttpResponse::BadRequest().json(serde_json::json!({"error": detail}));
-                    actix_web::error::InternalError::from_response(err, response).into()
-                }))
+                // Request-parsing failures and unknown routes answer in the
+                // shared error format (#202), not actix's plain text.
+                .app_data(image_api::error::json_config())
+                .app_data(image_api::error::query_config())
+                .app_data(image_api::error::path_config())
+                .default_service(web::to(image_api::error::unknown_route))
                 .app_data::<Data<InsightGenerator>>(Data::new(app_data.insight_generator.clone()))
                 .wrap(prometheus.clone())
         })

src/tags.rs

@@ -1,5 +1,6 @@
 use crate::data::GetTagsRequest;
 use crate::database::ExifDao;
+use crate::error::{ApiError, ApiResult};
 use crate::libraries;
 use crate::otel::trace_db_call;
 use crate::state::AppState;
@@ -52,8 +53,7 @@ async fn add_tag<D: TagDao>(
     let normalized_path = normalize_path(&body.file_name);
 
     if tag_name.trim().is_empty() {
-        return Ok(HttpResponse::BadRequest()
-            .json(serde_json::json!({ "error": "Tag name must not be empty" })));
+        return Err(empty_tag_name());
     }
 
     let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao");
@@ -73,60 +73,55 @@ async fn update_tag<D: TagDao>(
     path: web::Path<i32>,
     body: web::Json<UpdateTagRequest>,
     tag_dao: web::Data<Mutex<D>>,
-) -> impl Responder {
+) -> ApiResult<HttpResponse> {
     let span = tracing::Span::current();
 
     let id = path.into_inner();
     let trimmed = body.name.trim();
     if trimmed.is_empty() {
-        return HttpResponse::BadRequest()
-            .json(serde_json::json!({ "error": "Tag name must not be empty" }));
+        return Err(empty_tag_name());
     }
 
     let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao");
-    match tag_dao.update_tag_name(id, trimmed) {
-        Ok(UpdateTagOutcome::Renamed(tag)) => {
+    match tag_dao.update_tag_name(id, trimmed)? {
+        UpdateTagOutcome::Renamed(tag) => {
             span.set_status(Status::Ok);
             info!("Renamed tag {} -> '{}'", id, trimmed);
-            HttpResponse::Ok().json(tag)
-        }
-        Ok(UpdateTagOutcome::NotFound) => {
-            HttpResponse::NotFound().json(serde_json::json!({ "error": "Tag not found" }))
-        }
-        Ok(UpdateTagOutcome::Conflict { existing }) => HttpResponse::Conflict().json(
-            serde_json::json!({ "error": "Tag name already exists", "existing_tag": existing }),
-        ),
-        Err(e) => {
-            log::error!("update_tag failed: {:?}", e);
-            HttpResponse::InternalServerError()
-                .json(serde_json::json!({ "error": "Update failed" }))
+            Ok(HttpResponse::Ok().json(tag))
         }
+        UpdateTagOutcome::NotFound => Err(tag_not_found()),
+        UpdateTagOutcome::Conflict { existing } => Err(ApiError::conflict(
+            "tag_name_taken",
+            format!("There's already a tag named \"{}\"", existing.name),
+        )
+        .with_legacy_field("existing_tag", existing)),
     }
 }
 
+fn empty_tag_name() -> ApiError {
+    ApiError::bad_request("invalid_tag_name", "Tag name must not be empty")
+}
+
+fn tag_not_found() -> ApiError {
+    ApiError::not_found("tag_not_found", "That tag no longer exists")
+}
+
 #[tracing::instrument(name = "delete_tag", skip_all)]
 async fn delete_tag<D: TagDao>(
     _: Claims,
     path: web::Path<i32>,
     tag_dao: web::Data<Mutex<D>>,
-) -> impl Responder {
+) -> ApiResult<HttpResponse> {
     let span = tracing::Span::current();
 
     let id = path.into_inner();
     let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao");
-    match tag_dao.delete_tag(id) {
-        Ok(true) => {
-            span.set_status(Status::Ok);
-            info!("Deleted tag {}", id);
-            HttpResponse::NoContent().finish()
-        }
-        Ok(false) => HttpResponse::NotFound().json(serde_json::json!({ "error": "Tag not found" })),
-        Err(e) => {
-            log::error!("delete_tag failed: {:?}", e);
-            HttpResponse::InternalServerError()
-                .json(serde_json::json!({ "error": "Delete failed" }))
-        }
+    if !tag_dao.delete_tag(id)? {
+        return Err(tag_not_found());
     }
+    span.set_status(Status::Ok);
+    info!("Deleted tag {}", id);
+    Ok(HttpResponse::NoContent().finish())
 }
 
 #[tracing::instrument(name = "get_tags", skip_all)]
@@ -226,23 +221,18 @@ async fn remove_tagged_photo<D: TagDao>(
     _: Claims,
     request: web::Json<AddTagRequest>,
     tag_dao: web::Data<Mutex<D>>,
-) -> impl Responder {
+) -> ApiResult<HttpResponse> {
     let span = tracing::Span::current();
     let normalized_path = normalize_path(&request.file_name);
 
     let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao");
-    tag_dao
-        .remove_tag(&request.tag_name, &normalized_path)
-        .map(|result| {
+    match tag_dao.remove_tag(&request.tag_name, &normalized_path)? {
+        Some(()) => {
             span.set_status(Status::Ok);
-
-            if result.is_some() {
-                HttpResponse::Ok()
-            } else {
-                HttpResponse::NotFound()
-            }
-        })
-        .into_http_internal_err()
+            Ok(HttpResponse::Ok().finish())
+        }
+        None => Err(tag_not_found()),
+    }
 }
 
 #[tracing::instrument(name = "update_tags", skip_all)]
@@ -322,9 +312,8 @@ async fn bulk_tag<D: TagDao>(
     _: Claims,
     body: web::Json<BulkTagRequest>,
     tag_dao: web::Data<Mutex<D>>,
-) -> impl Responder {
+) -> ApiResult<HttpResponse> {
     let span = tracing::Span::current();
-    let bad = |msg: &str| HttpResponse::BadRequest().json(serde_json::json!({ "error": msg }));
 
     let mut paths: Vec<String> = Vec::with_capacity(body.file_names.len());
     for p in body.file_names.iter().map(|p| normalize_path(p)) {
@@ -333,16 +322,20 @@ async fn bulk_tag<D: TagDao>(
         }
     }
     if paths.is_empty() {
-        return bad("file_names must not be empty");
+        return Err(ApiError::bad_request("no_files", "No photos were given"));
     }
     if paths.len() > BULK_TAG_MAX_FILES {
-        return bad(&format!("At most {} files per request", BULK_TAG_MAX_FILES));
+        return Err(ApiError::bad_request(
+            "too_many_files",
+            format!("Tag at most {} photos at a time", BULK_TAG_MAX_FILES),
+        )
+        .with_details(serde_json::json!({ "max": BULK_TAG_MAX_FILES })));
     }
 
     let mut add: Vec<String> = Vec::with_capacity(body.add.len());
     for name in body.add.iter().map(|n| n.trim()) {
         if name.is_empty() {
-            return bad("Tag name must not be empty");
+            return Err(empty_tag_name());
         }
         if !add.iter().any(|a| a.eq_ignore_ascii_case(name)) {
             add.push(name.to_string());
@@ -352,31 +345,32 @@ async fn bulk_tag<D: TagDao>(
     remove.sort_unstable();
     remove.dedup();
     if add.is_empty() && remove.is_empty() {
-        return bad("Nothing to add or remove");
+        return Err(ApiError::bad_request(
+            "nothing_to_change",
+            "Choose a tag to add or remove",
+        ));
     }
 
     span.set_attribute("path_count", paths.len() as i64);
     let mut dao = tag_dao.lock().expect("Unable to get TagDao");
-    match dao.bulk_tag(&paths, &add, &remove) {
-        Ok(BulkTagOutcome::Applied {
+    match dao.bulk_tag(&paths, &add, &remove)? {
+        BulkTagOutcome::Applied {
             added,
             removed,
             tags,
-        }) => {
+        } => {
             span.set_status(Status::Ok);
-            HttpResponse::Ok().json(BulkTagResponse {
+            Ok(HttpResponse::Ok().json(BulkTagResponse {
                 added,
                 removed,
                 tags,
-            })
-        }
-        Ok(BulkTagOutcome::Conflict { tag }) => HttpResponse::BadRequest()
-            .json(serde_json::json!({ "error": "A tag is both added and removed", "tag": tag })),
-        Err(e) => {
-            log::error!("bulk_tag failed: {:?}", e);
-            HttpResponse::InternalServerError()
-                .json(serde_json::json!({ "error": "Bulk tag failed" }))
+            }))
         }
+        BulkTagOutcome::Conflict { tag } => Err(ApiError::bad_request(
+            "tag_added_and_removed",
+            format!("\"{}\" can't be both added and removed", tag.name),
+        )
+        .with_details(serde_json::json!({ "tag": tag }))),
     }
 }
 
@@ -410,12 +404,12 @@ async fn lookup_tags_batch<D: TagDao>(
     body: web::Json<LookupTagsBatchRequest>,
     tag_dao: web::Data<Mutex<D>>,
     exif_dao: web::Data<Mutex<Box<dyn ExifDao>>>,
-) -> impl Responder {
+) -> ApiResult<HttpResponse> {
     use std::collections::{HashMap, HashSet};
     let span = tracing::Span::current();
 
     if body.paths.is_empty() {
-        return HttpResponse::Ok().json(HashMap::<String, Vec<Tag>>::new());
+        return Ok(HttpResponse::Ok().json(HashMap::<String, Vec<Tag>>::new()));
     }
 
     let query_paths: Vec<String> = body.paths.iter().map(|p| normalize_path(p)).collect();
@@ -431,13 +425,8 @@ async fn lookup_tags_batch<D: TagDao>(
     // every sibling tag set.
     let exif_records = {
         let mut dao = exif_dao.lock().expect("Unable to get ExifDao");
-        match dao.get_exif_batch(None, &query_paths) {
-            Ok(rows) => rows,
-            Err(e) => {
-                return HttpResponse::InternalServerError()
-                    .body(format!("exif batch lookup failed: {:?}", e));
-            }
-        }
+        dao.get_exif_batch(None, &query_paths)
+            .map_err(|e| ApiError::internal(format!("exif batch lookup failed: {:?}", e)))?
     };
     let mut hash_by_path: HashMap<String, String> = HashMap::with_capacity(exif_records.len());
     for record in exif_records {
@@ -457,13 +446,8 @@ async fn lookup_tags_batch<D: TagDao>(
         HashMap::new()
     } else {
         let mut dao = exif_dao.lock().expect("Unable to get ExifDao");
-        match dao.get_rel_paths_for_hashes(&unique_hashes) {
-            Ok(map) => map,
-            Err(e) => {
-                return HttpResponse::InternalServerError()
-                    .body(format!("hash sibling lookup failed: {:?}", e));
-            }
-        }
+        dao.get_rel_paths_for_hashes(&unique_hashes)
+            .map_err(|e| ApiError::internal(format!("hash sibling lookup failed: {:?}", e)))?
     };
 
     // Stage 3: build expanded path set and the reverse map
@@ -498,12 +482,7 @@ async fn lookup_tags_batch<D: TagDao>(
     let all_paths_vec: Vec<String> = all_paths.into_iter().collect();
     let tags_by_sibling = {
         let mut dao = tag_dao.lock().expect("Unable to get TagDao");
-        match dao.get_tags_grouped_by_paths(&all_paths_vec) {
-            Ok(map) => map,
-            Err(e) => {
-                return HttpResponse::InternalServerError().body(format!("{}", e));
-            }
-        }
+        dao.get_tags_grouped_by_paths(&all_paths_vec)?
     };
 
     // Stage 5: aggregate sibling tags back to original query paths,
@@ -524,7 +503,7 @@ async fn lookup_tags_batch<D: TagDao>(
     }
 
     span.set_status(Status::Ok);
-    HttpResponse::Ok().json(result)
+    Ok(HttpResponse::Ok().json(result))
 }
 
 #[derive(Serialize, Queryable, Clone, Debug, PartialEq)]
@@ -1707,7 +1686,7 @@ mod tests {
 
         let tag_data = Data::new(Mutex::new(tag_dao));
         add_tag(claims.clone(), web::Json(add_request), tag_data.clone()).await;
-        remove_tagged_photo(claims, web::Json(remove_request), tag_data.clone()).await;
+        let _ = remove_tagged_photo(claims, web::Json(remove_request), tag_data.clone()).await;
 
         let mut tag_dao = tag_data.lock().unwrap();
         let tags = tag_dao.get_all_tags(None).unwrap();
@@ -2059,6 +2038,32 @@ mod sqlite_tests {
         assert_eq!(fx.tag_names("2020/b.jpg"), vec!["beach"]);
     }
 
+    #[actix_rt::test]
+    async fn a_rename_conflict_keeps_existing_tag_at_the_top_level() {
+        let fx = Fx::new();
+        let beach = fx.tag_id("beach");
+        let other = fx.tag_id("other");
+        let resp = update_tag(
+            claims(),
+            web::Path::from(other),
+            web::Json(UpdateTagRequest {
+                name: "BEACH".into(),
+            }),
+            fx.data.clone(),
+        )
+        .await
+        .respond_to(&actix_web::test::TestRequest::default().to_http_request());
+        assert_eq!(resp.status(), StatusCode::CONFLICT);
+        let body: serde_json::Value =
+            serde_json::from_slice(&resp.into_body().try_into_bytes().unwrap()).unwrap();
+        assert_eq!(body["code"], "tag_name_taken");
+        assert_eq!(
+            body["existing_tag"]["id"], beach,
+            "older clients read it here"
+        );
+        assert_eq!(body["details"]["existing_tag"]["id"], beach);
+    }
+
     #[actix_rt::test]
     async fn removing_an_unknown_tag_is_404() {
         let fx = Fx::new();
@@ -2173,7 +2178,8 @@ mod sqlite_tests {
         let id = fx.tag_id("beach");
         let (st, body) = fx.bulk(&["a.jpg"], &["BEACH", "new"], &[id]).await;
         assert_eq!(st, StatusCode::BAD_REQUEST);
-        assert_eq!(body["tag"]["id"], id);
+        assert_eq!(body["code"], "tag_added_and_removed");
+        assert_eq!(body["details"]["tag"]["id"], id);
         assert_eq!(
             fx.count("SELECT COUNT(*) AS n FROM tags"),
             1,
@@ -2184,25 +2190,19 @@ mod sqlite_tests {
     #[actix_rt::test]
     async fn bulk_rejects_empty_requests() {
         let fx = Fx::new();
-        assert_eq!(
-            fx.bulk(&[], &["beach"], &[]).await.0,
-            StatusCode::BAD_REQUEST
-        );
-        assert_eq!(
-            fx.bulk(&["a.jpg"], &[], &[]).await.0,
-            StatusCode::BAD_REQUEST
-        );
-        assert_eq!(
-            fx.bulk(&["a.jpg"], &[" "], &[]).await.0,
-            StatusCode::BAD_REQUEST
-        );
         let many: Vec<String> = (0..=BULK_TAG_MAX_FILES)
             .map(|i| format!("{i}.jpg"))
             .collect();
         let many: Vec<&str> = many.iter().map(String::as_str).collect();
-        assert_eq!(
-            fx.bulk(&many, &["beach"], &[]).await.0,
-            StatusCode::BAD_REQUEST
-        );
+        for ((st, body), code) in [
+            (fx.bulk(&[], &["beach"], &[]).await, "no_files"),
+            (fx.bulk(&["a.jpg"], &[], &[]).await, "nothing_to_change"),
+            (fx.bulk(&["a.jpg"], &[" "], &[]).await, "invalid_tag_name"),
+            (fx.bulk(&many, &["beach"], &[]).await, "too_many_files"),
+        ] {
+            assert_eq!(st, StatusCode::BAD_REQUEST, "{code}");
+            assert_eq!(body["code"], code);
+            assert!(body["error"].as_str().is_some_and(|e| !e.is_empty()));
+        }
     }
 }