tags: add edit + delete endpoints, enable FK enforcement

PUT /image/tags/{id} renames a tag globally; DELETE /image/tags/{id}
removes a tag and every photo's reference. Rename returns 200/404/409
(case-insensitive name conflict) / 400 (empty name); delete returns
204/404. New migration adds a UNIQUE COLLATE NOCASE index on
tags.name with a pre-flight pass that collapses existing case-
insensitive duplicates onto the lowest id.

The connection setup now sets PRAGMA foreign_keys = ON. The schema
already declares ON DELETE CASCADE / SET NULL on several tables —
those clauses were documentation-only because SQLite has FK
enforcement off per-connection by default. Audited every
diesel::delete site; each touches either no inbound FKs or has a
matching policy. delete_tag relies on the tagged_photo cascade
instead of doing manual cleanup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

migrations/2026-04-30-000000_unique_tag_name/down.sql

@@ -0,0 +1 @@
+DROP INDEX IF EXISTS idx_tags_name_nocase;

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);

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

src/tags.rs

@@ -33,6 +33,11 @@ where
     .service(web::resource("image/tags/all").route(web::get().to(get_all_tags::<TagD>)))
     .service(web::resource("image/tags/batch").route(web::post().to(update_tags::<TagD>)))
     .service(web::resource("image/tags/lookup").route(web::post().to(lookup_tags_batch::<TagD>)))
+    .service(
+        web::resource("image/tags/{id}")
+            .route(web::put().to(update_tag::<TagD>))
+            .route(web::delete().to(delete_tag::<TagD>)),
+    )
 }
 
 async fn add_tag<D: TagDao>(
@@ -53,7 +58,14 @@ async fn add_tag<D: TagDao>(
     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<D: TagDao>(
         .into_http_internal_err()
 }
 
+async fn update_tag<D: TagDao>(
+    _: Claims,
+    http_request: HttpRequest,
+    path: web::Path<i32>,
+    body: web::Json<UpdateTagRequest>,
+    tag_dao: web::Data<Mutex<D>>,
+) -> 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<D: TagDao>(
+    _: Claims,
+    http_request: HttpRequest,
+    path: web::Path<i32>,
+    tag_dao: web::Data<Mutex<D>>,
+) -> 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<D: TagDao>(
     _: Claims,
     http_request: HttpRequest,
@@ -442,6 +522,22 @@ pub struct AddTagsRequest {
     pub tag_ids: Vec<i32>,
 }
 
+#[derive(Debug, Deserialize)]
+pub struct UpdateTagRequest {
+    pub name: String,
+}
+
+/// Result of an attempted tag rename. Returning a typed outcome (rather
+/// than `anyhow::Result<Tag>`) 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<std::collections::HashMap<String, i64>>;
+    /// 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<UpdateTagOutcome>;
+    /// 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<bool>;
 }
 
 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<UpdateTagOutcome> {
+        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::<Tag>(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::<Tag>(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<bool> {
+        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<UpdateTagOutcome> {
+            // 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<bool> {
+            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<Mutex<TestTagDao>>,
+        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<Mutex<TestTagDao>>,
+        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 {