From 96a2858a506f8b07c50058b7e9e86fa7f32cb4a8 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sun, 1 Nov 2026 12:49:31 -0500 Subject: [PATCH] Tag writes: POST /image/tags/bulk, and fix remove/re-add/duplicate bugs - POST /image/tags/bulk { file_names, add: [names], remove: [ids] }: one transaction over many photos; names match case-insensitively and are created when missing; 400 if a tag is both added and removed. - Adding a tag a photo already has is a no-op (was a 500 on UNIQUE(rel_path, tag_id)). - Adding a tag no photo carries reuses it (the lookup only saw tags in use, so create_tag hit idx_tags_name_nocase: 500). update_tags could likewise skip such ids silently. - Removing a tag matches the name case-insensitively and deletes it from every path sharing the photo's content, as reads union them; it used to answer 200 and leave the tag on the other copy. - update_tags returns errors instead of panicking with the TagDao lock held, which poisoned it for every later tag request. Closes #200 Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 8 +- src/tags.rs | 796 +++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 673 insertions(+), 131 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9020328..5a4b689 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -727,9 +727,11 @@ GET /video/original/stream?token=... (Range-capable original bytes) // Tags GET /image/tags/all (tags + counts, camera_makes, and cameras: make+model bodies) -POST /image/tags (add tag to file) -DELETE /image/tags (remove tag from file) -POST /image/tags/batch (bulk tag updates) +POST /image/tags (add tag to file; by name, case-insensitive, created when missing; already tagged = no-op) +DELETE /image/tags (remove tag from file: every path sharing its content, matching reads) +POST /image/tags/batch (replace ONE file's tag set: { file_name, tag_ids }) +POST /image/tags/bulk (many files, one transaction: { file_names, add: [names], remove: [ids] } + -> { added, removed, tags }; 400 if a tag is in both) // Photo versions (#191): AI restorations / upscales / edits shown as ONE photo GET /image/versions?path=...&library=... (path of the original or any version -> diff --git a/src/tags.rs b/src/tags.rs index fd0165f..91842b6 100644 --- a/src/tags.rs +++ b/src/tags.rs @@ -33,6 +33,7 @@ where .service(web::resource("image/tags/all").route(web::get().to(get_all_tags::))) .service(web::resource("image/tags/batch").route(web::post().to(update_tags::))) .service(web::resource("image/tags/lookup").route(web::post().to(lookup_tags_batch::))) + .service(web::resource("image/tags/bulk").route(web::post().to(bulk_tag::))) .service( web::resource("image/tags/{id}") .route(web::put().to(update_tag::)) @@ -50,32 +51,18 @@ async fn add_tag( let tag_name = body.tag_name.clone(); let normalized_path = normalize_path(&body.file_name); - let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao"); + if tag_name.trim().is_empty() { + return Ok(HttpResponse::BadRequest() + .json(serde_json::json!({ "error": "Tag name must not be empty" }))); + } + let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao"); tag_dao - .get_all_tags(None) - .and_then(|tags| { - // Case-insensitive match. With the unique-NOCASE index on - // tags.name now in place, a case-sensitive find here would - // miss a casing-only collision and let the subsequent - // create_tag INSERT crash on the constraint. - if let Some((_, tag)) = tags - .iter() - .find(|t| t.1.name.eq_ignore_ascii_case(&tag_name)) - { - Ok(tag.clone()) - } else { - info!( - "Creating missing tag: '{:?}' for file: '{}'", - tag_name, &normalized_path - ); - tag_dao.create_tag(tag_name.trim()) - } - }) + .find_or_create_tag(&tag_name) .and_then(|tag| tag_dao.tag_file(&normalized_path, tag.id)) .map(|_| { span.set_status(Status::Ok); - HttpResponse::Ok() + HttpResponse::Ok().finish() }) .into_http_internal_err() } @@ -268,50 +255,131 @@ async fn update_tags( let span = tracing::Span::current(); let normalized_path = normalize_path(&request.file_name); - dao.get_tags_for_path(&normalized_path) - .and_then(|existing_tags| dao.get_all_tags(None).map(|all| (existing_tags, all))) - .map(|(existing_tags, all_tags)| { - let tags_to_remove = existing_tags - .iter() - .filter(|&t| !request.tag_ids.contains(&t.id)) - .collect::>(); - - for tag in tags_to_remove { - info!( - "Removing tag {:?} from file: {:?}", - tag.name, normalized_path - ); - dao.remove_tag(&tag.name, &normalized_path) - .unwrap_or_else(|err| panic!("{:?} Unable to remove tag {:?}", err, &tag.name)); - } - - let new_tags = all_tags - .iter() - .filter(|(_, t)| !existing_tags.contains(t) && request.tag_ids.contains(&t.id)) - .collect::>(); - - for (_, new_tag) in new_tags { - info!( - "Adding tag {:?} to file: {:?}", - new_tag.name, normalized_path - ); - - dao.tag_file(&normalized_path, new_tag.id) - .with_context(|| { - format!( - "Unable to tag file {:?} with tag: {:?}", - normalized_path, new_tag.name - ) - }) - .unwrap(); - } + // Errors propagate instead of panicking: a panic here would poison the + // TagDao mutex and fail every later tag request until a restart. + let result = (|| -> anyhow::Result<()> { + let existing_tags = dao.get_tags_for_path(&normalized_path)?; + for tag in existing_tags + .iter() + .filter(|t| !request.tag_ids.contains(&t.id)) + { + info!( + "Removing tag {:?} from file: {:?}", + tag.name, normalized_path + ); + dao.remove_tag(&tag.name, &normalized_path)?; + } + for tag_id in request + .tag_ids + .iter() + .filter(|id| !existing_tags.iter().any(|t| t.id == **id)) + { + info!("Adding tag {} to file: {:?}", tag_id, normalized_path); + dao.tag_file(&normalized_path, *tag_id)?; + } + Ok(()) + })(); + result + .map(|_| { span.set_status(Status::Ok); - HttpResponse::Ok() + HttpResponse::Ok().finish() }) .into_http_internal_err() } +/// Most photos one `POST /image/tags/bulk` may name. A select-all over a +/// large folder stays well under it; the transaction holds SQLite's write +/// lock for the whole batch. +const BULK_TAG_MAX_FILES: usize = 5000; + +#[derive(Deserialize, Debug)] +pub struct BulkTagRequest { + pub file_names: Vec, + /// Tag names to add, created when missing (case-insensitive match). + #[serde(default)] + pub add: Vec, + /// Tag ids to remove. + #[serde(default)] + pub remove: Vec, +} + +#[derive(Serialize, Debug)] +pub struct BulkTagResponse { + /// `tagged_photo` rows written (photos that already had a tag don't count). + pub added: usize, + /// `tagged_photo` rows deleted, across every copy of each photo. + pub removed: usize, + /// The resolved `add` tags, so a client learns the ids of new ones. + pub tags: Vec, +} + +/// Add and remove tags on many photos in one transaction: all of it applies +/// or none does. Tags are library-agnostic (they follow the bytes), so this +/// takes no `library`. Removal follows the photo's content like reads do. +#[tracing::instrument(name = "bulk_tag", skip_all)] +async fn bulk_tag( + _: Claims, + body: web::Json, + tag_dao: web::Data>, +) -> impl Responder { + 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)) { + if !paths.contains(&p) { + paths.push(p); + } + } + if paths.is_empty() { + return bad("file_names must not be empty"); + } + if paths.len() > BULK_TAG_MAX_FILES { + return bad(&format!("At most {} files per request", 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"); + } + if !add.iter().any(|a| a.eq_ignore_ascii_case(name)) { + add.push(name.to_string()); + } + } + let mut remove = body.remove.clone(); + remove.sort_unstable(); + remove.dedup(); + if add.is_empty() && remove.is_empty() { + return bad("Nothing 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 { + added, + removed, + tags, + }) => { + span.set_status(Status::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" })) + } + } +} + #[derive(Deserialize, Debug)] pub struct LookupTagsBatchRequest { pub paths: Vec, @@ -552,6 +620,20 @@ pub enum UpdateTagOutcome { Conflict { existing: Tag }, } +/// Result of [`TagDao::bulk_tag`]. +#[derive(Debug, PartialEq)] +pub enum BulkTagOutcome { + /// `tags` are the resolved `add` tags (existing or just created); + /// `added` / `removed` count `tagged_photo` rows written / deleted. + Applied { + added: usize, + removed: usize, + tags: Vec, + }, + /// A tag named in `add` is also in `remove`. Nothing was written. + Conflict { tag: Tag }, +} + pub trait TagDao: Send + Sync { fn get_all_tags(&mut self, path: Option) -> anyhow::Result>; fn get_tags_for_path(&mut self, path: &str) -> anyhow::Result>; @@ -567,8 +649,27 @@ pub trait TagDao: Send + Sync { paths: &[String], ) -> anyhow::Result>>; fn create_tag(&mut self, name: &str) -> anyhow::Result; + /// The tag named `name` (case-insensitive, like `idx_tags_name_nocase`), + /// created when there is none. Unlike `get_all_tags`, this sees tags + /// that no photo carries any more. + fn find_or_create_tag(&mut self, name: &str) -> anyhow::Result; + /// Remove the tag named `tag_name` (case-insensitive) from the photo at + /// `path`: from every path sharing its content and, for a photo version, + /// from its original, matching what reads show. `None` when no tag has + /// that name. fn remove_tag(&mut self, tag_name: &str, path: &str) -> anyhow::Result>; + /// Tag the photo at `path` (a version's tag goes on its original). + /// Tagging a photo that already carries the tag is a no-op. fn tag_file(&mut self, path: &str, tag_id: i32) -> anyhow::Result; + /// Add the tags named `add` to every photo in `paths` and remove the tags + /// with ids `remove` from them, in one transaction. Names follow + /// [`TagDao::find_or_create_tag`], removal follows [`TagDao::remove_tag`]. + fn bulk_tag( + &mut self, + paths: &[String], + add: &[String], + remove: &[i32], + ) -> anyhow::Result; fn get_files_with_all_tag_ids( &mut self, tag_ids: Vec, @@ -614,6 +715,102 @@ fn canonical_path(conn: &mut SqliteConnection, path: &str) -> String { .unwrap_or_else(|| path.to_string()) } +/// The tag named `name`, matched case-insensitively like the +/// `idx_tags_name_nocase` index (SQLite's NOCASE folds ASCII only, as does +/// `eq_ignore_ascii_case`). The tags table is small; this mirrors the +/// conflict check in `update_tag_name`. +fn find_tag_by_name(conn: &mut SqliteConnection, name: &str) -> anyhow::Result> { + Ok(tags::table + .select((tags::id, tags::name, tags::created_time)) + .get_results::(conn) + .with_context(|| "Unable to list tags")? + .into_iter() + .find(|t| t.name.eq_ignore_ascii_case(name))) +} + +fn find_or_create_tag_in(conn: &mut SqliteConnection, name: &str) -> anyhow::Result { + let name = name.trim(); + if let Some(tag) = find_tag_by_name(conn, name)? { + return Ok(tag); + } + diesel::insert_into(tags::table) + .values(InsertTag { + name: name.to_string(), + created_time: Utc::now().timestamp(), + }) + .execute(conn) + .with_context(|| format!("Unable to insert tag {:?}", name))?; + info!("Created tag: {:?}", name); + find_tag_by_name(conn, name)?.with_context(|| format!("Tag {:?} vanished after insert", name)) +} + +/// Insert the `(path, tag_id)` row unless it exists (`UNIQUE(rel_path, +/// tag_id)`), on the canonical path. Returns that path and the number of rows +/// inserted (0 or 1). +fn insert_tag_row( + conn: &mut SqliteConnection, + path: &str, + tag_id: i32, +) -> anyhow::Result<(String, usize)> { + // Tagging a photo version tags its original (photo versions, #191): + // the tag binds to the original's bytes and path. + let path = canonical_path(conn, path); + + // Eagerly populate content_hash so this tag follows the bytes, not the + // path (see CLAUDE.md "Multi-library data model"). None is fine: the + // reconciliation pass backfills once image_exif has a hash for this + // file. Library-agnostic by design: any matching image_exif row's hash + // is acceptable. + let content_hash: Option = { + use crate::database::schema::image_exif as ie; + ie::table + .filter(ie::rel_path.eq(&path)) + .filter(ie::content_hash.is_not_null()) + .select(ie::content_hash) + .first::>(conn) + .ok() + .flatten() + }; + + let inserted = diesel::insert_or_ignore_into(tagged_photo::table) + .values(InsertTaggedPhoto { + tag_id, + photo_name: path.clone(), + created_time: Utc::now().timestamp(), + content_hash, + }) + .execute(conn) + .with_context(|| format!("Unable to tag file {:?}", path))?; + if inserted > 0 { + info!("Inserted tagged photo: {:#} -> {:?}", tag_id, path); + } + Ok((path, inserted)) +} + +/// Delete tag `tag_id` from the photo at `path` the way reads see it: the +/// canonical path, every `tagged_photo` row on a path holding the same +/// content (`image_exif.content_hash`), and every row already keyed on that +/// hash. Library-agnostic, like the tag reads in `get_tags` and +/// `lookup_tags_batch`. Returns the number of rows deleted. +fn remove_tag_rows(conn: &mut SqliteConnection, path: &str, tag_id: i32) -> anyhow::Result { + let path = canonical_path(conn, path); + diesel::sql_query( + "DELETE FROM tagged_photo WHERE tag_id = ?1 AND ( + rel_path = ?2 + OR content_hash IN ( + SELECT e.content_hash FROM image_exif e + WHERE e.rel_path = ?2 AND e.content_hash IS NOT NULL) + OR rel_path IN ( + SELECT s.rel_path FROM image_exif s WHERE s.content_hash IN ( + SELECT e.content_hash FROM image_exif e + WHERE e.rel_path = ?2 AND e.content_hash IS NOT NULL)))", + ) + .bind::(tag_id) + .bind::(&path) + .execute(conn) + .with_context(|| format!("Unable to remove tag {} from {:?}", tag_id, path)) +} + pub struct SqliteTagDao { connection: Arc>, } @@ -864,6 +1061,17 @@ impl TagDao for SqliteTagDao { }) } + fn find_or_create_tag(&mut self, name: &str) -> anyhow::Result { + let mut conn = self + .connection + .lock() + .expect("Unable to lock SqliteTagDao connection"); + trace_db_call("insert", "find_or_create_tag", |span| { + span.set_attribute("name", name.to_string()); + find_or_create_tag_in(conn.deref_mut(), name) + }) + } + fn remove_tag(&mut self, tag_name: &str, path: &str) -> anyhow::Result> { let mut conn = self .connection @@ -873,27 +1081,16 @@ impl TagDao for SqliteTagDao { span.set_attribute("tag_name", tag_name.to_string()); span.set_attribute("path", path.to_string()); - let path = &canonical_path(conn.deref_mut(), path); - tags::table - .filter(tags::name.eq(tag_name)) - .get_result::(conn.deref_mut()) - .optional() - .with_context(|| format!("Unable to get tag '{}'", tag_name)) - .and_then(|tag| { - if let Some(tag) = tag { - diesel::delete( - tagged_photo::table - .filter(tagged_photo::tag_id.eq(tag.id)) - .filter(tagged_photo::rel_path.eq(path)), - ) - .execute(conn.deref_mut()) - .with_context(|| format!("Unable to delete tag: '{}'", &tag.name)) - .map(|_| Some(())) - } else { - info!("No tag found with name '{}'", tag_name); - Ok(None) - } - }) + let Some(tag) = find_tag_by_name(conn.deref_mut(), tag_name.trim())? else { + info!("No tag found with name '{}'", tag_name); + return Ok(None); + }; + let removed = remove_tag_rows(conn.deref_mut(), path, tag.id)?; + debug!( + "Removed {} rows of tag {:?} for {:?}", + removed, tag.name, path + ); + Ok(Some(())) }) } @@ -906,58 +1103,64 @@ impl TagDao for SqliteTagDao { span.set_attribute("path", path.to_string()); span.set_attribute("tag_id", tag_id.to_string()); - // Tagging a photo version tags its original (photo versions, - // #191): the tag binds to the original's bytes and path. - let path = &canonical_path(conn.deref_mut(), path); + let (path, _) = insert_tag_row(conn.deref_mut(), path, tag_id)?; + tagged_photo::table + .filter(tagged_photo::rel_path.eq(&path)) + .filter(tagged_photo::tag_id.eq(tag_id)) + .first(conn.deref_mut()) + .with_context(|| format!("Error reading tag {} on {:?}", tag_id, path)) + }) + } - // Eagerly populate content_hash so this tag follows the bytes, - // not the path (see CLAUDE.md "Multi-library data model"). - // None is fine — the reconciliation pass will backfill once - // image_exif has a hash for this file. We deliberately don't - // require library_id here: the tag handler is library- - // agnostic by design, and any matching image_exif row's hash - // is acceptable. If the path resolves to different bytes in - // different libraries, reconciliation per-library refines. - let content_hash: Option = { - use crate::database::schema::image_exif as ie; - ie::table - .filter(ie::rel_path.eq(path)) - .filter(ie::content_hash.is_not_null()) - .select(ie::content_hash) - .first::>(conn.deref_mut()) - .ok() - .flatten() - }; + fn bulk_tag( + &mut self, + paths: &[String], + add: &[String], + remove: &[i32], + ) -> anyhow::Result { + let mut conn = self + .connection + .lock() + .expect("Unable to lock SqliteTagDao connection"); + trace_db_call("update", "bulk_tag", |span| { + span.set_attribute("path_count", paths.len() as i64); + span.set_attribute("add_count", add.len() as i64); + span.set_attribute("remove_count", remove.len() as i64); - diesel::insert_into(tagged_photo::table) - .values(InsertTaggedPhoto { - tag_id, - photo_name: path.to_string(), - created_time: Utc::now().timestamp(), - content_hash, - }) - .execute(conn.deref_mut()) - .with_context(|| format!("Unable to tag file {:?} in sqlite", path)) - .and_then(|_| { - info!("Inserted tagged photo: {:#} -> {:?}", tag_id, path); - define_sql_function! { - fn last_insert_rowid() -> diesel::sql_types::Integer; + conn.immediate_transaction::(|conn| { + let mut tags = Vec::with_capacity(add.len()); + for name in add { + // A name already in `remove` must not be created first. + if let Some(tag) = find_tag_by_name(conn, name.trim())? + && remove.contains(&tag.id) + { + return Ok(BulkTagOutcome::Conflict { tag }); } - diesel::select(last_insert_rowid()) - .get_result::(conn.deref_mut()) - .with_context(|| "Unable to get last inserted tag from Sqlite") - }) - .and_then(|tagged_id| { - tagged_photo::table - .find(tagged_id) - .first(conn.deref_mut()) - .with_context(|| { - format!( - "Error getting inserted tagged photo with id: {:?}", - tagged_id - ) - }) + } + for name in add { + tags.push(find_or_create_tag_in(conn, name)?); + } + let (mut added, mut removed) = (0, 0); + for path in paths { + for tag_id in remove { + removed += remove_tag_rows(conn, path, *tag_id)?; + } + for tag in &tags { + added += insert_tag_row(conn, path, tag.id)?.1; + } + } + info!( + "Bulk tag: {} photos, +{} rows, -{} rows", + paths.len(), + added, + removed + ); + Ok(BulkTagOutcome::Applied { + added, + removed, + tags, }) + }) }) } @@ -1270,6 +1473,28 @@ mod tests { Ok(tag) } + fn find_or_create_tag(&mut self, name: &str) -> anyhow::Result { + let existing = self + .tags + .borrow() + .iter() + .find(|t| t.name.eq_ignore_ascii_case(name.trim())) + .cloned(); + match existing { + Some(tag) => Ok(tag), + None => self.create_tag(name.trim()), + } + } + + fn bulk_tag( + &mut self, + _paths: &[String], + _add: &[String], + _remove: &[i32], + ) -> anyhow::Result { + unimplemented!("bulk_tag is tested against SQLite") + } + fn remove_tag(&mut self, tag_name: &str, path: &str) -> anyhow::Result> { let mut clone = { let photo_tags = &self.tagged_photos.borrow()[path]; @@ -1666,3 +1891,318 @@ pub struct FileWithTagCount { #[diesel(sql_type = BigInt)] pub(crate) tag_count: i64, } + +/// Tag writes against a real SQLite schema (`UNIQUE(rel_path, tag_id)`, +/// `idx_tags_name_nocase`, content-hash siblings), which the `TestTagDao` +/// mock can't model. +#[cfg(test)] +mod sqlite_tests { + use super::*; + use crate::database::test::in_memory_db_connection; + use actix_web::body::MessageBody; + use actix_web::http::StatusCode; + + struct Fx { + conn: Arc>, + data: web::Data>, + } + + impl Fx { + fn new() -> Self { + let conn = Arc::new(Mutex::new(in_memory_db_connection())); + let data = web::Data::new(Mutex::new(SqliteTagDao::new(conn.clone()))); + Fx { conn, data } + } + fn exec(&self, sql: &str) { + diesel::sql_query(sql) + .execute(self.conn.lock().unwrap().deref_mut()) + .unwrap(); + } + fn count(&self, sql: &str) -> i64 { + #[derive(QueryableByName)] + struct N { + #[diesel(sql_type = BigInt)] + n: i64, + } + diesel::sql_query(sql) + .get_result::(self.conn.lock().unwrap().deref_mut()) + .unwrap() + .n + } + /// An indexed file in `library` holding bytes `hash`. + fn photo(&self, library: i32, path: &str, hash: &str) { + self.exec(&format!( + "INSERT INTO image_exif (library_id, rel_path, created_time, last_modified, content_hash) \ + VALUES ({library}, '{path}', 0, 0, '{hash}')" + )); + } + fn tag_names(&self, path: &str) -> Vec { + let mut names: Vec = self + .data + .lock() + .unwrap() + .get_tags_for_path(path) + .unwrap() + .into_iter() + .map(|t| t.name) + .collect(); + names.sort(); + names + } + async fn add(&self, file: &str, tag: &str) -> StatusCode { + let body = AddTagRequest { + file_name: file.into(), + tag_name: tag.into(), + }; + status(add_tag(claims(), web::Json(body), self.data.clone()).await) + } + async fn remove(&self, file: &str, tag: &str) -> StatusCode { + let body = AddTagRequest { + file_name: file.into(), + tag_name: tag.into(), + }; + status(remove_tagged_photo(claims(), web::Json(body), self.data.clone()).await) + } + async fn bulk( + &self, + files: &[&str], + add: &[&str], + remove: &[i32], + ) -> (StatusCode, serde_json::Value) { + let body = BulkTagRequest { + file_names: files.iter().map(|f| f.to_string()).collect(), + add: add.iter().map(|a| a.to_string()).collect(), + remove: remove.to_vec(), + }; + let resp = bulk_tag(claims(), web::Json(body), self.data.clone()) + .await + .respond_to(&actix_web::test::TestRequest::default().to_http_request()); + let status = resp.status(); + let bytes = resp.into_body().try_into_bytes().unwrap_or_default(); + (status, serde_json::from_slice(&bytes).unwrap_or_default()) + } + fn tag_id(&self, name: &str) -> i32 { + self.data + .lock() + .unwrap() + .find_or_create_tag(name) + .unwrap() + .id + } + } + + fn claims() -> Claims { + Claims::valid_user("1".into()) + } + + fn status(r: impl Responder) -> StatusCode { + r.respond_to(&actix_web::test::TestRequest::default().to_http_request()) + .status() + } + + #[actix_rt::test] + async fn adding_a_tag_the_photo_already_has_is_a_no_op() { + let fx = Fx::new(); + assert_eq!(fx.add("a.jpg", "beach").await, StatusCode::OK); + assert_eq!(fx.add("a.jpg", "beach").await, StatusCode::OK); + assert_eq!(fx.add("a.jpg", "Beach").await, StatusCode::OK); + assert_eq!(fx.count("SELECT COUNT(*) AS n FROM tagged_photo"), 1); + assert_eq!(fx.count("SELECT COUNT(*) AS n FROM tags"), 1); + } + + #[actix_rt::test] + async fn a_tag_no_photo_carries_can_be_added_again() { + let fx = Fx::new(); + fx.add("a.jpg", "beach").await; + assert_eq!(fx.remove("a.jpg", "beach").await, StatusCode::OK); + assert_eq!(fx.add("b.jpg", "BEACH").await, StatusCode::OK); + assert_eq!( + fx.count("SELECT COUNT(*) AS n FROM tags"), + 1, + "reused, not duplicated" + ); + assert_eq!(fx.tag_names("b.jpg"), vec!["beach"]); + } + + #[actix_rt::test] + async fn an_empty_tag_name_is_rejected() { + let fx = Fx::new(); + assert_eq!(fx.add("a.jpg", " ").await, StatusCode::BAD_REQUEST); + assert_eq!(fx.count("SELECT COUNT(*) AS n FROM tags"), 0); + } + + #[actix_rt::test] + async fn removal_ignores_case_and_reaches_every_copy_of_the_photo() { + let fx = Fx::new(); + fx.photo(1, "2020/a.jpg", "h-a"); + fx.photo(2, "archive/2020/a.jpg", "h-a"); + fx.photo(1, "2020/b.jpg", "h-b"); + // The tag was written while viewing the archive copy, and once more on + // a path that no longer exists but kept the hash. + fx.add("archive/2020/a.jpg", "beach").await; + let id = fx.tag_id("beach"); + fx.exec(&format!( + "INSERT INTO tagged_photo (rel_path, tag_id, created_time, content_hash) \ + VALUES ('old/a.jpg', {id}, 0, 'h-a')" + )); + fx.add("2020/b.jpg", "beach").await; + + assert_eq!(fx.remove("2020/a.jpg", "BEACH").await, StatusCode::OK); + + assert_eq!( + fx.count(&format!( + "SELECT COUNT(*) AS n FROM tagged_photo WHERE tag_id = {id}" + )), + 1, + "only the other photo keeps it" + ); + assert_eq!(fx.tag_names("2020/b.jpg"), vec!["beach"]); + } + + #[actix_rt::test] + async fn removing_an_unknown_tag_is_404() { + let fx = Fx::new(); + assert_eq!(fx.remove("a.jpg", "nope").await, StatusCode::NOT_FOUND); + } + + #[actix_rt::test] + async fn replacing_a_tag_set_can_add_a_tag_no_photo_carries() { + let fx = Fx::new(); + let unused = fx.tag_id("unused"); + fx.add("a.jpg", "beach").await; + let body = AddTagsRequest { + file_name: "a.jpg".into(), + tag_ids: vec![unused], + }; + assert_eq!( + status(update_tags(claims(), fx.data.clone(), web::Json(body)).await), + StatusCode::OK + ); + assert_eq!(fx.tag_names("a.jpg"), vec!["unused"]); + } + + #[actix_rt::test] + async fn a_failing_tag_set_replace_answers_500_and_tagging_keeps_working() { + let fx = Fx::new(); + fx.exec( + "CREATE TRIGGER boom BEFORE INSERT ON tagged_photo WHEN NEW.rel_path = 'boom.jpg' \ + BEGIN SELECT RAISE(ABORT, 'boom'); END", + ); + let id = fx.tag_id("beach"); + let body = AddTagsRequest { + file_name: "boom.jpg".into(), + tag_ids: vec![id], + }; + assert_eq!( + status(update_tags(claims(), fx.data.clone(), web::Json(body)).await), + StatusCode::INTERNAL_SERVER_ERROR + ); + assert!(!fx.data.is_poisoned(), "the TagDao lock must survive"); + assert_eq!(fx.add("a.jpg", "beach").await, StatusCode::OK); + } + + #[actix_rt::test] + async fn bulk_adds_existing_and_new_tags_to_every_photo() { + let fx = Fx::new(); + fx.add("a.jpg", "beach").await; + let (st, body) = fx + .bulk( + &["a.jpg", "x/b.jpg", "x\\b.jpg"], + &["Beach", "Road trip", "road TRIP"], + &[], + ) + .await; + assert_eq!(st, StatusCode::OK); + // a.jpg already had beach: 3 new rows, not 4. + assert_eq!(body["added"], 3); + assert_eq!(body["removed"], 0); + let names: Vec<&str> = body["tags"] + .as_array() + .unwrap() + .iter() + .map(|t| t["name"].as_str().unwrap()) + .collect(); + assert_eq!(names, vec!["beach", "Road trip"]); + assert_eq!(fx.tag_names("a.jpg"), vec!["Road trip", "beach"]); + assert_eq!(fx.tag_names("x/b.jpg"), vec!["Road trip", "beach"]); + } + + #[actix_rt::test] + async fn bulk_removes_from_every_copy() { + let fx = Fx::new(); + fx.photo(1, "a.jpg", "h-a"); + fx.photo(2, "archive/a.jpg", "h-a"); + fx.add("archive/a.jpg", "beach").await; + fx.add("b.jpg", "beach").await; + fx.add("c.jpg", "beach").await; + let id = fx.tag_id("beach"); + + let (st, body) = fx.bulk(&["a.jpg", "b.jpg"], &[], &[id]).await; + assert_eq!(st, StatusCode::OK); + assert_eq!(body["removed"], 2); + assert!(fx.tag_names("archive/a.jpg").is_empty()); + assert!(fx.tag_names("b.jpg").is_empty()); + assert_eq!( + fx.tag_names("c.jpg"), + vec!["beach"], + "unselected photos keep it" + ); + } + + #[actix_rt::test] + async fn bulk_writes_nothing_when_any_row_fails() { + let fx = Fx::new(); + fx.exec( + "CREATE TRIGGER boom BEFORE INSERT ON tagged_photo WHEN NEW.rel_path = 'boom.jpg' \ + BEGIN SELECT RAISE(ABORT, 'boom'); END", + ); + let (st, _) = fx.bulk(&["a.jpg", "boom.jpg"], &["brand new"], &[]).await; + assert_eq!(st, StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!(fx.count("SELECT COUNT(*) AS n FROM tagged_photo"), 0); + assert_eq!( + fx.count("SELECT COUNT(*) AS n FROM tags"), + 0, + "the new tag rolled back too" + ); + assert!(!fx.data.is_poisoned()); + } + + #[actix_rt::test] + async fn bulk_rejects_a_tag_both_added_and_removed() { + let fx = Fx::new(); + 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!( + fx.count("SELECT COUNT(*) AS n FROM tags"), + 1, + "'new' not created" + ); + } + + #[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 + ); + } +}