From a534b1b70b9d9632346b89b855e569d3a6856bd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=BE=9D=E7=91=AA=E8=B2=93?= Date: Tue, 4 Aug 2026 08:44:21 +0800 Subject: [PATCH] Move the invariant checks into the importers and drop find_violations Co-Authored-By: Claude Fable 5 --- docs/decision_log.md | 8 ++ tools/src/pop_fem_audit_tools/build_db.py | 146 ++++++++++++---------- tools/tests/test_build_db.py | 53 +++++++- 3 files changed, 142 insertions(+), 65 deletions(-) diff --git a/docs/decision_log.md b/docs/decision_log.md index b36c5b9..3baccc5 100644 --- a/docs/decision_log.md +++ b/docs/decision_log.md @@ -216,6 +216,14 @@ 拆成個人後，男女混合是歌曲層（song_artists＋各歌手 gender）可推導的事實，不屬歌手實體。非人非團體者 （Pinkfong）type 留空由人工判定，維持現狀。 +- **獨立驗證步驟解散，不變量檢查歸屬各匯入工作**：榜單 + 覆蓋（(year, rank) 網格恰好齊全、無缺漏、無多出、無 + 重複）檢在 `SongImporter` 匯入結尾；署名解析結果（至少 + 一人、含 primary、名字非空白）檢在 `ArtistImporter` 逐筆 + 解析後立即失敗。違規拋 `BuildError` 走既有的失敗路徑， + `find_violations` 刪除。理由：檢查跟著產生資料的工作走， + main 不再有驗證分支；逐筆即時失敗使錯誤指向出錯的那筆 + 署名。 - **fetch-lyrics 移除缺漏報表**：刪去 `missing_csv` 位置引數 與缺漏報表 CSV 輸出。理由：該報表只寫不讀（指令從未讀取 它），缺漏數字每次執行皆由工作儲存比對歌詞目錄重新算出； diff --git a/tools/src/pop_fem_audit_tools/build_db.py b/tools/src/pop_fem_audit_tools/build_db.py index e1860ca..8a27b8b 100644 --- a/tools/src/pop_fem_audit_tools/build_db.py +++ b/tools/src/pop_fem_audit_tools/build_db.py @@ -50,6 +50,7 @@ import argparse import csv import re import sys +from collections import Counter from collections.abc import Iterable, Sequence from dataclasses import dataclass from pathlib import Path @@ -67,10 +68,6 @@ from .models import ( SongArtist, ) -YEARS: Sequence[int] = range(2016, 2026) -"""The expected chart years.""" -RANKS_PER_YEAR: int = 100 -"""The expected number of ranks on the chart of each year.""" ARTIST_FIELDS: dict[str, str] = { "qid": "wikidata_qid", "gender": "gender", @@ -114,6 +111,10 @@ class SongImporter: """The song-import job: loads the chart CSV into songs and chart entries.""" + YEARS: Sequence[int] = range(2016, 2026) + """The expected chart years.""" + RANKS_PER_YEAR: int = 100 + """The expected number of ranks on the chart of each year.""" CANONICAL_ARTIST_CREDITS: dict[str, str] = { "benny blanco, Halsey & Khalid": "Benny Blanco, Halsey" " & Khalid", @@ -144,8 +145,12 @@ class SongImporter: :param path: The chart CSV file with the columns year, rank, title, and artist. :return: None. + :raises BuildError: When the chart entries do not cover + each of ``YEARS`` and each rank from 1 to + ``RANKS_PER_YEAR`` exactly once. :raises OSError: When the file cannot be read. """ + counts: Counter[tuple[int, int]] = Counter() with open(path, encoding="utf-8", newline="") as file: row: dict[str, str] for row in csv.DictReader(file): @@ -160,10 +165,48 @@ class SongImporter: artist_credit=credit) self.__session.add(song) self.__songs[key] = song - self.__session.add(ChartEntry( - year=int(row["year"]), rank=int(row["rank"]), - song=self.__songs[key])) + year: int = int(row["year"]) + rank: int = int(row["rank"]) + counts[(year, rank)] += 1 + if counts[(year, rank)] == 1: + self.__session.add(ChartEntry( + year=year, rank=rank, + song=self.__songs[key])) self.__session.flush() + self.__check_chart_coverage(counts) + + @classmethod + def __check_chart_coverage( + cls, counts: Counter[tuple[int, int]]) -> None: + """Verify the chart entries cover the expected grid exactly. + + :param counts: The number of chart entries seen for each + (year, rank) pair. + :return: None. + :raises BuildError: When a (year, rank) pair from the + expected grid is missing, an unexpected pair is + present, or a pair is duplicated. + """ + expected: set[tuple[int, int]] = { + (year, rank) for year in cls.YEARS + for rank in range(1, cls.RANKS_PER_YEAR + 1)} + actual: set[tuple[int, int]] = set(counts) + violations: list[str] = [] + year: int + rank: int + for year, rank in sorted(expected - actual): + violations.append( + f"missing chart entry: year {year} rank {rank}") + for year, rank in sorted(actual - expected): + violations.append( + f"unexpected chart entry: year {year} rank {rank}") + for year, rank in sorted(counts): + if counts[(year, rank)] > 1: + violations.append( + f"duplicated chart entry: year {year} rank" + f" {rank}") + if len(violations) > 0: + raise BuildError("\n".join(violations)) @staticmethod def song_identity(title: str, credit: str) -> tuple[str, str]: @@ -306,13 +349,18 @@ class ArtistImporter: :param song: The song with its stored artist credit. :return: None. + :raises BuildError: When the parsed credit has no primary + artist or contains a blank artist name (see + `__check_parsed_credit`). """ + parsed: list[tuple[str, Role]] = self.parse_artist_credit( + song.artist_credit) + self.__check_parsed_credit(song, parsed) seen: set[str] = set() position: int = 0 name: str role: Role - for name, role in self.parse_artist_credit( - song.artist_credit): + for name, role in parsed: key: str stored_name: str key, stored_name = self.resolve_artist_identity(name) @@ -329,6 +377,32 @@ class ArtistImporter: position=position)) position += 1 + @staticmethod + def __check_parsed_credit( + song: Song, parsed: list[tuple[str, Role]]) -> None: + """Verify a song's parsed artist credit is well-formed. + + :param song: The song whose credit was parsed. + :param parsed: The (name, role) pairs parsed from + ``song.artist_credit``. + :return: None. + :raises BuildError: When ``parsed`` is empty, has no + ``Role.PRIMARY`` entry, or contains a name blank after + stripping. + """ + role: Role + if len(parsed) == 0 or not any( + role == Role.PRIMARY for _, role in parsed): + raise BuildError( + f"song {song.id} \"{song.artist_credit}\": no" + " primary artist parsed") + name: str + for name, role in parsed: + if name.strip() == "": + raise BuildError( + f"song {song.id} \"{song.artist_credit}\":" + " blank artist name parsed") + @staticmethod def parse_artist_credit(credit: str) -> list[tuple[str, Role]]: """Parse a combined artist credit into artists and roles. @@ -515,53 +589,6 @@ class CaptureImporter: setattr(artist, attribute, row[column]) -def find_violations(session: Session, years: Iterable[int], - ranks_per_year: int) -> list[str]: - """Find the invariant violations in the loaded data. - - The invariants: the chart entries cover each expected year - and rank exactly once and nothing else, every song has at - least one primary artist, and every artist name is non-empty. - - :param session: The database session with the loaded data - flushed. - :param years: The expected chart years. - :param ranks_per_year: The expected number of ranks per year. - :return: The violation messages, empty when the data is - valid. - """ - violations: list[str] = [] - expected: set[tuple[int, int]] = { - (year, rank) for year in years - for rank in range(1, ranks_per_year + 1)} - actual: set[tuple[int, int]] = { - (x.year, x.rank) - for x in session.scalars(sa.select(ChartEntry))} - year: int - rank: int - for year, rank in sorted(expected - actual): - violations.append( - f"missing chart entry: year {year} rank {rank}") - for year, rank in sorted(actual - expected): - violations.append( - f"unexpected chart entry: year {year} rank {rank}") - primary_ids: set[int] = set(session.scalars( - sa.select(SongArtist.song_id) - .where(SongArtist.role == Role.PRIMARY))) - song: Song - for song in session.scalars(sa.select(Song).order_by(Song.id)): - if song.id not in primary_ids: - violations.append( - f"song {song.id} \"{song.title}\" has no primary" - " artist") - artist: Artist - for artist in session.scalars(sa.select(Artist)): - if artist.name.strip() == "": - violations.append( - f"artist {artist.id} has an empty name") - return violations - - @dataclass class StoreCounts: """The row counts of the working store, for the build summary.""" @@ -803,13 +830,6 @@ def main(argv: list[str] | None = None) -> int: ArtistImporter(session).import_artists() CaptureImporter(session).import_captures( args.lyrics_dir, args.wikidata_csv) - violations: list[str] = find_violations( - session, YEARS, RANKS_PER_YEAR) - if len(violations) > 0: - session.rollback() - for violation in violations: - print(f"error: {violation}", file=sys.stderr) - return 1 counts = StoreCounts.get_instance(session) CSVExporter(session, args.derived_dir).write() session.commit() diff --git a/tools/tests/test_build_db.py b/tools/tests/test_build_db.py index 348feb0..4c1beb2 100644 --- a/tools/tests/test_build_db.py +++ b/tools/tests/test_build_db.py @@ -273,8 +273,10 @@ class TestBuildDB(unittest.TestCase): self.__ds: DataSource = DataSource() patchers: list[Any] = [ mock.patch.object(build_db, "ds", self.__ds), - mock.patch.object(build_db, "YEARS", [2016, 2017]), - mock.patch.object(build_db, "RANKS_PER_YEAR", 2)] + mock.patch.object( + build_db.SongImporter, "YEARS", [2016, 2017]), + mock.patch.object( + build_db.SongImporter, "RANKS_PER_YEAR", 2)] for patcher in patchers: patcher.start() self.addCleanup(patcher.stop) @@ -578,6 +580,53 @@ class TestBuildDB(unittest.TestCase): list(session.scalars(sa.select(ChartEntry))), []) self.assertEqual(list(session.scalars(sa.select(Song))), []) + def test_duplicate_chart_entry_fails(self) -> None: + """Test that a duplicated (year, rank) row fails without + partial data, even though the set of distinct (year, rank) + pairs still covers the expected grid exactly.""" + self.__write_chart( + "year,rank,title,artist\n" + "2016,1,Hello,Adele\n" + "2016,1,Hello,Adele\n" + "2016,2,One Dance,Drake featuring Wizkid\n" + "2017,1,One Dance,Drake featuring Wizkid\n" + "2017,2,Shape of You,Ed Sheeran\n") + status: int + stderr: str + status, stderr = self.__run_build() + self.assertNotEqual(status, 0) + self.assertIn( + "duplicated chart entry: year 2016 rank 1", stderr) + session: Session = self.__session() + self.assertEqual( + list(session.scalars(sa.select(ChartEntry))), []) + self.assertEqual(list(session.scalars(sa.select(Song))), []) + + def test_blank_artist_name_fails(self) -> None: + """Test that a parsed credit with a name blank after + stripping fails the build.""" + with mock.patch.object( + build_db.ArtistImporter, "parse_artist_credit", + return_value=[("Adele", Role.PRIMARY), + (" ", Role.FEATURED)]): + status: int + stderr: str + status, stderr = self.__run_build() + self.assertNotEqual(status, 0) + self.assertIn("blank artist name parsed", stderr) + + def test_no_primary_artist_fails(self) -> None: + """Test that a parsed credit without a primary artist + fails the build.""" + with mock.patch.object( + build_db.ArtistImporter, "parse_artist_credit", + return_value=[("Wizkid", Role.FEATURED)]): + status: int + stderr: str + status, stderr = self.__run_build() + self.assertNotEqual(status, 0) + self.assertIn("no primary artist parsed", stderr) + def test_lyrics_loaded(self) -> None: """Test loading the lyrics cache into the songs.""" self.__lyrics.mkdir()