From 9f50843479e90490f0b3727b8455ad7386843a0e Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Fri, 30 Oct 2026 14:27:54 -0400 Subject: [PATCH] fix(versions): review findings on PR #193 - A version no longer hides behind an original that duplicates resolve demoted (visible_version_clause, hidden_version_rows). - Recording a link drops the version's faces, CLIP data and clip failures (covers link-at-ingest and link_versions --apply). - The display file must live in the original's library; otherwise the original shows, and PUT /image/versions/display answers 400. - link_new_files looks up hashes in chunks of 500 and logs errors. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 11 +- src/database/version_dao.rs | 231 ++++++++++++++++++++++++++---- src/versions/backfill.rs | 48 +++++++ src/versions/handlers.rs | 23 ++- src/versions/integration_tests.rs | 101 +++++++++++++ src/versions/mod.rs | 35 +++-- 6 files changed, 402 insertions(+), 47 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 631a702..7320b69 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -390,13 +390,18 @@ for the `link_versions` binary). date drains never fight the copy. Memories therefore put a standalone version on the original's day. - **Hiding** uses the same reachability rule as `duplicate_of_hash`, with one - addition: a version row is hidden only while a row for its original exists *in - the current view* (`visible_version_clause`; unscoped = any library). A version - whose original was deleted, or lives outside the requested library, shows as a + addition: a version row is hidden only while its original is *shown in the + current view*: a row exists there and was not demoted by duplicates resolve + (`visible_version_clause`; unscoped = any library). A version + whose original was deleted, demoted, or lives outside the requested library, shows as a normal photo, so nothing disappears silently. Applied in `/photos` (SQL for the recursive listing, `hidden_version_rows` for the folder walk and tag branch), `/memories` and reels, `/photos/exif`, `/photos/gps-summary`, unified search, the camera counts and the media gauges; `include_versions=true` lists them. +- **Display file** is fetched through the original's library, so the display + choice only counts when the chosen version has a row there (else the original + shows; `PUT /image/versions/display` answers 400). Recording a link also drops + the version's `face_detections`, CLIP columns and `clip_failures`. - **Tags and favorites** written or read on a version path bind to the original (`canonical_rel_path` in the tag/favorite DAOs, `get_rel_paths_sharing_content` for reads). Insight generation on a version path runs against the original. A diff --git a/src/database/version_dao.rs b/src/database/version_dao.rs index 68bed30..355312a 100644 --- a/src/database/version_dao.rs +++ b/src/database/version_dao.rs @@ -47,20 +47,33 @@ pub fn is_known_kind(kind: &str) -> bool { /// the original may be gone.) Library ids are `i32`, so inlining the list /// is safe. pub fn visible_version_clause(view_library_ids: &[i32]) -> String { - let scope = if view_library_ids.is_empty() { - String::new() - } else { - let list = view_library_ids - .iter() - .map(i32::to_string) - .collect::>() - .join(","); - format!(" AND orig.library_id IN ({list})") - }; format!( "NOT EXISTS (SELECT 1 FROM photo_versions pv \ JOIN image_exif orig ON orig.content_hash = pv.original_hash \ - WHERE pv.version_hash = image_exif.content_hash{scope})" + WHERE pv.version_hash = image_exif.content_hash AND {})", + reachable_original_condition(view_library_ids) + ) +} + +/// Condition over an `image_exif` row aliased `orig`: the row is an original +/// that is itself shown in the view. It must be in the view, and not demoted +/// by duplicates resolve, using the duplicate-listing rule: unscoped, any +/// `duplicate_of_hash` hides it; scoped, only when the survivor is in the +/// view too. Otherwise its versions would vanish along with it. +fn reachable_original_condition(view_library_ids: &[i32]) -> String { + if view_library_ids.is_empty() { + return "orig.duplicate_of_hash IS NULL".to_string(); + } + let list = view_library_ids + .iter() + .map(i32::to_string) + .collect::>() + .join(","); + format!( + "orig.library_id IN ({list}) AND (orig.duplicate_of_hash IS NULL OR NOT EXISTS \ + (SELECT 1 FROM image_exif AS survivor \ + WHERE survivor.content_hash = orig.duplicate_of_hash \ + AND survivor.library_id IN ({list})))" ) } @@ -157,7 +170,8 @@ pub struct HashedFile { #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct PhotoVersionSummary { pub version_count: u32, - /// `(library_id, rel_path)` of the chosen version file. + /// `(library_id, rel_path)` of the chosen version file; the library is + /// always the original's own. pub display: Option<(i32, String)>, } @@ -368,6 +382,21 @@ impl VersionDao for SqliteVersionDao { .bind::(&link.rel_path) .execute(conn)?; + // A version is never a face / CLIP candidate; if it was + // ingested before the link, drop what was already derived. + sql_query("DELETE FROM face_detections WHERE content_hash = ?") + .bind::(&link.version_hash) + .execute(conn)?; + sql_query( + "UPDATE image_exif SET clip_embedding = NULL, clip_model_version = NULL \ + WHERE content_hash = ?", + ) + .bind::(&link.version_hash) + .execute(conn)?; + sql_query("DELETE FROM clip_failures WHERE content_hash = ?") + .bind::(&link.version_hash) + .execute(conn)?; + // Versions that pointed at this file now point at the root. sql_query("UPDATE photo_versions SET original_hash = ? WHERE original_hash = ?") .bind::(&root) @@ -639,13 +668,12 @@ impl VersionDao for SqliteVersionDao { if o.version_count == 0 { continue; } - let display = display_by_hash.get(&o.original_hash).and_then(|files| { - files - .iter() - .find(|(lib, _)| *lib == o.library_id) - .or_else(|| files.first()) - .cloned() - }); + // Clients fetch the display file through the original's + // library, so a chosen version with no row there falls back + // to the original. + let display = display_by_hash + .get(&o.original_hash) + .and_then(|files| files.iter().find(|(lib, _)| *lib == o.library_id).cloned()); out.insert( (o.library_id, o.rel_path), PhotoVersionSummary { @@ -671,22 +699,13 @@ impl VersionDao for SqliteVersionDao { #[diesel(sql_type = Text)] rel_path: String, } - let scope = if view_library_ids.is_empty() { - String::new() - } else { - let list = view_library_ids - .iter() - .map(i32::to_string) - .collect::>() - .join(","); - format!(" AND orig.library_id IN ({list})") - }; let sql = format!( "SELECT image_exif.library_id AS library_id, image_exif.rel_path AS rel_path \ FROM image_exif \ JOIN photo_versions pv ON pv.version_hash = image_exif.content_hash \ WHERE EXISTS (SELECT 1 FROM image_exif orig \ - WHERE orig.content_hash = pv.original_hash{scope})" + WHERE orig.content_hash = pv.original_hash AND {})", + reachable_original_condition(view_library_ids) ); let mut conn = self.connection.lock().expect("Unable to get VersionDao"); sql_query(sql) @@ -1089,4 +1108,156 @@ mod tests { let files = dao.files_for_hashes(&["o".to_string()], None).unwrap(); assert_eq!(files["o"].0, 1); } + fn visible_in(c: &Arc>, libs: &[i32]) -> Vec { + #[derive(QueryableByName)] + struct R { + #[diesel(sql_type = Text)] + rel_path: String, + } + let mut rows: Vec = sql_query(format!( + "SELECT rel_path FROM image_exif WHERE {} ORDER BY rel_path", + visible_version_clause(libs) + )) + .load::(&mut *c.lock().unwrap()) + .unwrap() + .into_iter() + .map(|r| r.rel_path) + .collect(); + rows.sort(); + rows + } + + #[test] + fn a_version_surfaces_when_its_original_is_demoted_as_a_duplicate() { + let (mut dao, c) = dao(); + insert_exif(&c, 1, "orig.jpg", "o"); + insert_exif(&c, 1, "orig.jpg.restored.png", "v"); + insert_exif(&c, 1, "survivor.jpg", "s"); + dao.link_version(&link("v", "o", "orig.jpg.restored.png")) + .unwrap(); + assert_eq!(visible_in(&c, &[]), vec!["orig.jpg", "survivor.jpg"]); + + exec( + &c, + "UPDATE image_exif SET duplicate_of_hash = 's' WHERE content_hash = 'o'", + ); + // Unscoped: O is hidden by the duplicate rule, so V stands alone. + let shown = visible_in(&c, &[]); + assert!( + shown.contains(&"orig.jpg.restored.png".to_string()), + "{shown:?}" + ); + assert!(dao.hidden_version_rows(&[]).unwrap().is_empty()); + // Scoped with the survivor in view: same. + assert!(visible_in(&c, &[1]).contains(&"orig.jpg.restored.png".to_string())); + assert!(dao.hidden_version_rows(&[1]).unwrap().is_empty()); + + // Scoped to a view that lacks the survivor, O stays visible (the + // duplicate rule), so V hides behind it again. + insert_exif(&c, 2, "elsewhere.jpg", "e"); + exec( + &c, + "UPDATE image_exif SET duplicate_of_hash = 'e' WHERE content_hash = 'o'", + ); + assert!(!visible_in(&c, &[1]).contains(&"orig.jpg.restored.png".to_string())); + assert_eq!(dao.hidden_version_rows(&[1]).unwrap().len(), 1); + } + + #[test] + fn display_file_comes_from_the_originals_library_or_not_at_all() { + let (mut dao, c) = dao(); + insert_exif(&c, 1, "orig.jpg", "o"); + // Same bytes in two libraries. + insert_exif(&c, 2, "copy/v.png", "v"); + insert_exif(&c, 1, "aiedits/v.png", "v"); + dao.link_version(&link("v", "o", "aiedits/v.png")).unwrap(); + dao.set_display("o", Some("v")).unwrap(); + let s = dao.photo_summaries().unwrap(); + assert_eq!( + s[&(1, "orig.jpg".to_string())].display, + Some((1, "aiedits/v.png".to_string())) + ); + + // Only the other library holds the chosen bytes: show the original. + exec( + &c, + "DELETE FROM image_exif WHERE library_id = 1 AND content_hash = 'v'", + ); + let s = dao.photo_summaries().unwrap(); + let sum = &s[&(1, "orig.jpg".to_string())]; + assert_eq!(sum.version_count, 1); + assert_eq!(sum.display, None); + } + + #[test] + fn linking_drops_faces_and_clip_data_derived_before_the_link() { + let (mut dao, c) = dao(); + insert_exif(&c, 1, "orig.jpg", "o"); + insert_exif(&c, 1, "v.png", "v"); + exec( + &c, + "UPDATE image_exif SET clip_embedding = x'00', clip_model_version = 'm' \ + WHERE content_hash IN ('o', 'v')", + ); + for h in ["o", "v"] { + exec( + &c, + &format!( + "INSERT INTO face_detections (library_id, content_hash, rel_path, bbox_x, \ + embedding, source, status, model_version, created_at) \ + VALUES (1, '{h}', 'x', 0.1, x'00', 'auto', 'detected', 'm', 0)" + ), + ); + exec( + &c, + &format!( + "INSERT INTO clip_failures (library_id, rel_path, content_hash, error, failed_at) \ + VALUES (1, 'p-{h}', '{h}', 'boom', 0)" + ), + ); + } + dao.link_version(&link("v", "o", "v.png")).unwrap(); + + let n = |sql: &str| -> i64 { + #[derive(QueryableByName)] + struct N { + #[diesel(sql_type = BigInt)] + n: i64, + } + sql_query(sql) + .get_result::(&mut *c.lock().unwrap()) + .unwrap() + .n + }; + assert_eq!( + n("SELECT COUNT(*) AS n FROM face_detections WHERE content_hash='v'"), + 0 + ); + assert_eq!( + n("SELECT COUNT(*) AS n FROM clip_failures WHERE content_hash='v'"), + 0 + ); + assert_eq!( + n( + "SELECT COUNT(*) AS n FROM image_exif WHERE content_hash='v' \ + AND (clip_embedding IS NOT NULL OR clip_model_version IS NOT NULL)" + ), + 0 + ); + // The original keeps its own. + assert_eq!( + n("SELECT COUNT(*) AS n FROM face_detections WHERE content_hash='o'"), + 1 + ); + assert_eq!( + n("SELECT COUNT(*) AS n FROM clip_failures WHERE content_hash='o'"), + 1 + ); + assert_eq!( + n( + "SELECT COUNT(*) AS n FROM image_exif WHERE content_hash='o' AND clip_embedding IS NOT NULL" + ), + 1 + ); + } } diff --git a/src/versions/backfill.rs b/src/versions/backfill.rs index 0d7af8b..32d965b 100644 --- a/src/versions/backfill.rs +++ b/src/versions/backfill.rs @@ -928,4 +928,52 @@ mod tests { let report = apply(&mut dao, &[], &reviewed, &stale_rows); assert_eq!(report.stale, 1); } + #[test] + fn apply_drops_derived_data_of_an_already_ingested_version() { + let conn = Arc::new(Mutex::new(in_memory_db_connection())); + let exec = |sql: &str| { + diesel::RunQueryDsl::execute(diesel::sql_query(sql), &mut *conn.lock().unwrap()) + .unwrap_or_else(|e| panic!("{sql}: {e}")); + }; + exec( + "INSERT INTO image_exif (library_id, rel_path, created_time, last_modified, content_hash, clip_embedding, clip_model_version) VALUES (1, 'v.png', 0, 0, 'v', x'00', 'm')", + ); + exec( + "INSERT INTO face_detections (library_id, content_hash, rel_path, bbox_x, embedding, source, status, model_version, created_at) VALUES (1, 'v', 'v.png', 0.1, x'00', 'auto', 'detected', 'm', 0)", + ); + exec( + "INSERT INTO clip_failures (library_id, rel_path, content_hash, error, failed_at) VALUES (1, 'v.png', 'v', 'boom', 0)", + ); + let mut dao = SqliteVersionDao::from_shared(conn.clone()); + let rows = vec![row("v.png", "v", None)]; + let reviewed = vec![ReviewedLink { + method: "phash".into(), + library_id: 1, + version_path: "v.png".into(), + kind: "edit".into(), + version_hash: "v".into(), + original_hash: "o".into(), + references: vec![], + }]; + assert_eq!(apply(&mut dao, &[], &reviewed, &rows).linked, 1); + + #[derive(diesel::QueryableByName)] + struct N { + #[diesel(sql_type = diesel::sql_types::BigInt)] + n: i64, + } + for sql in [ + "SELECT COUNT(*) AS n FROM face_detections WHERE content_hash = 'v'", + "SELECT COUNT(*) AS n FROM clip_failures WHERE content_hash = 'v'", + "SELECT COUNT(*) AS n FROM image_exif WHERE content_hash = 'v' AND (clip_embedding IS NOT NULL OR clip_model_version IS NOT NULL)", + ] { + let n = diesel::RunQueryDsl::get_result::( + diesel::sql_query(sql), + &mut *conn.lock().unwrap(), + ) + .unwrap() + .n; + assert_eq!(n, 0, "{sql}"); + } + } } diff --git a/src/versions/handlers.rs b/src/versions/handlers.rs index acdb25c..f88481d 100644 --- a/src/versions/handlers.rs +++ b/src/versions/handlers.rs @@ -159,9 +159,16 @@ pub fn build_response( }); } - let display_hash = dao - .get_display(original_hash)? - .filter(|d| versions.iter().any(|v| &v.hash == d)); + // Clients fetch the display file through the original's library, so a + // choice with no row there reads as the original (as `/photos` does). + let display_hash = dao.get_display(original_hash)?.filter(|d| { + versions.iter().any(|v| { + &v.hash == d + && original + .as_ref() + .is_none_or(|o| o.library_id == v.library_id) + }) + }); Ok(VersionsResponse { original, versions, @@ -232,6 +239,16 @@ pub async fn put_display( if !current.versions.iter().any(|v| v.hash == hash) { return HttpResponse::BadRequest().body("display_hash is not a version of this photo"); } + // The display file is fetched through the original's library. + let original_lib = current.original.as_ref().map(|o| o.library_id); + if !current + .versions + .iter() + .any(|v| v.hash == hash && original_lib.is_none_or(|l| l == v.library_id)) + { + return HttpResponse::BadRequest() + .body("display_hash has no file in the original's library"); + } } if let Err(e) = dao.set_display(&resolved.original_hash, wanted) { warn!("put_display failed: {:?}", e); diff --git a/src/versions/integration_tests.rs b/src/versions/integration_tests.rs index a97296f..0ebae04 100644 --- a/src/versions/integration_tests.rs +++ b/src/versions/integration_tests.rs @@ -837,3 +837,104 @@ async fn face_stats_denominator_leaves_versions_out() { // denominator because they are never scanned. assert_eq!(stats.total_photos, 2); } + +#[actix_rt::test] +async fn linking_at_ingest_drops_derived_data_of_an_already_ingested_version() { + let fx = fx(); + let original = fx.put("scan.jpg", b"original bytes"); + let xmp = producer_xmp(&original, "restored", &[]); + let vh = fx.put_png("renamed.png", Some(&xmp), 1); + fx.exec(&format!( + "UPDATE image_exif SET clip_embedding = x'00', clip_model_version = 'm' \ + WHERE content_hash = '{vh}'" + )); + fx.exec(&format!( + "INSERT INTO face_detections (library_id, content_hash, rel_path, bbox_x, embedding, \ + source, status, model_version, created_at) \ + VALUES (1, '{vh}', 'renamed.png', 0.1, x'00', 'auto', 'detected', 'm', 0)" + )); + fx.exec(&format!( + "INSERT INTO clip_failures (library_id, rel_path, content_hash, error, failed_at) \ + VALUES (1, 'renamed.png', '{vh}', 'boom', 0)" + )); + + let files = vec![(fx.root.join("renamed.png"), "renamed.png".to_string())]; + assert_eq!( + link_new_files(&fx.library, &files, &fx.exif_arc(), &fx.state.version_dao), + 1 + ); + assert_eq!( + fx.count(&format!( + "SELECT COUNT(*) AS n FROM face_detections WHERE content_hash = '{vh}'" + )), + 0 + ); + assert_eq!( + fx.count(&format!( + "SELECT COUNT(*) AS n FROM clip_failures WHERE content_hash = '{vh}'" + )), + 0 + ); + assert_eq!( + fx.count(&format!( + "SELECT COUNT(*) AS n FROM image_exif WHERE content_hash = '{vh}' \ + AND (clip_embedding IS NOT NULL OR clip_model_version IS NOT NULL)" + )), + 0 + ); +} + +#[actix_rt::test] +async fn link_new_files_links_across_more_than_one_lookup_chunk() { + let fx = fx(); + let original = fx.put("scan.jpg", b"original bytes"); + let xmp = producer_xmp(&original, "restored", &[]); + let vh = fx.put_png("zz/renamed.png", Some(&xmp), 1); + // Enough unrelated PNG paths to force several lookup chunks; the real + // one sits in the last. + let mut files: Vec<(PathBuf, String)> = (0..1200) + .map(|i| (fx.root.join(format!("n/{i}.png")), format!("n/{i}.png"))) + .collect(); + files.push((fx.root.join("zz/renamed.png"), "zz/renamed.png".to_string())); + assert_eq!( + link_new_files(&fx.library, &files, &fx.exif_arc(), &fx.state.version_dao), + 1 + ); + assert!( + fx.state + .version_dao + .lock() + .unwrap() + .get_link(&vh) + .unwrap() + .is_some() + ); +} + +#[actix_rt::test] +async fn display_choice_needs_a_file_in_the_originals_library() { + let fx = fx(); + let original = fx.put("scan.jpg", b"original bytes"); + let xmp = producer_xmp(&original, "restored", &[]); + let vh = fx.put_png("aiedits/scan.jpg.restored.png", Some(&xmp), 1); + fx.link(&vh, &original, "aiedits/scan.jpg.restored.png"); + // The version's bytes live only in a second library. + fx.exec("INSERT INTO libraries (id, name, root_path, created_at) VALUES (2, 'two', '/tmp/two-nope', 0)"); + fx.exec( + "UPDATE image_exif SET library_id = 2 WHERE rel_path = 'aiedits/scan.jpg.restored.png'", + ); + let app = photos_app!(fx); + let req = awtest::TestRequest::put() + .uri("/image/versions/display") + .insert_header(("Authorization", format!("Bearer {}", Fx::token()))) + .set_json(serde_json::json!({"path": "scan.jpg", "display_hash": vh})) + .to_request(); + assert_eq!(awtest::call_service(&app, req).await.status(), 400); + // Clearing is still fine. + let req = awtest::TestRequest::put() + .uri("/image/versions/display") + .insert_header(("Authorization", format!("Bearer {}", Fx::token()))) + .set_json(serde_json::json!({"path": "scan.jpg", "display_hash": null})) + .to_request(); + assert_eq!(awtest::call_service(&app, req).await.status(), 200); +} diff --git a/src/versions/mod.rs b/src/versions/mod.rs index 2ef7803..c46359a 100644 --- a/src/versions/mod.rs +++ b/src/versions/mod.rs @@ -221,22 +221,35 @@ pub fn link_new_files( return 0; } let rels: Vec = pngs.iter().map(|(_, r)| r.clone()).collect(); - let rows = exif_dao - .lock() - .expect("Unable to lock ExifDao") - .get_exif_batch(Some(library.id), &rels) - .unwrap_or_default(); + // Chunked: a first scan can register far more paths than SQLite's + // bound-variable limit allows in one `IN (...)`. + let mut rows = Vec::with_capacity(rels.len()); + for chunk in rels.chunks(500) { + match exif_dao + .lock() + .expect("Unable to lock ExifDao") + .get_exif_batch(Some(library.id), chunk) + { + Ok(r) => rows.extend(r), + Err(e) => warn!( + "versions: XMP link lookup failed for {} paths in '{}': {:?}", + chunk.len(), + library.name, + e + ), + } + } + let hashes: std::collections::HashMap<&str, &str> = rows + .iter() + .filter_map(|r| Some((r.file_path.as_str(), r.content_hash.as_deref()?))) + .collect(); let mut linked = 0; for (abs, rel) in pngs { - let Some(hash) = rows - .iter() - .find(|r| &r.file_path == rel) - .and_then(|r| r.content_hash.clone()) - else { + let Some(hash) = hashes.get(rel.as_str()) else { continue; }; - if let Some(link) = xmp_link(library, rel, abs, &hash) + if let Some(link) = xmp_link(library, rel, abs, hash) && record_link(version_dao, &link) { linked += 1;