From 1639efa7727569c3a1ff401ea5f733020018269b Mon Sep 17 00:00:00 2001 From: Matthew Spah Date: Thu, 1 Oct 2026 08:30:25 -0700 Subject: [PATCH 1/2] db: enforce SQLite foreign keys on application connections SQLite ignores declared FOREIGN KEY clauses unless each connection runs PRAGMA foreign_keys = ON. build_engine() now attaches an engine-scoped connect listener for SQLite URLs only, so orphan player_seasons and player_season_hitting rows are rejected. Alembic keeps its own engine with enforcement off; migrations were verified to pass either way. Closes #63 Co-Authored-By: Claude Opus 5.5 --- app/database/engine.py | 37 ++++- docs/architecture-deep-dive.md | 17 +++ tests/test_database_foreign_keys.py | 224 ++++++++++++++++++++++++++++ 3 files changed, 273 insertions(+), 5 deletions(-) create mode 100644 tests/test_database_foreign_keys.py diff --git a/app/database/engine.py b/app/database/engine.py index 37708e6..5eb8fba 100644 --- a/app/database/engine.py +++ b/app/database/engine.py @@ -1,18 +1,45 @@ """Database engine and session factory construction.""" -from sqlalchemy import create_engine -from sqlalchemy.engine import Engine +from sqlalchemy import create_engine, event +from sqlalchemy.engine import Engine, make_url +from sqlalchemy.engine.interfaces import DBAPIConnection from sqlalchemy.orm import Session, sessionmaker +from sqlalchemy.pool import ConnectionPoolEntry def build_engine(database_url: str) -> Engine: - """Create a SQLAlchemy engine for the given database URL.""" + """Create a SQLAlchemy engine for the given database URL. + + SQLite engines enable foreign-key enforcement on every new connection. + """ + is_sqlite = make_url(database_url).get_backend_name() == "sqlite" connect_args: dict[str, object] = {} - if database_url.startswith("sqlite"): + if is_sqlite: connect_args["check_same_thread"] = False - return create_engine(database_url, connect_args=connect_args) + engine = create_engine(database_url, connect_args=connect_args) + if is_sqlite: + event.listen(engine, "connect", _enable_sqlite_foreign_keys) + return engine def build_session_factory(engine: Engine) -> sessionmaker[Session]: """Create a session factory bound to ``engine``.""" return sessionmaker(bind=engine, autocommit=False, autoflush=False) + + +def _enable_sqlite_foreign_keys( + dbapi_connection: DBAPIConnection, + connection_record: ConnectionPoolEntry, +) -> None: + """Turn on enforcement of the schema's declared foreign keys. + + SQLite parses ``FOREIGN KEY`` clauses but ignores them unless each + connection sets this pragma. The pragma is a no-op inside an open + transaction; it takes effect here because the driver's default (legacy) + transaction control has not begun one when a connection is first opened. + """ + cursor = dbapi_connection.cursor() + try: + cursor.execute("PRAGMA foreign_keys = ON") + finally: + cursor.close() diff --git a/docs/architecture-deep-dive.md b/docs/architecture-deep-dive.md index 2cdaa19..c63876d 100644 --- a/docs/architecture-deep-dive.md +++ b/docs/architecture-deep-dive.md @@ -155,6 +155,23 @@ Migrations are Alembic (`alembic/versions/`); three so far, matching the project's milestones (create batting lines table → add league ingestion state → add batting strikeouts column). +**Foreign keys (added in issue #63).** SQLite parses `FOREIGN KEY` clauses +but ignores them unless each connection runs `PRAGMA foreign_keys = ON`. +`build_engine()` (`app/database/engine.py`) registers an engine-scoped +`connect` listener that sets the pragma for SQLite URLs only, so the web +app, import scripts, and tests all reject orphan `player_seasons` and +`player_season_hitting` rows. Repositories still add Player identity before +membership and hitting; the ORM flushes parents first because the ordering +comes from the declared `ForeignKey`s, not from `relationship()`s. + +Alembic builds its own engine in `alembic/env.py` and does not go through +`build_engine()`, so migrations run with SQLite's default (enforcement off). +That was left in place on purpose: fresh upgrades and downgrades pass +either way, and SQLite's `batch_alter_table` table rebuilds are safest with +enforcement off. Enforcement also applies only to new writes. Rows +committed earlier are not re-checked; `PRAGMA foreign_key_check` lists any +orphans. + ## 3. Analytics — `app/analytics/` Pure functions: given `Sequence[TeamGameBattingLine]` in, a typed diff --git a/tests/test_database_foreign_keys.py b/tests/test_database_foreign_keys.py new file mode 100644 index 0000000..6cd0518 --- /dev/null +++ b/tests/test_database_foreign_keys.py @@ -0,0 +1,224 @@ +"""Tests that application SQLite connections enforce declared foreign keys. + +SQLite ignores ``FOREIGN KEY`` clauses unless each connection enables +``PRAGMA foreign_keys``. These tests prove enforcement against a database +migrated through Alembic, using the same ``build_engine`` path the +application, CLI scripts, and repository tests use. +""" + +from collections.abc import Generator +from pathlib import Path +from unittest.mock import patch + +import pytest +from sqlalchemy import create_engine, func, select, text +from sqlalchemy.engine import Engine +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session + +from app.database import engine as engine_module +from app.database.engine import build_engine, build_session_factory +from app.database.models import PlayerSeasonCatalogRecord, PlayerSeasonHittingRecord +from app.database.repositories import ( + ensure_player_season_catalog_membership, + get_player, + get_player_catalog_entry, + get_player_season_hitting, + upsert_player, + upsert_player_season_hitting, +) +from app.schemas.players import PlayerIdentity, PlayerSeasonHitting +from tests.conftest import run_alembic_upgrade + +PLAYER_ID = 677594 +SEASON = 2025 + + +def make_identity() -> PlayerIdentity: + return PlayerIdentity( + player_id=PLAYER_ID, full_name="Julio Rodriguez", primary_position="CF" + ) + + +def make_hitting() -> PlayerSeasonHitting: + return 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, + ) + + +@pytest.fixture +def migrated_engine(migrated_db_path: Path) -> Generator[Engine, None, None]: + engine = build_engine(f"sqlite:///{migrated_db_path}") + try: + yield engine + finally: + engine.dispose() + + +def foreign_keys_pragma(engine: Engine) -> int: + with engine.connect() as connection: + return connection.execute(text("PRAGMA foreign_keys")).scalar_one() + + +def count_rows(session: Session, model: type) -> int: + return session.execute(select(func.count()).select_from(model)).scalar_one() + + +def test_sqlite_engine_connections_enable_foreign_keys( + migrated_engine: Engine, +) -> None: + assert foreign_keys_pragma(migrated_engine) == 1 + + +def test_every_new_sqlite_connection_enables_foreign_keys( + migrated_engine: Engine, +) -> None: + # Hold two connections at once so the pool must open a second DBAPI + # connection rather than reusing the first. + with migrated_engine.connect() as first, migrated_engine.connect() as second: + assert first.execute(text("PRAGMA foreign_keys")).scalar_one() == 1 + assert second.execute(text("PRAGMA foreign_keys")).scalar_one() == 1 + + +def test_orphan_player_season_membership_is_rejected( + migrated_session: Session, +) -> None: + ensure_player_season_catalog_membership( + migrated_session, identity=make_identity(), season=SEASON + ) + + with pytest.raises(IntegrityError, match="FOREIGN KEY constraint failed"): + migrated_session.commit() + + migrated_session.rollback() + assert count_rows(migrated_session, PlayerSeasonCatalogRecord) == 0 + + +def test_orphan_player_season_hitting_is_rejected( + migrated_session: Session, +) -> None: + upsert_player_season_hitting(migrated_session, hitting=make_hitting()) + + with pytest.raises(IntegrityError, match="FOREIGN KEY constraint failed"): + migrated_session.commit() + + migrated_session.rollback() + assert count_rows(migrated_session, PlayerSeasonHittingRecord) == 0 + + +def test_raw_orphan_player_season_hitting_insert_is_rejected( + migrated_engine: Engine, +) -> None: + """Enforcement is a database guarantee, not an ORM side effect.""" + with ( + pytest.raises(IntegrityError, match="FOREIGN KEY constraint failed"), + migrated_engine.begin() as connection, + ): + connection.execute( + text( + """ + INSERT INTO player_season_hitting ( + 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 + ) VALUES ( + 999999, 2025, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, '2025-01-01 00:00:00', + '2025-01-01 00:00:00' + ) + """ + ) + ) + + +def test_valid_player_parent_child_persistence_succeeds_in_one_commit( + migrated_session: Session, +) -> None: + """Identity, membership, and hitting added together flush parent-first.""" + identity = make_identity() + upsert_player(migrated_session, identity=identity) + ensure_player_season_catalog_membership( + migrated_session, identity=identity, season=SEASON + ) + upsert_player_season_hitting(migrated_session, hitting=make_hitting()) + migrated_session.commit() + + assert get_player(migrated_session, player_id=PLAYER_ID) == identity + catalog_entry = get_player_catalog_entry( + migrated_session, player_id=PLAYER_ID, season=SEASON + ) + assert catalog_entry is not None + assert catalog_entry.player_id == PLAYER_ID + assert ( + get_player_season_hitting(migrated_session, player_id=PLAYER_ID, season=SEASON) + == make_hitting() + ) + + +def test_freshly_migrated_database_has_no_foreign_key_violations( + tmp_path: Path, +) -> None: + db_path = tmp_path / "fresh.db" + url = f"sqlite:///{db_path}" + run_alembic_upgrade(url) + + engine = build_engine(url) + try: + assert foreign_keys_pragma(engine) == 1 + with engine.connect() as connection: + violations = connection.execute(text("PRAGMA foreign_key_check")).all() + assert violations == [] + finally: + engine.dispose() + + +def test_non_sqlite_url_does_not_install_sqlite_foreign_key_pragma() -> None: + """A non-SQLite URL must not be coupled to SQLite connection setup. + + No PostgreSQL driver is installed, so ``create_engine`` is intercepted and + returns an in-memory SQLite engine. If ``build_engine`` wrongly attached + the SQLite hook, that engine's connections would report the pragma on. + """ + stand_in = create_engine("sqlite://") + with patch.object( + engine_module, "create_engine", return_value=stand_in + ) as fake_create_engine: + engine = build_engine("postgresql://user@localhost/mlb") + + try: + fake_create_engine.assert_called_once_with( + "postgresql://user@localhost/mlb", connect_args={} + ) + assert foreign_keys_pragma(engine) == 0 + finally: + engine.dispose() + + +def test_session_factory_sessions_enforce_foreign_keys( + migrated_engine: Engine, +) -> None: + session = build_session_factory(migrated_engine)() + try: + assert session.execute(text("PRAGMA foreign_keys")).scalar_one() == 1 + finally: + session.close() From 2037b51b2f7a402796299062de9c7ffe2a4fbcbc Mon Sep 17 00:00:00 2001 From: Matthew Spah Date: Thu, 1 Oct 2026 14:18:38 -0700 Subject: [PATCH 2/2] fix: guarantee Player parent-before-child writes under SQLite FKs The Player models declare ForeignKeys but no relationship(), so SQLAlchemy's unit of work has no dependency edge between the players, player_seasons, and player_season_hitting mappers and orders their INSERT batches by module + class name. Valid writes only succeeded because PlayerRecord sorts first alphabetically. upsert_player() now flushes a newly added player immediately, so the parent row exists in the caller's open transaction before child rows reference it. Ingestion stays one transaction with one commit; updates and unchanged players do not flush. Harden the SQLite connect hook per SQLAlchemy's dialect guidance: run PRAGMA foreign_keys = ON with sqlite3 autocommit temporarily enabled, restore the previous mode, and read the pragma back, raising SQLiteForeignKeyEnforcementError if enforcement is not on. Tests run valid single-player and catalog ingestion (including reruns and updates) under both unit-of-work orders, prove a failed child write rolls back the flushed parent, and cover the hook with an already-open transaction. Docs now state precisely that Alembic runs on its own non-enforcing engine and that only the migrated result is checked under enforcement, correcting the earlier "pass either way" and "ORM flushes parents first" claims. Refs #63 Co-Authored-By: Claude Opus 5.5 --- app/database/engine.py | 38 +++++- app/database/repositories.py | 9 +- docs/architecture-deep-dive.md | 38 ++++-- tests/test_database_foreign_keys.py | 181 ++++++++++++++++++++++++++-- 4 files changed, 236 insertions(+), 30 deletions(-) diff --git a/app/database/engine.py b/app/database/engine.py index 5eb8fba..75cd891 100644 --- a/app/database/engine.py +++ b/app/database/engine.py @@ -1,5 +1,7 @@ """Database engine and session factory construction.""" +import sqlite3 + from sqlalchemy import create_engine, event from sqlalchemy.engine import Engine, make_url from sqlalchemy.engine.interfaces import DBAPIConnection @@ -27,6 +29,10 @@ def build_session_factory(engine: Engine) -> sessionmaker[Session]: return sessionmaker(bind=engine, autocommit=False, autoflush=False) +class SQLiteForeignKeyEnforcementError(RuntimeError): + """A SQLite connection could not be configured to enforce foreign keys.""" + + def _enable_sqlite_foreign_keys( dbapi_connection: DBAPIConnection, connection_record: ConnectionPoolEntry, @@ -34,12 +40,32 @@ def _enable_sqlite_foreign_keys( """Turn on enforcement of the schema's declared foreign keys. SQLite parses ``FOREIGN KEY`` clauses but ignores them unless each - connection sets this pragma. The pragma is a no-op inside an open - transaction; it takes effect here because the driver's default (legacy) - transaction control has not begun one when a connection is first opened. + connection sets ``PRAGMA foreign_keys = ON``, and silently ignores that + pragma inside an open transaction. Following SQLAlchemy's SQLite dialect + guidance, the pragma runs with ``sqlite3`` autocommit temporarily on, so + it never depends on the driver's transaction-control mode. The setting is + read back so a connection that cannot enforce foreign keys fails loudly. """ - cursor = dbapi_connection.cursor() + if not isinstance(dbapi_connection, sqlite3.Connection): + raise SQLiteForeignKeyEnforcementError( + "Expected a sqlite3 connection for a SQLite database URL, got " + f"{type(dbapi_connection).__name__}" + ) + + previous_autocommit = dbapi_connection.autocommit + dbapi_connection.autocommit = True try: - cursor.execute("PRAGMA foreign_keys = ON") + cursor = dbapi_connection.cursor() + try: + cursor.execute("PRAGMA foreign_keys = ON") + enabled = cursor.execute("PRAGMA foreign_keys").fetchone() + finally: + cursor.close() finally: - cursor.close() + dbapi_connection.autocommit = previous_autocommit + + if enabled != (1,): + raise SQLiteForeignKeyEnforcementError( + "SQLite did not enable foreign-key enforcement " + f"(PRAGMA foreign_keys returned {enabled!r})" + ) diff --git a/app/database/repositories.py b/app/database/repositories.py index 0914d57..4bf7ae2 100644 --- a/app/database/repositories.py +++ b/app/database/repositories.py @@ -636,13 +636,20 @@ def upsert_player( ) -> PlayerPersistenceOutcome: """Insert, update, or leave unchanged the one row for a player's identity. - Does not commit or roll back. + Does not commit or roll back. A newly inserted player is flushed + immediately, so the row exists inside the caller's open transaction before + any ``player_seasons`` or ``player_season_hitting`` row references it. """ 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)) + # The models declare ForeignKeys but no relationship(), so the unit of + # work has no parent/child dependency between these mappers and orders + # their INSERTs by class name. Flushing here makes the parent row + # visible to SQLite's foreign-key checks without relying on that. + session.flush() return PlayerPersistenceOutcome.INSERTED if record.to_domain() == identity: diff --git a/docs/architecture-deep-dive.md b/docs/architecture-deep-dive.md index c63876d..83e408a 100644 --- a/docs/architecture-deep-dive.md +++ b/docs/architecture-deep-dive.md @@ -158,19 +158,37 @@ state → add batting strikeouts column). **Foreign keys (added in issue #63).** SQLite parses `FOREIGN KEY` clauses but ignores them unless each connection runs `PRAGMA foreign_keys = ON`. `build_engine()` (`app/database/engine.py`) registers an engine-scoped -`connect` listener that sets the pragma for SQLite URLs only, so the web -app, import scripts, and tests all reject orphan `player_seasons` and -`player_season_hitting` rows. Repositories still add Player identity before -membership and hitting; the ORM flushes parents first because the ordering -comes from the declared `ForeignKey`s, not from `relationship()`s. +`connect` listener for SQLite URLs only, so the web app, import scripts, and +tests all reject orphan `player_seasons` and `player_season_hitting` rows. +SQLite silently ignores the pragma inside an open transaction, so the +listener follows SQLAlchemy's SQLite dialect guidance: it sets the `sqlite3` +connection's `autocommit` to `True` while running the pragma, restores the +previous value, and reads the pragma back, raising +`SQLiteForeignKeyEnforcementError` if enforcement did not turn on. +Non-SQLite URLs get no listener. + +**Parent-before-child writes.** The Player models declare `ForeignKey`s but +no `relationship()`s. SQLAlchemy's unit of work only orders INSERTs across +mappers when a relationship links them; otherwise it orders mapper batches +by module and class name. `PlayerRecord` happens to sort before +`PlayerSeasonCatalogRecord` and `PlayerSeasonHittingRecord`, but that is +coincidence, not a guarantee. `upsert_player()` therefore flushes +immediately after adding a new player, so the `players` row exists in the +caller's open transaction before membership or hitting rows reference it. +The flush does not commit: single-player and catalog ingestion still run in +one `session.begin()` block and roll back together on failure. Updates and +unchanged players do not flush. Alembic builds its own engine in `alembic/env.py` and does not go through `build_engine()`, so migrations run with SQLite's default (enforcement off). -That was left in place on purpose: fresh upgrades and downgrades pass -either way, and SQLite's `batch_alter_table` table rebuilds are safest with -enforcement off. Enforcement also applies only to new writes. Rows -committed earlier are not re-checked; `PRAGMA foreign_key_check` lists any -orphans. +That is deliberate: SQLite's `batch_alter_table` table rebuilds are safest +with enforcement off, and changing it is outside issue #63. The automated +test (`tests/test_database_foreign_keys.py`) upgrades a fresh database +through that normal Alembic path, then opens it with an enforcing +application engine and checks `PRAGMA foreign_key_check` reports no +violations. Migrations are **not** exercised with enforcement on. +Enforcement also applies only to new writes. Rows committed earlier are not +re-checked; `PRAGMA foreign_key_check` lists any orphans. ## 3. Analytics — `app/analytics/` diff --git a/tests/test_database_foreign_keys.py b/tests/test_database_foreign_keys.py index 6cd0518..e143532 100644 --- a/tests/test_database_foreign_keys.py +++ b/tests/test_database_foreign_keys.py @@ -2,13 +2,18 @@ SQLite ignores ``FOREIGN KEY`` clauses unless each connection enables ``PRAGMA foreign_keys``. These tests prove enforcement against a database -migrated through Alembic, using the same ``build_engine`` path the -application, CLI scripts, and repository tests use. +migrated through the project's Alembic path, using the same ``build_engine`` +path the application, CLI scripts, and repository tests use. + +Alembic itself runs on its own engine (``alembic/env.py``) without the +pragma. Nothing here runs migrations with enforcement on; the migration test +only checks the migrated result through an enforcing application engine. """ +import sqlite3 from collections.abc import Generator from pathlib import Path -from unittest.mock import patch +from unittest.mock import Mock, patch import pytest from sqlalchemy import create_engine, func, select, text @@ -17,8 +22,17 @@ from sqlalchemy.orm import Session from app.database import engine as engine_module -from app.database.engine import build_engine, build_session_factory -from app.database.models import PlayerSeasonCatalogRecord, PlayerSeasonHittingRecord +from app.database.engine import ( + SQLiteForeignKeyEnforcementError, + _enable_sqlite_foreign_keys, + build_engine, + build_session_factory, +) +from app.database.models import ( + PlayerRecord, + PlayerSeasonCatalogRecord, + PlayerSeasonHittingRecord, +) from app.database.repositories import ( ensure_player_season_catalog_membership, get_player, @@ -27,8 +41,14 @@ upsert_player, upsert_player_season_hitting, ) +from app.schemas.ingestion import PlayerPersistenceOutcome from app.schemas.players import PlayerIdentity, PlayerSeasonHitting +from app.services.player_catalog_ingestion import ingest_player_catalog +from app.services.player_season_ingestion import ingest_player_season from tests.conftest import run_alembic_upgrade +from tests.test_player_catalog_ingestion import FakeDirectory +from tests.test_player_catalog_ingestion import make_person as make_catalog_person +from tests.test_players_service import FakeMlb, make_person, make_split, make_stat PLAYER_ID = 677594 SEASON = 2025 @@ -73,6 +93,24 @@ def migrated_engine(migrated_db_path: Path) -> Generator[Engine, None, None]: engine.dispose() +@pytest.fixture(params=["declaration_order", "players_flushed_last"]) +def unit_of_work_order( + request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch +) -> str: + """Run a valid-write test under both possible unit-of-work INSERT orders. + + The Player models declare ``ForeignKey``s but no ``relationship()``, so + SQLAlchemy's unit of work has no parent/child edge between the mappers and + orders their INSERT batches by ``mapper._sort_key`` (module + class name). + ``PlayerRecord`` currently sorts first only by alphabetical coincidence. + ``players_flushed_last`` simulates a rename that would sort it last, so + these tests prove valid writes do not depend on class names. + """ + if request.param == "players_flushed_last": + monkeypatch.setattr(PlayerRecord.__mapper__, "_sort_key", "~players_last") + return request.param + + def foreign_keys_pragma(engine: Engine) -> int: with engine.connect() as connection: return connection.execute(text("PRAGMA foreign_keys")).scalar_one() @@ -151,17 +189,33 @@ def test_raw_orphan_player_season_hitting_insert_is_rejected( ) -def test_valid_player_parent_child_persistence_succeeds_in_one_commit( - migrated_session: Session, +def test_new_player_identity_and_membership_succeed_in_one_transaction( + migrated_session: Session, unit_of_work_order: str ) -> None: - """Identity, membership, and hitting added together flush parent-first.""" identity = make_identity() - upsert_player(migrated_session, identity=identity) - ensure_player_season_catalog_membership( - migrated_session, identity=identity, season=SEASON + with migrated_session.begin(): + upsert_player(migrated_session, identity=identity) + ensure_player_season_catalog_membership( + migrated_session, identity=identity, season=SEASON + ) + + assert get_player(migrated_session, player_id=PLAYER_ID) == identity + assert ( + get_player_catalog_entry(migrated_session, player_id=PLAYER_ID, season=SEASON) + is not None ) - upsert_player_season_hitting(migrated_session, hitting=make_hitting()) - migrated_session.commit() + + +def test_new_player_identity_membership_and_hitting_succeed_in_one_transaction( + migrated_session: Session, unit_of_work_order: str +) -> None: + identity = make_identity() + with migrated_session.begin(): + upsert_player(migrated_session, identity=identity) + ensure_player_season_catalog_membership( + migrated_session, identity=identity, season=SEASON + ) + upsert_player_season_hitting(migrated_session, hitting=make_hitting()) assert get_player(migrated_session, player_id=PLAYER_ID) == identity catalog_entry = get_player_catalog_entry( @@ -175,9 +229,80 @@ def test_valid_player_parent_child_persistence_succeeds_in_one_commit( ) +def test_failed_child_write_rolls_back_flushed_new_player( + migrated_session: Session, +) -> None: + """The early parent flush stays inside the caller's single transaction.""" + with pytest.raises(IntegrityError), migrated_session.begin(): + upsert_player(migrated_session, identity=make_identity()) + # A hitting row for a different, unknown player is still an orphan. + upsert_player_season_hitting( + migrated_session, + hitting=make_hitting().model_copy(update={"player_id": 1}), + ) + + assert count_rows(migrated_session, PlayerRecord) == 0 + assert count_rows(migrated_session, PlayerSeasonHittingRecord) == 0 + + +def test_player_season_ingestion_succeeds_and_reruns_with_enforcement( + migrated_session: Session, unit_of_work_order: str +) -> None: + client = FakeMlb( + person=make_person(), + player_stats={"hitting": {"season": make_stat([make_split()])}}, + ) + + first = ingest_player_season( + session=migrated_session, player_id=PLAYER_ID, season=SEASON, client=client + ) + rerun = ingest_player_season( + session=migrated_session, player_id=PLAYER_ID, season=SEASON, client=client + ) + + assert first.identity_outcome is PlayerPersistenceOutcome.INSERTED + assert first.hitting_outcome is PlayerPersistenceOutcome.INSERTED + assert rerun.identity_outcome is PlayerPersistenceOutcome.UNCHANGED + assert rerun.hitting_outcome is PlayerPersistenceOutcome.UNCHANGED + assert count_rows(migrated_session, PlayerRecord) == 1 + assert count_rows(migrated_session, PlayerSeasonCatalogRecord) == 1 + assert count_rows(migrated_session, PlayerSeasonHittingRecord) == 1 + + +def test_player_catalog_ingestion_succeeds_and_reruns_with_enforcement( + migrated_session: Session, unit_of_work_order: str +) -> None: + people = [ + make_catalog_person(player_id=PLAYER_ID, full_name="Julio Rodriguez"), + make_catalog_person(player_id=663728, full_name="Cal Raleigh"), + ] + + first = ingest_player_catalog( + session=migrated_session, season=SEASON, client=FakeDirectory(people) + ) + renamed = [people[0], make_catalog_person(player_id=663728, full_name="Big Dumper")] + rerun = ingest_player_catalog( + session=migrated_session, season=SEASON, client=FakeDirectory(renamed) + ) + next_season = ingest_player_catalog( + session=migrated_session, season=SEASON + 1, client=FakeDirectory(renamed) + ) + + assert (first.inserted, first.updated, first.unchanged) == (2, 0, 0) + assert (rerun.inserted, rerun.updated, rerun.unchanged) == (0, 1, 1) + assert next_season.inserted == 2 + assert count_rows(migrated_session, PlayerRecord) == 2 + assert count_rows(migrated_session, PlayerSeasonCatalogRecord) == 4 + + def test_freshly_migrated_database_has_no_foreign_key_violations( tmp_path: Path, ) -> None: + """Upgrade through the project's Alembic path, then inspect with enforcement. + + Alembic runs on its own non-enforcing engine; this verifies the migrated + result, not migration execution under enforcement. + """ db_path = tmp_path / "fresh.db" url = f"sqlite:///{db_path}" run_alembic_upgrade(url) @@ -222,3 +347,33 @@ def test_session_factory_sessions_enforce_foreign_keys( assert session.execute(text("PRAGMA foreign_keys")).scalar_one() == 1 finally: session.close() + + +def test_hook_enables_foreign_keys_even_if_a_transaction_is_open() -> None: + """SQLite ignores the pragma inside a transaction; the hook must not.""" + connection = sqlite3.connect(":memory:", autocommit=False) + try: + assert connection.in_transaction + + _enable_sqlite_foreign_keys(connection, Mock()) + + assert connection.execute("PRAGMA foreign_keys").fetchone() == (1,) + assert connection.autocommit is False + finally: + connection.close() + + +def test_hook_restores_legacy_transaction_control() -> None: + connection = sqlite3.connect(":memory:") + try: + _enable_sqlite_foreign_keys(connection, Mock()) + + assert connection.autocommit == sqlite3.LEGACY_TRANSACTION_CONTROL + assert connection.execute("PRAGMA foreign_keys").fetchone() == (1,) + finally: + connection.close() + + +def test_hook_rejects_non_sqlite3_connection() -> None: + with pytest.raises(SQLiteForeignKeyEnforcementError, match="sqlite3"): + _enable_sqlite_foreign_keys(Mock(), Mock())