diff --git a/.gitignore b/.gitignore index 112d1e3..2bd4d6e 100644 --- a/.gitignore +++ b/.gitignore @@ -2,8 +2,12 @@ database/target *.db *.db.bak +*.db-shm +*.db-wal .env /tmp +/docs +/specs # Default ignored files .idea/shelf/ diff --git a/migrations/2026-04-30-000000_unique_tag_name/down.sql b/migrations/2026-04-30-000000_unique_tag_name/down.sql new file mode 100644 index 0000000..da8c915 --- /dev/null +++ b/migrations/2026-04-30-000000_unique_tag_name/down.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS idx_tags_name_nocase; diff --git a/migrations/2026-04-30-000000_unique_tag_name/up.sql b/migrations/2026-04-30-000000_unique_tag_name/up.sql new file mode 100644 index 0000000..3f7ee9b --- /dev/null +++ b/migrations/2026-04-30-000000_unique_tag_name/up.sql @@ -0,0 +1,28 @@ +-- Tags only enforced uniqueness in application code (the add_tag handler +-- looks up by name before inserting). The schema itself accepted dupes, +-- so a divergent code path could land two tags with the same name. Now +-- that we expose a rename endpoint we want a hard guarantee: case- +-- insensitive UNIQUE on tags.name. + +-- Pre-flight: collapse exact-name duplicates (case-insensitive) onto the +-- lowest-id row before adding the constraint, otherwise the index +-- creation fails on any DB that ever produced dupes. On a clean DB this +-- is a no-op. +UPDATE tagged_photo +SET tag_id = ( + SELECT MIN(t2.id) FROM tags t2 + WHERE LOWER(t2.name) = LOWER((SELECT name FROM tags WHERE id = tagged_photo.tag_id)) +) +WHERE tag_id IN ( + SELECT t.id FROM tags t + WHERE t.id <> ( + SELECT MIN(t2.id) FROM tags t2 WHERE LOWER(t2.name) = LOWER(t.name) + ) +); + +DELETE FROM tags +WHERE id <> ( + SELECT MIN(t2.id) FROM tags t2 WHERE LOWER(t2.name) = LOWER(tags.name) +); + +CREATE UNIQUE INDEX idx_tags_name_nocase ON tags (name COLLATE NOCASE); diff --git a/src/database/mod.rs b/src/database/mod.rs index 0ab74a1..15fec32 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -136,10 +136,19 @@ pub fn connect() -> SqliteConnection { // rollback-journal durability; we accept the narrow last-fsync // window for the 2–10× write throughput). use diesel::connection::SimpleConnection; + // foreign_keys = ON is per-connection in SQLite (off by default), so + // it has to be set here alongside the other pragmas. Without it + // every `REFERENCES … ON DELETE CASCADE / SET NULL` clause in the + // schema is documentation-only — orphan rows would survive the + // referenced row's deletion. With it, the cascade fires + // automatically and code that previously did manual two-step + // cleanup (delete child rows, then parent) becomes redundant but + // still correct. conn.batch_execute( "PRAGMA journal_mode = WAL; \ PRAGMA busy_timeout = 5000; \ - PRAGMA synchronous = NORMAL;", + PRAGMA synchronous = NORMAL; \ + PRAGMA foreign_keys = ON;", ) .expect("set sqlite pragmas"); conn diff --git a/src/faces.rs b/src/faces.rs index 0c8ea16..35c8995 100644 --- a/src/faces.rs +++ b/src/faces.rs @@ -2311,6 +2311,12 @@ async fn update_face_handler( let mut new_embedding: Option> = None; if let Some((bx, by, bw, bh)) = bbox_patch { if !face_client.is_enabled() { + warn!( + "PATCH /image/faces/{}: 503 — face client not enabled \ + (APOLLO_FACE_API_BASE_URL / APOLLO_API_BASE_URL both unset). \ + Bbox edit requires Apollo to re-embed.", + id + ); return HttpResponse::ServiceUnavailable() .body("face client disabled — bbox edit requires Apollo"); } @@ -2387,9 +2393,19 @@ async fn update_face_handler( ); } Err(FaceDetectError::Transient(e)) => { + warn!( + "PATCH /image/faces/{}: 503 — Apollo face client transient \ + error during re-embed: {}", + id, e + ); return HttpResponse::ServiceUnavailable().body(format!("{}", e)); } Err(FaceDetectError::Disabled) => { + warn!( + "PATCH /image/faces/{}: 503 — face client became disabled \ + mid-flight", + id + ); return HttpResponse::ServiceUnavailable().body("face client disabled mid-flight"); } } diff --git a/src/tags.rs b/src/tags.rs index 150b3a3..eb3c01c 100644 --- a/src/tags.rs +++ b/src/tags.rs @@ -33,6 +33,11 @@ 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/{id}") + .route(web::put().to(update_tag::)) + .route(web::delete().to(delete_tag::)), + ) } async fn add_tag( @@ -53,7 +58,14 @@ async fn add_tag( tag_dao .get_all_tags(&span_context, None) .and_then(|tags| { - if let Some((_, tag)) = tags.iter().find(|t| t.1.name == tag_name) { + // 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!( @@ -71,6 +83,74 @@ async fn add_tag( .into_http_internal_err() } +async fn update_tag( + _: Claims, + http_request: HttpRequest, + path: web::Path, + body: web::Json, + tag_dao: web::Data>, +) -> impl Responder { + let tracer = global_tracer(); + let context = extract_context_from_request(&http_request); + let span = tracer.start_with_context("update_tag", &context); + let span_context = opentelemetry::Context::current_with_span(span); + + 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" })); + } + + let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao"); + match tag_dao.update_tag_name(&span_context, id, trimmed) { + Ok(UpdateTagOutcome::Renamed(tag)) => { + span_context.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" })) + } + } +} + +async fn delete_tag( + _: Claims, + http_request: HttpRequest, + path: web::Path, + tag_dao: web::Data>, +) -> impl Responder { + let tracer = global_tracer(); + let context = extract_context_from_request(&http_request); + let span = tracer.start_with_context("delete_tag", &context); + let span_context = opentelemetry::Context::current_with_span(span); + + let id = path.into_inner(); + let mut tag_dao = tag_dao.lock().expect("Unable to get TagDao"); + match tag_dao.delete_tag(&span_context, id) { + Ok(true) => { + span_context.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" })) + } + } +} + async fn get_tags( _: Claims, http_request: HttpRequest, @@ -442,6 +522,22 @@ pub struct AddTagsRequest { pub tag_ids: Vec, } +#[derive(Debug, Deserialize)] +pub struct UpdateTagRequest { + pub name: String, +} + +/// Result of an attempted tag rename. Returning a typed outcome (rather +/// than `anyhow::Result`) lets the handler map each case to a +/// distinct HTTP status without sniffing error strings, and keeps the +/// 409 path a normal control-flow result instead of a DB constraint +/// violation surfacing as a generic 500. +pub enum UpdateTagOutcome { + Renamed(Tag), + NotFound, + Conflict { existing: Tag }, +} + pub trait TagDao: Send + Sync { fn get_all_tags( &mut self, @@ -511,6 +607,26 @@ pub trait TagDao: Send + Sync { context: &opentelemetry::Context, file_paths: &[String], ) -> anyhow::Result>; + /// Rename a tag in place. The tag id stays stable so existing + /// `tagged_photo` rows automatically reflect the new name without + /// a join-table rewrite. Conflict is resolved against the rest of + /// the table case-insensitively (mirroring the + /// `idx_tags_name_nocase` UNIQUE index) — a rename that changes + /// only the case of the tag's own current name is allowed. + fn update_tag_name( + &mut self, + context: &opentelemetry::Context, + id: i32, + new_name: &str, + ) -> anyhow::Result; + /// Globally remove a tag and every `tagged_photo` row that + /// references it. Returns `true` if a tag was deleted, `false` if + /// no row matched the id. The schema's FK is `ON DELETE CASCADE` + /// but SQLite only honors that with `PRAGMA foreign_keys = ON`, + /// which this project doesn't set — the impl deletes both tables + /// explicitly in a single transaction so partial state is + /// impossible. + fn delete_tag(&mut self, context: &opentelemetry::Context, id: i32) -> anyhow::Result; } pub struct SqliteTagDao { @@ -704,6 +820,83 @@ impl TagDao for SqliteTagDao { }) } + fn update_tag_name( + &mut self, + context: &opentelemetry::Context, + id: i32, + new_name: &str, + ) -> anyhow::Result { + let mut conn = self + .connection + .lock() + .expect("Unable to lock SqliteTagDao connection"); + trace_db_call(context, "update", "update_tag_name", |span| { + span.set_attributes(vec![ + KeyValue::new("tag_id", id as i64), + KeyValue::new("new_name", new_name.to_string()), + ]); + + let target = tags::table + .filter(tags::id.eq(id)) + .select((tags::id, tags::name, tags::created_time)) + .get_result::(conn.deref_mut()) + .optional() + .with_context(|| format!("Unable to look up tag id {}", id))?; + let target = match target { + Some(t) => t, + None => return Ok(UpdateTagOutcome::NotFound), + }; + + // Case-insensitive collision check on every other row. + // Belt-and-suspenders: idx_tags_name_nocase enforces this at + // the index level, but checking up front gives the handler + // a clean 409 with the existing tag's id instead of a + // generic constraint-violation 500. Tags table is small; + // loading peers and comparing in Rust avoids a fragile + // dsl::sql composition for case-insensitive equality. + let conflict = tags::table + .filter(tags::id.ne(id)) + .select((tags::id, tags::name, tags::created_time)) + .get_results::(conn.deref_mut()) + .with_context(|| "Unable to query for tag-name conflict")? + .into_iter() + .find(|t| t.name.eq_ignore_ascii_case(new_name)); + if let Some(existing) = conflict { + return Ok(UpdateTagOutcome::Conflict { existing }); + } + + diesel::update(tags::table.filter(tags::id.eq(id))) + .set(tags::name.eq(new_name)) + .execute(conn.deref_mut()) + .with_context(|| format!("Unable to rename tag {}", id))?; + + Ok(UpdateTagOutcome::Renamed(Tag { + id: target.id, + name: new_name.to_string(), + created_time: target.created_time, + })) + }) + } + + fn delete_tag(&mut self, context: &opentelemetry::Context, id: i32) -> anyhow::Result { + let mut conn = self + .connection + .lock() + .expect("Unable to lock SqliteTagDao connection"); + trace_db_call(context, "delete", "delete_tag", |span| { + span.set_attribute(KeyValue::new("tag_id", id as i64)); + + // tagged_photo.tag_id is `ON DELETE CASCADE` and the + // connection now sets `PRAGMA foreign_keys = ON`, so a + // single DELETE on tags removes its tagged_photo rows + // atomically. + let removed = diesel::delete(tags::table.filter(tags::id.eq(id))) + .execute(conn.deref_mut()) + .with_context(|| format!("Unable to delete tag {}", id))?; + Ok(removed > 0) + }) + } + fn remove_tag( &mut self, context: &opentelemetry::Context, @@ -1238,6 +1431,54 @@ mod tests { } Ok(counts) } + + fn update_tag_name( + &mut self, + _context: &opentelemetry::Context, + id: i32, + new_name: &str, + ) -> anyhow::Result { + // Conflict pass first so the target tag's own old name + // doesn't collide with itself. + let conflict = self + .tags + .borrow() + .iter() + .find(|t| t.id != id && t.name.eq_ignore_ascii_case(new_name)) + .cloned(); + if let Some(existing) = conflict { + return Ok(UpdateTagOutcome::Conflict { existing }); + } + let mut tags = self.tags.borrow_mut(); + match tags.iter_mut().find(|t| t.id == id) { + Some(t) => { + t.name = new_name.to_string(); + Ok(UpdateTagOutcome::Renamed(t.clone())) + } + None => Ok(UpdateTagOutcome::NotFound), + } + } + + fn delete_tag( + &mut self, + _context: &opentelemetry::Context, + id: i32, + ) -> anyhow::Result { + let target_name = { + let tags = self.tags.borrow(); + tags.iter().find(|t| t.id == id).map(|t| t.name.clone()) + }; + let Some(name) = target_name else { + return Ok(false); + }; + // Mirror the cascade: drop any tagged_photo references, then + // remove the tag itself. + for (_path, tags) in self.tagged_photos.borrow_mut().iter_mut() { + tags.retain(|t| t.id != id && t.name != name); + } + self.tags.borrow_mut().retain(|t| t.id != id); + Ok(true) + } } #[actix_rt::test] @@ -1390,6 +1631,177 @@ mod tests { None ); } + + async fn rename_tag( + dao: &Data>, + id: i32, + new_name: &str, + ) -> actix_web::http::StatusCode { + use actix_web::Responder; + let req = TestRequest::default().to_http_request(); + let body = web::Json(UpdateTagRequest { + name: new_name.to_string(), + }); + let claims = Claims::valid_user(String::from("1")); + let resp = update_tag(claims, req.clone(), web::Path::from(id), body, dao.clone()).await; + resp.respond_to(&req).status() + } + + #[actix_rt::test] + async fn update_tag_renames_successfully() { + let mut dao = TestTagDao::new(); + let tag = dao + .create_tag(&opentelemetry::Context::current(), "old") + .unwrap(); + let dao = Data::new(Mutex::new(dao)); + + assert_eq!( + rename_tag(&dao, tag.id, "new").await, + actix_web::http::StatusCode::OK + ); + + let mut locked = dao.lock().unwrap(); + let all = locked + .get_all_tags(&opentelemetry::Context::current(), None) + .unwrap(); + assert_eq!(all.len(), 1); + assert_eq!(all[0].1.name, "new"); + } + + #[actix_rt::test] + async fn update_tag_not_found_returns_404() { + let dao = Data::new(Mutex::new(TestTagDao::new())); + assert_eq!( + rename_tag(&dao, 99999, "nope").await, + actix_web::http::StatusCode::NOT_FOUND + ); + } + + #[actix_rt::test] + async fn update_tag_empty_name_returns_400() { + let mut dao = TestTagDao::new(); + let tag = dao + .create_tag(&opentelemetry::Context::current(), "keep") + .unwrap(); + let dao = Data::new(Mutex::new(dao)); + + assert_eq!( + rename_tag(&dao, tag.id, " ").await, + actix_web::http::StatusCode::BAD_REQUEST + ); + + let mut locked = dao.lock().unwrap(); + let all = locked + .get_all_tags(&opentelemetry::Context::current(), None) + .unwrap(); + assert_eq!(all[0].1.name, "keep", "name must not change on 400"); + } + + #[actix_rt::test] + async fn update_tag_conflict_returns_409() { + let mut dao = TestTagDao::new(); + let _a = dao + .create_tag(&opentelemetry::Context::current(), "a") + .unwrap(); + let b = dao + .create_tag(&opentelemetry::Context::current(), "b") + .unwrap(); + let dao = Data::new(Mutex::new(dao)); + + // Case-insensitive collision: renaming b -> "A" must conflict with a. + assert_eq!( + rename_tag(&dao, b.id, "A").await, + actix_web::http::StatusCode::CONFLICT + ); + + let mut locked = dao.lock().unwrap(); + let all = locked + .get_all_tags(&opentelemetry::Context::current(), None) + .unwrap(); + let b_after = all.iter().find(|(_, t)| t.id == b.id).unwrap(); + assert_eq!(b_after.1.name, "b", "no DB change on 409"); + } + + async fn delete_via_handler( + dao: &Data>, + id: i32, + ) -> actix_web::http::StatusCode { + use actix_web::Responder; + let req = TestRequest::default().to_http_request(); + let claims = Claims::valid_user(String::from("1")); + let resp = delete_tag(claims, req.clone(), web::Path::from(id), dao.clone()).await; + resp.respond_to(&req).status() + } + + #[actix_rt::test] + async fn delete_tag_removes_tag_and_cascades_tagged_photos() { + let mut dao = TestTagDao::new(); + let tag = dao + .create_tag(&opentelemetry::Context::current(), "doomed") + .unwrap(); + dao.tag_file(&opentelemetry::Context::current(), "a.jpg", tag.id) + .unwrap(); + dao.tag_file(&opentelemetry::Context::current(), "b.jpg", tag.id) + .unwrap(); + let dao = Data::new(Mutex::new(dao)); + + assert_eq!( + delete_via_handler(&dao, tag.id).await, + actix_web::http::StatusCode::NO_CONTENT + ); + + let mut locked = dao.lock().unwrap(); + assert!( + locked + .get_all_tags(&opentelemetry::Context::current(), None) + .unwrap() + .is_empty() + ); + assert!( + locked + .get_tags_for_path(&opentelemetry::Context::current(), "a.jpg") + .unwrap() + .is_empty(), + "tagged_photo references must be cleaned up by the cascade" + ); + assert!( + locked + .get_tags_for_path(&opentelemetry::Context::current(), "b.jpg") + .unwrap() + .is_empty() + ); + } + + #[actix_rt::test] + async fn delete_tag_unknown_id_returns_404() { + let dao = Data::new(Mutex::new(TestTagDao::new())); + assert_eq!( + delete_via_handler(&dao, 99999).await, + actix_web::http::StatusCode::NOT_FOUND + ); + } + + #[actix_rt::test] + async fn update_tag_case_only_change_succeeds() { + let mut dao = TestTagDao::new(); + let tag = dao + .create_tag(&opentelemetry::Context::current(), "vacation") + .unwrap(); + let dao = Data::new(Mutex::new(dao)); + + // The conflict check excludes the target's own row, so changing + // only the case of the tag's current name must succeed. + assert_eq!( + rename_tag(&dao, tag.id, "Vacation").await, + actix_web::http::StatusCode::OK + ); + + let mut locked = dao.lock().unwrap(); + let all = locked + .get_all_tags(&opentelemetry::Context::current(), None) + .unwrap(); + assert_eq!(all[0].1.name, "Vacation"); + } } #[derive(QueryableByName, Debug, Clone)] pub struct FileWithTagCount {