From 108bbeb0292a0dce7ffe92afc5ac373d3fff938f Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sat, 9 May 2026 21:21:25 -0400 Subject: [PATCH 1/2] date-override: union semantics across libraries + slash forms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The date-override path used to look up `image_exif` strictly by `(library_id, rel_path)` with only the forward-slash form, while `/image/metadata`'s `get_exif` falls back across libraries and tries both slash forms. A photo whose row sat under a different library_id than its filesystem-resolved one — or whose rel_path was stored with backslashes — rendered fine in the modal but 404'd on save. `set_manual_date_taken` / `clear_manual_date_taken` now share a `locate_image_exif_row` helper that mirrors `get_exif`'s union semantics (scoped lookup first, library-agnostic fallback by rel_path in both slash forms), then update by primary key so the write hits exactly the row read. Inner anyhow errors are logged with `(library_id, rel_path)` so the next failure mode is debuggable. Handler-side: `resolve_library_param` errors no longer silently fall back to the primary library (which would have masked the original bug with a different "row not found"); a malformed library param now returns 400. New `DbErrorKind::NotFound` lets the handler distinguish genuine misses (404) from real DB failures (500). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/database/mod.rs | 159 ++++++++++++++++++++++++++++++++------------ src/main.rs | 36 ++++++---- 2 files changed, 139 insertions(+), 56 deletions(-) diff --git a/src/database/mod.rs b/src/database/mod.rs index b275988..47d5871 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -210,6 +210,7 @@ pub enum DbErrorKind { InsertError, QueryError, UpdateError, + NotFound, } pub trait FavoriteDao: Sync + Send { @@ -713,6 +714,66 @@ pub trait ExifDao: Sync + Send { ) -> Result, DbError>; } +/// Locate an `image_exif` row for a `(library_id, rel_path)` mutation, +/// mirroring `get_exif`'s union semantics so the write side is no +/// stricter than the read side. Tries the requested library first +/// (forward-slash and Windows backslash forms), then falls back to any +/// library with a matching `rel_path`. The latter handles two cases +/// `/image/metadata` already absorbs but the date-override path used to +/// 404 on: +/// 1. The metadata endpoint resolves `library_id` from the filesystem +/// (where the file currently lives), but the only `image_exif` row +/// sits under a different library_id (older mount, multi-library +/// duplicate). The mutation by `(library_id, rel_path)` would miss. +/// 2. A row imported from Windows still has backslash separators. +fn locate_image_exif_row( + connection: &mut SqliteConnection, + library_id_val: i32, + rel_path_val: &str, +) -> anyhow::Result { + use schema::image_exif::dsl::*; + + let normalized = rel_path_val.replace('\\', "/"); + let windows_form = rel_path_val.replace('/', "\\"); + + let scoped = image_exif + .filter(library_id.eq(library_id_val)) + .filter(rel_path.eq(&normalized).or(rel_path.eq(&windows_form))) + .first::(connection); + + match scoped { + Ok(row) => Ok(row), + Err(diesel::result::Error::NotFound) => { + // Fall back to any library with this rel_path. `image_exif` + // can carry the same path under multiple library_ids; the + // metadata endpoint already resolves loosely (see `get_exif`), + // and the date-override write must agree. + image_exif + .filter(rel_path.eq(&normalized).or(rel_path.eq(&windows_form))) + .first::(connection) + .map_err(|e| match e { + diesel::result::Error::NotFound => { + anyhow::Error::msg("row_not_found") + } + other => anyhow::Error::from(other).context("lookup_failed"), + }) + } + Err(other) => Err(anyhow::Error::from(other).context("lookup_failed")), + } +} + +/// Map a `locate_image_exif_row` / mutation closure error onto the +/// appropriate `DbErrorKind`. The `row_not_found` sentinel from +/// `locate_image_exif_row` becomes `NotFound` so the handler can return +/// 404; everything else is a real DB failure (`UpdateError` → 500). +fn classify_image_exif_error(e: &anyhow::Error) -> DbErrorKind { + if e.chain().any(|c| c.to_string() == "row_not_found") { + DbErrorKind::NotFound + } else { + DbErrorKind::UpdateError + } +} + pub struct SqliteExifDao { connection: Arc>, } @@ -1261,11 +1322,11 @@ impl ExifDao for SqliteExifDao { // Read-modify-write under the dao mutex so the snapshot is // consistent with the value being overwritten. The mutex holds // for the duration of this closure — no other writer can race. - let current: ImageExif = image_exif - .filter(library_id.eq(library_id_val)) - .filter(rel_path.eq(rel_path_val)) - .first(connection.deref_mut()) - .map_err(|_| anyhow::anyhow!("row not found"))?; + let current = locate_image_exif_row( + connection.deref_mut(), + library_id_val, + rel_path_val, + )?; // Snapshot only on first override. Subsequent overrides keep // the original snapshot intact so a single revert restores @@ -1279,27 +1340,33 @@ impl ExifDao for SqliteExifDao { ) }; - diesel::update( - image_exif - .filter(library_id.eq(library_id_val)) - .filter(rel_path.eq(rel_path_val)), - ) - .set(( - date_taken.eq(Some(date_taken_val)), - date_taken_source.eq(Some("manual".to_string())), - original_date_taken.eq(orig_dt), - original_date_taken_source.eq(orig_src), - )) - .execute(connection.deref_mut()) - .map_err(|_| anyhow::anyhow!("Update error"))?; + // Update by primary key so we hit the exact row we read, + // even when the lookup fell back across libraries / slash forms. + let row_id = current.id; + diesel::update(image_exif.find(row_id)) + .set(( + date_taken.eq(Some(date_taken_val)), + date_taken_source.eq(Some("manual".to_string())), + original_date_taken.eq(orig_dt), + original_date_taken_source.eq(orig_src), + )) + .execute(connection.deref_mut()) + .map_err(|e| anyhow::Error::from(e).context("update_failed"))?; image_exif - .filter(library_id.eq(library_id_val)) - .filter(rel_path.eq(rel_path_val)) + .find(row_id) .first::(connection.deref_mut()) - .map_err(|_| anyhow::anyhow!("Re-read error")) + .map_err(|e| anyhow::Error::from(e).context("reread_failed")) + }) + .map_err(|e| { + log::warn!( + "set_manual_date_taken(library_id={}, rel_path={:?}): {:#}", + library_id_val, + rel_path_val, + e, + ); + DbError::new(classify_image_exif_error(&e)) }) - .map_err(|_| DbError::new(DbErrorKind::UpdateError)) } fn clear_manual_date_taken( @@ -1313,11 +1380,11 @@ impl ExifDao for SqliteExifDao { let mut connection = self.connection.lock().expect("Unable to get ExifDao"); - let current: ImageExif = image_exif - .filter(library_id.eq(library_id_val)) - .filter(rel_path.eq(rel_path_val)) - .first(connection.deref_mut()) - .map_err(|_| anyhow::anyhow!("row not found"))?; + let current = locate_image_exif_row( + connection.deref_mut(), + library_id_val, + rel_path_val, + )?; // No override active — nothing to revert. Return the current // row unchanged so the endpoint is idempotent. @@ -1325,27 +1392,31 @@ impl ExifDao for SqliteExifDao { return Ok(current); } - diesel::update( - image_exif - .filter(library_id.eq(library_id_val)) - .filter(rel_path.eq(rel_path_val)), - ) - .set(( - date_taken.eq(current.original_date_taken), - date_taken_source.eq(current.original_date_taken_source.clone()), - original_date_taken.eq::>(None), - original_date_taken_source.eq::>(None), - )) - .execute(connection.deref_mut()) - .map_err(|_| anyhow::anyhow!("Update error"))?; + let row_id = current.id; + diesel::update(image_exif.find(row_id)) + .set(( + date_taken.eq(current.original_date_taken), + date_taken_source.eq(current.original_date_taken_source.clone()), + original_date_taken.eq::>(None), + original_date_taken_source.eq::>(None), + )) + .execute(connection.deref_mut()) + .map_err(|e| anyhow::Error::from(e).context("update_failed"))?; image_exif - .filter(library_id.eq(library_id_val)) - .filter(rel_path.eq(rel_path_val)) + .find(row_id) .first::(connection.deref_mut()) - .map_err(|_| anyhow::anyhow!("Re-read error")) + .map_err(|e| anyhow::Error::from(e).context("reread_failed")) + }) + .map_err(|e| { + log::warn!( + "clear_manual_date_taken(library_id={}, rel_path={:?}): {:#}", + library_id_val, + rel_path_val, + e, + ); + DbError::new(classify_image_exif_error(&e)) }) - .map_err(|_| DbError::new(DbErrorKind::UpdateError)) } fn get_memories_in_window( diff --git a/src/main.rs b/src/main.rs index f998160..5257afe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -754,10 +754,14 @@ async fn set_image_date( let span_context = opentelemetry::Context::new().with_remote_span_context(span.span_context().clone()); - let library = libraries::resolve_library_param(&app_state, body.library.as_deref()) - .ok() - .flatten() - .unwrap_or_else(|| app_state.primary_library()); + let library = match libraries::resolve_library_param(&app_state, body.library.as_deref()) { + Ok(Some(lib)) => lib, + Ok(None) => app_state.primary_library(), + Err(msg) => { + span.set_status(Status::error(msg.clone())); + return HttpResponse::BadRequest().body(msg); + } + }; // Path normalization matches set_image_gps so a Windows-import client // doesn't end up with a backslash variant that misses the row. @@ -781,9 +785,10 @@ async fn set_image_date( let msg = format!("set_manual_date_taken failed: {:?}", e); error!("{}", msg); span.set_status(Status::error(msg.clone())); - // Likely "row not found" — the file isn't indexed under this - // (library, path). 404 lets the client distinguish from a 5xx. - HttpResponse::NotFound().body(msg) + match e.kind { + DbErrorKind::NotFound => HttpResponse::NotFound().body(msg), + _ => HttpResponse::InternalServerError().body(msg), + } } } } @@ -802,10 +807,14 @@ async fn clear_image_date( let span_context = opentelemetry::Context::new().with_remote_span_context(span.span_context().clone()); - let library = libraries::resolve_library_param(&app_state, body.library.as_deref()) - .ok() - .flatten() - .unwrap_or_else(|| app_state.primary_library()); + let library = match libraries::resolve_library_param(&app_state, body.library.as_deref()) { + Ok(Some(lib)) => lib, + Ok(None) => app_state.primary_library(), + Err(msg) => { + span.set_status(Status::error(msg.clone())); + return HttpResponse::BadRequest().body(msg); + } + }; let normalized_path = body.path.replace('\\', "/"); @@ -827,7 +836,10 @@ async fn clear_image_date( let msg = format!("clear_manual_date_taken failed: {:?}", e); error!("{}", msg); span.set_status(Status::error(msg.clone())); - HttpResponse::NotFound().body(msg) + match e.kind { + DbErrorKind::NotFound => HttpResponse::NotFound().body(msg), + _ => HttpResponse::InternalServerError().body(msg), + } } } } From 9871c685b4a56bf5850b76787313eaeb0d7208d8 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sat, 9 May 2026 21:23:11 -0400 Subject: [PATCH 2/2] date-override: cargo fmt Co-Authored-By: Claude Opus 4.7 (1M context) --- src/database/mod.rs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/database/mod.rs b/src/database/mod.rs index 47d5871..049f8ac 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -752,9 +752,7 @@ fn locate_image_exif_row( .filter(rel_path.eq(&normalized).or(rel_path.eq(&windows_form))) .first::(connection) .map_err(|e| match e { - diesel::result::Error::NotFound => { - anyhow::Error::msg("row_not_found") - } + diesel::result::Error::NotFound => anyhow::Error::msg("row_not_found"), other => anyhow::Error::from(other).context("lookup_failed"), }) } @@ -1322,11 +1320,8 @@ impl ExifDao for SqliteExifDao { // Read-modify-write under the dao mutex so the snapshot is // consistent with the value being overwritten. The mutex holds // for the duration of this closure — no other writer can race. - let current = locate_image_exif_row( - connection.deref_mut(), - library_id_val, - rel_path_val, - )?; + let current = + locate_image_exif_row(connection.deref_mut(), library_id_val, rel_path_val)?; // Snapshot only on first override. Subsequent overrides keep // the original snapshot intact so a single revert restores @@ -1380,11 +1375,8 @@ impl ExifDao for SqliteExifDao { let mut connection = self.connection.lock().expect("Unable to get ExifDao"); - let current = locate_image_exif_row( - connection.deref_mut(), - library_id_val, - rel_path_val, - )?; + let current = + locate_image_exif_row(connection.deref_mut(), library_id_val, rel_path_val)?; // No override active — nothing to revert. Return the current // row unchanged so the endpoint is idempotent.