fix(zot): pre-delete decision log + facet test coverage (refs #665)

src/zot/merge.py

@@ -162,9 +162,12 @@ def merge_url_duplicates(
     Clusters whose members don't all share the keeper's ``itemTypeID``
     are skipped (counted in ``skipped_type_mismatch``) rather than
     merged, since a type mismatch usually means the "duplicate" isn't
-    actually the same record. Writes a JSON decision log to *log_dir*
+    actually the same record. Every decision (merge or skip) is computed
-    (one entry per cluster) before any delete happens, even in
+    first and the resulting JSON log is written to *log_dir* — one entry
-    ``dry_run`` mode, so a run can be audited or replayed.
+    per cluster — before any merge actually runs, in both ``dry_run``
+    and live mode. That ordering is load-bearing: a crash mid-run still
+    leaves an on-disk record of what was about to happen, so a run can
+    always be audited or replayed even if it dies partway through.
     """
     stats = {
         "clusters": 0,
@@ -180,9 +183,16 @@ def merge_url_duplicates(
     log_dir = Path(log_dir)
     log_dir.mkdir(parents=True, exist_ok=True)
     run_id = now_iso().replace(" ", "T").replace(":", "")
+    log_path = log_dir / f"merge-log-{run_id}.json"
 
     with Db(str(db_path)) as db:
         clusters = find_url_clusters(db)
+
+        # Decide what would happen for every cluster first, without
+        # touching any row, and write that decision log to disk. Only
+        # after the log is on disk (i.e. before any delete can happen)
+        # do we actually run the merges.
+        merge_plan: list[tuple[tuple, list[tuple]]] = []
         for members in clusters:
             keeper = members[0]
             losers = members[1:]
@@ -197,19 +207,23 @@ def merge_url_duplicates(
                 )
                 continue
             stats["clusters"] += 1
-            entry = {
+            log.append(
-                "keeper_key": keeper[1],
+                {
-                "loser_keys": [m[1] for m in losers],
+                    "keeper_key": keeper[1],
-                "action": "dry-run" if dry_run else "merged",
+                    "loser_keys": [m[1] for m in losers],
-            }
+                    "action": "dry-run" if dry_run else "merged",
-            if not dry_run:
+                }
+            )
+            merge_plan.append((keeper, losers))
+
+        log_path.write_text(json.dumps(log, indent=1))
+
+        if not dry_run:
+            for keeper, losers in merge_plan:
                 for m in losers:
                     for k, v in merge_into(db, keeper[0], m[0]).items():
                         stats[k] += v
                     stats["merged"] += 1
-            log.append(entry)
-        if not dry_run:
             db.commit()
 
-    (log_dir / f"merge-log-{run_id}.json").write_text(json.dumps(log, indent=1))
     return stats

tests/zot/test_merge.py

@@ -35,6 +35,42 @@ def dup_db(tmp_path: Path) -> Path:
                 "INSERT INTO itemNotes (itemID, parentItemID, note, title) VALUES (?,?,?,?)",
                 (dn, parent, "<p>same on both</p>", ""),
             )
+        # loser-only child attachment
+        att_loser_only = db.create_item(TYPE_MAP["attachment"], key="FFFFFFFF")
+        db.con.execute(
+            "INSERT INTO itemAttachments"
+            " (itemID, parentItemID, linkMode, contentType, path)"
+            " VALUES (?,?,?,?,?)",
+            (att_loser_only, loser, 0, "application/pdf", "loser-only.pdf"),
+        )
+        # identical attachment path on both — must NOT be doubled
+        for parent, key in ((keeper, "GGGGGGGG"), (loser, "HHHHHHHH")):
+            att = db.create_item(TYPE_MAP["attachment"], key=key)
+            db.con.execute(
+                "INSERT INTO itemAttachments"
+                " (itemID, parentItemID, linkMode, contentType, path)"
+                " VALUES (?,?,?,?,?)",
+                (att, parent, 0, "application/pdf", "same-on-both.pdf"),
+            )
+        # collections: both share one, loser is also in a second one
+        coll_shared = db.ensure_collection("Shared Collection")
+        coll_loser_only = db.ensure_collection("Loser Only Collection")
+        db.add_to_collection(keeper, collection_key=coll_shared)
+        db.add_to_collection(loser, collection_key=coll_shared)
+        db.add_to_collection(loser, collection_key=coll_loser_only)
+        # itemRelations: loser has a relation the keeper lacks
+        db.con.execute(
+            "INSERT INTO relationPredicates (predicate) VALUES (?)",
+            ("dc:relation",),
+        )
+        pred_id = db.con.execute(
+            "SELECT predicateID FROM relationPredicates WHERE predicate=?",
+            ("dc:relation",),
+        ).fetchone()[0]
+        db.con.execute(
+            "INSERT INTO itemRelations (itemID, predicateID, object) VALUES (?,?,?)",
+            (loser, pred_id, "http://zotero.org/users/local/items/OLDKEY01"),
+        )
         db.commit()
     return p
 
@@ -59,6 +95,27 @@ def _notes(db: Db, item_id: int) -> list[str]:
     ]
 
 
+def _attachment_paths(db: Db, item_id: int) -> list[str]:
+    return [
+        r[0]
+        for r in db.con.execute(
+            "SELECT path FROM itemAttachments WHERE parentItemID=?", (item_id,)
+        )
+    ]
+
+
+def _collection_names(db: Db, item_id: int) -> set[str]:
+    return {
+        r[0]
+        for r in db.con.execute(
+            "SELECT c.collectionName FROM collectionItems ci"
+            " JOIN collections c ON c.collectionID=ci.collectionID"
+            " WHERE ci.itemID=?",
+            (item_id,),
+        )
+    }
+
+
 class TestMergeInto:
     def test_unions_tags_and_adopts_missing_notes(self, dup_db):
         from zot.merge import merge_into
@@ -90,6 +147,52 @@ class TestMergeInto:
             ).fetchone()
             assert row[0] == "X"
 
+    def test_adopts_missing_attachments_and_dedups_by_path(self, dup_db):
+        from zot.merge import merge_into
+
+        with Db(str(dup_db)) as db:
+            keeper = db.find_item_by_key("AAAAAAAA")
+            loser = db.find_item_by_key("BBBBBBBB")
+            counts = merge_into(db, keeper, loser)
+            db.commit()
+            paths = _attachment_paths(db, keeper)
+            assert "loser-only.pdf" in paths
+            assert paths.count("same-on-both.pdf") == 1
+            assert counts["attachments_adopted"] == 1
+
+    def test_unions_collection_memberships(self, dup_db):
+        from zot.merge import merge_into
+
+        with Db(str(dup_db)) as db:
+            keeper = db.find_item_by_key("AAAAAAAA")
+            loser = db.find_item_by_key("BBBBBBBB")
+            counts = merge_into(db, keeper, loser)
+            db.commit()
+            assert _collection_names(db, keeper) == {
+                "Shared Collection",
+                "Loser Only Collection",
+            }
+            # "Shared Collection" already had keeper as a member — only
+            # the new one should count as added.
+            assert counts["collections_added"] == 1
+
+    def test_moves_relations_to_keeper(self, dup_db):
+        from zot.merge import merge_into
+
+        with Db(str(dup_db)) as db:
+            keeper = db.find_item_by_key("AAAAAAAA")
+            loser = db.find_item_by_key("BBBBBBBB")
+            counts = merge_into(db, keeper, loser)
+            db.commit()
+            objects = {
+                r[0]
+                for r in db.con.execute(
+                    "SELECT object FROM itemRelations WHERE itemID=?", (keeper,)
+                )
+            }
+            assert "http://zotero.org/users/local/items/OLDKEY01" in objects
+            assert counts["relations_moved"] == 1
+
 
 class TestDriver:
     def test_dry_run_reports_without_deleting(self, dup_db, tmp_path):