diff --git a/src/server/SerieScanner.py b/src/server/SerieScanner.py index 2f38c5f..ec6f6c8 100644 --- a/src/server/SerieScanner.py +++ b/src/server/SerieScanner.py @@ -776,18 +776,35 @@ class SerieScanner: # Create or update AnimeSeries in keyDict if key in self.keyDict: - # Update existing anime - rebuild episodeDict from episodes + # Update existing anime - rebuild episodeDict from the + # latest scan results. The previous implementation + # extended the existing list with ``missing_episodes``, + # which accumulated duplicates across rescans of the + # same series; the in-memory cache then propagated + # duplicates through ``_update_series_in_db`` and into + # the ``episodes`` table until the UNIQUE constraint + # was added. Replace, don't extend. existing = self.keyDict[key] - existing_ep_dict = existing.episodeDict - # Merge missing episodes + # Use ``dict.fromkeys`` to dedupe within a season, in + # case ``missing_episodes`` itself contains duplicate + # episode numbers from a buggy loader upstream. + rebuilt: dict = {} for season, eps in missing_episodes.items(): - if season not in existing_ep_dict: - existing_ep_dict[season] = [] - existing_ep_dict[season].extend(eps) + seen: set = set() + cleaned: list = [] + for ep_num in eps: + if ep_num in seen: + continue + seen.add(ep_num) + cleaned.append(ep_num) + if cleaned: + rebuilt[season] = cleaned + existing.episodeDict = rebuilt + existing.folder = folder logger.debug( "Updated existing series %s with %d missing episodes", key, - sum(len(eps) for eps in missing_episodes.values()) + sum(len(eps) for eps in rebuilt.values()), ) else: # Extract year from folder name if present, otherwise leave as None diff --git a/src/server/database/models.py b/src/server/database/models.py index 26b92b9..287e573 100644 --- a/src/server/database/models.py +++ b/src/server/database/models.py @@ -15,7 +15,17 @@ from datetime import datetime, timezone from enum import Enum from typing import Any, Dict, List, Optional -from sqlalchemy import Boolean, DateTime, ForeignKey, Index, Integer, String, Text, func +from sqlalchemy import ( + Boolean, + DateTime, + ForeignKey, + Index, + Integer, + String, + Text, + UniqueConstraint, + func, +) from sqlalchemy.orm import Mapped, mapped_column, relationship, validates from src.server.database.base import Base, TimestampMixin @@ -328,7 +338,25 @@ class Episode(Base, TimestampMixin): updated_at: Last update timestamp (from TimestampMixin) """ __tablename__ = "episodes" - + + # Table-level constraints. The UNIQUE constraint on + # (series_id, season, episode_number) is the schema-level guard + # against the duplicate-row pathology: every (series, season, + # episode) tuple can have at most one row. Rescans that try to + # create a duplicate row will fail at the DB layer rather than + # silently accumulating rows. Defense-in-depth on top of the + # write-side dedup in SerieScanner._sync_episodes_to_db and + # AnimeService._update_series_in_db, and the read-boundary dedup + # in AnimeSeries.episodeDict. + __table_args__ = ( + UniqueConstraint( + "series_id", + "season", + "episode_number", + name="uq_episode_per_series_season", + ), + ) + # Primary key id: Mapped[int] = mapped_column( Integer, primary_key=True, autoincrement=True diff --git a/tests/unit/test_clean_duplicate_episodes_cli.py b/tests/unit/test_clean_duplicate_episodes_cli.py index c652695..8282012 100644 --- a/tests/unit/test_clean_duplicate_episodes_cli.py +++ b/tests/unit/test_clean_duplicate_episodes_cli.py @@ -10,7 +10,7 @@ from __future__ import annotations from typing import List import pytest -from sqlalchemy import select +from sqlalchemy import select, text from sqlalchemy.ext.asyncio import ( AsyncEngine, AsyncSession, @@ -29,14 +29,55 @@ from src.server.database.models import AnimeSeries, Episode async def in_memory_engine(): """Provide an in-memory async SQLite engine with the schema already created, and patch the connection module's session - factory to use it for the duration of the test.""" + factory to use it for the duration of the test. + + The CLI test suite simulates the *pre-migration* state — a DB + that predates the UNIQUE constraint on + ``(series_id, season, episode_number)`` and has accumulated + duplicate rows from historical scans. To create that state, we + build the schema for everything except ``episodes``, then + recreate ``episodes`` with raw DDL that omits the + ``uq_episode_per_series_season`` constraint. SQLite ties + UNIQUE constraints to an internal auto-named index that can't + be dropped directly — table recreation is the only way to + simulate the pre-migration schema. + """ engine: AsyncEngine = create_async_engine( "sqlite+aiosqlite:///:memory:", echo=False, poolclass=StaticPool, ) async with engine.begin() as conn: - await conn.run_sync(Base.metadata.create_all) + # Create everything except episodes. + await conn.run_sync( + lambda sync_conn: [ + t.create(sync_conn) + for t in Base.metadata.sorted_tables + if t.name != "episodes" + ] + ) + # Recreate episodes without the UNIQUE constraint. + await conn.execute( + text( + """ + CREATE TABLE episodes ( + id INTEGER NOT NULL PRIMARY KEY, + series_id INTEGER NOT NULL + REFERENCES anime_series(id) ON DELETE CASCADE, + season INTEGER NOT NULL, + episode_number INTEGER NOT NULL, + title VARCHAR(500), + file_path VARCHAR(1000), + is_downloaded BOOLEAN NOT NULL DEFAULT 0, + created_at DATETIME DEFAULT CURRENT_TIMESTAMP NOT NULL, + updated_at DATETIME DEFAULT CURRENT_TIMESTAMP NOT NULL + ) + """ + ) + ) + await conn.execute( + text("CREATE INDEX ix_episodes_series_id ON episodes (series_id)") + ) factory = async_sessionmaker( bind=engine, @@ -75,6 +116,10 @@ async def _add_episode( is_downloaded: bool = False, title: str | None = None, ) -> int: + """Insert an Episode row. The test fixture drops the UNIQUE + constraint on the ``episodes`` table, so duplicate inserts + succeed — the cleanup tool can then find them, mirroring the + pre-migration pathology it's meant to repair.""" ep = Episode( series_id=series_id, season=season, @@ -120,8 +165,11 @@ async def test_find_groups_duplicates_per_tuple(in_memory_engine): count=3 and keep_id = the lowest id.""" async with AsyncSession(in_memory_engine) as session: sid = await _add_series(session, "triple") - e1 = await _add_episode(session, sid, 1, 1) - # Three rows for (S1, E2): keep_id should be e2 (lowest) + await _add_episode(session, sid, 1, 1) + # Three rows for (S1, E2): the first via the normal path, + # the next two via raw SQL to bypass the UNIQUE constraint. + # Mirrors the pre-migration pathology the cleanup tool exists + # to repair. e2 = await _add_episode(session, sid, 1, 2) e3 = await _add_episode(session, sid, 1, 2) e4 = await _add_episode(session, sid, 1, 2) @@ -147,8 +195,12 @@ async def test_delete_keeps_lowest_id_per_tuple(in_memory_engine): sid = await _add_series(session, "keep-lowest") e1 = await _add_episode(session, sid, 1, 1) # unique e2 = await _add_episode(session, sid, 1, 2) # lowest of dupes - await _add_episode(session, sid, 1, 2) # e3 - duplicate of e2 - await _add_episode(session, sid, 1, 2) # e4 - duplicate of e2 + await _add_episode( + session, sid, 1, 2 + ) # e3 - duplicate of e2 + await _add_episode( + session, sid, 1, 2 + ) # e4 - duplicate of e2 e5 = await _add_episode(session, sid, 1, 3) # unique duplicates = await cli.find_duplicate_episodes() @@ -179,7 +231,11 @@ async def test_delete_keeps_lowest_even_with_title_metadata(in_memory_engine): sid = await _add_series(session, "metadata") e1 = await _add_episode(session, sid, 1, 1) # no title e2 = await _add_episode( - session, sid, 1, 1, title="Better Episode 1" + session, + sid, + 1, + 1, + title="Better Episode 1", ) duplicates = await cli.find_duplicate_episodes() @@ -203,8 +259,13 @@ async def test_full_workflow_is_idempotent(in_memory_engine): async with AsyncSession(in_memory_engine) as session: sid = await _add_series(session, "idempotent") for ep_num in (1, 2, 3): - for _ in range(3): # 3 copies of each - await _add_episode(session, sid, 1, ep_num) + await _add_episode(session, sid, 1, ep_num) + await _add_episode( + session, sid, 1, ep_num + ) + await _add_episode( + session, sid, 1, ep_num + ) first = await cli.find_duplicate_episodes() assert len(first) == 3 @@ -225,8 +286,10 @@ async def test_max_series_filter_limits_report(in_memory_engine): s3 = await _add_series(session, "gamma") for sid in (s1, s2, s3): for ep_num in (1, 2): - for _ in range(2): # duplicates - await _add_episode(session, sid, 1, ep_num) + await _add_episode(session, sid, 1, ep_num) + await _add_episode( + session, sid, 1, ep_num + ) # Without filter: all three series have duplicates. full = await cli.find_duplicate_episodes() diff --git a/tests/unit/test_database_models.py b/tests/unit/test_database_models.py index cbb0daa..e43fb7a 100644 --- a/tests/unit/test_database_models.py +++ b/tests/unit/test_database_models.py @@ -309,48 +309,109 @@ class TestAnimeSeries: class TestEpisodeDictDedup: - """Regression tests for the duplicate-episode-row guard. + """Regression tests for the ``AnimeSeries.episodeDict`` dedup. - The ``episodes`` table has no UNIQUE constraint on - ``(series_id, season, episode_number)``, so historical scans can - leave duplicate rows behind. ``AnimeSeries.episodeDict`` is the - read boundary consumed by ``list_missing()`` and the queue UI; - it must never expose duplicates, or the frontend forwards them - verbatim to the queue API and every duplicate is rejected as - "already pending" — the user sees an empty queue and a misleading - "Added N" toast. + The ``episodes`` table has a UNIQUE constraint on + ``(series_id, season, episode_number)`` (added as the schema-level + prevention in commit f75d591..), so duplicate rows cannot be + created via normal write paths. The property's defensive dedup + still matters for two reasons: + + 1. DBs that predate the UNIQUE constraint may have stale duplicate + rows from historical scans (visible in the user's backup DB + before clean_duplicate_episodes was run). + 2. ``_episode_dict_cache`` is populated directly by scanners and + loaders, which can carry duplicates from their internal logic + (e.g. ``scan_single_series`` previously extended the dict on + every rescan). + + These tests cover both paths. The class uses its own engine + fixture that drops the UNIQUE constraint after schema creation, + so the duplicate-row tests can set up the pre-migration state + the property's dedup is meant to defend against. """ - def _make_series(self, db_session: Session, key: str) -> AnimeSeries: + @pytest.fixture + def legacy_engine(self): + """In-memory SQLite engine without the UNIQUE constraint + on episodes — simulates a pre-migration DB. + + SQLite ties UNIQUE constraints to an internal auto-named + index that can't be dropped directly, so we rebuild the + episodes table with raw DDL that omits the + ``uq_episode_per_series_season`` constraint. The rest of + the schema comes from ``Base.metadata.create_all``. + """ + from sqlalchemy import text + + engine = create_engine("sqlite:///:memory:", echo=False) + # Create everything except the episodes table. + for table in Base.metadata.sorted_tables: + if table.name != "episodes": + table.create(engine) + # Recreate episodes without the UNIQUE constraint. + with engine.begin() as conn: + conn.execute( + text( + """ + CREATE TABLE episodes ( + id INTEGER NOT NULL PRIMARY KEY, + series_id INTEGER NOT NULL + REFERENCES anime_series(id) ON DELETE CASCADE, + season INTEGER NOT NULL, + episode_number INTEGER NOT NULL, + title VARCHAR(500), + file_path VARCHAR(1000), + is_downloaded BOOLEAN NOT NULL DEFAULT 0, + created_at DATETIME DEFAULT CURRENT_TIMESTAMP NOT NULL, + updated_at DATETIME DEFAULT CURRENT_TIMESTAMP NOT NULL + ) + """ + ) + ) + conn.execute( + text("CREATE INDEX ix_episodes_series_id ON episodes (series_id)") + ) + SessionLocal = sessionmaker(bind=engine) + session = SessionLocal() + yield session + session.close() + engine.dispose() + + def _make_series(self, session: Session, key: str) -> AnimeSeries: series = AnimeSeries( key=key, name=key.replace("-", " ").title(), site="https://aniworld.to", folder=f"/anime/{key}", ) - db_session.add(series) - db_session.commit() + session.add(series) + session.commit() return series def test_episodeDict_dedupes_duplicate_relationship_rows( - self, db_session: Session + self, legacy_engine: Session ): - """Duplicate Episode rows in the same series must not + """Duplicate Episode rows for the same series — including + ones that pre-date the UNIQUE constraint — must not duplicate the entries in ``episodeDict``.""" - series = self._make_series(db_session, "dedup-rel") + session = legacy_engine + series = self._make_series(session, "dedup-rel") - # Three duplicate rows for (S1, E3) — exactly the pattern the - # scanner accumulates across repeated rescans. + # Three duplicate rows for (S1, E3) — exactly the pattern + # the scanner accumulated across repeated rescans in the + # pre-migration era. With the constraint dropped in the + # legacy_engine fixture, these inserts succeed. for _ in range(3): - db_session.add( + session.add( Episode(series_id=series.id, season=1, episode_number=3) ) # One row each for the surrounding unique episodes. for ep in (1, 2, 4): - db_session.add( + session.add( Episode(series_id=series.id, season=1, episode_number=ep) ) - db_session.commit() + session.commit() result = series.episodeDict @@ -367,8 +428,7 @@ class TestEpisodeDictDedup: def test_episodeDict_excludes_downloaded_episodes( self, db_session: Session ): - """is_downloaded rows must still be filtered out, and a - duplicate non-downloaded row must not appear twice.""" + """is_downloaded rows must still be filtered out.""" series = self._make_series(db_session, "dedup-downloaded") db_session.add( @@ -388,10 +448,10 @@ class TestEpisodeDictDedup: assert series.episodeDict == {1: [1]} def test_episodeDict_dedupes_cached_value(self, db_session: Session): - """The legacy ``_episode_dict_cache`` path (used by - ``SerieScanner.scan_single_series`` for new series and by the - legacy JSON loader) must also dedupe, since loaders that set - the cache directly can introduce duplicates too.""" + """The legacy ``_episode_dict_cache`` path (set directly by + scanners and loaders) must also dedupe, since loaders can + populate the cache with duplicated data — historically + ``scan_single_series`` extended the dict on every rescan.""" series = self._make_series(db_session, "dedup-cache") # No episodes in the DB at all — the property will fall back @@ -408,7 +468,7 @@ class TestEpisodeDictDedup: seasons is legitimate and must be preserved.""" series = self._make_series(db_session, "multi-season") - for season, ep in [(1, 1), (1, 2), (2, 1), (2, 2), (2, 2)]: + for season, ep in [(1, 1), (1, 2), (2, 1), (2, 2)]: db_session.add( Episode( series_id=series.id, diff --git a/tests/unit/test_serie_scanner.py b/tests/unit/test_serie_scanner.py index a9338d9..157649f 100644 --- a/tests/unit/test_serie_scanner.py +++ b/tests/unit/test_serie_scanner.py @@ -194,15 +194,23 @@ class TestSerieScannerSingleSeries: def test_scan_single_series_existing_entry( self, temp_directory, mock_loader, sample_serie ): - """Test scan_single_series updates existing entry in keyDict.""" + """Test scan_single_series replaces the existing entry's + ``episodeDict`` with the new scan's missing-episode list. + + Note: the previous implementation ``extend````ed the + existing list with the new one, which accumulated + duplicates across rescans. The fix is to replace, not + extend — see ``test_serie_scanner_scan_dedup.py`` for the + regression tests for that specific bug. + """ scanner = SerieScanner(temp_directory, mock_loader) - + # Pre-populate keyDict scanner.keyDict[sample_serie.key] = sample_serie - # Use deepcopy because episodeDict is modified in-place + # Use deepcopy because episodeDict is mutated by the scanner. import copy old_episode_dict = copy.deepcopy(sample_serie.episodeDict) - + with patch.object( scanner, '_SerieScanner__get_missing_episodes_and_season', @@ -212,11 +220,16 @@ class TestSerieScannerSingleSeries: key=sample_serie.key, folder=sample_serie.folder ) - - # Verify existing entry was updated - episodeDict is merged (not replaced) - # Old episodes [2, 3, 4] + new episodes [10, 11, 12] = merged result - assert scanner.keyDict[sample_serie.key].episodeDict != old_episode_dict - assert scanner.keyDict[sample_serie.key].episodeDict == {1: [2, 3, 4, 10, 11, 12]} + + # The cached episodeDict is REPLACED with the latest + # scan's missing-episode list — not merged. Old entries + # ([2, 3, 4]) are dropped because the latest scan + # reports only [10, 11, 12] as still missing. + new_episode_dict = scanner.keyDict[ + sample_serie.key + ].episodeDict + assert new_episode_dict != old_episode_dict + assert new_episode_dict == {1: [10, 11, 12]} def test_scan_single_series_empty_key_raises_error( self, temp_directory, mock_loader diff --git a/tests/unit/test_serie_scanner_scan_dedup.py b/tests/unit/test_serie_scanner_scan_dedup.py new file mode 100644 index 0000000..af58d0d --- /dev/null +++ b/tests/unit/test_serie_scanner_scan_dedup.py @@ -0,0 +1,179 @@ +"""Regression test for ``SerieScanner.scan_single_series``. + +The previous implementation ``extend````ed the in-memory +``episodeDict`` for series already present in the scanner's +``keyDict`` — every rescan of the same series appended the new +missing-episode list on top of the existing one, so the dict grew +with duplicates across rescans. Those duplicates then propagated +through ``_update_series_in_db`` into the ``episodes`` table. + +This test exercises the real ``scan_single_series`` method (not a +mock) to assert that two rescans of the same series produce a +canonical (deduplicated) ``episodeDict``, not an accumulated one. +""" +from __future__ import annotations + +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from src.server.SerieScanner import SerieScanner + + +class _StubLoader: + """A minimal loader that returns whatever ``missing_episodes`` + the test wants. Replaces the real loader on a + ``SerieScanner`` instance via ``monkeypatch.setattr``.""" + + def __init__(self, missing_per_call: list[dict]): + self._missing_per_call = list(missing_per_call) + self._call_index = 0 + + def get_season_episode_count(self, key): + # The real loader returns the total episode count per + # season; ``scan_single_series`` doesn't actually use it + # (it calls ``__get_missing_episodes_and_season`` directly), + # but the stub keeps the call site safe. + max_seen = 0 + for m in self._missing_per_call: + for season, eps in m.items(): + if eps: + max_seen = max(max_seen, max(eps)) + return {1: max_seen or 1} + + def is_language(self, season, ep, key): + return True + + def next_missing(self) -> dict: + if self._call_index >= len(self._missing_per_call): + return {} + result = self._missing_per_call[self._call_index] + self._call_index += 1 + return result + + +def _make_scanner( + tmp_path: Path, + missing_per_call: list[dict], +) -> tuple[SerieScanner, _StubLoader]: + """Build a real ``SerieScanner`` with the private + ``__get_missing_episodes_and_season`` method replaced by a + stub that returns the next ``missing_episodes`` dict on each + call. Everything else (events, directory) is stubbed so the + test runs without filesystem or scheduler setup.""" + scanner = SerieScanner.__new__(SerieScanner) + loader = _StubLoader(missing_per_call) + scanner.loader = loader # type: ignore[assignment] + scanner.keyDict = {} + # ``self.directory`` is the attribute ``scan_single_series`` + # reads at line 734. ``scan_single_series`` checks + # ``os.path.isdir(folder_path)`` and, if the folder does not + # exist, treats the scan as "no MP4 files on disk". Use a path + # whose subdirectories do not exist so the scan takes the + # empty-mp4-files branch without us having to populate any + # filesystem state. + scanner.directory = str(tmp_path) + scanner.directory_to_search = tmp_path + scanner.events = SimpleNamespace( + on_progress=lambda *a, **k: None, + on_completion=lambda *a, **k: None, + on_error=lambda *a, **k: None, + ) + + def fake_get_missing_episodes_and_season(key, mp4_files): + return loader.next_missing(), "aniworld.to" + + scanner._SerieScanner__get_missing_episodes_and_season = ( # type: ignore[attr-defined] + fake_get_missing_episodes_and_season + ) + return scanner, loader + + +def test_scan_single_series_replaces_not_extends(tmp_path): + """Two rescans of the same series with the same missing + episodes must produce a canonical (deduplicated) episodeDict — + not a list with duplicates from accumulation.""" + canonical = {1: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]} + scanner, _ = _make_scanner( + tmp_path, + # Same missing-episode list reported on each scan. + missing_per_call=[canonical, canonical], + ) + + # First scan: key not in keyDict, so the else branch fires + # and the cache is set to the missing-episode list. + scanner.keyDict.clear() + result_first = scanner.scan_single_series( + key="erased", folder="Erased" + ) + assert result_first == canonical + cached = scanner.keyDict["erased"].episodeDict + assert cached == canonical + + # Second scan: key IS in keyDict, so the if branch fires. + # Before the fix, this would extend the cached list with + # [1..12] again, producing {1: [1..12, 1..12]}. After the + # fix, the cache is replaced, not extended. + result_second = scanner.scan_single_series( + key="erased", folder="Erased" + ) + assert result_second == canonical + cached_second = scanner.keyDict["erased"].episodeDict + assert cached_second == canonical, ( + "second scan extended the dict instead of replacing it: " + f"{cached_second}" + ) + + # Defensive: the dict has no duplicates within any season. + for season, eps in cached_second.items(): + assert len(eps) == len(set(eps)), ( + f"season {season} has duplicate episode numbers: {eps}" + ) + + +def test_scan_single_series_resets_when_becomes_complete(tmp_path): + """When a rescan finds the series is complete (no missing + episodes), the episodeDict must be empty — extending the + previous dict would leave stale entries behind.""" + scanner, _ = _make_scanner( + tmp_path, + # First scan: missing 1..12. Second scan: nothing missing. + missing_per_call=[ + {1: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]}, + {}, + ], + ) + + scanner.keyDict.clear() + scanner.scan_single_series(key="complete-me", folder="Complete Me") + assert scanner.keyDict["complete-me"].episodeDict == { + 1: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + } + + scanner.scan_single_series(key="complete-me", folder="Complete Me") + assert scanner.keyDict["complete-me"].episodeDict == {}, ( + "rescan with no missing episodes must reset the dict, " + f"got: {scanner.keyDict['complete-me'].episodeDict}" + ) + + +def test_scan_single_series_dedupes_within_a_single_call(tmp_path): + """If the loader itself returns duplicate episode numbers in + ``missing_episodes`` (a buggy upstream loader), the scanner + must still produce a canonical dict — defense in depth on top + of the loader and the read-boundary dedup in + ``AnimeSeries.episodeDict``.""" + scanner, _ = _make_scanner( + tmp_path, + # The loader returns the same episode numbers multiple + # times within a single call. + missing_per_call=[{1: [1, 1, 2, 2, 3, 3, 3, 4]}], + ) + + scanner.keyDict.clear() + scanner.scan_single_series(key="buggy", folder="Buggy") + cached = scanner.keyDict["buggy"].episodeDict + assert cached == {1: [1, 2, 3, 4]}, ( + f"single-scan dedup failed: {cached}" + ) \ No newline at end of file