fix: library paths follow the artist's slug, not the import folder's name (4244)
CI / lint (push) Failing after 2s
CI / extension-version (push) Successful in 2s
Build images / sign-extension (push) Successful in 4s
Build images / build-agent (push) Successful in 6s
CI / frontend-build (push) Successful in 23s
CI / backend-lint-and-test (push) Successful in 33s
Build images / build-web (push) Successful in 1m11s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 2m4s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m50s
CI / lint (push) Failing after 2s
CI / extension-version (push) Successful in 2s
Build images / sign-extension (push) Successful in 4s
Build images / build-agent (push) Successful in 6s
CI / frontend-build (push) Successful in 23s
CI / backend-lint-and-test (push) Successful in 33s
Build images / build-web (push) Successful in 1m11s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 2m4s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m50s
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVjrnpQjRgHdvq95rASoiR
This commit is contained in:
@@ -35,6 +35,7 @@ from ..utils import safe_probe
|
|||||||
from ..utils.paths import (
|
from ..utils.paths import (
|
||||||
derive_subdir,
|
derive_subdir,
|
||||||
derive_top_level_artist,
|
derive_top_level_artist,
|
||||||
|
canonical_subdir,
|
||||||
filehash_from_url,
|
filehash_from_url,
|
||||||
hash_suffixed_name,
|
hash_suffixed_name,
|
||||||
safe_ext,
|
safe_ext,
|
||||||
@@ -944,7 +945,7 @@ class Importer:
|
|||||||
)
|
)
|
||||||
return ImportResult(status="superseded", image_id=match_id)
|
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(
|
record = ImageRecord(
|
||||||
path=str(dest),
|
path=str(dest),
|
||||||
@@ -1600,7 +1601,8 @@ class Importer:
|
|||||||
)
|
)
|
||||||
|
|
||||||
def _copy_to_library(
|
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:
|
) -> Path:
|
||||||
"""Copy `source` to its final library path. Returns the destination.
|
"""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
|
_import_media (filesystem scan) and _supersede (when new_path is
|
||||||
not passed). FC-3c's attach_in_place skips this helper entirely
|
not passed). FC-3c's attach_in_place skips this helper entirely
|
||||||
— the file is already at its final home.
|
— 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 = self.images_root / subdir if subdir else self.images_root
|
||||||
dest_dir.mkdir(parents=True, exist_ok=True)
|
dest_dir.mkdir(parents=True, exist_ok=True)
|
||||||
dest_name = hash_suffixed_name(source.stem, sha, source.suffix)
|
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.
|
that path (FC-3c attach_in_place case) — skip the copy step.
|
||||||
Otherwise the file is copied via _copy_to_library."""
|
Otherwise the file is copied via _copy_to_library."""
|
||||||
if new_path is None:
|
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:
|
else:
|
||||||
dest = new_path
|
dest = new_path
|
||||||
|
|
||||||
|
|||||||
@@ -60,6 +60,37 @@ def derive_subdir(source_path: Path, import_root: Path) -> str:
|
|||||||
return str(rel) if str(rel) != "." else ""
|
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
|
||||||
|
`<images_root>/Conto/...` while the download path wrote `<root>/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:
|
def hash_suffixed_name(stem: str, sha256_hex: str, ext: str) -> str:
|
||||||
"""Builds 'stem__<first10ofhash><ext>'.
|
"""Builds 'stem__<first10ofhash><ext>'.
|
||||||
|
|
||||||
|
|||||||
+27
-1
@@ -58,10 +58,36 @@ def test_import_one_happy_path(importer, import_layout):
|
|||||||
result = importer.import_one(src)
|
result = importer.import_one(src)
|
||||||
assert result.status == "imported"
|
assert result.status == "imported"
|
||||||
record = importer.session.get(ImageRecord, result.image_id)
|
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")
|
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 `<images>/Conto/...` while the downloader
|
||||||
|
wrote `<images>/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):
|
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
|
# 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;
|
# image_record.artist_id — no artist-kind Tag (that path was retired;
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
from backend.app.utils.paths import (
|
from backend.app.utils.paths import (
|
||||||
|
canonical_subdir,
|
||||||
derive_subdir,
|
derive_subdir,
|
||||||
derive_top_level_artist,
|
derive_top_level_artist,
|
||||||
filehash_from_url,
|
filehash_from_url,
|
||||||
@@ -59,3 +60,38 @@ def test_derive_top_level_artist_nested():
|
|||||||
|
|
||||||
def test_derive_top_level_artist_root():
|
def test_derive_top_level_artist_root():
|
||||||
assert derive_top_level_artist(Path("/import/x.png"), IMPORT) is None
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user