Enforce SQLite foreign keys on application connections

[Mattsface]

Summary

Closes #63.

SQLite parses declared FOREIGN KEY constraints but does not enforce them unless each connection enables:

PRAGMA foreign_keys = ON;

This PR enables FK enforcement for application-created SQLite connections and fixes Player persistence ordering so valid parent/child writes remain atomic and reliable under enforcement.

What changed

• Enable SQLite foreign-key enforcement in build_engine() using an engine-scoped connection listener.
• Follow SQLAlchemy's SQLite guidance by temporarily enabling DBAPI autocommit while setting the pragma, then restoring the prior mode.
• Read the pragma back and fail loudly if enforcement did not become active.
• Keep SQLite-specific behavior scoped away from non-SQLite URLs.
• Flush a newly inserted PlayerRecord before dependent season membership or hitting rows are added, without committing the transaction.
• Preserve single-transaction rollback semantics for Player ingestion.
• Add behavioral tests for orphan rejection, valid Player persistence, ingestion, reruns, rollback, and multiple SQLite connections.
• Update the architecture documentation to describe the FK and parent/child ordering guarantees accurately.

Parent-before-child persistence

The Player mappers declare ForeignKey constraints but do not define ORM relationship() dependencies. With FK enforcement enabled, relying on incidental mapper ordering would be unsafe.

upsert_player() now flushes a newly added Player row immediately so the parent exists inside the same open transaction before player_seasons or player_season_hitting rows reference it.

This is a flush only, not a commit. A later failure still rolls back the entire logical ingestion.

Alembic note

Alembic continues to construct its own engine in alembic/env.py and therefore does not use the application FK-enforcement hook.

That is deliberate for this issue. The test suite verifies:

1. a fresh database upgrades through the project's normal Alembic path;
2. the migrated database is then opened through the enforcing application engine; and
3. PRAGMA foreign_key_check reports no violations.

This PR does not claim that migrations themselves execute with FK enforcement enabled.

Verification

Expected PR checks:

poetry run pytest
poetry run ruff check .
poetry run ruff format --check .

[Mattsface]

@claude review

[claude]

Claude finished @Mattsface's task in 25s —— View job

---

Review of PR #65: Enforce SQLite foreign keys

• Read the diff (engine.py, repositories.py, docs, tests)
• Check the Python version floor and other engine construction sites
• Post the review

I found no blocking issues. I read the code and did not run the test suite, so CI is the source for pass/fail.

What I checked

app/database/engine.py

• The pragma is set under temporary autocommit = True and then restored. This is the right way to make PRAGMA foreign_keys take effect regardless of the driver's transaction mode.
• Connection.autocommit needs Python 3.12 or later. pyproject.toml has python = "^3.12" and CI uses 3.12, so it's safe. The attribute is why the floor matters, so it's worth keeping in mind if the floor is ever lowered.
• The read-back check, enabled != (1,), makes a failure loud instead of silent.
• make_url(...).get_backend_name() is more robust than the old startswith("sqlite"). For example, it handles sqlite+pysqlite://.
• build_engine() is the only create_engine call site in app/, so every application connection gets the hook.

app/database/repositories.py

• session.flush() after adding a new PlayerRecord is a minimal fix. It stays inside the caller's transaction, so rollback behavior is preserved. The comment explains why it is needed, namely that the models have no relationship().
• Updates and unchanged players don't flush, so existing performance and behavior are untouched.
• Catalog ingestion of many new players now issues one INSERT flush per new player. For a full-league catalog that is a few hundred extra flushes. That is acceptable for an ingestion path and keeps the design simple. It's only worth revisiting if ingestion time becomes a measurable problem.

Tests

• The unit_of_work_order fixture patches _sort_key to simulate the worst-case ordering, so the tests check that valid writes don't depend on class-name sorting. It does rely on the private attribute Mapper._sort_key. If SQLAlchemy renames it, monkeypatch.setattr on a missing attribute will raise, so the tests would fail loudly. That is acceptable.
• There is coverage for orphan rejection through the ORM and through raw SQL, multiple pooled connections, rollback of the flushed parent, ingestion reruns, and a non-SQLite URL.
• Tests that import _enable_sqlite_foreign_keys use a private name. It is reasonable for hook-level tests, and the alternative of testing only via build_engine is already covered.

Docs

• The Alembic caveat is stated accurately: migrations are not run with enforcement on, and the PR doesn't claim they are. That matches the AGENTS.md rule on documentation honesty.

Minor suggestions (non-blocking)

1. Docs claim about batch_alter_table. The doc says table rebuilds are "safest with enforcement off". That is true. A FK-enabled rebuild can fail or cascade on DROP TABLE. It might help to add a one-line pointer to SQLite's documented table-rebuild procedure so a future maintainer can verify the reasoning.
2. test_non_sqlite_url_does_not_install_sqlite_foreign_key_pragma. It patches create_engine and asserts connect_args={}. This couples the test to the exact call signature. It is still a useful guard, but it would break if build_engine ever passes another keyword argument.
3. Existing data. The docs note that enforcement only applies to new writes and that PRAGMA foreign_key_check lists old orphans. Consider whether a small script or CLI step for running that check against a real local database is worth a follow-up issue. It doesn't belong in this PR.

Verdict

The change is focused, matches the repository's architecture rules, and the limitations are documented. It looks good to merge once the CI checks (pytest, ruff check, ruff format --check) are green.
· Branch: feature/issue-63-sqlite-foreign-keys