feat: change manifest PK from local_path to asset_resource

teh-hippo · Copilot · teh-hippo · commit 47d0757b3803 · 2026-04-09T23:19:56.000+10:00
Replace PK (asset_id, zone_id, local_path) with
(asset_id, zone_id, asset_resource) where asset_resource is the Apple
iCloud resource field prefix (e.g. resOriginal, resOriginalVidCompl).

This makes local_path an updateable column, so config changes like
toggling --keep-unicode-in-filenames update the existing manifest row
instead of creating a duplicate.

- Add asset_resource column (schema v3)
- Add version_to_resource() mapping in version_size.py
- Update all manifest.lookup/upsert/remove calls in base.py
- Migration: v0/v1/v2 DBs get asset_resource via ALTER + table rebuild
- 7 new unit tests for asset_resource PK behaviour

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/src/icloudpd/base.py b/src/icloudpd/base.py
@@ -92,7 +92,7 @@
     size_to_suffix,
     store_password_in_keyring,
 )
-from pyicloud_ipd.version_size import AssetVersionSize, LivePhotoVersionSize
+from pyicloud_ipd.version_size import AssetVersionSize, LivePhotoVersionSize, version_to_resource
 
 freeze_support()  # fmt: skip # fixing tqdm on macos
 
@@ -128,6 +128,7 @@ def _check_collision(
     directory: str,
     dir_cache: "DirCache",
     logger: Logger,
+    asset_resource: str = "resOriginal",
 ) -> tuple[str, str, bool, str | None]:
     """Check if another asset owns this path and self-heal if needed.
 
@@ -140,7 +141,7 @@ def _check_collision(
         dedup_suffix = f"_{suffix}"
         new_download_path = add_suffix_to_filename(dedup_suffix, download_path)
         new_rel_path = os.path.relpath(new_download_path, directory)
-        manifest.update_path(photo_id, manifest.zone_id, rel_path, new_rel_path)
+        manifest.update_path(photo_id, manifest.zone_id, asset_resource, new_rel_path)
         file_exists = dir_cache.isfile(new_download_path)
         logger.debug(
             "Collision on %s (owned by %s), deduping %s to %s",
@@ -912,16 +913,18 @@ def download_builder(
         # Compute relative path for manifest — use the actual file path on disk
         actual_path = original_download_path if original_download_path and file_exists else download_path
         rel_path = os.path.relpath(actual_path, directory) if manifest else ""
+        asset_res = version_to_resource(download_size)
 
         dedup_download = False
         if file_exists:
             if manifest is not None:
                 # Identity-based matching: check manifest for this asset
-                manifest_row = manifest.lookup(photo.id, manifest.zone_id, rel_path)
+                manifest_row = manifest.lookup(photo.id, manifest.zone_id, asset_res)
                 if manifest_row is not None:
                     # Check if this asset actually owns the file
                     rel_path, download_path, file_exists, dedup_suffix = _check_collision(
                         manifest, photo.id, rel_path, download_path, directory, dir_cache, logger,
+                        asset_resource=asset_res,
                     )
                     dedup_download = dedup_suffix is not None and not file_exists
                     if dedup_suffix is None and manifest_row.version_size != version.size:
@@ -951,6 +954,7 @@ def download_builder(
                             zone_id=manifest.zone_id,
                             local_path=rel_path,
                             version_size=version.size,
+                            asset_resource=asset_res,
                             **meta,
                         )
                 else:
@@ -971,6 +975,7 @@ def download_builder(
                                 zone_id=manifest.zone_id,
                                 local_path=rel_path,
                                 version_size=version.size,
+                                asset_resource=asset_res,
                                 **meta,
                             )
                             logger.debug(
@@ -987,6 +992,7 @@ def download_builder(
                             zone_id=manifest.zone_id,
                             local_path=rel_path,
                             version_size=version.size,
+                            asset_resource=asset_res,
                             **meta,
                         )
                         logger.debug(
@@ -1063,6 +1069,7 @@ def download_builder(
                                 zone_id=manifest.zone_id,
                                 local_path=rel_path,
                                 version_size=version.size,
+                                asset_resource=asset_res,
                                 **meta,
                             )
                     logger.info("Downloaded %s", truncated_path)
@@ -1078,6 +1085,7 @@ def download_builder(
     # Also download the live photo if present
     if not skip_live_photos:
         lp_size = live_photo_size
+        lp_res = version_to_resource(lp_size)
         photo_versions_with_policy = photo.versions_with_raw_policy(raw_policy)
         if lp_size in photo_versions_with_policy:
             version = photo_versions_with_policy[lp_size]
@@ -1108,7 +1116,7 @@ def download_builder(
             if only_print_filenames:
                 if lp_file_exists and manifest is not None:
                     # Check if version changed — need to print the filename for re-download
-                    lp_manifest_row = manifest.lookup(photo.id, manifest.zone_id, lp_rel_path)
+                    lp_manifest_row = manifest.lookup(photo.id, manifest.zone_id, lp_res)
                     if lp_manifest_row is not None and lp_manifest_row.version_size != version.size:
                         lp_file_exists = False
                 if not lp_file_exists:
@@ -1127,11 +1135,12 @@ def download_builder(
             else:
                 if lp_file_exists:
                     if manifest is not None:
-                        lp_manifest_row = manifest.lookup(photo.id, manifest.zone_id, lp_rel_path)
+                        lp_manifest_row = manifest.lookup(photo.id, manifest.zone_id, lp_res)
                         if lp_manifest_row is not None:
                             # Check if this asset owns the LP file
                             lp_rel_path, lp_download_path, lp_file_exists, lp_dedup = _check_collision(
                                 manifest, photo.id, lp_rel_path, lp_download_path, directory, dir_cache, logger,
+                                asset_resource=lp_res,
                             )
                             if lp_dedup is not None:
                                 # Collision handled — propagate suffix
@@ -1153,6 +1162,7 @@ def download_builder(
                                     zone_id=manifest.zone_id,
                                     local_path=lp_rel_path,
                                     version_size=version.size,
+                                    asset_resource=lp_res,
                                     **meta,
                                 )
                         else:
@@ -1171,6 +1181,7 @@ def download_builder(
                                         zone_id=manifest.zone_id,
                                         local_path=lp_rel_path,
                                         version_size=version.size,
+                                        asset_resource=lp_res,
                                         **meta,
                                     )
                                     logger.debug(
@@ -1185,6 +1196,7 @@ def download_builder(
                                     zone_id=manifest.zone_id,
                                     local_path=lp_rel_path,
                                     version_size=version.size,
+                                    asset_resource=lp_res,
                                     **meta,
                                 )
                     elif file_match_policy == FileMatchPolicy.NAME_SIZE_DEDUP_WITH_SUFFIX:
@@ -1223,6 +1235,7 @@ def download_builder(
                                     zone_id=manifest.zone_id,
                                     local_path=lp_rel_path,
                                     version_size=version.size,
+                                    asset_resource=lp_res,
                                     **meta,
                                 )
                         logger.info("Downloaded %s", truncated_path)
diff --git a/src/icloudpd/manifest.py b/src/icloudpd/manifest.py
@@ -10,6 +10,7 @@
 The manifest lives at {download_dir}/.icloudpd.db and travels with the library.
 """
 
+import contextlib
 import logging
 import os
 import sqlite3
@@ -19,12 +20,13 @@
 
 logger = logging.getLogger(__name__)
 
-SCHEMA_VERSION = 2
+SCHEMA_VERSION = 3
 
 _FRESH_SCHEMA = """\
 CREATE TABLE IF NOT EXISTS manifest (
     asset_id TEXT NOT NULL,
     zone_id TEXT NOT NULL DEFAULT '',
+    asset_resource TEXT NOT NULL DEFAULT 'resOriginal',
     local_path TEXT NOT NULL,
     version_size INTEGER NOT NULL,
     version_checksum TEXT,
@@ -58,7 +60,7 @@
     burst_id TEXT,
     original_orientation INTEGER,
     raw_fields TEXT,
-    PRIMARY KEY (asset_id, zone_id, local_path)
+    PRIMARY KEY (asset_id, zone_id, asset_resource)
 );
 CREATE INDEX IF NOT EXISTS idx_manifest_path ON manifest(local_path);
 """
@@ -76,6 +78,7 @@
     (2, "ALTER TABLE manifest ADD COLUMN burst_id TEXT"),
     (2, "ALTER TABLE manifest ADD COLUMN original_orientation INTEGER"),
     (2, "ALTER TABLE manifest ADD COLUMN raw_fields TEXT"),
+    (3, "ALTER TABLE manifest ADD COLUMN asset_resource TEXT NOT NULL DEFAULT 'resOriginal'"),
 ]
 
 
@@ -85,6 +88,7 @@ class ManifestRow:
 
     asset_id: str
     zone_id: str
+    asset_resource: str
     local_path: str
     version_size: int
     version_checksum: str | None
@@ -121,7 +125,7 @@ class ManifestRow:
 
 
 _ALL_COLUMNS = (
-    "asset_id, zone_id, local_path, version_size, version_checksum, "
+    "asset_id, zone_id, asset_resource, local_path, version_size, version_checksum, "
     "change_tag, downloaded_at, last_updated_at, item_type, filename, "
     "asset_date, added_date, is_favorite, is_hidden, is_deleted, "
     "original_width, original_height, duration, orientation, "
@@ -211,12 +215,14 @@ def _migrate_from_v0(self) -> None:
             ("burst_id", "TEXT"),
             ("original_orientation", "INTEGER"),
             ("raw_fields", "TEXT"),
+            ("asset_resource", "TEXT NOT NULL DEFAULT 'resOriginal'"),
         ]
         for col_name, col_def in new_columns:
             if col_name not in existing:
                 self._conn.execute(f"ALTER TABLE manifest ADD COLUMN {col_name} {col_def}")  # type: ignore[union-attr]
         logger.info("Migrated manifest DB from v0 to v%d (%d columns added)",
                      SCHEMA_VERSION, sum(1 for c, _ in new_columns if c not in existing))
+        self._rebuild_pk()
         self._conn.execute(  # type: ignore[union-attr]
             "CREATE INDEX IF NOT EXISTS idx_manifest_path ON manifest(local_path)"
         )
@@ -225,10 +231,28 @@ def _run_migrations(self, from_version: int) -> None:
         """Run incremental migrations from from_version to SCHEMA_VERSION."""
         for version, sql in _MIGRATIONS:
             if version > from_version:
-                self._conn.execute(sql)  # type: ignore[union-attr]
+                with contextlib.suppress(sqlite3.OperationalError):
+                    self._conn.execute(sql)  # type: ignore[union-attr]
+        if from_version < 3:
+            self._rebuild_pk()
         self._conn.execute(f"PRAGMA user_version={SCHEMA_VERSION}")  # type: ignore[union-attr]
         self._conn.commit()  # type: ignore[union-attr]
 
+    def _rebuild_pk(self) -> None:
+        """Rebuild the manifest table with the correct PK (asset_id, zone_id, asset_resource)."""
+        conn = self._conn
+        assert conn is not None
+        conn.execute("ALTER TABLE manifest RENAME TO manifest_old")
+        conn.executescript(_FRESH_SCHEMA)
+        # Copy data, keeping only one row per (asset_id, zone_id, asset_resource)
+        cols = _ALL_COLUMNS
+        conn.execute(
+            f"INSERT OR IGNORE INTO manifest ({cols}) "
+            f"SELECT {cols} FROM manifest_old"
+        )
+        conn.execute("DROP TABLE manifest_old")
+        conn.commit()
+
     def close(self) -> None:
         """Close the manifest DB, committing any pending writes."""
         if self._conn:
@@ -265,12 +289,12 @@ def __enter__(self) -> "ManifestDB":
     def __exit__(self, *_: object) -> None:
         self.close()
 
-    def lookup(self, asset_id: str, zone_id: str, local_path: str) -> ManifestRow | None:
+    def lookup(self, asset_id: str, zone_id: str, asset_resource: str) -> ManifestRow | None:
         """Look up a manifest entry by identity."""
         row = self._db.execute(
             f"SELECT {_ALL_COLUMNS} FROM manifest "
-            "WHERE asset_id = ? AND zone_id = ? AND local_path = ?",
-            (asset_id, zone_id, local_path),
+            "WHERE asset_id = ? AND zone_id = ? AND asset_resource = ?",
+            (asset_id, zone_id, asset_resource),
         ).fetchone()
         if row is None:
             return None
@@ -293,6 +317,7 @@ def upsert(
         zone_id: str,
         local_path: str,
         version_size: int,
+        asset_resource: str = "resOriginal",
         version_checksum: str | None = None,
         change_tag: str | None = None,
         item_type: str | None = None,
@@ -326,7 +351,7 @@ def upsert(
         """Insert or update a manifest entry. Auto-flushes every 500 writes."""
         now = datetime.now(tz=timezone.utc).isoformat()
         params = (
-            asset_id, zone_id, local_path, version_size, version_checksum,
+            asset_id, zone_id, asset_resource, local_path, version_size, version_checksum,
             change_tag, now, now, item_type, filename,
             asset_date, added_date, is_favorite, is_hidden, is_deleted,
             original_width, original_height, duration, orientation,
@@ -336,8 +361,9 @@ def upsert(
         )
         sql = (
             f"INSERT INTO manifest ({_ALL_COLUMNS}) "
-            "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) "
-            "ON CONFLICT(asset_id, zone_id, local_path) DO UPDATE SET "
+            "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) "
+            "ON CONFLICT(asset_id, zone_id, asset_resource) DO UPDATE SET "
+            "local_path=excluded.local_path, "
             "version_size=excluded.version_size, "
             "version_checksum=excluded.version_checksum, "
             "change_tag=excluded.change_tag, "
@@ -392,21 +418,21 @@ def upsert(
                 logger.warning("Manifest write failed for %s: %s", local_path, e)
                 return
 
-    def update_path(self, asset_id: str, zone_id: str, old_path: str, new_path: str) -> None:
+    def update_path(self, asset_id: str, zone_id: str, asset_resource: str, new_path: str) -> None:
         """Update local_path for an existing manifest entry."""
         try:
             self._db.execute(
                 "UPDATE manifest SET local_path = ?, last_updated_at = ? "
-                "WHERE asset_id = ? AND zone_id = ? AND local_path = ?",
+                "WHERE asset_id = ? AND zone_id = ? AND asset_resource = ?",
                 (new_path, datetime.now(tz=timezone.utc).isoformat(),
-                 asset_id, zone_id, old_path),
+                 asset_id, zone_id, asset_resource),
             )
             self._dirty = True
             self._pending_count += 1
         except sqlite3.Error as e:
             logger.warning(
                 "Manifest path update failed for %s -> %s: %s",
-                old_path, new_path, e,
+                asset_resource, new_path, e,
             )
 
     def count_by_path(self, local_path: str) -> int:
@@ -417,11 +443,11 @@ def count_by_path(self, local_path: str) -> int:
         ).fetchone()
         return row[0] if row else 0
 
-    def remove(self, asset_id: str, zone_id: str, local_path: str) -> None:
+    def remove(self, asset_id: str, zone_id: str, asset_resource: str) -> None:
         """Remove a manifest entry."""
         self._db.execute(
-            "DELETE FROM manifest WHERE asset_id = ? AND zone_id = ? AND local_path = ?",
-            (asset_id, zone_id, local_path),
+            "DELETE FROM manifest WHERE asset_id = ? AND zone_id = ? AND asset_resource = ?",
+            (asset_id, zone_id, asset_resource),
         )
         self._dirty = True
 
diff --git a/src/icloudpd/metadata_writer.py b/src/icloudpd/metadata_writer.py
@@ -210,8 +210,7 @@ def extract_metadata_update(
         gps_latitude = xmp_metadata.GPSLatitude if xmp_metadata.GPSLatitude is not None else None
         gps_longitude = xmp_metadata.GPSLongitude if xmp_metadata.GPSLongitude is not None else None
         gps_altitude = xmp_metadata.GPSAltitude if xmp_metadata.GPSAltitude is not None else None
-        # horzAcc is not in XMPMetadata — extract from raw locationEnc
-        gps_h_accuracy = _extract_h_accuracy(asset_record)
+        gps_h_accuracy = xmp_metadata.GPSHPositioningError
         # Extract created_date for datetime/dates categories
         created_date_val = None
         if xmp_metadata.CreateDate:
diff --git a/src/pyicloud_ipd/version_size.py b/src/pyicloud_ipd/version_size.py
@@ -22,3 +22,22 @@ def __str__(self) -> str:
 
 
 VersionSize = AssetVersionSize | LivePhotoVersionSize
+
+
+# Maps VersionSize enums to Apple's iCloud resource field prefixes.
+# These prefixes are stable identifiers from the CPLMaster record.
+_VERSION_TO_RESOURCE: dict[VersionSize, str] = {
+    AssetVersionSize.ORIGINAL: "resOriginal",
+    AssetVersionSize.ALTERNATIVE: "resOriginalAlt",
+    AssetVersionSize.ADJUSTED: "resJPEGFull",
+    AssetVersionSize.MEDIUM: "resJPEGMed",
+    AssetVersionSize.THUMB: "resJPEGThumb",
+    LivePhotoVersionSize.ORIGINAL: "resOriginalVidCompl",
+    LivePhotoVersionSize.MEDIUM: "resVidMed",
+    LivePhotoVersionSize.THUMB: "resVidSmall",
+}
+
+
+def version_to_resource(version_size: VersionSize) -> str:
+    """Map a VersionSize enum to its Apple resource field prefix."""
+    return _VERSION_TO_RESOURCE[version_size]
diff --git a/tests/test_manifest.py b/tests/test_manifest.py
diff --git a/tests/test_metadata_writer.py b/tests/test_metadata_writer.py
