diff --git a/alembic/versions/73d9fae8fafb_create_players_and_player_season_hitting.py b/alembic/versions/73d9fae8fafb_create_players_and_player_season_hitting.py new file mode 100644 index 0000000..686a205 --- /dev/null +++ b/alembic/versions/73d9fae8fafb_create_players_and_player_season_hitting.py @@ -0,0 +1,162 @@ +"""create_players_and_player_season_hitting + +Revision ID: 73d9fae8fafb +Revises: 27a202039134 +Create Date: 2026-08-28 23:53:40.528933 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "73d9fae8fafb" +down_revision: str | Sequence[str] | None = "27a202039134" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """Upgrade schema.""" + op.create_table( + "players", + sa.Column("id", sa.Integer(), autoincrement=True, nullable=False), + sa.Column("player_id", sa.Integer(), nullable=False), + sa.Column("full_name", sa.String(), nullable=False), + sa.Column("primary_position", sa.String(), nullable=False), + sa.Column("created_at", sa.DateTime(), nullable=False), + sa.Column("updated_at", sa.DateTime(), nullable=False), + sa.CheckConstraint("player_id > 0", name=op.f("ck_players_player_id_positive")), + sa.PrimaryKeyConstraint("id", name=op.f("pk_players")), + sa.UniqueConstraint("player_id", name="uq_players_player_id"), + ) + op.create_table( + "player_season_hitting", + 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("games_played", sa.Integer(), nullable=False), + sa.Column("plate_appearances", sa.Integer(), nullable=False), + sa.Column("at_bats", sa.Integer(), nullable=False), + sa.Column("runs", sa.Integer(), nullable=False), + sa.Column("hits", sa.Integer(), nullable=False), + sa.Column("doubles", sa.Integer(), nullable=False), + sa.Column("triples", sa.Integer(), nullable=False), + sa.Column("home_runs", sa.Integer(), nullable=False), + sa.Column("rbi", sa.Integer(), nullable=False), + sa.Column("base_on_balls", sa.Integer(), nullable=False), + sa.Column("intentional_walks", sa.Integer(), nullable=False), + sa.Column("hit_by_pitch", sa.Integer(), nullable=False), + sa.Column("strikeouts", sa.Integer(), nullable=False), + sa.Column("stolen_bases", sa.Integer(), nullable=False), + sa.Column("caught_stealing", sa.Integer(), nullable=False), + sa.Column("sac_flies", sa.Integer(), nullable=False), + sa.Column("sac_bunts", sa.Integer(), nullable=False), + sa.Column("created_at", sa.DateTime(), nullable=False), + sa.Column("updated_at", sa.DateTime(), nullable=False), + sa.CheckConstraint( + "player_id > 0", name=op.f("ck_player_season_hitting_player_id_positive") + ), + sa.CheckConstraint( + "season > 0", name=op.f("ck_player_season_hitting_season_positive") + ), + sa.CheckConstraint( + "games_played >= 0", + name=op.f("ck_player_season_hitting_games_played_nonnegative"), + ), + sa.CheckConstraint( + "plate_appearances >= 0", + name=op.f("ck_player_season_hitting_plate_appearances_nonnegative"), + ), + sa.CheckConstraint( + "at_bats >= 0", name=op.f("ck_player_season_hitting_at_bats_nonnegative") + ), + sa.CheckConstraint( + "runs >= 0", name=op.f("ck_player_season_hitting_runs_nonnegative") + ), + sa.CheckConstraint( + "hits >= 0", name=op.f("ck_player_season_hitting_hits_nonnegative") + ), + sa.CheckConstraint( + "doubles >= 0", name=op.f("ck_player_season_hitting_doubles_nonnegative") + ), + sa.CheckConstraint( + "triples >= 0", name=op.f("ck_player_season_hitting_triples_nonnegative") + ), + sa.CheckConstraint( + "home_runs >= 0", + name=op.f("ck_player_season_hitting_home_runs_nonnegative"), + ), + sa.CheckConstraint( + "rbi >= 0", name=op.f("ck_player_season_hitting_rbi_nonnegative") + ), + sa.CheckConstraint( + "base_on_balls >= 0", + name=op.f("ck_player_season_hitting_base_on_balls_nonnegative"), + ), + sa.CheckConstraint( + "intentional_walks >= 0", + name=op.f("ck_player_season_hitting_intentional_walks_nonnegative"), + ), + sa.CheckConstraint( + "hit_by_pitch >= 0", + name=op.f("ck_player_season_hitting_hit_by_pitch_nonnegative"), + ), + sa.CheckConstraint( + "strikeouts >= 0", + name=op.f("ck_player_season_hitting_strikeouts_nonnegative"), + ), + sa.CheckConstraint( + "stolen_bases >= 0", + name=op.f("ck_player_season_hitting_stolen_bases_nonnegative"), + ), + sa.CheckConstraint( + "caught_stealing >= 0", + name=op.f("ck_player_season_hitting_caught_stealing_nonnegative"), + ), + sa.CheckConstraint( + "sac_flies >= 0", + name=op.f("ck_player_season_hitting_sac_flies_nonnegative"), + ), + sa.CheckConstraint( + "sac_bunts >= 0", + name=op.f("ck_player_season_hitting_sac_bunts_nonnegative"), + ), + # Definitional, not empirical: an at-bat is a plate appearance, an + # extra-base hit is a hit, and an intentional walk is a walk. + # Spot-checked against real single-team, two-way, and traded-player + # seasons before being encoded here. + sa.CheckConstraint( + "at_bats <= plate_appearances", + name=op.f("ck_player_season_hitting_at_bats_within_plate_appearances"), + ), + sa.CheckConstraint( + "doubles + triples + home_runs <= hits", + name=op.f("ck_player_season_hitting_extra_base_hits_within_hits"), + ), + sa.CheckConstraint( + "intentional_walks <= base_on_balls", + name=op.f( + "ck_player_season_hitting_intentional_walks_within_base_on_balls" + ), + ), + sa.ForeignKeyConstraint( + ["player_id"], + ["players.player_id"], + name=op.f("fk_player_season_hitting_player_id_players"), + ), + sa.PrimaryKeyConstraint("id", name=op.f("pk_player_season_hitting")), + sa.UniqueConstraint( + "player_id", + "season", + name="uq_player_season_hitting_player_id_season", + ), + ) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_table("player_season_hitting") + op.drop_table("players") diff --git a/app/database/models.py b/app/database/models.py index fe937f6..b96492f 100644 --- a/app/database/models.py +++ b/app/database/models.py @@ -9,6 +9,7 @@ CheckConstraint, Date, DateTime, + ForeignKey, Index, Integer, String, @@ -22,6 +23,7 @@ LeagueSeasonIngestionState, LeagueSeasonIngestionStatus, ) +from app.schemas.players import PlayerIdentity, PlayerSeasonHitting class TeamGameBattingLineRecord(Base): @@ -449,3 +451,213 @@ def from_domain(state: LeagueSeasonIngestionState) -> LeagueSeasonIngestionRecor record = LeagueSeasonIngestionRecord() record.apply_domain(state) return record + + +class PlayerRecord(Base): + """Persistence representation of one player's identity. + + ``player_id`` is the MLB person id and is the natural identity other + player tables reference, not the surrogate ``id`` primary key. + """ + + __tablename__ = "players" + __table_args__ = ( + UniqueConstraint("player_id", name="uq_players_player_id"), + CheckConstraint("player_id > 0", name="player_id_positive"), + ) + + id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) + player_id: Mapped[int] = mapped_column(Integer, nullable=False) + full_name: Mapped[str] = mapped_column(String, nullable=False) + primary_position: Mapped[str] = mapped_column(String, nullable=False) + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=False), nullable=False + ) + updated_at: Mapped[datetime] = mapped_column( + DateTime(timezone=False), nullable=False + ) + + def to_domain(self) -> PlayerIdentity: + """Convert this row to the normalized Pydantic domain model.""" + return PlayerIdentity( + player_id=self.player_id, + full_name=self.full_name, + primary_position=self.primary_position, + ) + + def apply_domain(self, identity: PlayerIdentity) -> None: + """Copy persisted identity fields from a domain record onto this row.""" + self.full_name = identity.full_name + self.primary_position = identity.primary_position + + @staticmethod + def from_domain( + identity: PlayerIdentity, + *, + created_at: datetime, + updated_at: datetime, + ) -> PlayerRecord: + """Build a new ORM row from a domain record and timestamps.""" + return PlayerRecord( + player_id=identity.player_id, + full_name=identity.full_name, + primary_position=identity.primary_position, + created_at=created_at, + updated_at=updated_at, + ) + + +class PlayerSeasonHittingRecord(Base): + """Persistence representation of one player's full-season hitting aggregate. + + Only raw counting stats are stored; batting average, OBP, SLG, OPS, and + total bases are calculated on demand rather than persisted. There is no + ``team_id`` column: this row represents the MLB full-season aggregate, not + a team stint, so a player traded mid-season still has exactly one row here. + """ + + __tablename__ = "player_season_hitting" + __table_args__ = ( + UniqueConstraint( + "player_id", "season", name="uq_player_season_hitting_player_id_season" + ), + CheckConstraint("player_id > 0", name="player_id_positive"), + CheckConstraint("season > 0", name="season_positive"), + CheckConstraint("games_played >= 0", name="games_played_nonnegative"), + CheckConstraint("plate_appearances >= 0", name="plate_appearances_nonnegative"), + CheckConstraint("at_bats >= 0", name="at_bats_nonnegative"), + CheckConstraint("runs >= 0", name="runs_nonnegative"), + CheckConstraint("hits >= 0", name="hits_nonnegative"), + CheckConstraint("doubles >= 0", name="doubles_nonnegative"), + CheckConstraint("triples >= 0", name="triples_nonnegative"), + CheckConstraint("home_runs >= 0", name="home_runs_nonnegative"), + CheckConstraint("rbi >= 0", name="rbi_nonnegative"), + CheckConstraint("base_on_balls >= 0", name="base_on_balls_nonnegative"), + CheckConstraint("intentional_walks >= 0", name="intentional_walks_nonnegative"), + CheckConstraint("hit_by_pitch >= 0", name="hit_by_pitch_nonnegative"), + CheckConstraint("strikeouts >= 0", name="strikeouts_nonnegative"), + CheckConstraint("stolen_bases >= 0", name="stolen_bases_nonnegative"), + CheckConstraint("caught_stealing >= 0", name="caught_stealing_nonnegative"), + CheckConstraint("sac_flies >= 0", name="sac_flies_nonnegative"), + CheckConstraint("sac_bunts >= 0", name="sac_bunts_nonnegative"), + # Definitional, not empirical: an at-bat is a plate appearance, an + # extra-base hit is a hit, and an intentional walk is a walk. + # Spot-checked against real single-team, two-way, and traded-player + # seasons before being encoded here; see the Milestone 46 report. + CheckConstraint( + "at_bats <= plate_appearances", name="at_bats_within_plate_appearances" + ), + CheckConstraint( + "doubles + triples + home_runs <= hits", + name="extra_base_hits_within_hits", + ), + CheckConstraint( + "intentional_walks <= base_on_balls", + name="intentional_walks_within_base_on_balls", + ), + ) + + 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) + games_played: Mapped[int] = mapped_column(Integer, nullable=False) + plate_appearances: Mapped[int] = mapped_column(Integer, nullable=False) + at_bats: Mapped[int] = mapped_column(Integer, nullable=False) + runs: Mapped[int] = mapped_column(Integer, nullable=False) + hits: Mapped[int] = mapped_column(Integer, nullable=False) + doubles: Mapped[int] = mapped_column(Integer, nullable=False) + triples: Mapped[int] = mapped_column(Integer, nullable=False) + home_runs: Mapped[int] = mapped_column(Integer, nullable=False) + rbi: Mapped[int] = mapped_column(Integer, nullable=False) + base_on_balls: Mapped[int] = mapped_column(Integer, nullable=False) + intentional_walks: Mapped[int] = mapped_column(Integer, nullable=False) + hit_by_pitch: Mapped[int] = mapped_column(Integer, nullable=False) + strikeouts: Mapped[int] = mapped_column(Integer, nullable=False) + stolen_bases: Mapped[int] = mapped_column(Integer, nullable=False) + caught_stealing: Mapped[int] = mapped_column(Integer, nullable=False) + sac_flies: Mapped[int] = mapped_column(Integer, nullable=False) + sac_bunts: Mapped[int] = mapped_column(Integer, nullable=False) + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=False), nullable=False + ) + updated_at: Mapped[datetime] = mapped_column( + DateTime(timezone=False), nullable=False + ) + + def to_domain(self) -> PlayerSeasonHitting: + """Convert this row to the normalized Pydantic domain model.""" + return PlayerSeasonHitting( + player_id=self.player_id, + season=self.season, + games_played=self.games_played, + plate_appearances=self.plate_appearances, + at_bats=self.at_bats, + runs=self.runs, + hits=self.hits, + doubles=self.doubles, + triples=self.triples, + home_runs=self.home_runs, + rbi=self.rbi, + base_on_balls=self.base_on_balls, + intentional_walks=self.intentional_walks, + hit_by_pitch=self.hit_by_pitch, + strikeouts=self.strikeouts, + stolen_bases=self.stolen_bases, + caught_stealing=self.caught_stealing, + sac_flies=self.sac_flies, + sac_bunts=self.sac_bunts, + ) + + def apply_domain(self, hitting: PlayerSeasonHitting) -> None: + """Copy persisted counting stats from a domain record onto this row.""" + self.games_played = hitting.games_played + self.plate_appearances = hitting.plate_appearances + self.at_bats = hitting.at_bats + self.runs = hitting.runs + self.hits = hitting.hits + self.doubles = hitting.doubles + self.triples = hitting.triples + self.home_runs = hitting.home_runs + self.rbi = hitting.rbi + self.base_on_balls = hitting.base_on_balls + self.intentional_walks = hitting.intentional_walks + self.hit_by_pitch = hitting.hit_by_pitch + self.strikeouts = hitting.strikeouts + self.stolen_bases = hitting.stolen_bases + self.caught_stealing = hitting.caught_stealing + self.sac_flies = hitting.sac_flies + self.sac_bunts = hitting.sac_bunts + + @staticmethod + def from_domain( + hitting: PlayerSeasonHitting, + *, + created_at: datetime, + updated_at: datetime, + ) -> PlayerSeasonHittingRecord: + """Build a new ORM row from a domain record and timestamps.""" + return PlayerSeasonHittingRecord( + player_id=hitting.player_id, + season=hitting.season, + games_played=hitting.games_played, + plate_appearances=hitting.plate_appearances, + at_bats=hitting.at_bats, + runs=hitting.runs, + hits=hitting.hits, + doubles=hitting.doubles, + triples=hitting.triples, + home_runs=hitting.home_runs, + rbi=hitting.rbi, + base_on_balls=hitting.base_on_balls, + intentional_walks=hitting.intentional_walks, + hit_by_pitch=hitting.hit_by_pitch, + strikeouts=hitting.strikeouts, + stolen_bases=hitting.stolen_bases, + caught_stealing=hitting.caught_stealing, + sac_flies=hitting.sac_flies, + sac_bunts=hitting.sac_bunts, + created_at=created_at, + updated_at=updated_at, + ) diff --git a/app/database/repositories.py b/app/database/repositories.py index f86000d..320d466 100644 --- a/app/database/repositories.py +++ b/app/database/repositories.py @@ -9,6 +9,8 @@ from app.database.models import ( LeagueSeasonIngestionRecord, + PlayerRecord, + PlayerSeasonHittingRecord, TeamGameBattingLineRecord, TeamGamePitchingLineRecord, ) @@ -22,8 +24,10 @@ from app.schemas.ingestion import ( LeagueSeasonIngestionState, LeagueSeasonIngestionStatus, + PlayerPersistenceOutcome, TeamGamePersistenceResult, ) +from app.schemas.players import PlayerIdentity, 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. @@ -492,3 +496,94 @@ def _store_league_season_ingestion( session.add(LeagueSeasonIngestionRecord.from_domain(state)) return record.apply_domain(state) + + +def get_player(session: Session, *, player_id: int) -> PlayerIdentity | None: + """Return a player's stored identity, or None if never imported.""" + record = _load_player(session, player_id) + return None if record is None else record.to_domain() + + +def get_player_season_hitting( + session: Session, + *, + player_id: int, + season: int, +) -> PlayerSeasonHitting | None: + """Return a player's stored season hitting aggregate, or None if not stored.""" + record = _load_player_season_hitting(session, player_id=player_id, season=season) + return None if record is None else record.to_domain() + + +def upsert_player( + session: Session, + *, + identity: PlayerIdentity, +) -> PlayerPersistenceOutcome: + """Insert, update, or leave unchanged the one row for a player's identity. + + Does not commit or roll back. + """ + record = _load_player(session, identity.player_id) + now = datetime.now(UTC).replace(tzinfo=None) + + if record is None: + session.add(PlayerRecord.from_domain(identity, created_at=now, updated_at=now)) + return PlayerPersistenceOutcome.INSERTED + + if record.to_domain() == identity: + return PlayerPersistenceOutcome.UNCHANGED + + record.apply_domain(identity) + record.updated_at = now + return PlayerPersistenceOutcome.UPDATED + + +def upsert_player_season_hitting( + session: Session, + *, + hitting: PlayerSeasonHitting, +) -> PlayerPersistenceOutcome: + """Insert, update, or leave unchanged the one row for a player-season. + + Does not commit or roll back. + """ + record = _load_player_season_hitting( + session, player_id=hitting.player_id, season=hitting.season + ) + now = datetime.now(UTC).replace(tzinfo=None) + + if record is None: + session.add( + PlayerSeasonHittingRecord.from_domain( + hitting, created_at=now, updated_at=now + ) + ) + return PlayerPersistenceOutcome.INSERTED + + if record.to_domain() == hitting: + return PlayerPersistenceOutcome.UNCHANGED + + record.apply_domain(hitting) + record.updated_at = now + return PlayerPersistenceOutcome.UPDATED + + +def _load_player(session: Session, player_id: int) -> PlayerRecord | None: + return session.scalars( + select(PlayerRecord).where(PlayerRecord.player_id == player_id) + ).one_or_none() + + +def _load_player_season_hitting( + session: Session, + *, + player_id: int, + season: int, +) -> PlayerSeasonHittingRecord | None: + return session.scalars( + select(PlayerSeasonHittingRecord).where( + PlayerSeasonHittingRecord.player_id == player_id, + PlayerSeasonHittingRecord.season == season, + ) + ).one_or_none() diff --git a/app/schemas/ingestion.py b/app/schemas/ingestion.py index 971d1db..5606439 100644 --- a/app/schemas/ingestion.py +++ b/app/schemas/ingestion.py @@ -311,3 +311,29 @@ def _state_is_internally_consistent(self) -> LeagueSeasonIngestionState: "COMPLETE coverage requires at least one team and no failures" ) return self + + +class PlayerPersistenceOutcome(StrEnum): + """What happened to one row during an upsert: inserted, updated, or unchanged.""" + + INSERTED = "INSERTED" + UPDATED = "UPDATED" + UNCHANGED = "UNCHANGED" + + +class PlayerSeasonIngestionResult(BaseModel): + """Outcome of ingesting one player-season of hitting stats. + + Team-season ingestion upserts a batch of many game rows, so its result + reports counts. Player-season ingestion always touches exactly one player + identity row and one player-season hitting row, so each gets a single + ``PlayerPersistenceOutcome`` instead. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + player_id: int = Field(gt=0) + season: int = Field(gt=0) + full_name: str = Field(min_length=1) + identity_outcome: PlayerPersistenceOutcome + hitting_outcome: PlayerPersistenceOutcome diff --git a/app/schemas/players.py b/app/schemas/players.py new file mode 100644 index 0000000..3427d78 --- /dev/null +++ b/app/schemas/players.py @@ -0,0 +1,85 @@ +"""Normalized schemas for player identity and player-season hitting stats.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, Field, model_validator + + +class PlayerIdentity(BaseModel): + """A player's persisted identity fields. + + ``primary_position`` stores the MLB-reported position abbreviation exactly + as returned (for example ``"TWP"`` for a two-way player), never normalized + into another position. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + player_id: int = Field(gt=0, description="MLB person id.") + full_name: str = Field(min_length=1, description="Player's full name.") + primary_position: str = Field( + min_length=1, description="MLB-reported primary position abbreviation." + ) + + +class PlayerSeasonHitting(BaseModel): + """One player's raw hitting counting stats for one MLB season. + + Only raw components are stored. Batting average, OBP, SLG, OPS, and total + bases are calculated from these fields on demand rather than persisted, so + a stored rate can never drift from the components it came from. + + Represents the full-season aggregate: a player who played for more than + one club in a season is stored once, as the combined total, not once per + team. See ``app.services.players`` for how that aggregate is selected from + the MLB response. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + player_id: int = Field(gt=0, description="MLB person id.") + season: int = Field(gt=0, description="Season the stats belong to.") + games_played: int = Field(ge=0) + plate_appearances: int = Field(ge=0) + at_bats: int = Field(ge=0) + runs: int = Field(ge=0) + hits: int = Field(ge=0) + doubles: int = Field(ge=0) + triples: int = Field(ge=0) + home_runs: int = Field(ge=0) + rbi: int = Field(ge=0) + base_on_balls: int = Field(ge=0) + intentional_walks: int = Field(ge=0) + hit_by_pitch: int = Field(ge=0) + strikeouts: int = Field(ge=0) + stolen_bases: int = Field(ge=0) + caught_stealing: int = Field(ge=0) + sac_flies: int = Field(ge=0) + sac_bunts: int = Field(ge=0) + + @model_validator(mode="after") + def _counting_stats_are_internally_consistent(self) -> PlayerSeasonHitting: + """Reject a season whose components contradict each other. + + These are definitional relationships, not empirical ones: an at-bat is + a plate appearance, an extra-base hit is a hit, and an intentional walk + is a walk. Spot-checked across real single-team, two-way, and + traded-player seasons with no violations before being encoded here. + """ + if self.at_bats > self.plate_appearances: + raise ValueError( + f"at_bats ({self.at_bats}) cannot exceed plate_appearances " + f"({self.plate_appearances})" + ) + extra_base_hits = self.doubles + self.triples + self.home_runs + if extra_base_hits > self.hits: + raise ValueError( + f"doubles + triples + home_runs ({extra_base_hits}) cannot " + f"exceed hits ({self.hits})" + ) + if self.intentional_walks > self.base_on_balls: + raise ValueError( + f"intentional_walks ({self.intentional_walks}) cannot exceed " + f"base_on_balls ({self.base_on_balls})" + ) + return self diff --git a/app/services/player_season_ingestion.py b/app/services/player_season_ingestion.py new file mode 100644 index 0000000..ac3738b --- /dev/null +++ b/app/services/player_season_ingestion.py @@ -0,0 +1,80 @@ +"""Atomic player-season hitting ingestion into the local database.""" + +from mlbstatsapi import Mlb +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session + +from app.database.repositories import upsert_player, upsert_player_season_hitting +from app.schemas.ingestion import PlayerSeasonIngestionResult +from app.services.players import ( + MlbPlayerDataClient, + get_player_identity, + get_player_season_hitting, +) + + +class PlayerSeasonIngestionError(Exception): + """Player-season data could not be persisted.""" + + +def ingest_player_season( + *, + session: Session, + player_id: int, + season: int, + client: MlbPlayerDataClient | None = None, +) -> PlayerSeasonIngestionResult: + """Fetch one player-season of hitting stats from MLB, then persist it atomically. + + Parameters + ---------- + client: + An existing ``mlbstatsapi.Mlb`` client, reused for both the identity + and season hitting requests. When omitted, one client is created for + this logical import and closed afterwards, so a single import never + opens more than one MLB client. + """ + if client is not None: + return _ingest_player_season( + session=session, player_id=player_id, season=season, client=client + ) + with Mlb() as owned_client: + return _ingest_player_season( + session=session, player_id=player_id, season=season, client=owned_client + ) + + +def _ingest_player_season( + *, + session: Session, + player_id: int, + season: int, + client: MlbPlayerDataClient, +) -> PlayerSeasonIngestionResult: + """Fetch one player-season from MLB with ``client``, then persist it atomically. + + Both MLB requests -- identity and season hitting -- complete before the + database transaction begins. The player identity row and the player-season + hitting row then persist inside that same transaction, so a failure on the + second write rolls back the first: this ingestion can never leave a player + row with no matching season row, or a stale identity beside fresh stats. + """ + identity = get_player_identity(player_id, client=client) + hitting = get_player_season_hitting(player_id, season, client=client) + + try: + with session.begin(): + identity_outcome = upsert_player(session, identity=identity) + hitting_outcome = upsert_player_season_hitting(session, hitting=hitting) + except SQLAlchemyError as exc: + raise PlayerSeasonIngestionError( + f"Unable to persist player {player_id} season {season}" + ) from exc + + return PlayerSeasonIngestionResult( + player_id=player_id, + season=season, + full_name=identity.full_name, + identity_outcome=identity_outcome, + hitting_outcome=hitting_outcome, + ) diff --git a/app/services/players.py b/app/services/players.py new file mode 100644 index 0000000..de60b8e --- /dev/null +++ b/app/services/players.py @@ -0,0 +1,267 @@ +"""Retrieve and normalize player identity and season hitting stats 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. + +Season hitting comes from ``Mlb.get_player_stats`` in the ``hitting`` group and +``season`` stat type. That request returns a ``Stat`` whose ``splits`` are +``HittingSeason`` objects: one split for a player who played the whole season +with one club, or several splits for a player who changed clubs mid-season — +one full-season aggregate split with ``team is None``, plus one team-specific +split per club. Only the aggregate is ever normalized; see +``_select_season_aggregate_split`` for why picking ``splits[0]`` would be +wrong. +""" + +from typing import Protocol + +from mlbstatsapi import Mlb +from mlbstatsapi.exceptions import TheMlbStatsApiException +from mlbstatsapi.models.people.people import Person +from mlbstatsapi.models.stats import HittingSeason +from pydantic import ValidationError + +from app.schemas.players import PlayerIdentity, PlayerSeasonHitting + +HITTING_STAT_GROUP = "hitting" +SEASON_STAT_TYPE = "season" + + +class PlayerDataError(Exception): + """Upstream MLB player data was missing, ambiguous, or could not be normalized. + + Also raised directly when the MLB request itself fails, mirroring + ``TeamGameLogError`` in ``app.services.team_game_logs``. + """ + + +class PlayerNotFoundError(PlayerDataError): + """The requested player id is not a known MLB person.""" + + +class NoHittingStatsError(PlayerDataError): + """The player has no season hitting stats for the requested season.""" + + +class MlbPlayerDataClient(Protocol): + """The subset of ``mlbstatsapi.Mlb`` this service depends on.""" + + def get_person(self, player_id: int, **params: object) -> Person | None: ... + + def get_player_stats( + self, + person_id: int, + stats: list[str], + groups: list[str], + **params: object, + ) -> dict: ... + + +def get_player_identity( + player_id: int, + *, + client: MlbPlayerDataClient | None = None, +) -> PlayerIdentity: + """Fetch and normalize a player's identity. + + Parameters + ---------- + player_id: + MLB person id, for example 677594 for Julio Rodriguez. + client: + An existing ``mlbstatsapi.Mlb`` client. When omitted a client is + created and closed for this call. + + Raises + ------ + PlayerNotFoundError + No MLB person exists for ``player_id``. + PlayerDataError + The MLB request failed or the response could not be normalized. + """ + if client is not None: + return _fetch_player_identity(client, player_id) + with Mlb() as owned_client: + return _fetch_player_identity(owned_client, player_id) + + +def _fetch_player_identity( + client: MlbPlayerDataClient, player_id: int +) -> PlayerIdentity: + try: + person = client.get_person(player_id) + except TheMlbStatsApiException as exc: + raise PlayerDataError(f"Unable to retrieve MLB player {player_id}") from exc + + if person is None: + raise PlayerNotFoundError(f"No MLB player found for player id {player_id}") + if person.id != player_id: + raise PlayerDataError( + f"MLB returned person id {person.id} for requested player {player_id}" + ) + if not person.full_name: + raise PlayerDataError(f"No full name returned for player {player_id}") + if person.primary_position is None: + raise PlayerDataError(f"No primary position returned for player {player_id}") + + try: + return PlayerIdentity( + player_id=person.id, + full_name=person.full_name, + primary_position=person.primary_position.abbreviation, + ) + except ValidationError as exc: + raise PlayerDataError( + f"Could not normalize identity for player {player_id}: {exc}" + ) from exc + + +def get_player_season_hitting( + player_id: int, + season: int, + *, + client: MlbPlayerDataClient | None = None, +) -> PlayerSeasonHitting: + """Fetch and normalize a player's full-season hitting aggregate. + + Parameters + ---------- + player_id: + MLB person id. + season: + Four digit season year. + client: + An existing ``mlbstatsapi.Mlb`` client. When omitted a client is + created and closed for this call. + + Raises + ------ + NoHittingStatsError + The player has no season hitting stats for ``season``. + PlayerDataError + The MLB request failed, the aggregate split could not be determined, + or the response could not be normalized. + """ + if client is not None: + return _fetch_player_season_hitting(client, player_id, season) + with Mlb() as owned_client: + return _fetch_player_season_hitting(owned_client, player_id, season) + + +def _fetch_player_season_hitting( + client: MlbPlayerDataClient, player_id: int, season: int +) -> PlayerSeasonHitting: + try: + stat_groups = client.get_player_stats( + player_id, + stats=[SEASON_STAT_TYPE], + groups=[HITTING_STAT_GROUP], + season=season, + ) + except TheMlbStatsApiException as exc: + raise PlayerDataError( + f"Unable to retrieve MLB season hitting stats for player {player_id} " + f"in {season}" + ) from exc + + try: + season_stat = stat_groups[HITTING_STAT_GROUP][SEASON_STAT_TYPE] + except (KeyError, TypeError) as exc: + raise NoHittingStatsError( + f"No season hitting stats returned for player {player_id} in {season}" + ) from exc + + splits = season_stat.splits or [] + split = _select_season_aggregate_split(splits, player_id=player_id, season=season) + return _normalize_season_hitting_split(split, player_id=player_id, season=season) + + +def _select_season_aggregate_split( + splits: list[HittingSeason], + *, + player_id: int, + season: int, +) -> HittingSeason: + """Select the split representing the full-season aggregate. + + Zero splits means there are no usable season hitting stats. Exactly one + split is used as-is, whether or not it carries a team, since a player who + played for a single club that season has no separate aggregate row to + prefer. More than one split means the player changed clubs mid-season: + MLB then returns one aggregate split with ``team is None`` alongside one + team-specific split per club, and only the aggregate represents the full + season. A response with any other shape among several splits -- zero or + more than one aggregate row -- cannot be interpreted and is refused rather + than guessed at with ``splits[0]``. + """ + if not splits: + raise NoHittingStatsError( + f"No season hitting stats returned for player {player_id} in {season}" + ) + if len(splits) == 1: + return splits[0] + + aggregates = [split for split in splits if split.team is None] + if len(aggregates) != 1: + raise PlayerDataError( + f"Expected exactly one full-season aggregate split among " + f"{len(splits)} splits for player {player_id} in {season}, found " + f"{len(aggregates)}" + ) + return aggregates[0] + + +# (domain field name, upstream MLB field name) for every persisted counting +# stat. Every one of these is Optional on the upstream model because other +# stat types omit some of them; a missing value here means the payload is +# incomplete, not that the real count is zero. +_REQUIRED_STAT_FIELDS = ( + ("games_played", "gamesPlayed"), + ("plate_appearances", "plateAppearances"), + ("at_bats", "atBats"), + ("runs", "runs"), + ("hits", "hits"), + ("doubles", "doubles"), + ("triples", "triples"), + ("home_runs", "homeRuns"), + ("rbi", "rbi"), + ("base_on_balls", "baseOnBalls"), + ("intentional_walks", "intentionalWalks"), + ("hit_by_pitch", "hitByPitch"), + ("strikeouts", "strikeOuts"), + ("stolen_bases", "stolenBases"), + ("caught_stealing", "caughtStealing"), + ("sac_flies", "sacFlies"), + ("sac_bunts", "sacBunts"), +) + + +def _normalize_season_hitting_split( + split: HittingSeason, + *, + player_id: int, + season: int, +) -> PlayerSeasonHitting: + context = f"player {player_id} season {season}" + if split.stat is None: + raise PlayerDataError(f"No hitting stat line returned for {context}") + + values: dict[str, int] = {} + for field_name, raw_name in _REQUIRED_STAT_FIELDS: + value = getattr(split.stat, field_name) + if value is None: + raise PlayerDataError( + f"No {raw_name} in the season hitting stats for {context}" + ) + # ``bool`` is an ``int`` subclass, so it is rejected explicitly rather + # than being counted as 0 or 1. + if isinstance(value, bool) or not isinstance(value, int): + raise PlayerDataError( + f"{raw_name} {value!r} is not an integer for {context}" + ) + values[field_name] = value + + try: + return PlayerSeasonHitting(player_id=player_id, season=season, **values) + except ValidationError as exc: + raise PlayerDataError(f"Could not normalize {context}: {exc}") from exc diff --git a/scripts/import_player_season.py b/scripts/import_player_season.py new file mode 100644 index 0000000..c71505c --- /dev/null +++ b/scripts/import_player_season.py @@ -0,0 +1,127 @@ +"""Import one player-season of hitting stats into the local database. + +Examples +-------- +poetry run alembic upgrade head +poetry run python scripts/import_player_season.py --player-id 677594 --season 2025 +poetry run python scripts/import_player_season.py \\ + --player-id 677594 --season 2025 --format json + +This script calls the live MLB Stats API unless tests replace the client. It is +not part of the automated test suite. +""" + +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 PlayerSeasonIngestionResult +from app.services.player_season_ingestion import ( + PlayerSeasonIngestionError, + ingest_player_season, +) +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( + "--player-id", type=int, required=True, help="MLB player id, e.g. 677594." + ) + 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: PlayerSeasonIngestionResult) -> str: + """Format an ingestion result for human-readable output.""" + return "\n".join( + [ + f"Player: {result.full_name} ({result.player_id})", + f"Season: {result.season}", + f"Identity: {result.identity_outcome.value}", + f"Season hitting: {result.hitting_outcome.value}", + ] + ) + + +def format_json(result: PlayerSeasonIngestionResult) -> str: + """Serialize an ingestion result as JSON.""" + return json.dumps(result.model_dump(mode="json"), indent=2) + + +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.player_id <= 0: + print("error: --player-id must be a positive integer", file=sys.stderr) + return 1 + 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_season( + session=session, + player_id=args.player_id, + season=args.season, + ) + except PlayerDataError as exc: + print(f"error: {exc}", file=sys.stderr) + return 1 + except PlayerSeasonIngestionError as exc: + orig = exc.__cause__ + if isinstance(orig, OperationalError): + message = str(orig.orig) if orig.orig is not None else str(orig) + if "no such table" in message.lower(): + print( + f"error: database schema is missing ({message}). {MIGRATION_HINT}", + file=sys.stderr, + ) + return 1 + print(f"error: {exc}", file=sys.stderr) + return 1 + except OperationalError as exc: + 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 1 + print(f"error: {exc}", file=sys.stderr) + return 1 + finally: + session.close() + engine.dispose() + + if args.format == "json": + print(format_json(result)) + else: + print(format_table(result)) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_import_player_season.py b/tests/test_import_player_season.py new file mode 100644 index 0000000..6e4823f --- /dev/null +++ b/tests/test_import_player_season.py @@ -0,0 +1,159 @@ +"""Tests for the player-season import CLI.""" + +import json +from unittest.mock import patch + +import pytest +from scripts import import_player_season as import_cli + +from app.config import Settings +from app.schemas.ingestion import PlayerPersistenceOutcome, PlayerSeasonIngestionResult +from app.services.player_season_ingestion import PlayerSeasonIngestionError +from app.services.players import NoHittingStatsError, PlayerNotFoundError + +MEMORY_SETTINGS = Settings(database_url="sqlite:///:memory:") + +SAMPLE_RESULT = PlayerSeasonIngestionResult( + player_id=677594, + season=2025, + full_name="Julio Rodriguez", + identity_outcome=PlayerPersistenceOutcome.INSERTED, + hitting_outcome=PlayerPersistenceOutcome.INSERTED, +) + + +def test_required_argument_parsing() -> None: + parser = import_cli.build_parser() + args = parser.parse_args(["--player-id", "677594", "--season", "2025"]) + assert args.player_id == 677594 + assert args.season == 2025 + assert args.format == "table" + + +def test_missing_required_arguments_raises_system_exit() -> None: + parser = import_cli.build_parser() + with pytest.raises(SystemExit): + parser.parse_args(["--season", "2025"]) + with pytest.raises(SystemExit): + parser.parse_args(["--player-id", "677594"]) + + +def test_table_output() -> None: + text = import_cli.format_table(SAMPLE_RESULT) + assert "Player: Julio Rodriguez (677594)" in text + assert "Season: 2025" in text + assert "Identity: INSERTED" in text + assert "Season hitting: INSERTED" in text + + +def test_clean_json_output() -> None: + payload = json.loads(import_cli.format_json(SAMPLE_RESULT)) + assert payload == { + "player_id": 677594, + "season": 2025, + "full_name": "Julio Rodriguez", + "identity_outcome": "INSERTED", + "hitting_outcome": "INSERTED", + } + + +def test_invalid_player_id_produces_nonzero_exit_without_calling_service( + capsys: pytest.CaptureFixture[str], +) -> None: + with patch.object(import_cli, "ingest_player_season") as ingest: + code = import_cli.main(["--player-id", "-1", "--season", "2025"]) + assert code == 1 + assert "positive" in capsys.readouterr().err + ingest.assert_not_called() + + +def test_invalid_season_produces_nonzero_exit_without_calling_service( + capsys: pytest.CaptureFixture[str], +) -> None: + with patch.object(import_cli, "ingest_player_season") as ingest: + code = import_cli.main(["--player-id", "677594", "--season", "0"]) + assert code == 1 + assert "positive" in capsys.readouterr().err + ingest.assert_not_called() + + +def test_player_not_found_produces_nonzero_exit_code( + 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_season", + side_effect=PlayerNotFoundError("no such player"), + ), + ): + code = import_cli.main(["--player-id", "1", "--season", "2025"]) + assert code == 1 + assert "no such player" in capsys.readouterr().err + + +def test_no_hitting_stats_produces_nonzero_exit_code( + 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_season", + side_effect=NoHittingStatsError("no hitting stats"), + ), + ): + code = import_cli.main(["--player-id", "677594", "--season", "1901"]) + assert code == 1 + assert "no hitting stats" in capsys.readouterr().err + + +def test_persistence_error_produces_nonzero_exit_code( + 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_season", + side_effect=PlayerSeasonIngestionError("persist failed"), + ), + ): + code = import_cli.main(["--player-id", "677594", "--season", "2025"]) + assert code == 1 + assert "persist failed" in capsys.readouterr().err + + +def test_main_json_format_prints_only_json(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_season", return_value=SAMPLE_RESULT), + ): + code = import_cli.main( + ["--player-id", "677594", "--season", "2025", "--format", "json"] + ) + assert code == 0 + payload = json.loads(capsys.readouterr().out) + assert payload["full_name"] == "Julio Rodriguez" + assert payload["identity_outcome"] == "INSERTED" + + +def test_main_table_format_prints_table(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_season", return_value=SAMPLE_RESULT), + ): + code = import_cli.main(["--player-id", "677594", "--season", "2025"]) + assert code == 0 + assert "Player: Julio Rodriguez (677594)" in capsys.readouterr().out diff --git a/tests/test_migrations.py b/tests/test_migrations.py index e7a64fd..79ecbbf 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 = "27a202039134" +REVISION_HEAD = "73d9fae8fafb" def database_url_for(path: Path) -> str: @@ -902,3 +902,269 @@ def test_baserunners_revision_follows_the_league_revision() -> None: script = ScriptDirectory.from_config(Config("alembic.ini")) revision = script.get_revision(BASERUNNERS_REVISION) assert revision.down_revision == PRE_BASERUNNERS_REVISION + + +# --------------------------------------------------------------------------- +# Issue #46: players and player_season_hitting tables +# --------------------------------------------------------------------------- + +PRE_PLAYERS_REVISION = "27a202039134" +PLAYERS_REVISION = "73d9fae8fafb" + +PLAYER_HITTING_COLUMNS = { + "id", + "player_id", + "season", + "games_played", + "plate_appearances", + "at_bats", + "runs", + "hits", + "doubles", + "triples", + "home_runs", + "rbi", + "base_on_balls", + "intentional_walks", + "hit_by_pitch", + "strikeouts", + "stolen_bases", + "caught_stealing", + "sac_flies", + "sac_bunts", + "created_at", + "updated_at", +} + + +def test_pre_players_revision_has_no_player_tables(tmp_path: Path) -> None: + db_path = tmp_path / "pre_players.db" + run_alembic_upgrade_to(database_url_for(db_path), PRE_PLAYERS_REVISION) + engine = build_engine(database_url_for(db_path)) + inspector = inspect(engine) + assert not inspector.has_table("players") + assert not inspector.has_table("player_season_hitting") + engine.dispose() + + +def test_players_migration_creates_the_tables(migrated_db_path: Path) -> None: + engine = build_engine(database_url_for(migrated_db_path)) + inspector = inspect(engine) + assert inspector.has_table("players") + assert inspector.has_table("player_season_hitting") + engine.dispose() + + +def test_players_expected_columns_exist(migrated_db_path: Path) -> None: + engine = build_engine(database_url_for(migrated_db_path)) + columns = {col["name"] for col in inspect(engine).get_columns("players")} + engine.dispose() + assert columns == { + "id", + "player_id", + "full_name", + "primary_position", + "created_at", + "updated_at", + } + + +def test_player_season_hitting_expected_columns_exist( + migrated_db_path: Path, +) -> None: + engine = build_engine(database_url_for(migrated_db_path)) + columns = { + col["name"] for col in inspect(engine).get_columns("player_season_hitting") + } + engine.dispose() + assert columns == PLAYER_HITTING_COLUMNS + + +def test_player_season_hitting_declares_a_foreign_key_to_players( + migrated_db_path: Path, +) -> None: + engine = build_engine(database_url_for(migrated_db_path)) + foreign_keys = inspect(engine).get_foreign_keys("player_season_hitting") + 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_unique_player_id_is_enforced(migrated_session: Session) -> None: + migrated_session.execute( + text( + """ + INSERT INTO players (player_id, full_name, primary_position, + created_at, updated_at) + VALUES (677594, 'Julio Rodriguez', 'CF', + '2025-01-01 00:00:00', '2025-01-01 00:00:00') + """ + ) + ) + migrated_session.commit() + with pytest.raises(IntegrityError): + migrated_session.execute( + text( + """ + INSERT INTO players (player_id, full_name, primary_position, + created_at, updated_at) + VALUES (677594, 'Someone Else', '1B', + '2025-01-02 00:00:00', '2025-01-02 00:00:00') + """ + ) + ) + migrated_session.commit() + + +def test_negative_player_id_is_rejected(migrated_session: Session) -> None: + with pytest.raises(IntegrityError): + migrated_session.execute( + text( + """ + INSERT INTO players (player_id, full_name, primary_position, + created_at, updated_at) + VALUES (-1, 'Nobody', 'OF', + '2025-01-01 00:00:00', '2025-01-01 00:00:00') + """ + ) + ) + migrated_session.commit() + + +PLAYER_SEASON_HITTING_INSERT_COLUMNS = ( + "player_id, season, games_played, plate_appearances, at_bats, runs, hits, " + "doubles, triples, home_runs, rbi, base_on_balls, intentional_walks, " + "hit_by_pitch, strikeouts, stolen_bases, caught_stealing, sac_flies, " + "sac_bunts, created_at, updated_at" +) + +VALID_PLAYER_SEASON_HITTING_VALUES = ( + "677594, 2025, 150, 600, 500, 80, 150, 30, 3, 20, 90, 60, 5, 5, 100, 10, 3, " + "4, 2, '2025-01-01 00:00:00', '2025-01-01 00:00:00'" +) + + +@pytest.fixture +def player_row_session(migrated_session: Session) -> Session: + """A migrated session with one player row already committed.""" + migrated_session.execute( + text( + """ + INSERT INTO players (player_id, full_name, primary_position, + created_at, updated_at) + VALUES (677594, 'Julio Rodriguez', 'CF', + '2025-01-01 00:00:00', '2025-01-01 00:00:00') + """ + ) + ) + migrated_session.commit() + return migrated_session + + +def test_valid_player_season_hitting_row_is_accepted( + player_row_session: Session, +) -> None: + player_row_session.execute( + text( + f""" + INSERT INTO player_season_hitting ({PLAYER_SEASON_HITTING_INSERT_COLUMNS}) + VALUES ({VALID_PLAYER_SEASON_HITTING_VALUES}) + """ + ) + ) + player_row_session.commit() + + +def test_unique_player_id_season_is_enforced(player_row_session: Session) -> None: + player_row_session.execute( + text( + f""" + INSERT INTO player_season_hitting ({PLAYER_SEASON_HITTING_INSERT_COLUMNS}) + VALUES ({VALID_PLAYER_SEASON_HITTING_VALUES}) + """ + ) + ) + player_row_session.commit() + with pytest.raises(IntegrityError): + player_row_session.execute( + text( + f""" + INSERT INTO player_season_hitting + ({PLAYER_SEASON_HITTING_INSERT_COLUMNS}) + VALUES ({VALID_PLAYER_SEASON_HITTING_VALUES}) + """ + ) + ) + player_row_session.commit() + + +@pytest.mark.parametrize( + "column,value", + [ + ("at_bats", 700), # at_bats > plate_appearances + ("doubles", 200), # extra base hits > hits + ("intentional_walks", 200), # IBB > BB + ("hits", -1), + ("stolen_bases", -1), + ], +) +def test_definitional_check_constraints_are_enforced( + player_row_session: Session, column: str, value: int +) -> None: + columns = { + col: val + for col, val in zip( + PLAYER_SEASON_HITTING_INSERT_COLUMNS.split(", "), + VALID_PLAYER_SEASON_HITTING_VALUES.split(", "), + strict=True, + ) + } + columns[column] = str(value) + values = ", ".join(columns.values()) + with pytest.raises(IntegrityError): + player_row_session.execute( + text( + f""" + INSERT INTO player_season_hitting + ({PLAYER_SEASON_HITTING_INSERT_COLUMNS}) + VALUES ({values}) + """ + ) + ) + player_row_session.commit() + + +def test_players_downgrade_removes_only_the_new_tables( + migrated_db_path: Path, +) -> None: + url = database_url_for(migrated_db_path) + run_alembic_downgrade_to(url, PRE_PLAYERS_REVISION) + engine = build_engine(url) + inspector = inspect(engine) + assert not inspector.has_table("players") + assert not inspector.has_table("player_season_hitting") + assert inspector.has_table("team_game_batting_lines") + engine.dispose() + + +def test_players_upgrade_downgrade_upgrade_round_trips(tmp_path: Path) -> None: + db_path = tmp_path / "players_roundtrip.db" + url = database_url_for(db_path) + run_alembic_upgrade(url) + run_alembic_downgrade_to(url, PRE_PLAYERS_REVISION) + run_alembic_upgrade(url) + engine = build_engine(url) + inspector = inspect(engine) + assert inspector.has_table("players") + assert inspector.has_table("player_season_hitting") + engine.dispose() + + +def test_players_revision_follows_the_pitching_lines_revision() -> None: + from alembic.script import ScriptDirectory + + script = ScriptDirectory.from_config(Config("alembic.ini")) + revision = script.get_revision(PLAYERS_REVISION) + assert revision.down_revision == PRE_PLAYERS_REVISION diff --git a/tests/test_player_schemas.py b/tests/test_player_schemas.py new file mode 100644 index 0000000..2531d50 --- /dev/null +++ b/tests/test_player_schemas.py @@ -0,0 +1,144 @@ +"""Tests for player identity and player-season hitting domain schemas.""" + +import pytest +from pydantic import ValidationError + +from app.schemas.players import PlayerIdentity, PlayerSeasonHitting + +PLAYER_ID = 677594 +SEASON = 2025 + + +def make_identity(**overrides: object) -> PlayerIdentity: + base = { + "player_id": PLAYER_ID, + "full_name": "Julio Rodriguez", + "primary_position": "CF", + } + base.update(overrides) + return PlayerIdentity(**base) + + +def make_hitting(**overrides: object) -> PlayerSeasonHitting: + base = { + "player_id": PLAYER_ID, + "season": SEASON, + "games_played": 150, + "plate_appearances": 600, + "at_bats": 500, + "runs": 80, + "hits": 150, + "doubles": 30, + "triples": 3, + "home_runs": 20, + "rbi": 90, + "base_on_balls": 60, + "intentional_walks": 5, + "hit_by_pitch": 5, + "strikeouts": 100, + "stolen_bases": 10, + "caught_stealing": 3, + "sac_flies": 4, + "sac_bunts": 2, + } + base.update(overrides) + return PlayerSeasonHitting(**base) + + +def test_valid_identity_is_accepted() -> None: + identity = make_identity() + assert identity.player_id == PLAYER_ID + assert identity.full_name == "Julio Rodriguez" + assert identity.primary_position == "CF" + + +def test_two_way_player_position_abbreviation_is_preserved() -> None: + """The MLB-reported abbreviation is stored exactly, never normalized.""" + identity = make_identity(primary_position="TWP") + assert identity.primary_position == "TWP" + + +@pytest.mark.parametrize("player_id", [0, -1]) +def test_nonpositive_player_id_is_rejected(player_id: int) -> None: + with pytest.raises(ValidationError): + make_identity(player_id=player_id) + + +def test_blank_full_name_is_rejected() -> None: + with pytest.raises(ValidationError): + make_identity(full_name="") + + +def test_blank_primary_position_is_rejected() -> None: + with pytest.raises(ValidationError): + make_identity(primary_position="") + + +def test_identity_is_frozen() -> None: + identity = make_identity() + with pytest.raises(ValidationError): + identity.full_name = "Someone Else" + + +def test_valid_season_hitting_is_accepted() -> None: + hitting = make_hitting() + assert hitting.hits == 150 + + +@pytest.mark.parametrize( + "field", + [ + "games_played", + "plate_appearances", + "at_bats", + "runs", + "hits", + "doubles", + "triples", + "home_runs", + "rbi", + "base_on_balls", + "intentional_walks", + "hit_by_pitch", + "strikeouts", + "stolen_bases", + "caught_stealing", + "sac_flies", + "sac_bunts", + ], +) +def test_negative_counting_stats_are_rejected(field: str) -> None: + with pytest.raises(ValidationError): + make_hitting(**{field: -1}) + + +def test_at_bats_exceeding_plate_appearances_is_rejected() -> None: + with pytest.raises(ValidationError): + make_hitting(plate_appearances=400, at_bats=500) + + +def test_at_bats_equal_to_plate_appearances_is_accepted() -> None: + hitting = make_hitting(plate_appearances=500, at_bats=500) + assert hitting.at_bats == hitting.plate_appearances + + +def test_extra_base_hits_exceeding_hits_is_rejected() -> None: + with pytest.raises(ValidationError): + make_hitting(hits=10, doubles=5, triples=3, home_runs=5) + + +def test_intentional_walks_exceeding_base_on_balls_is_rejected() -> None: + with pytest.raises(ValidationError): + make_hitting(base_on_balls=5, intentional_walks=10) + + +@pytest.mark.parametrize("season", [0, -1]) +def test_nonpositive_season_is_rejected(season: int) -> None: + with pytest.raises(ValidationError): + make_hitting(season=season) + + +def test_season_hitting_is_frozen() -> None: + hitting = make_hitting() + with pytest.raises(ValidationError): + hitting.hits = 200 diff --git a/tests/test_player_season_ingestion.py b/tests/test_player_season_ingestion.py new file mode 100644 index 0000000..31ef370 --- /dev/null +++ b/tests/test_player_season_ingestion.py @@ -0,0 +1,365 @@ +"""Tests for the player-season ingestion service.""" + +from unittest.mock import patch + +import pytest +from sqlalchemy import func, select +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session + +from app.database.models import PlayerRecord, 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 ( + PlayerSeasonIngestionError, + ingest_player_season, +) +from app.services.players import NoHittingStatsError, PlayerNotFoundError +from tests.test_players_service import FakeMlb, make_person, make_split, make_stat + +PLAYER_ID = 677594 +SEASON = 2025 +MARINERS_STATS = {"hitting": {"season": make_stat([make_split()])}} + + +def make_client() -> FakeMlb: + return FakeMlb(person=make_person(), player_stats=MARINERS_STATS) + + +def test_first_import_inserts_both_rows(migrated_session: Session) -> None: + result = ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + assert result.identity_outcome is PlayerPersistenceOutcome.INSERTED + assert result.hitting_outcome is PlayerPersistenceOutcome.INSERTED + assert result.full_name == "Julio Rodriguez" + assert get_player(migrated_session, player_id=PLAYER_ID) is not None + assert ( + get_player_season_hitting(migrated_session, player_id=PLAYER_ID, season=SEASON) + is not None + ) + + +def test_second_identical_import_reports_unchanged(migrated_session: Session) -> None: + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + result = ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + assert result.identity_outcome is PlayerPersistenceOutcome.UNCHANGED + assert result.hitting_outcome is PlayerPersistenceOutcome.UNCHANGED + + player_count = migrated_session.scalar( + select(func.count()).select_from(PlayerRecord) + ) + hitting_count = migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonHittingRecord) + ) + assert player_count == 1 + assert hitting_count == 1 + + +def test_changed_season_stats_update_without_duplicate( + migrated_session: Session, +) -> None: + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + changed_stats = {"hitting": {"season": make_stat([make_split(hits=200)])}} + result = ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=FakeMlb(person=make_person(), player_stats=changed_stats), + ) + assert result.hitting_outcome is PlayerPersistenceOutcome.UPDATED + hitting_count = migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonHittingRecord) + ) + assert hitting_count == 1 + stored = get_player_season_hitting( + migrated_session, player_id=PLAYER_ID, season=SEASON + ) + assert stored.hits == 200 + + +def test_changed_identity_updates_without_duplicate(migrated_session: Session) -> None: + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + renamed_client = FakeMlb( + person=make_person(full_name="J-Rod"), player_stats=MARINERS_STATS + ) + result = ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=renamed_client, + ) + assert result.identity_outcome is PlayerPersistenceOutcome.UPDATED + player_count = migrated_session.scalar( + select(func.count()).select_from(PlayerRecord) + ) + assert player_count == 1 + assert get_player(migrated_session, player_id=PLAYER_ID).full_name == "J-Rod" + + +def test_in_progress_season_totals_increase_and_stay_idempotent( + migrated_session: Session, +) -> None: + """A season whose totals grow over time never produces duplicate rows.""" + early_stats = {"hitting": {"season": make_stat([make_split(games_played=50)])}} + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=FakeMlb(person=make_person(), player_stats=early_stats), + ) + later_stats = {"hitting": {"season": make_stat([make_split(games_played=100)])}} + first_rerun = ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=FakeMlb(person=make_person(), player_stats=later_stats), + ) + assert first_rerun.hitting_outcome is PlayerPersistenceOutcome.UPDATED + + second_rerun = ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=FakeMlb(person=make_person(), player_stats=later_stats), + ) + assert second_rerun.hitting_outcome is PlayerPersistenceOutcome.UNCHANGED + + hitting_count = migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonHittingRecord) + ) + assert hitting_count == 1 + + +def test_player_not_found_leaves_database_unchanged(migrated_session: Session) -> None: + client = FakeMlb(person=None) + with pytest.raises(PlayerNotFoundError): + ingest_player_season( + session=migrated_session, player_id=PLAYER_ID, season=SEASON, client=client + ) + player_count = migrated_session.scalar( + select(func.count()).select_from(PlayerRecord) + ) + assert player_count == 0 + + +def test_no_hitting_stats_leaves_database_unchanged(migrated_session: Session) -> None: + client = FakeMlb(person=make_person(), player_stats={}) + with pytest.raises(NoHittingStatsError): + ingest_player_season( + session=migrated_session, player_id=PLAYER_ID, season=SEASON, client=client + ) + player_count = migrated_session.scalar( + select(func.count()).select_from(PlayerRecord) + ) + hitting_count = migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonHittingRecord) + ) + assert player_count == 0 + assert hitting_count == 0 + + +def test_database_error_rolls_back_both_writes(migrated_session: Session) -> None: + """The second persistence operation failing must not leave the first committed.""" + with ( + patch( + "app.services.player_season_ingestion.upsert_player_season_hitting", + side_effect=SQLAlchemyError("db failed"), + ), + pytest.raises(PlayerSeasonIngestionError), + ): + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + player_count = migrated_session.scalar( + select(func.count()).select_from(PlayerRecord) + ) + hitting_count = migrated_session.scalar( + select(func.count()).select_from(PlayerSeasonHittingRecord) + ) + assert player_count == 0 + assert hitting_count == 0 + + +def test_no_partial_state_after_failed_transaction(migrated_session: Session) -> None: + """Failing after the player row is staged still rolls both rows back.""" + + def failing_hitting_upsert(session: Session, *, hitting: object) -> None: + raise SQLAlchemyError("fail after player staged") + + with ( + patch( + "app.services.player_season_ingestion.upsert_player_season_hitting", + side_effect=failing_hitting_upsert, + ), + pytest.raises(PlayerSeasonIngestionError), + ): + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=make_client(), + ) + assert get_player(migrated_session, player_id=PLAYER_ID) is None + assert ( + get_player_season_hitting(migrated_session, player_id=PLAYER_ID, season=SEASON) + is None + ) + + +def test_mlb_retrieval_runs_before_transaction_opens(migrated_session: Session) -> None: + """Both MLB requests must complete before any DB transaction is opened.""" + fetch_order: list[str] = [] + + class TrackingSession: + def __init__(self, real: Session) -> None: + self._real = real + + def begin(self) -> object: + fetch_order.append("transaction_begin") + return self._real.begin() + + def __getattr__(self, name: str) -> object: + return getattr(self._real, name) + + class TrackingClient(FakeMlb): + def get_person(self, player_id: int, **params: object) -> object: + fetch_order.append("get_person") + return super().get_person(player_id, **params) + + def get_player_stats( + self, person_id: int, stats: list, groups: list, **params: object + ) -> dict: + fetch_order.append("get_player_stats") + return super().get_player_stats(person_id, stats, groups, **params) + + client = TrackingClient(person=make_person(), player_stats=MARINERS_STATS) + tracking_session = TrackingSession(migrated_session) + + ingest_player_season( + session=tracking_session, # type: ignore[arg-type] + player_id=PLAYER_ID, + season=SEASON, + client=client, + ) + + assert fetch_order == ["get_person", "get_player_stats", "transaction_begin"] + + +class ClosableFakeMlb(FakeMlb): + """A ``FakeMlb`` that also tracks whether it was used as a context manager.""" + + def __init__(self, *args: object, **kwargs: object) -> None: + super().__init__(*args, **kwargs) + self.closed = False + + def __enter__(self) -> "ClosableFakeMlb": + return self + + def __exit__(self, *exc_info: object) -> None: + self.closed = True + + +def test_no_client_supplied_shares_one_owned_client_for_both_mlb_calls( + monkeypatch, migrated_session: Session +) -> None: + """A missing ``client`` must open exactly one MLB client for the import. + + Fails against the previous implementation, which passed ``client=None`` + into ``get_player_identity`` and ``get_player_season_hitting`` + independently, causing each to construct and close its own ``Mlb()``. + """ + from app.services import player_season_ingestion as ingestion_module + + owned = ClosableFakeMlb(person=make_person(), player_stats=MARINERS_STATS) + construction_count = 0 + + def fake_mlb_factory() -> ClosableFakeMlb: + nonlocal construction_count + construction_count += 1 + return owned + + monkeypatch.setattr(ingestion_module, "Mlb", fake_mlb_factory) + + ingest_player_season(session=migrated_session, player_id=PLAYER_ID, season=SEASON) + + assert construction_count == 1 + assert owned.calls == ["get_person", "get_player_stats"] + assert owned.closed is True + + +def test_supplied_client_is_reused_for_both_calls_and_never_closed( + migrated_session: Session, +) -> None: + """A caller-supplied client must be reused for both MLB calls, never closed.""" + client = ClosableFakeMlb(person=make_person(), player_stats=MARINERS_STATS) + + ingest_player_season( + session=migrated_session, + player_id=PLAYER_ID, + season=SEASON, + client=client, + ) + + assert client.calls == ["get_person", "get_player_stats"] + assert client.closed is False + + +def test_owned_client_closes_when_identity_lookup_fails( + monkeypatch, migrated_session: Session +) -> None: + """The service-owned client must still close if identity retrieval fails.""" + from app.services import player_season_ingestion as ingestion_module + + owned = ClosableFakeMlb(person=None) + monkeypatch.setattr(ingestion_module, "Mlb", lambda: owned) + + with pytest.raises(PlayerNotFoundError): + ingest_player_season( + session=migrated_session, player_id=PLAYER_ID, season=SEASON + ) + + assert owned.closed is True + + +def test_owned_client_closes_when_season_hitting_lookup_fails( + monkeypatch, migrated_session: Session +) -> None: + """The service-owned client must still close if season hitting retrieval fails.""" + from app.services import player_season_ingestion as ingestion_module + + owned = ClosableFakeMlb(person=make_person(), player_stats={}) + monkeypatch.setattr(ingestion_module, "Mlb", lambda: owned) + + with pytest.raises(NoHittingStatsError): + ingest_player_season( + session=migrated_session, player_id=PLAYER_ID, season=SEASON + ) + + assert owned.closed is True diff --git a/tests/test_players_service.py b/tests/test_players_service.py new file mode 100644 index 0000000..f9cf546 --- /dev/null +++ b/tests/test_players_service.py @@ -0,0 +1,318 @@ +"""Tests for player identity and season hitting retrieval/normalization. + +Nothing here touches the network: the ``mlbstatsapi.Mlb`` client is replaced at +the service boundary with ``FakeMlb``, and payloads are built directly from the +library's own Pydantic models -- the same models the real client returns. +""" + +from typing import Any + +import pytest +from mlbstatsapi.exceptions import TheMlbStatsApiException +from mlbstatsapi.models.people import Person, Position +from mlbstatsapi.models.stats import Stat +from mlbstatsapi.models.stats.hitting import HittingSeason, SimpleHittingSplit +from mlbstatsapi.models.teams import Team + +from app.schemas.players import PlayerIdentity, PlayerSeasonHitting +from app.services.players import ( + NoHittingStatsError, + PlayerDataError, + PlayerNotFoundError, + get_player_identity, + get_player_season_hitting, +) + +PLAYER_ID = 677594 +SEASON = 2025 + +CF_POSITION = Position( + code="8", name="Outfielder", type="Outfielder", abbreviation="CF" +) +MARINERS = Team(id=136, link="/api/v1/teams/136", name="Seattle Mariners") +NATIONALS = Team(id=120, link="/api/v1/teams/120", name="Washington Nationals") +PADRES = Team(id=135, link="/api/v1/teams/135", name="San Diego Padres") + + +def make_person(**overrides: object) -> Person: + base: dict[str, Any] = { + "id": PLAYER_ID, + "link": f"/api/v1/people/{PLAYER_ID}", + "full_name": "Julio Rodriguez", + "primary_position": CF_POSITION, + } + base.update(overrides) + return Person(**base) + + +def make_simple_hitting(**overrides: object) -> SimpleHittingSplit: + base: dict[str, Any] = { + "games_played": 150, + "plate_appearances": 600, + "at_bats": 500, + "runs": 80, + "hits": 150, + "doubles": 30, + "triples": 3, + "home_runs": 20, + "rbi": 90, + "base_on_balls": 60, + "intentional_walks": 5, + "hit_by_pitch": 5, + "strikeouts": 100, + "stolen_bases": 10, + "caught_stealing": 3, + "sac_flies": 4, + "sac_bunts": 2, + } + base.update(overrides) + return SimpleHittingSplit(**base) + + +def make_split(*, team: Team | None = None, **stat_overrides: object) -> HittingSeason: + return HittingSeason( + season=str(SEASON), team=team, stat=make_simple_hitting(**stat_overrides) + ) + + +def make_stat(splits: list[HittingSeason]) -> Stat: + return Stat(group="hitting", type="season", totalSplits=len(splits), splits=splits) + + +_DEFAULT = object() + + +class FakeMlb: + """Stands in for ``mlbstatsapi.Mlb`` at the service boundary. + + Either return value may be an exception instance, which is raised instead + of returned. Passing ``person=None`` means the player was not found, which + is different from omitting ``person`` altogether. + """ + + def __init__( + self, + *, + person: Person | Exception | None = _DEFAULT, + player_stats: dict | Exception | None = None, + ) -> None: + self._person = make_person() if person is _DEFAULT else person + self._player_stats = {} if player_stats is None else player_stats + self.calls: list[str] = [] + + def get_person(self, player_id: int, **params: object) -> Person | None: + self.calls.append("get_person") + if isinstance(self._person, Exception): + raise self._person + return self._person + + def get_player_stats( + self, person_id: int, stats: list, groups: list, **params: object + ) -> dict: + self.calls.append("get_player_stats") + if isinstance(self._player_stats, Exception): + raise self._player_stats + return self._player_stats + + +# --------------------------------------------------------------------------- +# Identity normalization +# --------------------------------------------------------------------------- + + +def test_valid_person_normalizes_to_identity() -> None: + client = FakeMlb(person=make_person()) + identity = get_player_identity(PLAYER_ID, client=client) + assert identity == PlayerIdentity( + player_id=PLAYER_ID, full_name="Julio Rodriguez", primary_position="CF" + ) + + +def test_two_way_player_position_abbreviation_is_preserved() -> None: + twp = Position( + code="Y", name="Two-Way Player", type="Two-Way Player", abbreviation="TWP" + ) + client = FakeMlb(person=make_person(primary_position=twp)) + identity = get_player_identity(PLAYER_ID, client=client) + assert identity.primary_position == "TWP" + + +def test_person_not_found_raises_player_not_found() -> None: + client = FakeMlb(person=None) + with pytest.raises(PlayerNotFoundError): + get_player_identity(PLAYER_ID, client=client) + + +def test_missing_full_name_raises_player_data_error() -> None: + client = FakeMlb(person=make_person(full_name=None)) + with pytest.raises(PlayerDataError): + get_player_identity(PLAYER_ID, client=client) + + +def test_missing_primary_position_raises_player_data_error() -> None: + client = FakeMlb(person=make_person(primary_position=None)) + with pytest.raises(PlayerDataError): + get_player_identity(PLAYER_ID, client=client) + + +def test_person_id_mismatch_raises_player_data_error() -> None: + client = FakeMlb(person=make_person(id=999999)) + with pytest.raises(PlayerDataError): + get_player_identity(PLAYER_ID, client=client) + + +def test_identity_request_failure_raises_player_data_error() -> None: + client = FakeMlb(person=TheMlbStatsApiException("network down")) + with pytest.raises(PlayerDataError): + get_player_identity(PLAYER_ID, client=client) + + +# --------------------------------------------------------------------------- +# Season hitting: split selection +# --------------------------------------------------------------------------- + + +def test_single_split_is_used_as_is() -> None: + stat = make_stat([make_split(team=MARINERS)]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + hitting = get_player_season_hitting(PLAYER_ID, SEASON, client=client) + assert hitting.hits == 150 + assert hitting.at_bats == 500 + + +def test_traded_player_selects_the_aggregate_split() -> None: + """An aggregate split (team=None) plus two team-specific splits.""" + aggregate = make_split(team=None, hits=127, at_bats=524, plate_appearances=664) + washington = make_split(team=NATIONALS, hits=84, at_bats=342, plate_appearances=436) + san_diego = make_split(team=PADRES, hits=43, at_bats=182, plate_appearances=228) + stat = make_stat([aggregate, washington, san_diego]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + + hitting = get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + assert hitting.hits == 127 + assert hitting.at_bats == 524 + assert hitting.plate_appearances == 664 + + +def test_zero_splits_raises_no_hitting_stats() -> None: + stat = make_stat([]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + with pytest.raises(NoHittingStatsError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +def test_empty_response_raises_no_hitting_stats() -> None: + client = FakeMlb(player_stats={}) + with pytest.raises(NoHittingStatsError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +def test_missing_season_stat_type_raises_no_hitting_stats() -> None: + client = FakeMlb(player_stats={"hitting": {}}) + with pytest.raises(NoHittingStatsError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +def test_multiple_splits_with_no_aggregate_raises_player_data_error() -> None: + washington = make_split(team=NATIONALS) + san_diego = make_split(team=PADRES) + stat = make_stat([washington, san_diego]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + with pytest.raises(PlayerDataError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +def test_multiple_splits_with_two_aggregates_raises_player_data_error() -> None: + aggregate_one = make_split(team=None, hits=100) + aggregate_two = make_split(team=None, hits=50) + team_specific = make_split(team=NATIONALS) + stat = make_stat([aggregate_one, aggregate_two, team_specific]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + with pytest.raises(PlayerDataError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +def test_hitting_request_failure_raises_player_data_error() -> None: + client = FakeMlb(player_stats=TheMlbStatsApiException("network down")) + with pytest.raises(PlayerDataError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +# --------------------------------------------------------------------------- +# Season hitting: required-field normalization +# --------------------------------------------------------------------------- + + +def test_missing_required_stat_field_raises_player_data_error() -> None: + """A missing counting stat is a data-integrity failure, never a zero.""" + stat = make_stat([make_split(team=MARINERS, hits=None)]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + with pytest.raises(PlayerDataError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +def test_missing_field_does_not_normalize_to_zero() -> None: + """A missing stolen_bases value must fail loudly, not become 0.""" + stat = make_stat([make_split(team=MARINERS, stolen_bases=None)]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + with pytest.raises(PlayerDataError) as exc_info: + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + assert "stolenBases" in str(exc_info.value) + + +def test_normalized_result_matches_expected_domain_model() -> None: + stat = make_stat([make_split(team=MARINERS)]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + hitting = get_player_season_hitting(PLAYER_ID, SEASON, client=client) + assert hitting == PlayerSeasonHitting( + player_id=PLAYER_ID, + season=SEASON, + games_played=150, + plate_appearances=600, + at_bats=500, + runs=80, + hits=150, + doubles=30, + triples=3, + home_runs=20, + rbi=90, + base_on_balls=60, + intentional_walks=5, + hit_by_pitch=5, + strikeouts=100, + stolen_bases=10, + caught_stealing=3, + sac_flies=4, + sac_bunts=2, + ) + + +def test_normalization_validation_failure_raises_player_data_error() -> None: + """Domain-level invariants (e.g. IBB <= BB) surface as PlayerDataError.""" + stat = make_stat([make_split(team=MARINERS, base_on_balls=5, intentional_walks=10)]) + client = FakeMlb(player_stats={"hitting": {"season": stat}}) + with pytest.raises(PlayerDataError): + get_player_season_hitting(PLAYER_ID, SEASON, client=client) + + +def test_no_client_supplied_creates_and_closes_owned_client(monkeypatch) -> None: + """When no client is supplied, an owned ``Mlb`` client is used.""" + from app.services import players as players_module + + class OwnedClient(FakeMlb): + closed = False + + def __enter__(self) -> "OwnedClient": + return self + + def __exit__(self, *exc_info: object) -> None: + self.closed = True + + owned = OwnedClient(person=make_person()) + monkeypatch.setattr(players_module, "Mlb", lambda: owned) + + identity = get_player_identity(PLAYER_ID) + + assert identity.player_id == PLAYER_ID + assert owned.closed is True diff --git a/tests/test_repositories_players.py b/tests/test_repositories_players.py new file mode 100644 index 0000000..0ba6da0 --- /dev/null +++ b/tests/test_repositories_players.py @@ -0,0 +1,233 @@ +"""Tests for player identity and player-season hitting repository functions.""" + +from sqlalchemy.orm import Session + +from app.database.models import PlayerRecord, PlayerSeasonHittingRecord +from app.database.repositories import ( + get_player, + get_player_season_hitting, + upsert_player, + upsert_player_season_hitting, +) +from app.schemas.ingestion import PlayerPersistenceOutcome +from app.schemas.players import PlayerIdentity, PlayerSeasonHitting + +PLAYER_ID = 677594 +SEASON = 2025 + + +def make_identity(**overrides: object) -> PlayerIdentity: + base = { + "player_id": PLAYER_ID, + "full_name": "Julio Rodriguez", + "primary_position": "CF", + } + base.update(overrides) + return PlayerIdentity(**base) + + +def make_hitting(**overrides: object) -> PlayerSeasonHitting: + base = { + "player_id": PLAYER_ID, + "season": SEASON, + "games_played": 150, + "plate_appearances": 600, + "at_bats": 500, + "runs": 80, + "hits": 150, + "doubles": 30, + "triples": 3, + "home_runs": 20, + "rbi": 90, + "base_on_balls": 60, + "intentional_walks": 5, + "hit_by_pitch": 5, + "strikeouts": 100, + "stolen_bases": 10, + "caught_stealing": 3, + "sac_flies": 4, + "sac_bunts": 2, + } + base.update(overrides) + return PlayerSeasonHitting(**base) + + +# --------------------------------------------------------------------------- +# upsert_player +# --------------------------------------------------------------------------- + + +def test_new_player_is_inserted(migrated_session: Session) -> None: + identity = make_identity() + outcome = upsert_player(migrated_session, identity=identity) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.INSERTED + assert get_player(migrated_session, player_id=PLAYER_ID) == identity + + +def test_identical_rerun_is_unchanged(migrated_session: Session) -> None: + identity = make_identity() + upsert_player(migrated_session, identity=identity) + migrated_session.commit() + outcome = upsert_player(migrated_session, identity=identity) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.UNCHANGED + + +def test_changed_identity_updates_the_same_row(migrated_session: Session) -> None: + upsert_player(migrated_session, identity=make_identity()) + migrated_session.commit() + + updated = make_identity(full_name="J-Rod", primary_position="OF") + outcome = upsert_player(migrated_session, identity=updated) + migrated_session.commit() + + assert outcome is PlayerPersistenceOutcome.UPDATED + stored = migrated_session.query(PlayerRecord).all() + assert len(stored) == 1 + assert get_player(migrated_session, player_id=PLAYER_ID) == updated + + +def test_update_preserves_created_at_and_bumps_updated_at( + migrated_session: Session, +) -> None: + upsert_player(migrated_session, identity=make_identity()) + migrated_session.commit() + original = migrated_session.query(PlayerRecord).one() + original_created_at = original.created_at + original_updated_at = original.updated_at + + upsert_player(migrated_session, identity=make_identity(full_name="J-Rod")) + migrated_session.commit() + + stored = migrated_session.query(PlayerRecord).one() + assert stored.created_at == original_created_at + assert stored.updated_at >= original_updated_at + + +def test_unchanged_rerun_does_not_bump_updated_at(migrated_session: Session) -> None: + upsert_player(migrated_session, identity=make_identity()) + migrated_session.commit() + original_updated_at = migrated_session.query(PlayerRecord).one().updated_at + + upsert_player(migrated_session, identity=make_identity()) + migrated_session.commit() + + assert migrated_session.query(PlayerRecord).one().updated_at == original_updated_at + + +def test_unknown_player_returns_none(migrated_session: Session) -> None: + assert get_player(migrated_session, player_id=999999) is None + + +# --------------------------------------------------------------------------- +# upsert_player_season_hitting +# --------------------------------------------------------------------------- + + +def _seed_player(session: Session) -> None: + upsert_player(session, identity=make_identity()) + session.commit() + + +def test_new_season_hitting_is_inserted(migrated_session: Session) -> None: + _seed_player(migrated_session) + hitting = make_hitting() + outcome = upsert_player_season_hitting(migrated_session, hitting=hitting) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.INSERTED + stored = get_player_season_hitting( + migrated_session, player_id=PLAYER_ID, season=SEASON + ) + assert stored == hitting + + +def test_identical_season_hitting_rerun_is_unchanged(migrated_session: Session) -> None: + _seed_player(migrated_session) + hitting = make_hitting() + upsert_player_season_hitting(migrated_session, hitting=hitting) + migrated_session.commit() + outcome = upsert_player_season_hitting(migrated_session, hitting=hitting) + migrated_session.commit() + assert outcome is PlayerPersistenceOutcome.UNCHANGED + + +def test_changed_season_hitting_updates_the_same_row( + migrated_session: Session, +) -> None: + _seed_player(migrated_session) + upsert_player_season_hitting(migrated_session, hitting=make_hitting()) + migrated_session.commit() + + updated = make_hitting(hits=175, home_runs=25) + outcome = upsert_player_season_hitting(migrated_session, hitting=updated) + migrated_session.commit() + + assert outcome is PlayerPersistenceOutcome.UPDATED + stored = migrated_session.query(PlayerSeasonHittingRecord).all() + assert len(stored) == 1 + assert ( + get_player_season_hitting(migrated_session, player_id=PLAYER_ID, season=SEASON) + == updated + ) + + +def test_in_progress_season_totals_can_increase_without_duplicating( + migrated_session: Session, +) -> None: + """Simulates a mid-season rerun where counting stats have only grown.""" + _seed_player(migrated_session) + upsert_player_season_hitting( + migrated_session, + hitting=make_hitting( + games_played=50, hits=50, at_bats=180, doubles=5, triples=1, home_runs=5 + ), + ) + migrated_session.commit() + + grown = make_hitting( + games_played=100, hits=110, at_bats=360, doubles=10, triples=2, home_runs=10 + ) + upsert_player_season_hitting(migrated_session, hitting=grown) + migrated_session.commit() + + stored_rows = migrated_session.query(PlayerSeasonHittingRecord).all() + assert len(stored_rows) == 1 + assert stored_rows[0].hits == 110 + + +def test_update_preserves_created_at_and_bumps_updated_at_for_hitting( + migrated_session: Session, +) -> None: + _seed_player(migrated_session) + upsert_player_season_hitting(migrated_session, hitting=make_hitting()) + migrated_session.commit() + original = migrated_session.query(PlayerSeasonHittingRecord).one() + original_created_at = original.created_at + + upsert_player_season_hitting(migrated_session, hitting=make_hitting(hits=160)) + migrated_session.commit() + + stored = migrated_session.query(PlayerSeasonHittingRecord).one() + assert stored.created_at == original_created_at + assert stored.updated_at >= original.updated_at + + +def test_different_seasons_for_same_player_are_separate_rows( + migrated_session: Session, +) -> None: + _seed_player(migrated_session) + upsert_player_season_hitting(migrated_session, hitting=make_hitting(season=2024)) + upsert_player_season_hitting(migrated_session, hitting=make_hitting(season=2025)) + migrated_session.commit() + + stored = migrated_session.query(PlayerSeasonHittingRecord).all() + assert {row.season for row in stored} == {2024, 2025} + + +def test_unknown_player_season_returns_none(migrated_session: Session) -> None: + _seed_player(migrated_session) + assert ( + get_player_season_hitting(migrated_session, player_id=PLAYER_ID, season=1999) + is None + )