diff --git a/src/data/mod.rs b/src/data/mod.rs index f872f0f..add389b 100644 --- a/src/data/mod.rs +++ b/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, ) diff --git a/src/duplicates.rs b/src/duplicates.rs index 69e17ec..2a86620 100644 --- a/src/duplicates.rs +++ b/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) }; diff --git a/src/enhance/handlers.rs b/src/enhance/handlers.rs index 44ae37e..7a342a8 100644 --- a/src/enhance/handlers.rs +++ b/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>>; +/// 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 { diff --git a/src/error.rs b/src/error.rs index 01b6bae..3a88997 100644 --- a/src/error.rs +++ b/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, +} + +/// 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, + /// Fields merged into the top level of the body, for responses that + /// carried them before this format existed. New code uses `details`. + legacy: Option>, +} + +pub type ApiResult = Result; + +/// 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) -> Self { + ApiError { + status, + code, + message: message.into(), + details: None, + legacy: None, + } + } + + pub fn bad_request(code: &'static str, message: impl Into) -> Self { + Self::new(StatusCode::BAD_REQUEST, code, message) + } + + pub fn unauthorized(code: &'static str, message: impl Into) -> Self { + Self::new(StatusCode::UNAUTHORIZED, code, message) + } + + pub fn not_found(code: &'static str, message: impl Into) -> Self { + Self::new(StatusCode::NOT_FOUND, code, message) + } + + pub fn conflict(code: &'static str, message: impl Into) -> 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 for ApiError { + fn from(e: anyhow::Error) -> Self { + ApiError::internal(e) + } +} + +impl From 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 { - fn into_http_internal_err(self) -> Result; + fn into_http_internal_err(self) -> ApiResult; } impl IntoHttpError for Result { - fn into_http_internal_err(self) -> Result { - 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 { + 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| async { "ok" })) + .route("/q", web::get().to(|_: web::Query| async { "ok" })) + .route("/p/{n}", web::get().to(|_: web::Path| 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()); + } } } diff --git a/src/files.rs b/src/files.rs index 8f38412..586fa7b 100644 --- a/src/files.rs +++ b/src/files.rs @@ -578,7 +578,7 @@ pub async fn list_photos( }) }) .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); diff --git a/src/main.rs b/src/main.rs index 411a096..fe46de7 100644 --- a/src/main.rs +++ b/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::new(app_data.insight_generator.clone())) .wrap(prometheus.clone()) }) diff --git a/src/tags.rs b/src/tags.rs index 91842b6..f49b6ae 100644 --- a/src/tags.rs +++ b/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( 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( path: web::Path, body: web::Json, tag_dao: web::Data>, -) -> impl Responder { +) -> ApiResult { 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( _: Claims, path: web::Path, tag_dao: web::Data>, -) -> impl Responder { +) -> ApiResult { 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( _: Claims, request: web::Json, tag_dao: web::Data>, -) -> impl Responder { +) -> ApiResult { 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( _: Claims, body: web::Json, tag_dao: web::Data>, -) -> impl Responder { +) -> ApiResult { let span = tracing::Span::current(); - let bad = |msg: &str| HttpResponse::BadRequest().json(serde_json::json!({ "error": msg })); let mut paths: Vec = 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( } } 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 = 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( 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( body: web::Json, tag_dao: web::Data>, exif_dao: web::Data>>, -) -> impl Responder { +) -> ApiResult { use std::collections::{HashMap, HashSet}; let span = tracing::Span::current(); if body.paths.is_empty() { - return HttpResponse::Ok().json(HashMap::>::new()); + return Ok(HttpResponse::Ok().json(HashMap::>::new())); } let query_paths: Vec = body.paths.iter().map(|p| normalize_path(p)).collect(); @@ -431,13 +425,8 @@ async fn lookup_tags_batch( // 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 = HashMap::with_capacity(exif_records.len()); for record in exif_records { @@ -457,13 +446,8 @@ async fn lookup_tags_batch( 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( let all_paths_vec: Vec = 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( } 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 = (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())); + } } }