Merge pull request 'fix/manual-date-update' (#86) from fix/manual-date-update into master

Reviewed-on: #86

src/database/mod.rs

@@ -210,6 +210,7 @@ pub enum DbErrorKind {
     InsertError,
     QueryError,
     UpdateError,
+    NotFound,
 }
 
 pub trait FavoriteDao: Sync + Send {
@@ -713,6 +714,64 @@ pub trait ExifDao: Sync + Send {
     ) -> Result<Vec<(i32, String)>, 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<ImageExif> {
+    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::<ImageExif>(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::<ImageExif>(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<Mutex<SqliteConnection>>,
 }
@@ -1261,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: 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 +1335,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::<ImageExif>(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 +1375,8 @@ 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 +1384,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::<Option<i64>>(None),
-                original_date_taken_source.eq::<Option<String>>(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::<Option<i64>>(None),
+                    original_date_taken_source.eq::<Option<String>>(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::<ImageExif>(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(

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),
+            }
         }
     }
 }