diff --git a/README.md b/README.md index e29343b..623bfce 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,9 @@ Normal browser requests do **not** call the MLB Stats API. - Offline deterministic test suite and GitHub Actions CI Player visualizations, additional team metrics, multi-season overlays, and a more -organized Team/Player UI are planned but not implemented yet. +organized Team/Player UI are planned but not implemented yet. A season-level +Player catalog can be imported for future DB-only search and selection; see +[Player season catalog](docs/player-season-catalog.md). ## Screenshots @@ -112,6 +114,18 @@ Import an entire MLB season: poetry run python scripts/import_league_season.py --season 2025 ``` +Import the MLB player directory for a season, then import hitting data for an +individual player as needed: + +```bash +poetry run python scripts/import_player_catalog.py --season 2025 +poetry run python scripts/import_player_season.py --player-id 677594 --season 2025 +``` + +The Player catalog is one bulk request. It stores identities and season +memberships for future DB-only search; it does not fetch statistics for every +discovered player. + League imports record whether every discovered team was refreshed successfully. MLB-wide comparison statistics are only presented when the persisted coverage state supports describing the stored data as league-wide. diff --git a/alembic/versions/8b3f31d9a5c2_create_player_season_catalog.py b/alembic/versions/8b3f31d9a5c2_create_player_season_catalog.py new file mode 100644 index 0000000..58cbc15 --- /dev/null +++ b/alembic/versions/8b3f31d9a5c2_create_player_season_catalog.py @@ -0,0 +1,80 @@ +"""create_player_season_catalog + +Add the season-membership table used by Player discovery. Existing +``player_season_hitting`` rows are authoritative evidence that their players +belong to those seasons, so those memberships are backfilled without +fabricating any identity or statistic. + +Revision ID: 8b3f31d9a5c2 +Revises: 73d9fae8fafb +Create Date: 2026-09-09 00:00:00.000000 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "8b3f31d9a5c2" +down_revision: str | Sequence[str] | None = "73d9fae8fafb" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """Create player-season membership and backfill known hitting seasons.""" + op.create_table( + "player_seasons", + sa.Column("id", sa.Integer(), autoincrement=True, nullable=False), + sa.Column("player_id", sa.Integer(), nullable=False), + sa.Column("season", sa.Integer(), nullable=False), + sa.Column("created_at", sa.DateTime(), nullable=False), + sa.CheckConstraint( + "player_id > 0", name=op.f("ck_player_seasons_player_id_positive") + ), + sa.CheckConstraint( + "season > 0", name=op.f("ck_player_seasons_season_positive") + ), + sa.ForeignKeyConstraint( + ["player_id"], + ["players.player_id"], + name=op.f("fk_player_seasons_player_id_players"), + ), + sa.PrimaryKeyConstraint("id", name=op.f("pk_player_seasons")), + sa.UniqueConstraint( + "player_id", "season", name="uq_player_seasons_player_id_season" + ), + ) + op.create_index( + "ix_player_seasons_season_player_id", + "player_seasons", + ["season", "player_id"], + unique=False, + ) + + player_seasons = sa.table( + "player_seasons", + sa.column("player_id", sa.Integer()), + sa.column("season", sa.Integer()), + sa.column("created_at", sa.DateTime()), + ) + hitting = sa.table( + "player_season_hitting", + sa.column("player_id", sa.Integer()), + sa.column("season", sa.Integer()), + sa.column("created_at", sa.DateTime()), + ) + op.execute( + player_seasons.insert().from_select( + ["player_id", "season", "created_at"], + sa.select(hitting.c.player_id, hitting.c.season, hitting.c.created_at), + ) + ) + + +def downgrade() -> None: + """Remove Player directory membership while preserving identities and stats.""" + op.drop_index("ix_player_seasons_season_player_id", table_name="player_seasons") + op.drop_table("player_seasons") diff --git a/app/database/models.py b/app/database/models.py index b96492f..a397ab8 100644 --- a/app/database/models.py +++ b/app/database/models.py @@ -23,7 +23,11 @@ LeagueSeasonIngestionState, LeagueSeasonIngestionStatus, ) -from app.schemas.players import PlayerIdentity, PlayerSeasonHitting +from app.schemas.players import ( + PlayerIdentity, + PlayerSeasonCatalogEntry, + PlayerSeasonHitting, +) class TeamGameBattingLineRecord(Base): @@ -507,6 +511,61 @@ def from_domain( ) +class PlayerSeasonCatalogRecord(Base): + """Persistence representation of season membership in MLB's player directory. + + Identity attributes remain normalized in ``players``. This table stores + only the many-to-many fact needed for season-scoped discovery: MLB listed + this player for this Major League season. + """ + + __tablename__ = "player_seasons" + __table_args__ = ( + UniqueConstraint( + "player_id", "season", name="uq_player_seasons_player_id_season" + ), + CheckConstraint("player_id > 0", name="player_id_positive"), + CheckConstraint("season > 0", name="season_positive"), + Index("ix_player_seasons_season_player_id", "season", "player_id"), + ) + + id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) + player_id: Mapped[int] = mapped_column( + Integer, ForeignKey("players.player_id"), nullable=False + ) + season: Mapped[int] = mapped_column(Integer, nullable=False) + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=False), nullable=False + ) + + def to_domain(self, identity: PlayerIdentity) -> PlayerSeasonCatalogEntry: + """Combine this membership with its normalized player identity.""" + if identity.player_id != self.player_id: + raise ValueError( + f"identity player {identity.player_id} does not match catalog " + f"player {self.player_id}" + ) + return PlayerSeasonCatalogEntry( + player_id=identity.player_id, + full_name=identity.full_name, + primary_position=identity.primary_position, + season=self.season, + ) + + @staticmethod + def from_domain( + entry: PlayerSeasonCatalogEntry, + *, + created_at: datetime, + ) -> PlayerSeasonCatalogRecord: + """Build a new immutable season-membership row.""" + return PlayerSeasonCatalogRecord( + player_id=entry.player_id, + season=entry.season, + created_at=created_at, + ) + + class PlayerSeasonHittingRecord(Base): """Persistence representation of one player's full-season hitting aggregate. diff --git a/app/database/repositories.py b/app/database/repositories.py index 320d466..d78bf41 100644 --- a/app/database/repositories.py +++ b/app/database/repositories.py @@ -10,6 +10,7 @@ from app.database.models import ( LeagueSeasonIngestionRecord, PlayerRecord, + PlayerSeasonCatalogRecord, PlayerSeasonHittingRecord, TeamGameBattingLineRecord, TeamGamePitchingLineRecord, @@ -27,7 +28,11 @@ PlayerPersistenceOutcome, TeamGamePersistenceResult, ) -from app.schemas.players import PlayerIdentity, PlayerSeasonHitting +from app.schemas.players import ( + PlayerIdentity, + PlayerSeasonCatalogEntry, + PlayerSeasonHitting, +) # The two line tables the generic upsert below reconciles. They hold different # columns but expose the same to_domain / apply_domain / from_domain interface. @@ -504,6 +509,28 @@ def get_player(session: Session, *, player_id: int) -> PlayerIdentity | None: return None if record is None else record.to_domain() +def list_player_catalog( + session: Session, *, season: int +) -> list[PlayerSeasonCatalogEntry]: + """Return the locally stored MLB player directory for one season, by name. + + Ordering is applied to the domain entries rather than in SQL because the + database's default collation sorts accented names after every unaccented + one; see ``PlayerIdentity.name_sort_key``. + """ + stmt = ( + select(PlayerSeasonCatalogRecord, PlayerRecord) + .join( + PlayerRecord, + PlayerRecord.player_id == PlayerSeasonCatalogRecord.player_id, + ) + .where(PlayerSeasonCatalogRecord.season == season) + ) + rows = session.execute(stmt).all() + entries = [membership.to_domain(player.to_domain()) for membership, player in rows] + return sorted(entries, key=PlayerSeasonCatalogEntry.name_sort_key) + + def get_player_season_hitting( session: Session, *, @@ -539,6 +566,55 @@ def upsert_player( return PlayerPersistenceOutcome.UPDATED +def upsert_player_catalog_entry( + session: Session, + *, + entry: PlayerSeasonCatalogEntry, +) -> PlayerPersistenceOutcome: + """Upsert one logical catalog entry without committing or rolling back. + + Identity is normalized into ``players`` and season membership into + ``player_seasons``. The returned outcome describes the logical catalog + entry rather than either physical row in isolation. + """ + identity_outcome = upsert_player(session, identity=entry.to_identity()) + membership = _load_player_season_catalog( + session, player_id=entry.player_id, season=entry.season + ) + if membership is None: + now = datetime.now(UTC).replace(tzinfo=None) + session.add(PlayerSeasonCatalogRecord.from_domain(entry, created_at=now)) + return PlayerPersistenceOutcome.INSERTED + return identity_outcome + + +def ensure_player_season_catalog_membership( + session: Session, + *, + identity: PlayerIdentity, + season: int, +) -> None: + """Ensure a known player-season import is represented in the catalog. + + The caller remains responsible for persisting ``identity`` itself. This + focused helper exists so the one-player ingestion path can maintain the + membership invariant without changing what its identity outcome reports. + """ + existing = _load_player_season_catalog( + session, player_id=identity.player_id, season=season + ) + if existing is not None: + return + entry = PlayerSeasonCatalogEntry( + player_id=identity.player_id, + full_name=identity.full_name, + primary_position=identity.primary_position, + season=season, + ) + now = datetime.now(UTC).replace(tzinfo=None) + session.add(PlayerSeasonCatalogRecord.from_domain(entry, created_at=now)) + + def upsert_player_season_hitting( session: Session, *, @@ -575,6 +651,20 @@ def _load_player(session: Session, player_id: int) -> PlayerRecord | None: ).one_or_none() +def _load_player_season_catalog( + session: Session, + *, + player_id: int, + season: int, +) -> PlayerSeasonCatalogRecord | None: + return session.scalars( + select(PlayerSeasonCatalogRecord).where( + PlayerSeasonCatalogRecord.player_id == player_id, + PlayerSeasonCatalogRecord.season == season, + ) + ).one_or_none() + + def _load_player_season_hitting( session: Session, *, diff --git a/app/schemas/ingestion.py b/app/schemas/ingestion.py index 5606439..c4c5a63 100644 --- a/app/schemas/ingestion.py +++ b/app/schemas/ingestion.py @@ -337,3 +337,31 @@ class PlayerSeasonIngestionResult(BaseModel): full_name: str = Field(min_length=1) identity_outcome: PlayerPersistenceOutcome hitting_outcome: PlayerPersistenceOutcome + + +class PlayerCatalogIngestionResult(BaseModel): + """Outcome of refreshing one season's MLB player directory. + + Counts describe logical catalog entries. ``INSERTED`` means a new + ``(player_id, season)`` membership was stored. ``UPDATED`` means that + membership already existed but MLB supplied changed global identity data. + ``UNCHANGED`` means both membership and identity already matched. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + season: int = Field(gt=0) + players_discovered: int = Field(ge=1) + inserted: int = Field(ge=0) + updated: int = Field(ge=0) + unchanged: int = Field(ge=0) + + @model_validator(mode="after") + def _discovered_matches_counts(self) -> PlayerCatalogIngestionResult: + total = self.inserted + self.updated + self.unchanged + if self.players_discovered != total: + raise ValueError( + f"players_discovered ({self.players_discovered}) must equal " + f"inserted + updated + unchanged ({total})" + ) + return self diff --git a/app/schemas/players.py b/app/schemas/players.py index 3427d78..a19addc 100644 --- a/app/schemas/players.py +++ b/app/schemas/players.py @@ -2,6 +2,8 @@ from __future__ import annotations +import unicodedata + from pydantic import BaseModel, ConfigDict, Field, model_validator @@ -21,6 +23,48 @@ class PlayerIdentity(BaseModel): min_length=1, description="MLB-reported primary position abbreviation." ) + def name_sort_key(self) -> tuple[str, str, int]: + """Return this player's position in alphabetical-by-name display order. + + MLB rosters contain many accented names, and both of the orderings + available by default place them after every unaccented name: SQLite's + default collation compares raw bytes, and Python's ``casefold`` lowers + case without folding accents. Neither puts ``Ángel Martínez`` under A. + + Ordering therefore lives here rather than in an ``ORDER BY`` clause, so + that MLB discovery and the persisted catalog return the same players in + the same order. ``full_name`` breaks ties between names that fold + together (``Zoë`` and ``zoë``) and ``player_id`` makes the result total. + """ + decomposed = unicodedata.normalize("NFKD", self.full_name) + folded = "".join( + character + for character in decomposed + if not unicodedata.combining(character) + ).casefold() + return (folded, self.full_name, self.player_id) + + +class PlayerSeasonCatalogEntry(PlayerIdentity): + """One MLB player identity associated with one season's player directory. + + The season scopes membership, not the identity fields. MLB's historical + ``get_people`` responses return current biographical details for a person, + so ``full_name`` and ``primary_position`` continue to live on the global + player identity while this model adds the season in which MLB listed that + person as a Major League player. + """ + + season: int = Field(gt=0, description="MLB season directory membership.") + + def to_identity(self) -> PlayerIdentity: + """Return the global identity portion used by ``players`` persistence.""" + return PlayerIdentity( + player_id=self.player_id, + full_name=self.full_name, + primary_position=self.primary_position, + ) + class PlayerSeasonHitting(BaseModel): """One player's raw hitting counting stats for one MLB season. diff --git a/app/services/player_catalog_ingestion.py b/app/services/player_catalog_ingestion.py new file mode 100644 index 0000000..a9b4391 --- /dev/null +++ b/app/services/player_catalog_ingestion.py @@ -0,0 +1,64 @@ +"""Atomic season-level MLB player catalog ingestion.""" + +from mlbstatsapi import Mlb +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session + +from app.database.repositories import upsert_player_catalog_entry +from app.schemas.ingestion import ( + PlayerCatalogIngestionResult, + PlayerPersistenceOutcome, +) +from app.services.players import MlbPlayerDirectoryClient, discover_mlb_players + + +class PlayerCatalogIngestionError(Exception): + """A discovered MLB player catalog could not be persisted.""" + + +def ingest_player_catalog( + *, + session: Session, + season: int, + client: MlbPlayerDirectoryClient | None = None, +) -> PlayerCatalogIngestionResult: + """Discover and atomically persist MLB's player directory for one season. + + The endpoint is one bulk request. When no client is supplied, one ``Mlb`` + instance is created for that complete logical import and closed afterward. + All response validation finishes before the database transaction begins. + """ + if client is not None: + return _ingest_player_catalog(session=session, season=season, client=client) + with Mlb() as owned_client: + return _ingest_player_catalog( + session=session, season=season, client=owned_client + ) + + +def _ingest_player_catalog( + *, + session: Session, + season: int, + client: MlbPlayerDirectoryClient, +) -> PlayerCatalogIngestionResult: + entries = discover_mlb_players(season, client=client) + counts = {outcome: 0 for outcome in PlayerPersistenceOutcome} + + try: + with session.begin(): + for entry in entries: + outcome = upsert_player_catalog_entry(session, entry=entry) + counts[outcome] += 1 + except SQLAlchemyError as exc: + raise PlayerCatalogIngestionError( + f"Unable to persist the MLB player catalog for {season}" + ) from exc + + return PlayerCatalogIngestionResult( + season=season, + players_discovered=len(entries), + inserted=counts[PlayerPersistenceOutcome.INSERTED], + updated=counts[PlayerPersistenceOutcome.UPDATED], + unchanged=counts[PlayerPersistenceOutcome.UNCHANGED], + ) diff --git a/app/services/player_season_ingestion.py b/app/services/player_season_ingestion.py index ac3738b..98ff125 100644 --- a/app/services/player_season_ingestion.py +++ b/app/services/player_season_ingestion.py @@ -4,7 +4,11 @@ from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm import Session -from app.database.repositories import upsert_player, upsert_player_season_hitting +from app.database.repositories import ( + ensure_player_season_catalog_membership, + upsert_player, + upsert_player_season_hitting, +) from app.schemas.ingestion import PlayerSeasonIngestionResult from app.services.players import ( MlbPlayerDataClient, @@ -65,6 +69,9 @@ def _ingest_player_season( try: with session.begin(): identity_outcome = upsert_player(session, identity=identity) + ensure_player_season_catalog_membership( + session, identity=identity, season=season + ) hitting_outcome = upsert_player_season_hitting(session, hitting=hitting) except SQLAlchemyError as exc: raise PlayerSeasonIngestionError( diff --git a/app/services/players.py b/app/services/players.py index de60b8e..a6858bd 100644 --- a/app/services/players.py +++ b/app/services/players.py @@ -1,4 +1,4 @@ -"""Retrieve and normalize player identity and season hitting stats from MLB. +"""Retrieve and normalize player identity, discovery, and hitting data from MLB. Identity comes from ``Mlb.get_person``, which returns exactly one biographical record per player id or ``None`` if the id is not a known MLB person. @@ -21,7 +21,13 @@ from mlbstatsapi.models.stats import HittingSeason from pydantic import ValidationError -from app.schemas.players import PlayerIdentity, PlayerSeasonHitting +from app.schemas.players import ( + PlayerIdentity, + PlayerSeasonCatalogEntry, + PlayerSeasonHitting, +) + +MLB_SPORT_ID = 1 HITTING_STAT_GROUP = "hitting" SEASON_STAT_TYPE = "season" @@ -43,6 +49,10 @@ class NoHittingStatsError(PlayerDataError): """The player has no season hitting stats for the requested season.""" +class NoPlayersDiscoveredError(PlayerDataError): + """MLB returned no players for the requested Major League season.""" + + class MlbPlayerDataClient(Protocol): """The subset of ``mlbstatsapi.Mlb`` this service depends on.""" @@ -57,6 +67,88 @@ def get_player_stats( ) -> dict: ... +class MlbPlayerDirectoryClient(Protocol): + """The subset of ``mlbstatsapi.Mlb`` season discovery depends on.""" + + def get_people(self, sport_id: int = ..., **params: object) -> list[Person]: ... + + +def discover_mlb_players( + season: int, + *, + client: MlbPlayerDirectoryClient | None = None, +) -> list[PlayerSeasonCatalogEntry]: + """Return the players MLB lists for one Major League season. + + ``season`` scopes directory membership. The Person payload's biographical + fields are current identity data even for historical seasons, so this + function does not represent the name or primary position as historical. + + Entries are returned in ``PlayerIdentity.name_sort_key`` order, the same + order the persisted catalog reads back in, so a discovered directory and a + stored one can be compared directly. + """ + if client is not None: + return _discover_mlb_players(client, season) + with Mlb() as owned_client: + return _discover_mlb_players(owned_client, season) + + +def _discover_mlb_players( + client: MlbPlayerDirectoryClient, + season: int, +) -> list[PlayerSeasonCatalogEntry]: + try: + people = client.get_people(sport_id=MLB_SPORT_ID, season=season) + except TheMlbStatsApiException as exc: + raise PlayerDataError( + f"Unable to retrieve the MLB player directory for {season}" + ) from exc + + if not people: + raise NoPlayersDiscoveredError(f"MLB returned no players for {season}") + + entries: list[PlayerSeasonCatalogEntry] = [] + seen: dict[int, str | None] = {} + for person in people: + known_name = seen.get(person.id) + if person.id in seen: + names = ( + f"twice as {known_name!r}" + if known_name == person.full_name + else f"as both {known_name!r} and {person.full_name!r}" + ) + raise PlayerDataError( + f"MLB returned player {person.id} more than once for {season} ({names})" + ) + seen[person.id] = person.full_name + + if person.is_player is not True: + raise PlayerDataError( + f"MLB person {person.id} returned for {season} is not marked " + "as a player" + ) + identity = _normalize_player_identity( + person, + context=f"player {person.id} in the {season} MLB player directory", + ) + try: + entries.append( + PlayerSeasonCatalogEntry( + player_id=identity.player_id, + full_name=identity.full_name, + primary_position=identity.primary_position, + season=season, + ) + ) + except ValidationError as exc: + raise PlayerDataError( + f"Could not normalize player {person.id} for {season}: {exc}" + ) from exc + + return sorted(entries, key=PlayerSeasonCatalogEntry.name_sort_key) + + def get_player_identity( player_id: int, *, @@ -99,10 +191,15 @@ def _fetch_player_identity( raise PlayerDataError( f"MLB returned person id {person.id} for requested player {player_id}" ) + return _normalize_player_identity(person, context=f"player {player_id}") + + +def _normalize_player_identity(person: Person, *, context: str) -> PlayerIdentity: + """Normalize identity fields shared by direct lookup and bulk discovery.""" if not person.full_name: - raise PlayerDataError(f"No full name returned for player {player_id}") + raise PlayerDataError(f"No full name returned for {context}") if person.primary_position is None: - raise PlayerDataError(f"No primary position returned for player {player_id}") + raise PlayerDataError(f"No primary position returned for {context}") try: return PlayerIdentity( @@ -112,7 +209,7 @@ def _fetch_player_identity( ) except ValidationError as exc: raise PlayerDataError( - f"Could not normalize identity for player {player_id}: {exc}" + f"Could not normalize identity for {context}: {exc}" ) from exc diff --git a/docs/player-season-catalog.md b/docs/player-season-catalog.md new file mode 100644 index 0000000..136dd5c --- /dev/null +++ b/docs/player-season-catalog.md @@ -0,0 +1,136 @@ +# Player season catalog + +## Decision + +Player discovery uses the bulk call: + +```python +mlb.get_people(sport_id=1, season=season) +``` + +One response supplies the identities MLB associates with that Major League +season. Catalog ingestion does not make one request per player and does not +fetch hitting or pitching statistics. + +The normalized persistence grain is deliberately small: + +- `players` remains the single current identity row keyed by MLB person id; +- `player_seasons` stores only `(player_id, season)` directory membership; +- `player_season_hitting` remains the independently imported full-season + hitting aggregate. + +This avoids copying names and positions into every season while preserving the +season boundary needed by a future DB-only player selector. A catalog entry +does not claim that hitting statistics have been imported. + +## Installed-library audit + +The installed dependency is `python-mlb-statsapi` 1.1.0. Its +`Mlb.get_people(sport_id=1, **params)` implementation sends the request to +`sports/1/players` and passes `season` through as an endpoint parameter. The +method returns an empty list for a 4xx response, so this application treats an +empty directory as an explicit discovery failure rather than a valid empty MLB +season. + +The initial live audit on 2026-09-09 used one shared `Mlb()` client for all of +its calls. A follow-up audit on 2026-09-10 added the 2022 evidence from one bulk +request through a real shared `Mlb()` client: + +| Request | People | Unique ids | Missing names | Missing positions | +| --- | ---: | ---: | ---: | ---: | +| `season=2025` | 1,470 | 1,470 | 0 | 0 | +| `season=2024` | 1,454 | 1,454 | 0 | 0 | +| `season=2022` | 1,495 | 1,495 | 0 | 0 | +| `season=2001` | 1,220 | 1,220 | 0 | 0 | +| `season=2026` | 1,449 | 1,449 | 0 | 0 | + +The 2024 and 2025 id sets were materially different: 344 ids appeared only in +2025 and 328 only in 2024. Every audited person was marked `isPlayer=true`, and +no record had a debut after its requested season. An unscoped request matched +the current 2026 set, while `season=9999` returned the library's empty-list +failure shape. Adding `gameType=R` did not change the 2025 set, so the catalog +does not add that redundant parameter. + +These observations support season-scoped membership. They do **not** support +historical identity claims: fields such as current team and current age in a +2024 response reflected their 2026 values. Therefore the catalog persists +season membership separately and continues to treat name and primary position +as the current MLB identity attributes already modeled by `players`. + +Exact counts are audit evidence, not application invariants. In-progress +seasons grow as players debut, and upstream corrections may change historical +responses. + +### Juan Soto 2022 + +A follow-up live audit on 2026-09-10 reused one real `Mlb()` client and inspected +the single bulk `season=2022` response. MLB player id `665742` appeared exactly +once, as `Juan Soto` with primary-position abbreviation `LF`. The directory did +not return one copy for each of his 2022 team stints. This supports the catalog's +season-level Player membership grain; it does not support adding team-stint +persistence. + +### Two-way player + +The same 2022 response represented MLB player id `660271` as `Shohei Ohtani` +with primary-position abbreviation `TWP`. Searching the complete response for +`primary_position.abbreviation == "TWP"` found one player: Ohtani. The catalog +therefore preserves `TWP` literally, as it does every MLB position +abbreviation, without translating it to `P`, `DH`, or another value. + +Like the other identity values in this historical directory, +`primary_position` is treated as a current MLB identity attribute in `players`, +not as a historically accurate position for the `player_seasons` membership. + +## Ingestion behavior + +```bash +poetry run alembic upgrade head +poetry run python scripts/import_player_catalog.py --season 2025 +``` + +The network response is fully normalized and checked for empty results, +non-player records, missing required identity fields, and duplicate person ids +before persistence begins. All identities and memberships are then written in +one database transaction. A database failure rolls back the entire catalog +refresh. + +Reruns are idempotent. New season memberships are inserted, changed current +identity fields are updated, and matching entries are unchanged. A successful +refresh does not delete memberships absent from a later response: without a +separate completeness/run-state model, destructive reconciliation would turn a +transient partial upstream response into silent local data loss. This is a +search/discovery catalog, not a claim that local season coverage exactly equals +MLB forever after the import. + +The existing one-player season importer also records the corresponding +membership atomically. The migration backfills memberships for existing +`player_season_hitting` rows because those rows are direct evidence of the +player-season relationship. + +There is no async catalog path. Discovery is already one bulk request, so +concurrency would add no useful work and no second client lifecycle is needed. + +## Name ordering + +Both the discovered directory and the persisted catalog are returned in +`PlayerIdentity.name_sort_key` order. Neither default ordering is usable for a +player list: SQLite's default collation compares raw bytes, so `aaron Judge` +sorts after `Zack Wheeler`, and Python's `casefold` lowers case without folding +accents, so `Ángel Martínez` sorts after every unaccented name. MLB rosters +contain many accented names, so the key decomposes the name with NFKD, drops +combining marks, and folds case, falling back to the raw name and then the +player id so the order is total. + +Ordering therefore lives in the domain layer rather than in an `ORDER BY` +clause. The practical consequence is that discovery output and a catalog read +can be compared directly, which is what makes rerun behavior checkable. A +future player selector inherits the same order without restating the rule. This +is not full locale-aware collation; adding ICU was rejected as a dependency the +application does not otherwise need. + +## Scope boundary + +This foundation adds no Player UI, search route, charts, player game logs, +pitching ingestion, team-stint model, or browser-side MLB request. Future Player +pages must query persisted catalog and statistic rows only. diff --git a/scripts/import_player_catalog.py b/scripts/import_player_catalog.py new file mode 100644 index 0000000..41c3085 --- /dev/null +++ b/scripts/import_player_catalog.py @@ -0,0 +1,113 @@ +"""Import MLB's season-level player directory into the local database. + +Examples +-------- +poetry run alembic upgrade head +poetry run python scripts/import_player_catalog.py --season 2025 +poetry run python scripts/import_player_catalog.py --season 2025 --format json + +This is one bulk MLB request and does not import player statistics. It stores +the identities and season memberships needed for later DB-only Player search +and selection. All ingestion behavior lives in +``app.services.player_catalog_ingestion``. +""" + +import argparse +import json +import sys + +from sqlalchemy.exc import OperationalError + +from app.config import get_settings +from app.database.engine import build_engine, build_session_factory +from app.schemas.ingestion import PlayerCatalogIngestionResult +from app.services.player_catalog_ingestion import ( + PlayerCatalogIngestionError, + ingest_player_catalog, +) +from app.services.players import PlayerDataError + +MIGRATION_HINT = "Run: poetry run alembic upgrade head" + + +def build_parser() -> argparse.ArgumentParser: + """Build the command-line parser.""" + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + "--season", type=int, required=True, help="Season year, e.g. 2025." + ) + parser.add_argument( + "--format", + choices=("table", "json"), + default="table", + help="Output format (default: table).", + ) + return parser + + +def format_table(result: PlayerCatalogIngestionResult) -> str: + """Format an ingestion result for human-readable output.""" + return "\n".join( + [ + f"Season: {result.season}", + f"Players discovered: {result.players_discovered}", + f"Inserted: {result.inserted}", + f"Updated: {result.updated}", + f"Unchanged: {result.unchanged}", + ] + ) + + +def format_json(result: PlayerCatalogIngestionResult) -> str: + """Serialize an ingestion result as JSON.""" + return json.dumps(result.model_dump(mode="json"), indent=2) + + +def _report_operational_error(exc: OperationalError) -> None: + message = str(exc.orig) if exc.orig is not None else str(exc) + if "no such table" in message.lower(): + print( + f"error: database schema is missing ({message}). {MIGRATION_HINT}", + file=sys.stderr, + ) + return + print(f"error: {exc}", file=sys.stderr) + + +def main(argv: list[str] | None = None) -> int: + """Run the import command and return a process exit code.""" + parser = build_parser() + args = parser.parse_args(argv) + if args.season <= 0: + print("error: --season must be a positive integer", file=sys.stderr) + return 1 + + settings = get_settings() + engine = build_engine(settings.database_url) + session_factory = build_session_factory(engine) + session = session_factory() + try: + result = ingest_player_catalog(session=session, season=args.season) + except PlayerDataError as exc: + print(f"error: {exc}", file=sys.stderr) + return 1 + except PlayerCatalogIngestionError as exc: + cause = exc.__cause__ + if isinstance(cause, OperationalError): + _report_operational_error(cause) + else: + print(f"error: {exc}", file=sys.stderr) + return 1 + except OperationalError as exc: + _report_operational_error(exc) + return 1 + finally: + session.close() + engine.dispose() + + print(format_json(result) if args.format == "json" else format_table(result)) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_import_player_catalog.py b/tests/test_import_player_catalog.py new file mode 100644 index 0000000..d502504 --- /dev/null +++ b/tests/test_import_player_catalog.py @@ -0,0 +1,110 @@ +"""Tests for the season-level Player catalog import CLI.""" + +import json +from unittest.mock import patch + +import pytest +from scripts import import_player_catalog as import_cli +from sqlalchemy.exc import OperationalError + +from app.config import Settings +from app.schemas.ingestion import PlayerCatalogIngestionResult +from app.services.player_catalog_ingestion import PlayerCatalogIngestionError +from app.services.players import NoPlayersDiscoveredError + +MEMORY_SETTINGS = Settings(database_url="sqlite:///:memory:") +RESULT = PlayerCatalogIngestionResult( + season=2025, + players_discovered=3, + inserted=2, + updated=1, + unchanged=0, +) + + +def test_argument_parsing() -> None: + args = import_cli.build_parser().parse_args(["--season", "2025"]) + assert args.season == 2025 + assert args.format == "table" + + +def test_table_and_json_output() -> None: + assert "Players discovered: 3" in import_cli.format_table(RESULT) + assert json.loads(import_cli.format_json(RESULT)) == { + "season": 2025, + "players_discovered": 3, + "inserted": 2, + "updated": 1, + "unchanged": 0, + } + + +def test_nonpositive_season_fails_without_calling_service( + capsys: pytest.CaptureFixture[str], +) -> None: + with patch.object(import_cli, "ingest_player_catalog") as ingest: + code = import_cli.main(["--season", "0"]) + assert code == 1 + assert "positive" in capsys.readouterr().err + ingest.assert_not_called() + + +def test_discovery_error_is_reported(capsys: pytest.CaptureFixture[str]) -> None: + with ( + patch.object(import_cli, "get_settings", return_value=MEMORY_SETTINGS), + patch.object(import_cli, "build_engine"), + patch.object(import_cli, "build_session_factory"), + patch.object( + import_cli, + "ingest_player_catalog", + side_effect=NoPlayersDiscoveredError("no players"), + ), + ): + code = import_cli.main(["--season", "2025"]) + assert code == 1 + assert "no players" in capsys.readouterr().err + + +def test_missing_migration_is_reported(capsys: pytest.CaptureFixture[str]) -> None: + cause = OperationalError("insert", None, Exception("no such table: player_seasons")) + error = PlayerCatalogIngestionError("persist failed") + error.__cause__ = cause + with ( + patch.object(import_cli, "get_settings", return_value=MEMORY_SETTINGS), + patch.object(import_cli, "build_engine"), + patch.object(import_cli, "build_session_factory"), + patch.object(import_cli, "ingest_player_catalog", side_effect=error), + ): + code = import_cli.main(["--season", "2025"]) + assert code == 1 + assert "alembic upgrade head" in capsys.readouterr().err + + +def test_other_persistence_error_is_reported( + capsys: pytest.CaptureFixture[str], +) -> None: + with ( + patch.object(import_cli, "get_settings", return_value=MEMORY_SETTINGS), + patch.object(import_cli, "build_engine"), + patch.object(import_cli, "build_session_factory"), + patch.object( + import_cli, + "ingest_player_catalog", + side_effect=PlayerCatalogIngestionError("persist failed"), + ), + ): + code = import_cli.main(["--season", "2025"]) + assert code == 1 + assert "persist failed" in capsys.readouterr().err + + +def test_success_prints_requested_format(capsys: pytest.CaptureFixture[str]) -> None: + with ( + patch.object(import_cli, "get_settings", return_value=MEMORY_SETTINGS), + patch.object(import_cli, "build_engine"), + patch.object(import_cli, "build_session_factory"), + patch.object(import_cli, "ingest_player_catalog", return_value=RESULT), + ): + code = import_cli.main(["--season", "2025", "--format", "json"]) + assert code == 0 + assert json.loads(capsys.readouterr().out)["players_discovered"] == 3 diff --git a/tests/test_ingestion_schemas.py b/tests/test_ingestion_schemas.py index a640537..864bfef 100644 --- a/tests/test_ingestion_schemas.py +++ b/tests/test_ingestion_schemas.py @@ -14,6 +14,7 @@ LeagueSeasonIngestionStatus, LeagueTeamIngestionResult, LeagueTeamIngestionStatus, + PlayerCatalogIngestionResult, TeamSeasonIngestionResult, ) @@ -213,3 +214,25 @@ def test_a_failed_team_cannot_claim_persisted_records() -> None: unchanged=0, error="TeamGameLogError: nope", ) + + +def test_player_catalog_result_accounts_for_every_discovered_player() -> None: + result = PlayerCatalogIngestionResult( + season=SEASON, + players_discovered=1470, + inserted=1400, + updated=20, + unchanged=50, + ) + assert result.players_discovered == 1470 + + +def test_player_catalog_result_rejects_unaccounted_players() -> None: + with pytest.raises(ValueError, match="players_discovered"): + PlayerCatalogIngestionResult( + season=SEASON, + players_discovered=1470, + inserted=1400, + updated=20, + unchanged=49, + ) diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 79ecbbf..db3a98c 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -12,7 +12,7 @@ from app.database.engine import build_engine from tests.conftest import run_alembic_downgrade_base, run_alembic_upgrade -REVISION_HEAD = "73d9fae8fafb" +REVISION_HEAD = "8b3f31d9a5c2" def database_url_for(path: Path) -> str: @@ -1168,3 +1168,139 @@ def test_players_revision_follows_the_pitching_lines_revision() -> None: script = ScriptDirectory.from_config(Config("alembic.ini")) revision = script.get_revision(PLAYERS_REVISION) assert revision.down_revision == PRE_PLAYERS_REVISION + + +# --------------------------------------------------------------------------- +# M6 Player discovery: player_seasons catalog membership +# --------------------------------------------------------------------------- + +PRE_PLAYER_CATALOG_REVISION = PLAYERS_REVISION +PLAYER_CATALOG_REVISION = REVISION_HEAD + + +def _insert_pre_catalog_player_and_hitting(database_url: str) -> None: + run_alembic_upgrade_to(database_url, PRE_PLAYER_CATALOG_REVISION) + connection = sqlite3.connect(database_url.removeprefix("sqlite:///")) + connection.execute( + """ + INSERT INTO players ( + player_id, full_name, primary_position, created_at, updated_at + ) VALUES ( + 677594, 'Julio Rodríguez', 'CF', + '2026-08-28 12:00:00', '2026-08-28 12:00:00' + ) + """ + ) + connection.execute( + f""" + INSERT INTO player_season_hitting ({PLAYER_SEASON_HITTING_INSERT_COLUMNS}) + VALUES ({VALID_PLAYER_SEASON_HITTING_VALUES}) + """ + ) + connection.commit() + connection.close() + + +def test_player_catalog_migration_creates_expected_table_and_index( + migrated_db_path: Path, +) -> None: + engine = build_engine(database_url_for(migrated_db_path)) + inspector = inspect(engine) + assert {column["name"] for column in inspector.get_columns("player_seasons")} == { + "id", + "player_id", + "season", + "created_at", + } + assert {index["name"] for index in inspector.get_indexes("player_seasons")} == { + "ix_player_seasons_season_player_id" + } + engine.dispose() + + +def test_player_catalog_membership_has_player_foreign_key( + migrated_db_path: Path, +) -> None: + engine = build_engine(database_url_for(migrated_db_path)) + foreign_keys = inspect(engine).get_foreign_keys("player_seasons") + engine.dispose() + assert len(foreign_keys) == 1 + assert foreign_keys[0]["referred_table"] == "players" + assert foreign_keys[0]["constrained_columns"] == ["player_id"] + assert foreign_keys[0]["referred_columns"] == ["player_id"] + + +def test_player_catalog_membership_is_unique_and_positive( + player_row_session: Session, +) -> None: + values = "677594, 2025, '2026-09-09 00:00:00'" + player_row_session.execute( + text( + "INSERT INTO player_seasons (player_id, season, created_at) " + f"VALUES ({values})" + ) + ) + player_row_session.commit() + with pytest.raises(IntegrityError): + player_row_session.execute( + text( + "INSERT INTO player_seasons (player_id, season, created_at) " + f"VALUES ({values})" + ) + ) + player_row_session.commit() + + player_row_session.rollback() + with pytest.raises(IntegrityError): + player_row_session.execute( + text( + "INSERT INTO player_seasons (player_id, season, created_at) " + "VALUES (677594, 0, '2026-09-09 00:00:00')" + ) + ) + player_row_session.commit() + + +def test_player_catalog_upgrade_backfills_existing_hitting_membership( + tmp_path: Path, +) -> None: + db_path = tmp_path / "pre_catalog.db" + url = database_url_for(db_path) + _insert_pre_catalog_player_and_hitting(url) + run_alembic_upgrade(url) + connection = sqlite3.connect(db_path) + rows = connection.execute( + "SELECT player_id, season, created_at FROM player_seasons" + ).fetchall() + connection.close() + assert rows == [(677594, 2025, "2025-01-01 00:00:00")] + + +def test_player_catalog_downgrade_preserves_player_and_hitting_rows( + tmp_path: Path, +) -> None: + db_path = tmp_path / "catalog_downgrade.db" + url = database_url_for(db_path) + _insert_pre_catalog_player_and_hitting(url) + run_alembic_upgrade(url) + run_alembic_downgrade_to(url, PRE_PLAYER_CATALOG_REVISION) + engine = build_engine(url) + inspector = inspect(engine) + assert not inspector.has_table("player_seasons") + assert inspector.has_table("players") + assert inspector.has_table("player_season_hitting") + engine.dispose() + connection = sqlite3.connect(db_path) + assert connection.execute("SELECT count(*) FROM players").fetchone() == (1,) + assert connection.execute( + "SELECT count(*) FROM player_season_hitting" + ).fetchone() == (1,) + connection.close() + + +def test_player_catalog_revision_follows_players_revision() -> None: + from alembic.script import ScriptDirectory + + script = ScriptDirectory.from_config(Config("alembic.ini")) + revision = script.get_revision(PLAYER_CATALOG_REVISION) + assert revision.down_revision == PRE_PLAYER_CATALOG_REVISION diff --git a/tests/test_player_catalog.py b/tests/test_player_catalog.py new file mode 100644 index 0000000..d70fb03 --- /dev/null +++ b/tests/test_player_catalog.py @@ -0,0 +1,176 @@ +"""Offline tests for bulk, season-scoped MLB player discovery.""" + +from unittest.mock import Mock + +import pytest +from mlbstatsapi.exceptions import TheMlbStatsApiException +from mlbstatsapi.models.people import Person, Position + +from app.schemas.players import PlayerSeasonCatalogEntry +from app.services.players import ( + NoPlayersDiscoveredError, + PlayerDataError, + discover_mlb_players, +) + +SEASON = 2025 +CF = Position(code="8", name="Outfielder", type="Outfielder", abbreviation="CF") +PITCHER = Position(code="1", name="Pitcher", type="Pitcher", abbreviation="P") + + +def make_person( + player_id: int = 677594, + full_name: str | None = "Julio Rodríguez", + position: Position | None = CF, + *, + is_player: bool | None = True, +) -> Person: + return Person( + id=player_id, + link=f"/api/v1/people/{player_id}", + full_name=full_name, + primary_position=position, + is_player=is_player, + ) + + +class FakeDirectory: + def __init__(self, result: list[Person] | Exception) -> None: + self.result = result + self.calls: list[tuple[int, dict[str, object]]] = [] + self.closed = False + + def get_people(self, sport_id: int = 1, **params: object) -> list[Person]: + self.calls.append((sport_id, params)) + if isinstance(self.result, Exception): + raise self.result + return self.result + + def __enter__(self) -> "FakeDirectory": + return self + + def __exit__(self, *exc_info: object) -> None: + self.closed = True + + +def test_discovery_uses_one_season_scoped_major_league_request() -> None: + client = FakeDirectory([make_person()]) + + discovered = discover_mlb_players(SEASON, client=client) + + assert client.calls == [(1, {"season": SEASON})] + assert discovered == [ + PlayerSeasonCatalogEntry( + player_id=677594, + full_name="Julio Rodríguez", + primary_position="CF", + season=SEASON, + ) + ] + + +def test_discovery_returns_stable_name_then_id_order() -> None: + client = FakeDirectory( + [ + make_person(3, "zoë", PITCHER), + make_person(2, "Ángel", CF), + make_person(1, "Zoë", CF), + ] + ) + + discovered = discover_mlb_players(SEASON, client=client) + + assert [entry.player_id for entry in discovered] == [2, 1, 3] + + +def test_discovery_sorts_accented_names_with_their_unaccented_letter() -> None: + client = FakeDirectory( + [ + make_person(1, "Zack Wheeler", PITCHER), + make_person(2, "Ángel Martínez", CF), + make_person(3, "aaron Judge", CF), + ] + ) + + discovered = discover_mlb_players(SEASON, client=client) + + assert [entry.full_name for entry in discovered] == [ + "aaron Judge", + "Ángel Martínez", + "Zack Wheeler", + ] + + +def test_empty_directory_is_an_explicit_discovery_failure() -> None: + with pytest.raises(NoPlayersDiscoveredError, match="no players"): + discover_mlb_players(SEASON, client=FakeDirectory([])) + + +@pytest.mark.parametrize( + ("person", "message"), + [ + (make_person(full_name=None), "No full name"), + (make_person(position=None), "No primary position"), + (make_person(is_player=False), "not marked as a player"), + (make_person(is_player=None), "not marked as a player"), + ], +) +def test_incomplete_or_non_player_records_are_rejected( + person: Person, message: str +) -> None: + with pytest.raises(PlayerDataError, match=message): + discover_mlb_players(SEASON, client=FakeDirectory([person])) + + +def test_duplicate_player_ids_are_rejected_instead_of_deduplicated() -> None: + client = FakeDirectory( + [make_person(10, "Same Person"), make_person(10, "Different Name")] + ) + with pytest.raises(PlayerDataError, match="more than once"): + discover_mlb_players(SEASON, client=client) + + +def test_invalid_season_cannot_be_normalized_into_catalog_entries() -> None: + with pytest.raises(PlayerDataError, match="Could not normalize"): + discover_mlb_players(0, client=FakeDirectory([make_person()])) + + +def test_upstream_failure_preserves_the_cause() -> None: + failure = TheMlbStatsApiException("network down") + with pytest.raises(PlayerDataError) as caught: + discover_mlb_players(SEASON, client=FakeDirectory(failure)) + assert caught.value.__cause__ is failure + + +def test_owned_client_is_created_once_and_closed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from app.services import players as players_module + + owned = FakeDirectory([make_person()]) + factory = Mock(return_value=owned) + monkeypatch.setattr(players_module, "Mlb", factory) + + discover_mlb_players(SEASON) + + factory.assert_called_once_with() + assert owned.calls == [(1, {"season": SEASON})] + assert owned.closed is True + + +def test_caller_supplied_client_is_not_closed() -> None: + client = FakeDirectory([make_person()]) + discover_mlb_players(SEASON, client=client) + assert client.closed is False + + +def test_owned_client_closes_when_discovery_fails( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from app.services import players as players_module + + owned = FakeDirectory([]) + monkeypatch.setattr(players_module, "Mlb", Mock(return_value=owned)) + with pytest.raises(NoPlayersDiscoveredError): + discover_mlb_players(SEASON) + assert owned.closed is True diff --git a/tests/test_player_catalog_ingestion.py b/tests/test_player_catalog_ingestion.py new file mode 100644 index 0000000..2d6ce4e --- /dev/null +++ b/tests/test_player_catalog_ingestion.py @@ -0,0 +1,242 @@ +"""Tests for atomic season-level Player catalog ingestion.""" + +from unittest.mock import Mock, patch + +import pytest +from mlbstatsapi.models.people import Person, Position +from sqlalchemy import func, select +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session + +from app.database.models import ( + PlayerRecord, + PlayerSeasonCatalogRecord, + PlayerSeasonHittingRecord, +) +from app.database.repositories import list_player_catalog +from app.schemas.ingestion import PlayerPersistenceOutcome +from app.schemas.players import PlayerSeasonCatalogEntry +from app.services.player_catalog_ingestion import ( + PlayerCatalogIngestionError, + ingest_player_catalog, +) +from app.services.players import discover_mlb_players + +SEASON = 2025 +CF = Position(code="8", name="Outfielder", type="Outfielder", abbreviation="CF") +PITCHER = Position(code="1", name="Pitcher", type="Pitcher", abbreviation="P") + + +def make_person( + player_id: int = 677594, + full_name: str = "Julio Rodríguez", + position: Position = CF, +) -> Person: + return Person( + id=player_id, + link=f"/api/v1/people/{player_id}", + full_name=full_name, + primary_position=position, + is_player=True, + ) + + +class FakeDirectory: + def __init__(self, people: list[Person]) -> None: + self.people = people + self.calls = 0 + self.closed = False + + def get_people(self, sport_id: int = 1, **params: object) -> list[Person]: + self.calls += 1 + return self.people + + def __enter__(self) -> "FakeDirectory": + return self + + def __exit__(self, *exc_info: object) -> None: + self.closed = True + + +def make_client() -> FakeDirectory: + return FakeDirectory( + [ + make_person(), + make_person(660271, "Shohei Ohtani", PITCHER), + ] + ) + + +def test_import_inserts_identities_and_memberships_but_no_stats( + migrated_session: Session, +) -> None: + result = ingest_player_catalog( + session=migrated_session, season=SEASON, client=make_client() + ) + + assert result.players_discovered == 2 + assert result.inserted == 2 + assert result.updated == 0 + assert result.unchanged == 0 + assert len(list_player_catalog(migrated_session, season=SEASON)) == 2 + assert migrated_session.scalar(select(func.count()).select_from(PlayerRecord)) == 2 + assert ( + migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonHittingRecord) + ) + == 0 + ) + + +def test_identical_rerun_is_unchanged(migrated_session: Session) -> None: + ingest_player_catalog(session=migrated_session, season=SEASON, client=make_client()) + result = ingest_player_catalog( + session=migrated_session, season=SEASON, client=make_client() + ) + assert result.inserted == 0 + assert result.updated == 0 + assert result.unchanged == 2 + assert ( + migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonCatalogRecord) + ) + == 2 + ) + + +def test_existing_membership_with_changed_identity_is_updated( + migrated_session: Session, +) -> None: + ingest_player_catalog(session=migrated_session, season=SEASON, client=make_client()) + changed = FakeDirectory( + [ + make_person(full_name="Julio Rodriguez"), + make_person(660271, "Shohei Ohtani", PITCHER), + ] + ) + result = ingest_player_catalog( + session=migrated_session, season=SEASON, client=changed + ) + assert result.updated == 1 + assert result.unchanged == 1 + entries = list_player_catalog(migrated_session, season=SEASON) + assert entries[0].full_name == "Julio Rodriguez" + + +def test_same_player_in_another_season_gets_a_second_membership( + migrated_session: Session, +) -> None: + one = FakeDirectory([make_person()]) + ingest_player_catalog(session=migrated_session, season=2024, client=one) + result = ingest_player_catalog( + session=migrated_session, season=2025, client=FakeDirectory([make_person()]) + ) + assert result.inserted == 1 + assert migrated_session.scalar(select(func.count()).select_from(PlayerRecord)) == 1 + assert ( + migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonCatalogRecord) + ) + == 2 + ) + + +def test_stored_catalog_reads_back_in_discovery_order( + migrated_session: Session, +) -> None: + """Discovery and the persisted read must agree on one order, including accents.""" + client = FakeDirectory( + [ + make_person(1, "Zack Wheeler", PITCHER), + make_person(2, "Ángel Martínez", CF), + make_person(3, "aaron Judge", CF), + ] + ) + discovered = discover_mlb_players(SEASON, client=client) + + ingest_player_catalog(session=migrated_session, season=SEASON, client=client) + + assert list_player_catalog(migrated_session, season=SEASON) == discovered + + +def test_database_failure_rolls_back_entire_catalog(migrated_session: Session) -> None: + from app.services import player_catalog_ingestion as ingestion_module + + real_upsert = ingestion_module.upsert_player_catalog_entry + calls = 0 + + def fail_after_first_write( + session: Session, *, entry: PlayerSeasonCatalogEntry + ) -> PlayerPersistenceOutcome: + nonlocal calls + calls += 1 + if calls == 2: + raise SQLAlchemyError("db failed") + return real_upsert(session, entry=entry) + + with ( + patch( + "app.services.player_catalog_ingestion.upsert_player_catalog_entry", + side_effect=fail_after_first_write, + ), + pytest.raises(PlayerCatalogIngestionError), + ): + ingest_player_catalog( + session=migrated_session, season=SEASON, client=make_client() + ) + assert calls == 2 + assert migrated_session.scalar(select(func.count()).select_from(PlayerRecord)) == 0 + assert ( + migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonCatalogRecord) + ) + == 0 + ) + + +def test_discovery_finishes_before_transaction_begins( + migrated_session: Session, +) -> None: + events: list[str] = [] + + class TrackingClient(FakeDirectory): + def get_people(self, sport_id: int = 1, **params: object) -> list[Person]: + events.append("get_people") + return super().get_people(sport_id, **params) + + class TrackingSession: + def begin(self) -> object: + events.append("transaction_begin") + return migrated_session.begin() + + def __getattr__(self, name: str) -> object: + return getattr(migrated_session, name) + + ingest_player_catalog( + session=TrackingSession(), # type: ignore[arg-type] + season=SEASON, + client=TrackingClient([make_person()]), + ) + assert events == ["get_people", "transaction_begin"] + + +def test_no_client_supplied_uses_one_owned_client( + monkeypatch: pytest.MonkeyPatch, migrated_session: Session +) -> None: + from app.services import player_catalog_ingestion as ingestion_module + + owned = make_client() + factory = Mock(return_value=owned) + monkeypatch.setattr(ingestion_module, "Mlb", factory) + + ingest_player_catalog(session=migrated_session, season=SEASON) + + factory.assert_called_once_with() + assert owned.calls == 1 + assert owned.closed is True + + +def test_supplied_client_is_not_closed(migrated_session: Session) -> None: + client = make_client() + ingest_player_catalog(session=migrated_session, season=SEASON, client=client) + assert client.closed is False diff --git a/tests/test_player_schemas.py b/tests/test_player_schemas.py index 2531d50..ad1c18d 100644 --- a/tests/test_player_schemas.py +++ b/tests/test_player_schemas.py @@ -3,7 +3,11 @@ import pytest from pydantic import ValidationError -from app.schemas.players import PlayerIdentity, PlayerSeasonHitting +from app.schemas.players import ( + PlayerIdentity, + PlayerSeasonCatalogEntry, + PlayerSeasonHitting, +) PLAYER_ID = 677594 SEASON = 2025 @@ -45,6 +49,49 @@ def make_hitting(**overrides: object) -> PlayerSeasonHitting: return PlayerSeasonHitting(**base) +def test_catalog_entry_preserves_identity_and_adds_season() -> None: + entry = PlayerSeasonCatalogEntry( + player_id=677594, + full_name="Julio Rodríguez", + primary_position="CF", + season=2025, + ) + assert entry.to_identity() == make_identity(full_name="Julio Rodríguez") + + +def test_name_sort_key_folds_case_and_accents() -> None: + folded, original, player_id = make_identity( + full_name="Ángel Martínez" + ).name_sort_key() + assert folded == "angel martinez" + assert original == "Ángel Martínez" + assert player_id == PLAYER_ID + + +def test_name_sort_key_breaks_folded_ties_by_name_then_id() -> None: + names = [ + make_identity(player_id=2, full_name="zoë"), + make_identity(player_id=1, full_name="zoë"), + make_identity(player_id=9, full_name="Zoë"), + ] + ordered = sorted(names, key=PlayerIdentity.name_sort_key) + assert [(one.full_name, one.player_id) for one in ordered] == [ + ("Zoë", 9), + ("zoë", 1), + ("zoë", 2), + ] + + +def test_catalog_entry_requires_positive_season() -> None: + with pytest.raises(ValidationError): + PlayerSeasonCatalogEntry( + player_id=677594, + full_name="Julio Rodríguez", + primary_position="CF", + season=0, + ) + + def test_valid_identity_is_accepted() -> None: identity = make_identity() assert identity.player_id == PLAYER_ID diff --git a/tests/test_player_season_ingestion.py b/tests/test_player_season_ingestion.py index 31ef370..469bb75 100644 --- a/tests/test_player_season_ingestion.py +++ b/tests/test_player_season_ingestion.py @@ -7,7 +7,11 @@ from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm import Session -from app.database.models import PlayerRecord, PlayerSeasonHittingRecord +from app.database.models import ( + PlayerRecord, + PlayerSeasonCatalogRecord, + PlayerSeasonHittingRecord, +) from app.database.repositories import get_player, get_player_season_hitting from app.schemas.ingestion import PlayerPersistenceOutcome from app.services.player_season_ingestion import ( @@ -314,6 +318,19 @@ def fake_mlb_factory() -> ClosableFakeMlb: assert owned.closed is True +def test_player_season_import_also_records_catalog_membership( + migrated_session: Session, +) -> None: + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + memberships = migrated_session.query(PlayerSeasonCatalogRecord).all() + assert [(row.player_id, row.season) for row in memberships] == [(PLAYER_ID, SEASON)] + + def test_supplied_client_is_reused_for_both_calls_and_never_closed( migrated_session: Session, ) -> None: diff --git a/tests/test_repositories_player_catalog.py b/tests/test_repositories_player_catalog.py new file mode 100644 index 0000000..9105b3c --- /dev/null +++ b/tests/test_repositories_player_catalog.py @@ -0,0 +1,96 @@ +"""Tests for season-level Player catalog repository behavior.""" + +from sqlalchemy.orm import Session + +from app.database.repositories import ( + list_player_catalog, + upsert_player_catalog_entry, +) +from app.schemas.ingestion import PlayerPersistenceOutcome +from app.schemas.players import PlayerSeasonCatalogEntry + + +def entry( + player_id: int = 677594, + full_name: str = "Julio Rodríguez", + position: str = "CF", + season: int = 2025, +) -> PlayerSeasonCatalogEntry: + return PlayerSeasonCatalogEntry( + player_id=player_id, + full_name=full_name, + primary_position=position, + season=season, + ) + + +def test_new_entry_is_inserted_and_listed(migrated_session: Session) -> None: + outcome = upsert_player_catalog_entry(migrated_session, entry=entry()) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.INSERTED + assert list_player_catalog(migrated_session, season=2025) == [entry()] + + +def test_identical_entry_is_unchanged(migrated_session: Session) -> None: + upsert_player_catalog_entry(migrated_session, entry=entry()) + migrated_session.commit() + outcome = upsert_player_catalog_entry(migrated_session, entry=entry()) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.UNCHANGED + + +def test_changed_identity_is_updated_for_existing_membership( + migrated_session: Session, +) -> None: + upsert_player_catalog_entry(migrated_session, entry=entry()) + migrated_session.commit() + changed = entry(full_name="Julio Rodriguez", position="OF") + outcome = upsert_player_catalog_entry(migrated_session, entry=changed) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.UPDATED + assert list_player_catalog(migrated_session, season=2025) == [changed] + + +def test_new_season_membership_is_inserted_for_existing_identity( + migrated_session: Session, +) -> None: + upsert_player_catalog_entry(migrated_session, entry=entry(season=2024)) + migrated_session.commit() + outcome = upsert_player_catalog_entry(migrated_session, entry=entry(season=2025)) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.INSERTED + assert list_player_catalog(migrated_session, season=2024) == [entry(season=2024)] + assert list_player_catalog(migrated_session, season=2025) == [entry(season=2025)] + + +def test_catalog_is_season_scoped_and_sorted(migrated_session: Session) -> None: + entries = [ + entry(3, "Zach Player", "P"), + entry(2, "Aaron Player", "1B"), + entry(1, "Other Season", "C", season=2024), + ] + for catalog_entry in entries: + upsert_player_catalog_entry(migrated_session, entry=catalog_entry) + migrated_session.commit() + assert list_player_catalog(migrated_session, season=2025) == [ + entries[1], + entries[0], + ] + assert list_player_catalog(migrated_session, season=1900) == [] + + +def test_catalog_ordering_ignores_case_and_accents(migrated_session: Session) -> None: + """The stored directory reads back in name order, not database byte order.""" + for catalog_entry in [ + entry(1, "Zack Wheeler", "P"), + entry(2, "Ángel Martínez", "2B"), + entry(3, "aaron Judge", "RF"), + ]: + upsert_player_catalog_entry(migrated_session, entry=catalog_entry) + migrated_session.commit() + listed = list_player_catalog(migrated_session, season=2025) + assert [stored.full_name for stored in listed] == [ + "aaron Judge", + "Ángel Martínez", + "Zack Wheeler", + ]