feat: add SQLAlchemy models, Alembic migrations, and DB-backed health check #3

Merged

forgejo_admin merged 2 commits from 2-add-sqlalchemy-models-alembic-migrations into main 2026-03-16 01:37:51 +00:00

Conversation 1 Commits 2 Files changed 14 +507 -9

forgejo_admin commented 2026-03-16 01:05:54 +00:00

Contributor

Copy link

Summary

Implements the full data persistence layer for mcd-tracker-api: SQLAlchemy ORM models (Location, CouponUsage), Alembic migration infrastructure with initial schema, database module, migration-on-startup, and a DB-backed health endpoint. All patterns follow basketball-api conventions.

Changes

• src/mcd_tracker_api/database.py -- Engine, SessionLocal, Base, get_db() generator (basketball-api pattern)
• src/mcd_tracker_api/models.py -- Location (keycloak_sub, name, address, city, state, lat/lng) and CouponUsage (location FK with CASCADE, code, used_at, redeemed, redeemed_at, expires_at) models
• alembic.ini + alembic/env.py + alembic/script.py.mako -- Alembic config using MCD_TRACKER_DATABASE_URL env var
• alembic/versions/001_initial_schema.py -- Initial migration creating both tables with indexes on keycloak_sub and location_id
• src/mcd_tracker_api/main.py -- Lifespan now runs Alembic upgrade to head on startup (with graceful error handling)
• src/mcd_tracker_api/routes/health.py -- Health endpoint now executes SELECT 1 and returns {"status": "ok", "db": "connected"} or {"status": "degraded", "db": "disconnected"}
• pyproject.toml -- Added sqlalchemy, psycopg2-binary, alembic dependencies
• Dockerfile -- Copies alembic.ini and alembic/ directory into runtime image
• tests/conftest.py -- Postgres-backed test fixtures (create_all/drop_all per test, DB session, TestClient with dependency override)
• tests/test_health.py -- Updated for DB-backed health response, degraded simulation, metrics
• tests/test_models.py -- 6 model tests: create location, create coupon usage, redeem, cascade delete, relationship, expires_at application-set

Test Plan

• 9 tests pass against real Postgres (mcd_tracker_test database)
• ruff check and ruff format --check pass clean
• Health endpoint returns connected/degraded status based on DB connectivity
• CASCADE delete verified: deleting a Location removes its CouponUsages
• expires_at confirmed as application-set (not DB-generated)
• No postgresql.ENUM or sa.Enum used (no enum columns in this schema)

Review Checklist

• No sa.Enum usage -- avoids non-idempotent CREATE TYPE
• expires_at set in application logic, not as Postgres generated column
• No API routes added -- models, database, alembic, and health only
• Migration runs on startup in main.py lifespan
• Tests use real Postgres, not mocks
• MCD_TRACKER_DATABASE_URL env var used consistently
• Follows basketball-api patterns (database.py, models.py, alembic/, conftest.py)
• Dockerfile updated to include alembic files
• ruff check + format clean

Related

• Forgejo issue: #2

Closes #2

## Summary Implements the full data persistence layer for mcd-tracker-api: SQLAlchemy ORM models (Location, CouponUsage), Alembic migration infrastructure with initial schema, database module, migration-on-startup, and a DB-backed health endpoint. All patterns follow basketball-api conventions. ## Changes - **`src/mcd_tracker_api/database.py`** -- Engine, SessionLocal, Base, get_db() generator (basketball-api pattern) - **`src/mcd_tracker_api/models.py`** -- Location (keycloak_sub, name, address, city, state, lat/lng) and CouponUsage (location FK with CASCADE, code, used_at, redeemed, redeemed_at, expires_at) models - **`alembic.ini`** + **`alembic/env.py`** + **`alembic/script.py.mako`** -- Alembic config using MCD_TRACKER_DATABASE_URL env var - **`alembic/versions/001_initial_schema.py`** -- Initial migration creating both tables with indexes on keycloak_sub and location_id - **`src/mcd_tracker_api/main.py`** -- Lifespan now runs Alembic upgrade to head on startup (with graceful error handling) - **`src/mcd_tracker_api/routes/health.py`** -- Health endpoint now executes `SELECT 1` and returns `{"status": "ok", "db": "connected"}` or `{"status": "degraded", "db": "disconnected"}` - **`pyproject.toml`** -- Added sqlalchemy, psycopg2-binary, alembic dependencies - **`Dockerfile`** -- Copies alembic.ini and alembic/ directory into runtime image - **`tests/conftest.py`** -- Postgres-backed test fixtures (create_all/drop_all per test, DB session, TestClient with dependency override) - **`tests/test_health.py`** -- Updated for DB-backed health response, degraded simulation, metrics - **`tests/test_models.py`** -- 6 model tests: create location, create coupon usage, redeem, cascade delete, relationship, expires_at application-set ## Test Plan - 9 tests pass against real Postgres (`mcd_tracker_test` database) - `ruff check` and `ruff format --check` pass clean - Health endpoint returns connected/degraded status based on DB connectivity - CASCADE delete verified: deleting a Location removes its CouponUsages - `expires_at` confirmed as application-set (not DB-generated) - No `postgresql.ENUM` or `sa.Enum` used (no enum columns in this schema) ## Review Checklist - [x] No `sa.Enum` usage -- avoids non-idempotent CREATE TYPE - [x] `expires_at` set in application logic, not as Postgres generated column - [x] No API routes added -- models, database, alembic, and health only - [x] Migration runs on startup in main.py lifespan - [x] Tests use real Postgres, not mocks - [x] `MCD_TRACKER_DATABASE_URL` env var used consistently - [x] Follows basketball-api patterns (database.py, models.py, alembic/, conftest.py) - [x] Dockerfile updated to include alembic files - [x] ruff check + format clean ## Related - Forgejo issue: #2 Closes #2

forgejo_admin added 1 commit 2026-03-16 01:05:54 +00:00

feat: add SQLAlchemy models, Alembic migrations, and DB-backed health check ... 794d6e9f63

Implements the data persistence layer for mcd-tracker-api:
- Location and CouponUsage SQLAlchemy models with cascade delete
- Alembic migration infrastructure with 001_initial_schema
- Database module (engine, SessionLocal, Base, get_db)
- Migration-on-startup via lifespan handler
- Health endpoint now returns {"status": "ok", "db": "connected"}
  or {"status": "degraded", "db": "disconnected"}
- 9 tests against real Postgres (models, cascade, relationships, health)

Closes #2

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

forgejo_admin commented 2026-03-16 01:08:13 +00:00

Author

Contributor

Copy link

PR #3 Review

DOMAIN REVIEW

Tech stack: Python 3.12 / FastAPI / SQLAlchemy 2.0 / Alembic / Postgres / Pydantic Settings

This PR adds the full persistence layer: ORM models (Location, CouponUsage), Alembic migration infrastructure with an initial schema, a database.py module following the basketball-api pattern, migration-on-startup in the FastAPI lifespan, a DB-backed /healthz endpoint, Dockerfile updates, and Postgres-backed tests. 497 additions, 9 deletions across 13 files.

SQLAlchemy patterns:

• Mapped column annotations with Mapped[T] + mapped_column() -- modern SQLAlchemy 2.0 style. Good.
• DeclarativeBase used correctly.
• Session lifecycle in get_db() uses try/finally/close -- correct.
• Relationship with cascade="all, delete-orphan" on the parent side + ondelete="CASCADE" on the FK -- both ORM-level and DB-level cascade. Solid.
• No sa.Enum usage -- explicitly avoids the non-idempotent CREATE TYPE footgun learned from basketball-api migration 008. Good.

Alembic patterns:

• env.py reads MCD_TRACKER_DATABASE_URL from environment, overrides the driver:// placeholder in alembic.ini. Correct.
• Online migrations require the env var (raises RuntimeError if missing). Good guard.
• Migration 001 matches the ORM models exactly -- column types, nullable, indexes, FK constraints, server_defaults all align. Verified.
• Downgrade drops tables in correct dependency order (coupon_usages before locations).

FastAPI patterns:

• Lifespan context manager runs migrations on startup with exception handling -- logs but does not crash. This is intentional (per basketball-api pattern).
• Health endpoint uses Depends(get_db) for DI -- correct.
• DB health check uses text("SELECT 1") -- safe, no injection surface.

Test infrastructure:

• 9 tests total: 3 health tests (connected, degraded, metrics) + 6 model tests (CRUD, redeem, cascade delete, relationship, expires_at).
• Tests run against real Postgres (mcd_tracker_test DB), not mocks. Good.
• conftest.py uses create_all/drop_all per test (autouse fixture) -- clean isolation.
• raise_server_exceptions=False on TestClient to prevent Alembic lifespan conflicts with conftest table creation. Documented in docstring. Reasonable tradeoff.
• Degraded health test monkey-patches db.execute to simulate failure. Works but fragile -- acceptable for this scope.

Dockerfile:

• Multi-stage build. Alembic files (alembic.ini, alembic/) copied into runtime image. Correct.

BLOCKERS

None. All blocker criteria pass:

• New functionality has test coverage (9 tests covering models, health, cascade, relationships)
• No unvalidated user input (no API routes accepting user data yet -- models/health only)
• No secrets or credentials in code (DB URL comes from env var via pydantic-settings)
• No DRY violations in auth/security paths (no auth paths yet)

NITS

1. .gitignore missing .env -- The .gitignore does not include .env or *.env. Since pydantic-settings encourages .env files for local dev (with MCD_TRACKER_DATABASE_URL), there is a risk of accidentally committing one with credentials. Add .env and *.env to .gitignore.

2. CI test step has no Postgres service -- The Woodpecker CI test step runs pytest but there is no database service container configured. The conftest.py defaults to postgresql://mcd_tracker:mcd_tracker@localhost:5432/mcd_tracker_test. This will fail in CI unless a Postgres sidecar is added. This is likely a known deferral (scaffolding first, CI DB service later), but worth flagging.

3. Migration failure is swallowed on startup -- _run_migrations() catches all exceptions and only logs them. The app will start with an un-migrated database, which could lead to confusing runtime errors. Consider whether a failed migration should prevent startup (raise instead of swallow). The basketball-api does the same thing, so this is consistent, but it is a design choice worth revisiting as the schema grows.

4. alembic/env.py creates its own engine -- The run_migrations_online() function creates a new create_engine(db_url) from the raw env var, while database.py also creates an engine from settings.database_url (which reads the same env var via pydantic-settings). This is two engine instances for the same URL. Not a bug (Alembic CLI needs its own engine), but worth a comment noting the intentional duplication.

5. conftest.py hardcodes default credentials -- Line 8: "postgresql://mcd_tracker:mcd_tracker@localhost:5432/mcd_tracker_test". These are test-only local defaults and not production credentials, so this is not a blocker. But the password mcd_tracker for the test database is embedded in source. Fine for local dev, just noting it.

6. alembic.ini has sqlalchemy.url = driver:// -- This placeholder is overridden at runtime by both env.py and main.py. It works, but someone running bare alembic upgrade head without the env var set will get a confusing driver:// connection error rather than the clear RuntimeError from env.py (because the offline path falls back to this value). Minor -- the online path has the guard.

7. No updated_at column -- Neither Location nor CouponUsage has an updated_at timestamp. Not needed now, but will likely be needed when CRUD routes are added (for tracking when coupons are redeemed, locations edited, etc.). Consider adding it in the initial schema to avoid a migration later.

SOP COMPLIANCE

• Branch named after issue number (2-add-sqlalchemy-models-alembic-migrations references issue #2)
• PR body has ## Summary, ## Changes, ## Test Plan, ## Related
• Related section references the plan slug -- PR body says Forgejo issue: #2 but does not reference plan-mcd-tracker. Minor gap.
• Closes #2 present in PR body
• No secrets, .env files, or credentials committed
• No unnecessary file changes (all changes are in scope for issue #2)
• Commit messages are descriptive (PR title is clear and conventional)

PROCESS OBSERVATIONS

• Deployment frequency: This is the second PR for a new repo (first was scaffold, this is persistence layer). Good cadence -- small, focused increments.
• Change failure risk: Low. No API routes accepting user input yet. The persistence layer is well-tested against real Postgres. The Alembic migration is idempotent (create_table, not alter). The main risk is the CI pipeline lacking a Postgres service -- tests will pass locally but may fail in Woodpecker.
• Documentation gap: The README is minimal (one line). As the API grows, documenting the database setup (how to create mcd_tracker and mcd_tracker_test databases, required env vars) will help onboarding.
• Pattern compliance: Follows basketball-api conventions closely (database.py structure, conftest.py pattern, migration-on-startup, DB-backed health). Good institutional consistency.

VERDICT: APPROVED

Clean persistence layer. No blockers. The nits (.gitignore .env exclusion, CI Postgres service, plan slug reference) should be addressed in subsequent work but do not block merge.

## PR #3 Review ### DOMAIN REVIEW **Tech stack:** Python 3.12 / FastAPI / SQLAlchemy 2.0 / Alembic / Postgres / Pydantic Settings This PR adds the full persistence layer: ORM models (`Location`, `CouponUsage`), Alembic migration infrastructure with an initial schema, a `database.py` module following the basketball-api pattern, migration-on-startup in the FastAPI lifespan, a DB-backed `/healthz` endpoint, Dockerfile updates, and Postgres-backed tests. 497 additions, 9 deletions across 13 files. **SQLAlchemy patterns:** - Mapped column annotations with `Mapped[T]` + `mapped_column()` -- modern SQLAlchemy 2.0 style. Good. - `DeclarativeBase` used correctly. - Session lifecycle in `get_db()` uses try/finally/close -- correct. - Relationship with `cascade="all, delete-orphan"` on the parent side + `ondelete="CASCADE"` on the FK -- both ORM-level and DB-level cascade. Solid. - No `sa.Enum` usage -- explicitly avoids the non-idempotent `CREATE TYPE` footgun learned from basketball-api migration 008. Good. **Alembic patterns:** - `env.py` reads `MCD_TRACKER_DATABASE_URL` from environment, overrides the `driver://` placeholder in `alembic.ini`. Correct. - Online migrations require the env var (raises `RuntimeError` if missing). Good guard. - Migration 001 matches the ORM models exactly -- column types, nullable, indexes, FK constraints, server_defaults all align. Verified. - Downgrade drops tables in correct dependency order (coupon_usages before locations). **FastAPI patterns:** - Lifespan context manager runs migrations on startup with exception handling -- logs but does not crash. This is intentional (per basketball-api pattern). - Health endpoint uses `Depends(get_db)` for DI -- correct. - DB health check uses `text("SELECT 1")` -- safe, no injection surface. **Test infrastructure:** - 9 tests total: 3 health tests (connected, degraded, metrics) + 6 model tests (CRUD, redeem, cascade delete, relationship, expires_at). - Tests run against real Postgres (`mcd_tracker_test` DB), not mocks. Good. - `conftest.py` uses `create_all`/`drop_all` per test (autouse fixture) -- clean isolation. - `raise_server_exceptions=False` on TestClient to prevent Alembic lifespan conflicts with conftest table creation. Documented in docstring. Reasonable tradeoff. - Degraded health test monkey-patches `db.execute` to simulate failure. Works but fragile -- acceptable for this scope. **Dockerfile:** - Multi-stage build. Alembic files (`alembic.ini`, `alembic/`) copied into runtime image. Correct. ### BLOCKERS **None.** All blocker criteria pass: - [x] New functionality has test coverage (9 tests covering models, health, cascade, relationships) - [x] No unvalidated user input (no API routes accepting user data yet -- models/health only) - [x] No secrets or credentials in code (DB URL comes from env var via pydantic-settings) - [x] No DRY violations in auth/security paths (no auth paths yet) ### NITS 1. **`.gitignore` missing `.env`** -- The `.gitignore` does not include `.env` or `*.env`. Since `pydantic-settings` encourages `.env` files for local dev (with `MCD_TRACKER_DATABASE_URL`), there is a risk of accidentally committing one with credentials. Add `.env` and `*.env` to `.gitignore`. 2. **CI test step has no Postgres service** -- The Woodpecker CI `test` step runs `pytest` but there is no database service container configured. The `conftest.py` defaults to `postgresql://mcd_tracker:mcd_tracker@localhost:5432/mcd_tracker_test`. This will fail in CI unless a Postgres sidecar is added. This is likely a known deferral (scaffolding first, CI DB service later), but worth flagging. 3. **Migration failure is swallowed on startup** -- `_run_migrations()` catches all exceptions and only logs them. The app will start with an un-migrated database, which could lead to confusing runtime errors. Consider whether a failed migration should prevent startup (raise instead of swallow). The basketball-api does the same thing, so this is consistent, but it is a design choice worth revisiting as the schema grows. 4. **`alembic/env.py` creates its own engine** -- The `run_migrations_online()` function creates a new `create_engine(db_url)` from the raw env var, while `database.py` also creates an engine from `settings.database_url` (which reads the same env var via pydantic-settings). This is two engine instances for the same URL. Not a bug (Alembic CLI needs its own engine), but worth a comment noting the intentional duplication. 5. **`conftest.py` hardcodes default credentials** -- Line 8: `"postgresql://mcd_tracker:mcd_tracker@localhost:5432/mcd_tracker_test"`. These are test-only local defaults and not production credentials, so this is not a blocker. But the password `mcd_tracker` for the test database is embedded in source. Fine for local dev, just noting it. 6. **`alembic.ini` has `sqlalchemy.url = driver://`** -- This placeholder is overridden at runtime by both `env.py` and `main.py`. It works, but someone running bare `alembic upgrade head` without the env var set will get a confusing `driver://` connection error rather than the clear `RuntimeError` from `env.py` (because the offline path falls back to this value). Minor -- the online path has the guard. 7. **No `updated_at` column** -- Neither `Location` nor `CouponUsage` has an `updated_at` timestamp. Not needed now, but will likely be needed when CRUD routes are added (for tracking when coupons are redeemed, locations edited, etc.). Consider adding it in the initial schema to avoid a migration later. ### SOP COMPLIANCE - [x] Branch named after issue number (`2-add-sqlalchemy-models-alembic-migrations` references issue #2) - [x] PR body has `## Summary`, `## Changes`, `## Test Plan`, `## Related` - [ ] Related section references the plan slug -- PR body says `Forgejo issue: #2` but does not reference `plan-mcd-tracker`. **Minor gap.** - [x] `Closes #2` present in PR body - [x] No secrets, `.env` files, or credentials committed - [x] No unnecessary file changes (all changes are in scope for issue #2) - [x] Commit messages are descriptive (PR title is clear and conventional) ### PROCESS OBSERVATIONS - **Deployment frequency:** This is the second PR for a new repo (first was scaffold, this is persistence layer). Good cadence -- small, focused increments. - **Change failure risk:** Low. No API routes accepting user input yet. The persistence layer is well-tested against real Postgres. The Alembic migration is idempotent (create_table, not alter). The main risk is the CI pipeline lacking a Postgres service -- tests will pass locally but may fail in Woodpecker. - **Documentation gap:** The README is minimal (one line). As the API grows, documenting the database setup (how to create `mcd_tracker` and `mcd_tracker_test` databases, required env vars) will help onboarding. - **Pattern compliance:** Follows basketball-api conventions closely (database.py structure, conftest.py pattern, migration-on-startup, DB-backed health). Good institutional consistency. ### VERDICT: APPROVED Clean persistence layer. No blockers. The nits (`.gitignore` `.env` exclusion, CI Postgres service, plan slug reference) should be addressed in subsequent work but do not block merge.

forgejo_admin added 1 commit 2026-03-16 01:28:05 +00:00

ci: add Postgres service container for test step ... a43c822f31

Tests require a live Postgres database but the Woodpecker pipeline had
no service container. Add postgres:16-alpine service and wire
MCD_TRACKER_DATABASE_URL so pytest can connect in CI.

Closes #2

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

forgejo_admin merged commit 675be2609b into main 2026-03-16 01:37:51 +00:00

forgejo_admin deleted branch 2-add-sqlalchemy-models-alembic-migrations 2026-03-16 01:37:51 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-16 01:37:53 +00:00

feat: add SQLAlchemy models, Alembic migrations, and DB-backed health check (#3)

forgejo_admin referenced this pull request 2026-03-16 19:59:55 +00:00

feat: add GET /codes endpoint, enrich CodeResponse with location_name #15

Commenting is not possible because the repository is archived.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/mcd-tracker-api!3

No description provided.