From 30337a6c11c112d08dcabad94cbf7e8d3ef61aa9 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 08:48:46 -0400 Subject: [PATCH] fix: library paths follow the artist's slug, not the import folder's name (4244) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 1 of milestone #421. The images tree has 57 directory families for what the database says are single artists — `Conto`/`conto`, `InCaseArt`/`incaseart`, `StickySpoodge`/`Stickyspoodge`/`stickyspoodge`, and so on down to a four-way split for Pocket Ace Games. There was never a duplicate Artist row. `/api/artists/names` returns exactly one per artist. The files simply get written to two places for one row: `_copy_to_library` built its destination from `derive_subdir`, which mirrors the IMPORT tree's folder name verbatim, while the download path leaves files where the ingester wrote them — under the slug. Two writers, two conventions, one artist. This is the half that stops it re-growing, and it has to land before anything moves existing files: consolidate first and the next filesystem import out of a capitalised folder re-creates the directory that was just emptied. `canonical_subdir` replaces the top-level segment with the artist's slug and leaves everything below it alone — the post hierarchy is the downloader's business. Two deliberate pass-throughs: no resolved artist (nothing authoritative to canonicalise against) and an empty subdir (a file at the images root, whose fate is task #4247, not a side effect of this helper). `_supersede` resolves the KEPT row's artist for the same reason — a supersede rewrites `existing.path`, so writing it anywhere else would move a row back out of the tree being consolidated. ImageRecord carries `artist_id` with no relationship attribute, so that is a session lookup rather than an attribute. test_import_one_happy_path pinned the old `Alice/` destination and now pins `alice/` (rule 90). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LVjrnpQjRgHdvq95rASoiR --- backend/app/services/importer.py | 29 +++++++++++++++++++++---- backend/app/utils/paths.py | 31 +++++++++++++++++++++++++++ tests/test_importer.py | 28 ++++++++++++++++++++++++- tests/test_paths.py | 36 ++++++++++++++++++++++++++++++++ 4 files changed, 119 insertions(+), 5 deletions(-) diff --git a/backend/app/services/importer.py b/backend/app/services/importer.py index f4c0142..831966a 100644 --- a/backend/app/services/importer.py +++ b/backend/app/services/importer.py @@ -35,6 +35,7 @@ from ..utils import safe_probe from ..utils.paths import ( derive_subdir, derive_top_level_artist, + canonical_subdir, filehash_from_url, hash_suffixed_name, safe_ext, @@ -944,7 +945,7 @@ class Importer: ) return ImportResult(status="superseded", image_id=match_id) - dest = self._copy_to_library(source, sha, attribution_path) + dest = self._copy_to_library(source, sha, attribution_path, path_artist) record = ImageRecord( path=str(dest), @@ -1600,7 +1601,8 @@ class Importer: ) def _copy_to_library( - self, source: Path, sha: str, attribution_path: Path + self, source: Path, sha: str, attribution_path: Path, + artist: Artist | None = None, ) -> Path: """Copy `source` to its final library path. Returns the destination. @@ -1608,8 +1610,18 @@ class Importer: _import_media (filesystem scan) and _supersede (when new_path is not passed). FC-3c's attach_in_place skips this helper entirely — the file is already at its final home. + + `artist`, when resolved, decides the top-level directory: the + library is keyed on the Artist row's slug, NOT on however the + import folder happened to be capitalised. Without that, an import + from `/import/Conto/` and a download for the same artist write to + `Conto/` and `conto/` respectively and the library grows a second + home for one artist (milestone #421). """ - subdir = derive_subdir(attribution_path, self.import_root) + subdir = canonical_subdir( + derive_subdir(attribution_path, self.import_root), + artist.slug if artist else None, + ) dest_dir = self.images_root / subdir if subdir else self.images_root dest_dir.mkdir(parents=True, exist_ok=True) dest_name = hash_suffixed_name(source.stem, sha, source.suffix) @@ -1644,7 +1656,16 @@ class Importer: that path (FC-3c attach_in_place case) — skip the copy step. Otherwise the file is copied via _copy_to_library.""" if new_path is None: - dest = self._copy_to_library(source, sha, source) + # The KEPT row's artist decides the destination, not the incoming + # file's folder — a supersede rewrites `existing.path`, so writing + # it anywhere but that artist's canonical directory would move a + # row OUT of the tree milestone #421 is consolidating. ImageRecord + # carries `artist_id` with no relationship attribute, so this is a + # lookup rather than `existing.artist`. + kept_artist = artist + if kept_artist is None and existing.artist_id is not None: + kept_artist = self.session.get(Artist, existing.artist_id) + dest = self._copy_to_library(source, sha, source, kept_artist) else: dest = new_path diff --git a/backend/app/utils/paths.py b/backend/app/utils/paths.py index 7130e90..1214161 100644 --- a/backend/app/utils/paths.py +++ b/backend/app/utils/paths.py @@ -60,6 +60,37 @@ def derive_subdir(source_path: Path, import_root: Path) -> str: return str(rel) if str(rel) != "." else "" +def canonical_subdir(subdir: str, artist_slug: str | None) -> str: + """`subdir` with its TOP-LEVEL segment replaced by the artist's slug. + + canonical_subdir("Conto/patreon", "conto") -> "conto/patreon" + canonical_subdir("Conto", "conto") -> "conto" + canonical_subdir("Conto/patreon", None) -> "Conto/patreon" + canonical_subdir("", "conto") -> "" + + `derive_subdir` mirrors the IMPORT tree's folder names verbatim, so a + filesystem import out of `/import/Conto/...` used to write + `/Conto/...` while the download path wrote `/conto/...` + for the very same Artist row. One artist, two directories, forever — 57 + such families had accumulated by 2026-09-21, and the database never had + duplicate artists at all (milestone #421). + + The slug is the canonical name because it is the Artist row's own + identifier: it is what `/api/artists` reports, what the ingesters already + write, and the one spelling that cannot vary with how a folder happened to + be capitalised on the way in. + + Two deliberate pass-throughs. NO artist resolved means there is nothing + authoritative to canonicalise against, and an EMPTY subdir is a file + landing at the images root — those have no artist folder to correct, and + what becomes of them is its own decision (task #4247), not a side effect + of this helper. + """ + if not artist_slug or not subdir: + return subdir + return str(Path(artist_slug, *Path(subdir).parts[1:])) + + def hash_suffixed_name(stem: str, sha256_hex: str, ext: str) -> str: """Builds 'stem__'. diff --git a/tests/test_importer.py b/tests/test_importer.py index 72df212..f8b7af8 100644 --- a/tests/test_importer.py +++ b/tests/test_importer.py @@ -58,10 +58,36 @@ def test_import_one_happy_path(importer, import_layout): result = importer.import_one(src) assert result.status == "imported" record = importer.session.get(ImageRecord, result.image_id) - assert record.path.startswith(str(images_root / "Alice")) + # Under the artist's SLUG, not the import folder's "Alice" (milestone + # #421) — the import tree's capitalisation is an accident of how the + # operator named a folder, and honouring it gave one artist two homes. + assert record.path.startswith(str(images_root / "alice")) assert record.path.endswith(".jpg") +def test_library_path_uses_the_artist_slug_not_the_folder_name( + importer, import_layout, +): + """The regression that grew 57 duplicate artist directories: an import + from `/import/Conto/...` wrote `/Conto/...` while the downloader + wrote `/conto/...` for the same Artist row.""" + import_root, images_root = import_layout + src = import_root / "StickySpoodge" / "patreon" / "a.jpg" + _make_jpeg(src) + result = importer.import_one(src) + + record = importer.session.get(ImageRecord, result.image_id) + artist = importer.session.execute( + select(Artist).where(Artist.slug == "stickyspoodge") + ).scalar_one() + assert artist.name == "StickySpoodge" + # Artist segment canonicalised; the post hierarchy below it untouched. + assert record.path.startswith( + str(images_root / "stickyspoodge" / "patreon") + ) + assert not Path(record.path).is_relative_to(images_root / "StickySpoodge") + + def test_folder_creates_artist_and_links_artist_id(importer, import_layout): # FC-2d-vii-c: folder import creates the Artist and sets the canonical # image_record.artist_id — no artist-kind Tag (that path was retired; diff --git a/tests/test_paths.py b/tests/test_paths.py index 4b52f97..705c845 100644 --- a/tests/test_paths.py +++ b/tests/test_paths.py @@ -1,6 +1,7 @@ from pathlib import Path from backend.app.utils.paths import ( + canonical_subdir, derive_subdir, derive_top_level_artist, filehash_from_url, @@ -59,3 +60,38 @@ def test_derive_top_level_artist_nested(): def test_derive_top_level_artist_root(): assert derive_top_level_artist(Path("/import/x.png"), IMPORT) is None + + +# --- canonical_subdir (milestone #421) -------------------------------------- + + +def test_canonical_subdir_replaces_the_top_segment(): + assert canonical_subdir("Conto/patreon", "conto") == "conto/patreon" + assert canonical_subdir("Conto", "conto") == "conto" + + +def test_canonical_subdir_keeps_everything_below_the_artist(): + """Only the artist segment is authoritative — post folders below it are + the downloader's business and must survive untouched.""" + assert canonical_subdir( + "Big Bang/patreon/2026-01-02_123_A Post", "big-bang" + ) == "big-bang/patreon/2026-01-02_123_A Post" + + +def test_canonical_subdir_passes_through_without_an_artist(): + """No resolved artist means nothing authoritative to canonicalise against, + so the import folder's own name stands.""" + assert canonical_subdir("Conto/patreon", None) == "Conto/patreon" + assert canonical_subdir("Conto", "") == "Conto" + + +def test_canonical_subdir_leaves_the_images_root_alone(): + """An empty subdir is a file landing at the root — there is no artist + folder to correct, and what becomes of those is task #4247's call.""" + assert canonical_subdir("", "conto") == "" + assert canonical_subdir("", None) == "" + + +def test_canonical_subdir_is_idempotent(): + once = canonical_subdir("Conto/patreon", "conto") + assert canonical_subdir(once, "conto") == once