Rename tgfp_nfl id columns to provider-neutral external ids + add data_source #385

Merged

johnsturgeon merged 1 commit from rename-tgfp-nfl-id-columns-to-provider-neutral-external-ids-368 into main

2026-08-15 16:14:34 +02:00

johnsturgeon commented

2026-08-15 16:11:06 +02:00

Owner

Closes #368

Phase 2 of removing vestiges of the obsolete TGFPNfl library.

game.tgfp_nfl_game_id -> game.external_game_id
team.tgfp_nfl_team_id -> team.external_team_id

Both tables gain a data_source column recording which provider issued the external id, with a composite unique constraint on (data_source, external_*_id). The old single-column unique index is dropped (superseded by the composite constraint); the plain index is kept.

The migration is hand written. Autogenerate renders a column rename as drop_column + add_column, which would discard every external id in the table; alter_column preserves them. Postgres also leaves the index behind under its old name after a rename, so the migration drops ix_*_tgfp_nfl_* explicitly and recreates it non-unique.

Team lookups in create_picks now filter on data_source so a future provider reusing an external id cannot collide.

Verified against the dev database (32 teams, 48 games)

Every external id preserved; row counts unchanged
data_source backfilled to espn, zero nulls
Both composite unique constraints present; single-column indexes now non-unique
Autogenerate probe reports 0 ops (no model/schema drift)
Downgrade -> re-upgrade round trip clean, no data loss
pylint 10.00/10

Migrations run automatically via alembic upgrade head in docker/entrypoint.sh on container start.

Version bumped 2.8.0 -> 2.9.0.

Closes #368 Phase 2 of removing vestiges of the obsolete TGFPNfl library. - `game.tgfp_nfl_game_id` -> `game.external_game_id` - `team.tgfp_nfl_team_id` -> `team.external_team_id` Both tables gain a `data_source` column recording which provider issued the external id, with a composite unique constraint on `(data_source, external_*_id)`. The old single-column unique index is dropped (superseded by the composite constraint); the plain index is kept. The migration is **hand written**. Autogenerate renders a column rename as `drop_column` + `add_column`, which would discard every external id in the table; `alter_column` preserves them. Postgres also leaves the index behind under its old name after a rename, so the migration drops `ix_*_tgfp_nfl_*` explicitly and recreates it non-unique. Team lookups in `create_picks` now filter on `data_source` so a future provider reusing an external id cannot collide. ### Verified against the dev database (32 teams, 48 games) - Every external id preserved; row counts unchanged - `data_source` backfilled to `espn`, zero nulls - Both composite unique constraints present; single-column indexes now non-unique - Autogenerate probe reports **0 ops** (no model/schema drift) - Downgrade -> re-upgrade round trip clean, no data loss - pylint 10.00/10 Migrations run automatically via `alembic upgrade head` in `docker/entrypoint.sh` on container start. Version bumped 2.8.0 -> 2.9.0.

johnsturgeon added 1 commit

2026-08-15 16:11:06 +02:00

Rename tgfp_nfl id columns to external ids, add data_source (#368 )

Claude Code Review / claude-review (pull_request) Successful in 1m52s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 20s

Details

102f51a705

Phase 2 of removing vestiges of the obsolete TGFPNfl library.

  game.tgfp_nfl_game_id -> game.external_game_id
  team.tgfp_nfl_team_id -> team.external_team_id

Both tables gain a data_source column recording which provider issued the
external id, with a composite unique constraint on (data_source,
external_*_id). The old single-column unique index is dropped -- the
composite constraint supersedes it -- and the plain index is kept.

The migration is hand written. Autogenerate renders a column rename as
drop_column + add_column, which would discard every external id in the
table; alter_column preserves them. Note also that Postgres leaves the
index behind under its old name after a rename, so the migration drops
ix_*_tgfp_nfl_* explicitly and recreates it non-unique.

Team lookups in create_picks now filter on data_source so a future
provider that reuses an external id cannot collide.

Verified against the dev database (32 teams, 48 games): every external id
preserved, data_source backfilled to espn with no nulls, both composite
constraints present, autogenerate probe reports zero drift, and the
downgrade/re-upgrade round trip is clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

forgejo-actions commented

2026-08-15 16:12:57 +02:00

This is a clean, well-scoped PR. I inspected the models, the migration, all four call sites, the base class, and confirmed no lingering references to the old column names. I have no blocking concerns — the findings below are minor.

Review: Rename `tgfp_nfl` id columns to external ids + add `data_source`

Solid, careful change. The hand-written migration correctly uses alter_column renames (preserving data), explicitly drops the stale ix_*_tgfp_nfl_* indexes that a Postgres rename would leave behind, backfills data_source before setting NOT NULL, and the downgrade path is symmetric. All four consumers (create_picks, sync_team_records, update_game, and the models) are updated consistently, and the _espn_team lookup now matches the new composite unique key exactly, so the .one() call stays safe.

Nits

app/models/game.py:42 / app/models/team.py:28 — data_source is a Python-side default only; the migration sets the column NOT NULL with no server_default. Every insert here goes through the ORM (which supplies the default), so this is fine in practice, but it's slightly inconsistent with TGFPModelBase (app/models/base.py:18-32), where created_at/updated_at use server_default=now(). A raw-SQL or data-migration insert that omits data_source would fail. Consider sa_column_kwargs={"server_default": "espn"} for parity and robustness, since a provider-neutral column is exactly the kind of thing a future backfill might touch outside the ORM.
app/models/game.py:42 / app/models/team.py:28 — the field is typed str with an enum default, so a freshly constructed object holds a DataSource member in memory while a DB-loaded object holds a plain str. Comparisons still work because DataSource subclasses str, but typing the field as data_source: DataSource would make the round-trip type consistent and self-documenting. Optional.
_espn_team (app/jobs/create_picks.py:33) issues 2–3 separate queries per game. This mirrors the prior behavior and runs in a weekly background job (not a request handler), so it's not a regression — noting only in case a future batch load is desired.

Test coverage

There are no tests in the repo (pytest is configured in pyproject.toml but no test files exist), so this isn't a gap introduced by the PR. The PR description documents a thorough manual verification against the dev database (data preservation, backfill, constraint presence, autogenerate 0-ops probe, downgrade→upgrade round trip), which is the right diligence for a data-preserving migration.

This is a clean, well-scoped PR. I inspected the models, the migration, all four call sites, the base class, and confirmed no lingering references to the old column names. I have no blocking concerns — the findings below are minor. --- ## Review: Rename `tgfp_nfl` id columns to external ids + add `data_source` Solid, careful change. The hand-written migration correctly uses `alter_column` renames (preserving data), explicitly drops the stale `ix_*_tgfp_nfl_*` indexes that a Postgres rename would leave behind, backfills `data_source` before setting `NOT NULL`, and the downgrade path is symmetric. All four consumers (`create_picks`, `sync_team_records`, `update_game`, and the models) are updated consistently, and the `_espn_team` lookup now matches the new composite unique key exactly, so the `.one()` call stays safe. ### Nits - `app/models/game.py:42` / `app/models/team.py:28` — `data_source` is a Python-side default only; the migration sets the column `NOT NULL` with no `server_default`. Every insert here goes through the ORM (which supplies the default), so this is fine in practice, but it's slightly inconsistent with `TGFPModelBase` (`app/models/base.py:18-32`), where `created_at`/`updated_at` use `server_default=now()`. A raw-SQL or data-migration insert that omits `data_source` would fail. Consider `sa_column_kwargs={"server_default": "espn"}` for parity and robustness, since a provider-neutral column is exactly the kind of thing a future backfill might touch outside the ORM. - `app/models/game.py:42` / `app/models/team.py:28` — the field is typed `str` with an enum default, so a freshly constructed object holds a `DataSource` member in memory while a DB-loaded object holds a plain `str`. Comparisons still work because `DataSource` subclasses `str`, but typing the field as `data_source: DataSource` would make the round-trip type consistent and self-documenting. Optional. - `_espn_team` (`app/jobs/create_picks.py:33`) issues 2–3 separate queries per game. This mirrors the prior behavior and runs in a weekly background job (not a request handler), so it's not a regression — noting only in case a future batch load is desired. ### Test coverage There are no tests in the repo (pytest is configured in `pyproject.toml` but no test files exist), so this isn't a gap introduced by the PR. The PR description documents a thorough manual verification against the dev database (data preservation, backfill, constraint presence, autogenerate 0-ops probe, downgrade→upgrade round trip), which is the right diligence for a data-preserving migration.

johnsturgeon merged commit 6f3b459c88 into main

2026-08-15 16:14:34 +02:00

johnsturgeon referenced this pull request from a commit

2026-08-15 16:14:35 +02:00

Merge pull request 'Rename tgfp_nfl id columns to provider-neutral external ids + add data_source' (#385) from rename-tgfp-nfl-id-columns-to-provider-neutral-external-ids-368 into main