Merge branch 'fix/collection-members-by-fileid'

Collection membership merged on a column that is NULL on every scanned
library, so collections synced their names and arrived empty everywhere.
Keyed on oc:fileid now, as keyword assignment already was.

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

core/dr-catalog/src/merge.rs

@@ -383,29 +383,69 @@ fn merge_collections_within(tx: &Connection, report: &mut MergeReport) -> Result
 
     // ---- membership -------------------------------------------------------
     //
-    // Set union, keyed on (collection uuid, image content hash). The hash
+    // Set union, keyed on (collection uuid, image identity). Not the image id,
-    // rather than the image id, for the same reason collections use a uuid:
+    // for the same reason collections use a uuid: image ids are local. An image
-    // image ids are local. An image the remote has and we do not is skipped —
+    // the remote has and we do not is skipped — it will join when a scan or
-    // it will join when a scan or sync catalogues it, and the next merge picks
+    // sync catalogues it, and the next merge picks it up.
-    // 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(
+    let added = tx.execute(MEMBERS_BY_FILE_ID, [])? + tx.execute(MEMBERS_BY_CONTENT_HASH, [])?;
-        "INSERT OR IGNORE INTO main.collection_members(collection_id, image_id, position, added)
+    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",
+     WHERE ri.content_hash IS NOT NULL";
-        [],
-    )?;
-    report.members_added = added;
-
-    Ok(())
-}
 
 /// Schema name the downloaded remote catalog is attached under.
 ///
@@ -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
+/// matches nothing and this is the only identity available — the same pair
-/// collection membership already uses, so a library where membership merges
+/// collection membership uses, so a library where membership merges has
-/// has keywords that merge too.
+/// 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.