fix: raw SQL in migration 008 to prevent duplicate division enum

forgejo_admin commented

2026-03-15 01:08:04 +00:00

Owner

Summary

Replaces op.create_table() in migration 008 with raw SQL via op.execute() to prevent SQLAlchemy metadata from emitting a duplicate CREATE TYPE division, which caused CrashLoopBackOff on redeploy.

Changes

alembic/versions/008_add_team_model.py: Replaced op.create_table() with three op.execute() calls using raw DDL (CREATE TABLE IF NOT EXISTS, CREATE INDEX IF NOT EXISTS, exception-guarded CREATE TYPE). Removed unused postgresql import. downgrade() and op.add_column() left unchanged.

Test Plan

All 242 pytest tests pass
ruff lint and format clean
Deploy to verify migration runs idempotently against existing DB with division enum

Review Checklist

No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #85
Plan: plan-westside-basketball

## Summary Replaces `op.create_table()` in migration 008 with raw SQL via `op.execute()` to prevent SQLAlchemy metadata from emitting a duplicate `CREATE TYPE division`, which caused CrashLoopBackOff on redeploy. ## Changes - `alembic/versions/008_add_team_model.py`: Replaced `op.create_table()` with three `op.execute()` calls using raw DDL (`CREATE TABLE IF NOT EXISTS`, `CREATE INDEX IF NOT EXISTS`, exception-guarded `CREATE TYPE`). Removed unused `postgresql` import. `downgrade()` and `op.add_column()` left unchanged. ## Test Plan - [x] All 242 pytest tests pass - [x] ruff lint and format clean - [ ] Deploy to verify migration runs idempotently against existing DB with division enum ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #85 - Plan: `plan-westside-basketball`

forgejo_admin added 1 commit

2026-03-15 01:08:04 +00:00

fix: replace op.create_table with raw SQL in migration 008 to prevent duplicate division enum

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

dfe73100dd

SQLAlchemy metadata can emit CREATE TYPE even with create_type=False,
causing CrashLoopBackOff on redeploy. Switch to op.execute() with raw
DDL and IF NOT EXISTS / exception-guarded CREATE TYPE for full
idempotency.

Closes #85

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-15 01:08:33 +00:00

Author

Owner

Self-review: LGTM -- no issues found.

Reviewed the diff (1 file, +24/-41):

Raw SQL DO $$ BEGIN ... EXCEPTION WHEN duplicate_object is the standard PostgreSQL idiom for idempotent enum creation. Correct.
CREATE TABLE IF NOT EXISTS and CREATE INDEX IF NOT EXISTS are idempotent by design. Correct.
division and age_group columns reference existing enum types by name without any SQLAlchemy ENUM wrapper -- eliminates the metadata interference that caused duplicate CREATE TYPE. Correct.
Unused from sqlalchemy.dialects import postgresql import removed. Correct.
downgrade() and op.add_column() unchanged as required.
242/242 tests pass, ruff clean.

**Self-review: LGTM -- no issues found.** Reviewed the diff (1 file, +24/-41): - Raw SQL `DO $$ BEGIN ... EXCEPTION WHEN duplicate_object` is the standard PostgreSQL idiom for idempotent enum creation. Correct. - `CREATE TABLE IF NOT EXISTS` and `CREATE INDEX IF NOT EXISTS` are idempotent by design. Correct. - `division` and `age_group` columns reference existing enum types by name without any SQLAlchemy ENUM wrapper -- eliminates the metadata interference that caused duplicate `CREATE TYPE`. Correct. - Unused `from sqlalchemy.dialects import postgresql` import removed. Correct. - `downgrade()` and `op.add_column()` unchanged as required. - 242/242 tests pass, ruff clean.

forgejo_admin commented

2026-03-15 01:11:28 +00:00

Author

Owner

PR #86 Backend Review

Title: fix: raw SQL in migration 008 to prevent duplicate division enum
Branch: 85-bug-non-idempotent-division-enum-in-migr -- Parent issue: #85
Scope: 1 file changed, 24 additions, 41 deletions

DOMAIN REVIEW

PEP compliance:

PEP 8/484/257: No issues. The file has a proper module docstring, type annotations on upgrade()/downgrade(), and clean formatting.
Unused import from sqlalchemy.dialects import postgresql correctly removed.

Security (OWASP):

No user input flows into the raw SQL strings -- all DDL is static. No injection risk.
No secrets, credentials, or .env content in the diff.

Database / Migration discipline:

The core fix is correct. Replacing op.create_table() with raw CREATE TABLE IF NOT EXISTS DDL completely bypasses SQLAlchemy metadata's enum introspection, which is the root cause of the duplicate CREATE TYPE division crash documented in issue #85. This is the same proven pattern used in migration 007 for the emailtype enum.
agegroup enum creation uses the DO $$ BEGIN ... EXCEPTION WHEN duplicate_object ... END $$ idiom -- correct and idempotent.
CREATE INDEX IF NOT EXISTS on both ix_teams_tenant_id and ix_teams_coach_id -- correct.
Raw SQL column definitions match the SQLAlchemy ORM model in models.py (verified: id SERIAL PRIMARY KEY, tenant_id INTEGER NOT NULL REFERENCES tenants(id), name VARCHAR(200) NOT NULL, division division nullable, age_group agegroup nullable, coach_id INTEGER REFERENCES coaches(id) nullable, created_at TIMESTAMP NOT NULL DEFAULT now()).
AgeGroup enum values in raw SQL ('U8','U10','U12','U14','U16','U18') match the Python enum exactly.
division type name referenced in the CREATE TABLE matches the enum created in migration e09c9e678004.
The downgrade() function is unchanged and uses op.drop_table("teams") + sa.Enum(name="agegroup").drop() with checkfirst=True -- correct for teardown.

Tests:

PR body states all 242 pytest tests pass and ruff is clean. No new tests added, which is appropriate -- this is a migration-only fix, not a behavioral change. The existing test_teams.py exercises the Team model and endpoints, which implicitly validates the migration creates the correct schema.

API design:

No API changes in this PR. Scope is limited to migration DDL.

BLOCKERS

None.

NITS

op.add_column is not idempotent (lines 47-56): The op.add_column("players", ...) for team_id is not wrapped in any IF NOT EXISTS guard. If the teams table was created in a prior partial run but the migration was not stamped, this will fail with DuplicateColumn. This is pre-existing (unchanged from the original migration), so it is not a blocker for this PR, but worth noting as a follow-up hardening opportunity for full idempotency.
Consistency with migration 007: Migration 007 wraps its raw SQL enum creation in sa.text() (line 27-32 of 007), while this PR passes plain strings to op.execute(). Both work -- Alembic's op.execute() accepts both raw strings and sa.text() objects -- but using sa.text() is considered best practice for explicit SQL to avoid any future deprecation. Non-blocking, minor consistency point.

SOP COMPLIANCE

Branch named after issue (85-bug-non-idempotent-division-enum-in-migr references #85)
PR body has: Summary, Changes, Test Plan, Related
Related section references the plan slug (plan-westside-basketball)
Closes #85 present in PR body
No secrets committed
No unnecessary file changes (single migration file, tight scope)
Commit messages are descriptive

PROCESS OBSERVATIONS

The Test Plan includes an unchecked item: "Deploy to verify migration runs idempotently against existing DB with division enum." This is the critical validation for this bug fix -- the migration must be tested against a DB that already has the division enum from migration e09c9e678004. Manual deploy verification is appropriate here since migration idempotency against an existing production schema is difficult to automate in CI without a dedicated staging DB.
This is the second migration in this repo to hit the SQLAlchemy create_type=False footgun (007 had the same issue with emailtype). A convention note documenting "always use raw SQL for enum-dependent table creation in Alembic" would prevent this class of bug across repos.

VERDICT: APPROVED

## PR #86 Backend Review **Title:** fix: raw SQL in migration 008 to prevent duplicate division enum **Branch:** `85-bug-non-idempotent-division-enum-in-migr` -- Parent issue: #85 **Scope:** 1 file changed, 24 additions, 41 deletions ### DOMAIN REVIEW **PEP compliance:** - PEP 8/484/257: No issues. The file has a proper module docstring, type annotations on `upgrade()`/`downgrade()`, and clean formatting. - Unused import `from sqlalchemy.dialects import postgresql` correctly removed. **Security (OWASP):** - No user input flows into the raw SQL strings -- all DDL is static. No injection risk. - No secrets, credentials, or `.env` content in the diff. **Database / Migration discipline:** - The core fix is correct. Replacing `op.create_table()` with raw `CREATE TABLE IF NOT EXISTS` DDL completely bypasses SQLAlchemy metadata's enum introspection, which is the root cause of the duplicate `CREATE TYPE division` crash documented in issue #85. This is the same proven pattern used in migration 007 for the `emailtype` enum. - `agegroup` enum creation uses the `DO $$ BEGIN ... EXCEPTION WHEN duplicate_object ... END $$` idiom -- correct and idempotent. - `CREATE INDEX IF NOT EXISTS` on both `ix_teams_tenant_id` and `ix_teams_coach_id` -- correct. - Raw SQL column definitions match the SQLAlchemy ORM model in `models.py` (verified: `id SERIAL PRIMARY KEY`, `tenant_id INTEGER NOT NULL REFERENCES tenants(id)`, `name VARCHAR(200) NOT NULL`, `division division` nullable, `age_group agegroup` nullable, `coach_id INTEGER REFERENCES coaches(id)` nullable, `created_at TIMESTAMP NOT NULL DEFAULT now()`). - `AgeGroup` enum values in raw SQL (`'U8','U10','U12','U14','U16','U18'`) match the Python enum exactly. - `division` type name referenced in the `CREATE TABLE` matches the enum created in migration `e09c9e678004`. - The `downgrade()` function is unchanged and uses `op.drop_table("teams")` + `sa.Enum(name="agegroup").drop()` with `checkfirst=True` -- correct for teardown. **Tests:** - PR body states all 242 pytest tests pass and ruff is clean. No new tests added, which is appropriate -- this is a migration-only fix, not a behavioral change. The existing `test_teams.py` exercises the Team model and endpoints, which implicitly validates the migration creates the correct schema. **API design:** - No API changes in this PR. Scope is limited to migration DDL. ### BLOCKERS None. ### NITS 1. **`op.add_column` is not idempotent** (lines 47-56): The `op.add_column("players", ...)` for `team_id` is not wrapped in any `IF NOT EXISTS` guard. If the `teams` table was created in a prior partial run but the migration was not stamped, this will fail with `DuplicateColumn`. This is pre-existing (unchanged from the original migration), so it is not a blocker for this PR, but worth noting as a follow-up hardening opportunity for full idempotency. 2. **Consistency with migration 007**: Migration 007 wraps its raw SQL enum creation in `sa.text()` (line 27-32 of 007), while this PR passes plain strings to `op.execute()`. Both work -- Alembic's `op.execute()` accepts both raw strings and `sa.text()` objects -- but using `sa.text()` is considered best practice for explicit SQL to avoid any future deprecation. Non-blocking, minor consistency point. ### SOP COMPLIANCE - [x] Branch named after issue (`85-bug-non-idempotent-division-enum-in-migr` references #85) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related section references the plan slug (`plan-westside-basketball`) - [x] `Closes #85` present in PR body - [x] No secrets committed - [x] No unnecessary file changes (single migration file, tight scope) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - The Test Plan includes an unchecked item: "Deploy to verify migration runs idempotently against existing DB with division enum." This is the critical validation for this bug fix -- the migration must be tested against a DB that already has the `division` enum from migration `e09c9e678004`. Manual deploy verification is appropriate here since migration idempotency against an existing production schema is difficult to automate in CI without a dedicated staging DB. - This is the second migration in this repo to hit the SQLAlchemy `create_type=False` footgun (007 had the same issue with `emailtype`). A convention note documenting "always use raw SQL for enum-dependent table creation in Alembic" would prevent this class of bug across repos. ### VERDICT: APPROVED