From 67f15bffb7c73884fbc571f0bb84fc42eabd83b7 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 21:19:13 +0200 Subject: [PATCH] Key collection membership on the identity that exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collections synced their names and arrived empty on every device. The names are keyed on a uuid and worked; the membership union was keyed on `images.content_hash`, and the schema says plainly what that column is: "computed only when something needs it (import dedup, reconnect-by-hash), never in a scan". A library that has only ever been scanned has one for no image at all, so the join matched nothing and `WHERE ri.content_hash IS NOT NULL` discarded whatever survived. The union could never have moved a single row. Measured on a real catalog: 23,174 images, content hashes for 0 of them, `oc:fileid` for all 23,174, twelve collections, zero members. So membership now resolves through the file id first, exactly as keyword assignment already did — `ASSIGN_BY_FILE_ID` was added for this same reason and its doc comment even notes that membership was still on the hash. It is recorded for every image the moment a remote scan sees it, survives server-side rename and move (FR-NC-5), is the same integer on every device pointed at one Nextcloud, and is already what the thumbnail shards are keyed by. The content-hash union is kept rather than replaced: a local-only library has no `remote` rows, and where a hash has been computed it is a true identity that survives a library moving between servers. Both statements run; `INSERT OR IGNORE` against the primary key makes the overlap free. This repairs the merge. It cannot invent membership that no device recorded — where the rows were never written, collections stay empty until they are filled in again. Co-Authored-By: Claude Opus 5 (1M context) --- core/dr-catalog/src/merge.rs | 196 +++++++++++++++++++++++++++++++---- 1 file changed, 177 insertions(+), 19 deletions(-) diff --git a/core/dr-catalog/src/merge.rs b/core/dr-catalog/src/merge.rs index 7a1e50c..08cf47b 100644 --- a/core/dr-catalog/src/merge.rs +++ b/core/dr-catalog/src/merge.rs @@ -383,30 +383,70 @@ fn merge_collections_within(tx: &Connection, report: &mut MergeReport) -> Result // ---- membership ------------------------------------------------------- // - // Set union, keyed on (collection uuid, image content hash). The hash - // rather than the image id, for the same reason collections use a uuid: - // image ids are local. An image the remote has and we do not is skipped — - // it will join when a scan or sync catalogues it, and the next merge picks - // it up. + // Set union, keyed on (collection uuid, image identity). Not the image id, + // for the same reason collections use a uuid: image ids are local. An image + // the remote has and we do not is skipped — it will join when a scan or + // sync catalogues it, and the next merge picks it up. + // + // Both identities are tried, and the file id first. See + // [`MEMBERS_BY_FILE_ID`] for why keying on the content hash alone made this + // whole union a no-op on every ordinary library. // // Tombstoned collections are excluded, or a merge would repopulate a // collection it had just deleted. - let added = tx.execute( - "INSERT OR IGNORE INTO main.collection_members(collection_id, image_id, position, added) - SELECT lc.id, li.id, rm.position, rm.added - FROM remote_cat.collection_members rm - JOIN remote_cat.collections rc ON rc.id = rm.collection_id - JOIN main.collections lc ON lc.uuid = rc.uuid AND lc.deleted = 0 - JOIN remote_cat.images ri ON ri.id = rm.image_id - JOIN main.images li ON li.content_hash = ri.content_hash - WHERE ri.content_hash IS NOT NULL", - [], - )?; + let added = tx.execute(MEMBERS_BY_FILE_ID, [])? + tx.execute(MEMBERS_BY_CONTENT_HASH, [])?; report.members_added = added; Ok(()) } +/// Take membership for images both devices know by the server's file id. +/// +/// **This is the identity that exists.** Membership was keyed on +/// `images.content_hash` alone, and the schema is explicit that the column is +/// "computed only when something needs it (import dedup, reconnect-by-hash), +/// never in a scan" — so on an ordinary library it is NULL for every row, the +/// join matched nothing, and `WHERE ri.content_hash IS NOT NULL` discarded what +/// little was left. Collections synced their names, because those are keyed on +/// a uuid, and arrived empty on every device. A 23,000-image library had a +/// content hash for none of them and an `oc:fileid` for all of them. +/// +/// `oc:fileid` is recorded for every image the moment a remote scan sees it, is +/// stable across server-side rename and move (FR-NC-5), and is the same integer +/// on every device pointed at the same Nextcloud — which is exactly the +/// situation where two devices share collections. It is already what the +/// thumbnail shards are keyed by, and what [`ASSIGN_BY_FILE_ID`] uses for +/// keywords; membership was the one thing left behind. +const MEMBERS_BY_FILE_ID: &str = " + INSERT OR IGNORE INTO main.collection_members(collection_id, image_id, position, added) + SELECT lc.id, li.id, rm.position, rm.added + FROM remote_cat.collection_members rm + JOIN remote_cat.collections rc ON rc.id = rm.collection_id + JOIN main.collections lc ON lc.uuid = rc.uuid AND lc.deleted = 0 + JOIN remote_cat.remote rr ON rr.image_id = rm.image_id + JOIN main.remote lr ON lr.file_id = rr.file_id + JOIN main.images li ON li.id = lr.image_id"; + +/// The same union for a library with no server behind it. +/// +/// A local-only library has no `remote` rows at all, so [`MEMBERS_BY_FILE_ID`] +/// matches nothing and the content hash is the only identity available. Kept +/// rather than replaced: where a hash *has* been computed — an imported card, +/// a reconnect — it is a true identity, and one that survives a library moving +/// between servers. +/// +/// Both statements run. `INSERT OR IGNORE` against the +/// `(collection_id, image_id)` primary key makes the overlap free. +const MEMBERS_BY_CONTENT_HASH: &str = " + INSERT OR IGNORE INTO main.collection_members(collection_id, image_id, position, added) + SELECT lc.id, li.id, rm.position, rm.added + FROM remote_cat.collection_members rm + JOIN remote_cat.collections rc ON rc.id = rm.collection_id + JOIN main.collections lc ON lc.uuid = rc.uuid AND lc.deleted = 0 + JOIN remote_cat.images ri ON ri.id = rm.image_id + JOIN main.images li ON li.content_hash = ri.content_hash + WHERE ri.content_hash IS NOT NULL"; + /// Schema name the downloaded remote catalog is attached under. /// /// Repeated from [`crate::sync`] rather than shared, because the SQL below @@ -615,9 +655,9 @@ const ASSIGN_BY_FILE_ID: &str = " /// The same union for a library with no server behind it. /// /// A local-only library has no `remote` rows at all, so [`ASSIGN_BY_FILE_ID`] -/// matches nothing and this is the only identity available — and it is the one -/// collection membership already uses, so a library where membership merges -/// has keywords that merge too. +/// matches nothing and this is the only identity available — the same pair +/// collection membership uses, so a library where membership merges has +/// keywords that merge too. const ASSIGN_BY_CONTENT_HASH: &str = " INSERT OR IGNORE INTO main.keywords(version_id, keyword) SELECT lv.id, rk.keyword @@ -741,6 +781,43 @@ mod tests { c } + /// An image with no content hash — which is every image in a library that + /// has only ever been scanned. + fn add_image_without_hash(c: &Connection, db: &str, id: i64) { + c.execute( + &format!( + "INSERT INTO {db}.roots(id, kind, label) VALUES (1, 'remote', 'lib') + ON CONFLICT(id) DO NOTHING" + ), + [], + ) + .unwrap(); + c.execute( + &format!( + "INSERT INTO {db}.images(id, root_id, source_ref, added_at) + VALUES (?1, 1, ?2, 0)" + ), + rusqlite::params![id, format!("img{id}.CR3")], + ) + .unwrap(); + } + + fn count(c: &Connection, sql: &str) -> i64 { + c.query_row(sql, [], |r| r.get(0)).unwrap() + } + + /// Give an image the `oc:fileid` a remote scan records for it. + /// + /// What every image in a real synced library has, and what none of them + /// has a content hash for. + fn add_remote_id(c: &Connection, db: &str, image_id: i64, file_id: i64) { + c.execute( + &format!("INSERT INTO {db}.remote(image_id, file_id) VALUES (?1, ?2)"), + rusqlite::params![image_id, file_id], + ) + .unwrap(); + } + fn add_image(c: &Connection, db: &str, id: i64, hash: &str) { c.execute( &format!( @@ -910,6 +987,87 @@ mod tests { assert_eq!(n, 2, "both devices' additions survive"); } + #[test] + fn a_library_that_never_computed_a_hash_still_merges_membership() { + // The bug this replaces. `content_hash` is computed only by import + // dedup or a reconnect — never by a scan — so a synced library has one + // for no image at all. Keying membership on it alone meant collections + // arrived with their names and none of their contents, on every device, + // for every user. + let c = two_catalogs(); + add_collection(&c, "main", 1, "shared", "Trip", 1); + add_collection(&c, "remote_cat", 1, "shared", "Trip", 1); + + // No hashes anywhere, and a file id for both — a real library. + add_image_without_hash(&c, "main", 77); + add_image_without_hash(&c, "remote_cat", 3); + add_remote_id(&c, "main", 77, 90210); + add_remote_id(&c, "remote_cat", 3, 90210); + + c.execute( + "INSERT INTO remote_cat.collection_members(collection_id, image_id, added) + VALUES (1, 3, 0)", + [], + ) + .unwrap(); + + let report = merge_collections(&c).unwrap(); + assert_eq!(report.members_added, 1); + + let img: i64 = c + .query_row("SELECT image_id FROM main.collection_members", [], |r| { + r.get(0) + }) + .unwrap(); + assert_eq!(img, 77, "resolved through the server's file id"); + } + + #[test] + fn a_different_photograph_on_the_server_is_not_adopted() { + // The file id is an identity, so two different ids must not join. + let c = two_catalogs(); + add_collection(&c, "main", 1, "shared", "Trip", 1); + add_collection(&c, "remote_cat", 1, "shared", "Trip", 1); + add_image_without_hash(&c, "main", 77); + add_image_without_hash(&c, "remote_cat", 3); + add_remote_id(&c, "main", 77, 111); + add_remote_id(&c, "remote_cat", 3, 222); + + c.execute( + "INSERT INTO remote_cat.collection_members(collection_id, image_id, added) + VALUES (1, 3, 0)", + [], + ) + .unwrap(); + + let report = merge_collections(&c).unwrap(); + assert_eq!(report.members_added, 0); + assert_eq!(count(&c, "SELECT COUNT(*) FROM main.collection_members"), 0); + } + + #[test] + fn an_image_identified_both_ways_is_added_once() { + // Both statements run, and their overlap must be free rather than a + // constraint violation or a double count. + let c = two_catalogs(); + add_collection(&c, "main", 1, "shared", "Trip", 1); + add_collection(&c, "remote_cat", 1, "shared", "Trip", 1); + add_image(&c, "main", 77, "same-photo"); + add_image(&c, "remote_cat", 3, "same-photo"); + add_remote_id(&c, "main", 77, 90210); + add_remote_id(&c, "remote_cat", 3, 90210); + + c.execute( + "INSERT INTO remote_cat.collection_members(collection_id, image_id, added) + VALUES (1, 3, 0)", + [], + ) + .unwrap(); + + merge_collections(&c).unwrap(); + assert_eq!(count(&c, "SELECT COUNT(*) FROM main.collection_members"), 1); + } + #[test] fn membership_maps_across_devices_by_content_hash() { // The same photograph carries different integer ids on each device.