refactor(migrations): address code review findings (#2)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: nanotaboada

CHANGELOG.md

@@ -48,8 +48,8 @@ This project uses famous football coaches as release codenames, following an A-Z
   configured for async execution with `render_as_batch=True` (SQLite/PostgreSQL
   compatible); three migrations: `001` creates the `players` table, `002` seeds
   11 Starting XI players, `003` seeds 15 Substitute players (all with
-  deterministic UUID v5 values); `alembic upgrade head` runs automatically on
-  app startup via the lifespan handler (#2)
+  deterministic UUID v5 values); `alembic upgrade head` applied by
+  `entrypoint.sh` (Docker) or manually for local development (#2)
 - `alembic==1.18.4`, `asyncpg==0.31.0` added to dependencies (#2)
 - `tests/test_migrations.py`: integration tests for migration downgrade paths —
   verifies each step removes only its seeded rows and restores correctly (#2)
@@ -64,15 +64,18 @@ This project uses famous football coaches as release codenames, following an A-Z
 
 ### Changed
 
-- `databases/player_database.py`: engine URL now reads `DATABASE_URL`
-  environment variable (SQLite default, PostgreSQL compatible); `connect_args`
-  made conditional on SQLite dialect (#2)
-- `main.py`: lifespan handler now applies Alembic migrations on startup via
-  thread executor (#2)
+- `databases/player_database.py`: extracted `get_database_url()` helper
+  (reads `DATABASE_URL`, falls back to `STORAGE_PATH`, SQLite default);
+  `connect_args` made conditional on SQLite dialect (#2)
+- `alembic/env.py`: removed duplicated DATABASE_URL construction; now calls
+  `get_database_url()` from `databases.player_database` (#2)
+- `main.py`: removed `_apply_migrations` from lifespan — migrations are a
+  one-shot step, not a per-process startup concern; lifespan now logs startup
+  only (#2)
 - `Dockerfile`: removed `COPY storage/ ./hold/` and its associated comment;
   added `COPY alembic.ini` and `COPY alembic/` (#2)
-- `scripts/entrypoint.sh`: removed hold→volume copy and seed script logic,
-  now replaced by Alembic migrations applied at app startup (#2)
+- `scripts/entrypoint.sh`: runs `alembic upgrade head` before launching the
+  app; replaces hold→volume copy and manual seed script pattern (#2)
 - `compose.yaml`: replaced `STORAGE_PATH` with `DATABASE_URL` pointing to the
   SQLite volume path (#2)
 - `.gitignore`: added `*.db`; `storage/players-sqlite3.db` removed from git

alembic/env.py

@@ -7,7 +7,6 @@
 """
 
 import asyncio
-import os
 from logging.config import fileConfig
 
 from sqlalchemy import pool
@@ -16,15 +15,13 @@
 
 from alembic import context
 
-from databases.player_database import Base
+from databases.player_database import Base, get_database_url
 from schemas.player_schema import Player  # noqa: F401 — registers ORM model with Base
 
 # Supports both SQLite (local) and PostgreSQL (Docker, see #542):
-#   sqlite+aiosqlite:///./storage/players-sqlite3.db
+#   sqlite+aiosqlite:///./players-sqlite3.db
 #   postgresql+asyncpg://postgres:password@postgres:5432/playersdb
-_storage_path = os.getenv("STORAGE_PATH", "./players-sqlite3.db")
-_default_url = f"sqlite+aiosqlite:///{_storage_path}"
-database_url = os.getenv("DATABASE_URL", _default_url)
+database_url = get_database_url()
 
 config = context.config
 config.set_main_option("sqlalchemy.url", database_url)

databases/player_database.py

@@ -9,7 +9,7 @@
 
 Environment variables:
     DATABASE_URL: Full async database URL. Defaults to SQLite:
-        sqlite+aiosqlite:///./storage/players-sqlite3.db
+        sqlite+aiosqlite:///./players-sqlite3.db
     STORAGE_PATH: (legacy) SQLite file path. Ignored when DATABASE_URL is set.
 """
 
@@ -19,12 +19,21 @@
 from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession
 from sqlalchemy.orm import sessionmaker, declarative_base
 
-_database_url = os.getenv("DATABASE_URL")
-if not _database_url:
-    _storage_path = os.getenv("STORAGE_PATH", "./players-sqlite3.db")
-    _database_url = f"sqlite+aiosqlite:///{_storage_path}"
 
-DATABASE_URL: str = _database_url
+def get_database_url() -> str:
+    """Return the async database URL from environment variables.
+
+    Reads DATABASE_URL first; if unset, constructs a SQLite URL from
+    STORAGE_PATH (defaulting to ./players-sqlite3.db).
+    """
+    database_url = os.getenv("DATABASE_URL")
+    if not database_url:
+        storage_path = os.getenv("STORAGE_PATH", "./players-sqlite3.db")
+        database_url = f"sqlite+aiosqlite:///{storage_path}"
+    return database_url
+
+
+DATABASE_URL: str = get_database_url()
 
 _connect_args = (
     {"check_same_thread": False} if DATABASE_URL.startswith("sqlite") else {}

main.py

@@ -5,15 +5,16 @@
 - Defines the lifespan event handler for app startup/shutdown logging.
 - Includes API routers for player and health endpoints.
 
+Database migrations are applied by entrypoint.sh before the process starts
+(Docker). For local development, run `alembic upgrade head` once before
+starting the server.
+
 This serves as the entry point for running the API server.
 """
 
-import asyncio
 from contextlib import asynccontextmanager
 import logging
 from typing import AsyncIterator
-from alembic.config import Config
-from alembic import command
 from fastapi import FastAPI
 from routes import player_route, health_route
 
@@ -22,24 +23,12 @@
 logger = logging.getLogger(UVICORN_LOGGER)
 
 
-def _apply_migrations() -> None:
-    alembic_cfg = Config("alembic.ini")
-    command.upgrade(alembic_cfg, "head")
-
-
 @asynccontextmanager
 async def lifespan(_: FastAPI) -> AsyncIterator[None]:
     """
     Lifespan event handler for FastAPI.
-
-    Runs database migrations before the application starts accepting requests.
-    Alembic is invoked in a thread executor to avoid conflicts with the running
-    event loop (alembic's async env.py calls asyncio.run() internally).
     """
-    logger.info("Applying database migrations...")
-    loop = asyncio.get_running_loop()
-    await loop.run_in_executor(None, _apply_migrations)
-    logger.info("Database migrations applied successfully.")
+    logger.info("Application startup complete.")
     yield
 
 

scripts/entrypoint.sh

@@ -2,6 +2,8 @@
 set -e
 
 echo "Starting container..."
-echo "Database migrations will be applied by the application on startup."
+echo "Applying database migrations..."
+alembic upgrade head
+echo "Database migrations applied."
 echo "Launching app..."
 exec "$@"

tests/test_migrations.py

@@ -13,52 +13,53 @@
 
 import os
 import sqlite3
+from pathlib import Path
 
 from alembic import command
 from alembic.config import Config
 
 _DB_PATH = os.getenv("STORAGE_PATH", "./players-sqlite3.db")
-_ALEMBIC_CFG = Config("alembic.ini")
+_alembic_config = Config(str(Path(__file__).resolve().parent.parent / "alembic.ini"))
 
 
 def test_migration_downgrade_003_removes_substitutes_only():
     """Downgrade 003→002 removes the 15 seeded substitutes, leaves Starting XI."""
-    command.downgrade(_ALEMBIC_CFG, "-1")
+    command.downgrade(_alembic_config, "-1")
 
     conn = sqlite3.connect(_DB_PATH)
     total = conn.execute("SELECT COUNT(*) FROM players").fetchone()[0]
     subs = conn.execute("SELECT COUNT(*) FROM players WHERE starting11=0").fetchone()[0]
     conn.close()
 
-    command.upgrade(_ALEMBIC_CFG, "head")
+    command.upgrade(_alembic_config, "head")
 
     assert total == 11
     assert subs == 0
 
 
 def test_migration_downgrade_002_removes_starting11_only():
     """Downgrade 002→001 removes the 11 seeded Starting XI, leaves table empty."""
-    command.downgrade(_ALEMBIC_CFG, "-2")
+    command.downgrade(_alembic_config, "-2")
 
     conn = sqlite3.connect(_DB_PATH)
     total = conn.execute("SELECT COUNT(*) FROM players").fetchone()[0]
     conn.close()
 
-    command.upgrade(_ALEMBIC_CFG, "head")
+    command.upgrade(_alembic_config, "head")
 
     assert total == 0
 
 
 def test_migration_downgrade_001_drops_players_table():
     """Downgrade 001→base drops the players table entirely."""
-    command.downgrade(_ALEMBIC_CFG, "base")
+    command.downgrade(_alembic_config, "base")
 
     conn = sqlite3.connect(_DB_PATH)
     table = conn.execute(
         "SELECT name FROM sqlite_master WHERE type='table' AND name='players'"
     ).fetchone()
     conn.close()
 
-    command.upgrade(_ALEMBIC_CFG, "head")
+    command.upgrade(_alembic_config, "head")
 
     assert table is None