From e20299548ffa7ef0a743876c1accaf1fa576c596 Mon Sep 17 00:00:00 2001 From: AdrianoDev Date: Mon, 21 Sep 2026 22:49:38 +0200 Subject: [PATCH] fix(migrations): schema base 000 per DB vuoti, stamp head dopo create_all (#6) upgrade head partiva dalla 001, che sa solo aggiungere colonne a tabelle che assume gia' esistenti: su un DB davvero vuoto falliva subito. Nuova 000_base_schema.py (revision "000_base", down_revision None) crea lo schema pre-001 esplicitamente (create_table, non Base.metadata.create_all, che porterebbe gia' le colonne di 001-013 e farebbe fallire 001 su "Duplicate column"). No-op se la tabella recipes esiste gia' (sa.inspect(bind).has_table("recipes")). 001 ora ha down_revision = "000_base". setup.py: init-db e reset-db, dopo create_all/drop_all+create_all, eseguono l'equivalente di "alembic stamp head" (nuovo _stamp_head(), command.stamp con la config di src/backend/migrations/alembic.ini) cosi' un DB nato da create_all non ritenta le migrazioni 001-013 al prossimo deploy. Test: catena upgrade head su SQLite vuoto, tabelle/colonne confrontate con Base.metadata; DB con recipes gia' nello stato pre-001 -> upgrade head non fallisce (000 no-op). _stamp_head si appoggia a migrations/env.py, che costruisce una connessione propria da settings.database_url (non modificabile dai test tramite l'engine patchato di setup.py): nei test di setup.py e' quindi no-op via monkeypatch, cosi' init-db/reset-db continuano a girare su SQLite senza toccare un MySQL vero. Co-Authored-By: Claude Sonnet 5 --- src/backend/api/routers/setup.py | 20 +- .../migrations/versions/000_base_schema.py | 251 ++++++++++++++++++ ...01_add_image_path_to_recipe_and_subtask.py | 2 +- src/backend/tests/test_migrations.py | 109 ++++++++ src/backend/tests/test_setup.py | 5 + src/backend/tests/test_station_seed.py | 3 + 6 files changed, 388 insertions(+), 2 deletions(-) create mode 100644 src/backend/migrations/versions/000_base_schema.py create mode 100644 src/backend/tests/test_migrations.py diff --git a/src/backend/api/routers/setup.py b/src/backend/api/routers/setup.py index 73b93b2..36ff4d4 100644 --- a/src/backend/api/routers/setup.py +++ b/src/backend/api/routers/setup.py @@ -9,6 +9,8 @@ import secrets from datetime import datetime, timedelta from pathlib import Path +from alembic import command +from alembic.config import Config from fastapi import APIRouter, HTTPException, Request, status from fastapi.responses import HTMLResponse from fastapi.templating import Jinja2Templates @@ -28,10 +30,24 @@ from src.backend.services.measurement_service import calculate_pass_fail router = APIRouter(prefix="/api/setup", tags=["setup"]) -_templates_dir = Path(__file__).resolve().parent.parent.parent / "templates" +_backend_dir = Path(__file__).resolve().parent.parent.parent +_templates_dir = _backend_dir / "templates" +_alembic_ini = _backend_dir / "migrations" / "alembic.ini" templates = Jinja2Templates(directory=str(_templates_dir)) +def _stamp_head() -> None: + """Mark the schema as fully migrated - the equivalent of `alembic stamp + head` - after `Base.metadata.create_all()`. + + A database that got its tables this way instead of through + `upgrade head` has no row in `alembic_version`; without this, its next + deploy would try to replay migrations 001-013 and fail on a column that + is already there (see migration 000_base_schema.py). + """ + command.stamp(Config(str(_alembic_ini)), "head") + + # --------------------------------------------------------------------------- # Request schemas # --------------------------------------------------------------------------- @@ -296,6 +312,7 @@ async def init_db(body: PasswordBody): await _require_not_initialised() async with engine.begin() as conn: await conn.run_sync(Base.metadata.create_all) + _stamp_head() return {"status": "ok", "message": "Database tables created successfully"} @@ -734,5 +751,6 @@ async def reset_db(body: ResetDbBody): async with engine.begin() as conn: await conn.run_sync(Base.metadata.drop_all) await conn.run_sync(Base.metadata.create_all) + _stamp_head() return {"status": "ok", "message": "Database reset successfully"} diff --git a/src/backend/migrations/versions/000_base_schema.py b/src/backend/migrations/versions/000_base_schema.py new file mode 100644 index 0000000..685e9d3 --- /dev/null +++ b/src/backend/migrations/versions/000_base_schema.py @@ -0,0 +1,251 @@ +"""base schema: state of the database immediately before migration 001 + +Every database that has run this project since before Alembic was wired in +was born from `Base.metadata.create_all()` (via `/api/setup/init-db`, or +`init_db()` in dev) - never from a migration chain, because there wasn't one. +`upgrade head` on a genuinely empty database therefore had no starting point: +001 is the oldest migration, and it only knows how to ADD columns to tables +that 001-013 all assume already exist. + +This migration supplies that starting point, but only when there is nothing +there yet: on a database that already has `recipes` (the table 001 was +written against), it is a no-op, because that database was already born from +`create_all()` and already has this exact schema. + +Written explicitly with `create_table` rather than `Base.metadata.create_all`: +`create_all` builds the CURRENT model definitions, which already carry every +column 001-013 add, and 001 would then try to add a column that is already +there (`Duplicate column`). + +Revision ID: 000_base +Revises: +Create Date: 2026-09-21 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +from sqlalchemy import inspect as sa_inspect + +# revision identifiers, used by Alembic. +revision: str = '000_base' +down_revision: Union[str, None] = None +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# Table creation order matters: each one only references tables created +# before it (users -> recipes -> recipe_versions -> recipe_tasks -> +# recipe_subtasks -> measurements -> access_logs / system_settings / +# recipe_version_audit). Downgrade drops them in the opposite order. +_TABLES_NEWEST_FIRST = ( + 'recipe_version_audit', 'system_settings', 'access_logs', 'measurements', + 'recipe_subtasks', 'recipe_tasks', 'recipe_versions', 'recipes', 'users', +) + + +def upgrade() -> None: + bind = op.get_bind() + if sa_inspect(bind).has_table('recipes'): + # Already created (by create_all, or by a previous run of this same + # migration) - nothing to do. + return + + op.create_table( + 'users', + sa.Column('id', sa.Integer, primary_key=True, autoincrement=True), + sa.Column('username', sa.String(100), nullable=False), + sa.Column('password_hash', sa.String(255), nullable=False), + sa.Column('email', sa.String(255), nullable=True), + sa.Column('display_name', sa.String(255), nullable=False), + sa.Column('roles', sa.JSON, nullable=False), + sa.Column('is_admin', sa.Boolean, nullable=False, server_default='0'), + sa.Column( + 'language_pref', sa.Enum('it', 'en', name='language_enum'), + nullable=False, server_default='it', + ), + sa.Column( + 'theme_pref', sa.Enum('light', 'dark', name='theme_enum'), + nullable=False, server_default='light', + ), + sa.Column('active', sa.Boolean, nullable=False, server_default='1'), + sa.Column('api_key', sa.String(64), nullable=True), + sa.Column('created_at', sa.DateTime, nullable=False, server_default=sa.func.now()), + sa.Column('last_login', sa.DateTime, nullable=True), + sa.UniqueConstraint('username', name='uq_users_username'), + sa.UniqueConstraint('api_key', name='uq_users_api_key'), + sa.Index('ix_users_username', 'username'), + sa.Index('ix_users_api_key', 'api_key'), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'recipes', + sa.Column('id', sa.Integer, primary_key=True, autoincrement=True), + sa.Column('code', sa.String(100), nullable=False), + sa.Column('name', sa.String(255), nullable=False), + sa.Column('description', sa.Text, nullable=True), + sa.Column('created_by', sa.Integer, sa.ForeignKey('users.id'), nullable=False), + sa.Column('created_at', sa.DateTime, nullable=False, server_default=sa.func.now()), + sa.Column('active', sa.Boolean, nullable=False, server_default='1'), + sa.UniqueConstraint('code', name='uq_recipes_code'), + sa.Index('ix_recipes_code', 'code'), + sa.Index('ix_recipes_active', 'active'), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'recipe_versions', + sa.Column('id', sa.Integer, primary_key=True, autoincrement=True), + sa.Column('recipe_id', sa.Integer, sa.ForeignKey('recipes.id'), nullable=False), + sa.Column('version_number', sa.Integer, nullable=False), + sa.Column('is_current', sa.Boolean, nullable=False, server_default='0'), + sa.Column('created_by', sa.Integer, sa.ForeignKey('users.id'), nullable=False), + sa.Column('created_at', sa.DateTime, nullable=False, server_default=sa.func.now()), + sa.Column('change_notes', sa.Text, nullable=True), + sa.UniqueConstraint('recipe_id', 'version_number', name='uq_recipe_version'), + sa.Index('ix_recipe_version_current', 'recipe_id', 'is_current'), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'recipe_tasks', + sa.Column('id', sa.Integer, primary_key=True, autoincrement=True), + sa.Column('version_id', sa.Integer, sa.ForeignKey('recipe_versions.id'), nullable=False), + sa.Column('order_index', sa.Integer, nullable=False, server_default='0'), + sa.Column('title', sa.String(255), nullable=False), + sa.Column('directive', sa.Text, nullable=True), + sa.Column('description', sa.Text, nullable=True), + sa.Column('file_path', sa.String(500), nullable=True), + sa.Column( + 'file_type', sa.Enum('image', 'pdf', name='file_type_enum'), + nullable=True, + ), + sa.Column('annotations_json', sa.JSON, nullable=True), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'recipe_subtasks', + sa.Column('id', sa.Integer, primary_key=True, autoincrement=True), + sa.Column( + 'task_id', sa.Integer, + sa.ForeignKey('recipe_tasks.id', ondelete='CASCADE'), nullable=False, + ), + sa.Column('marker_number', sa.Integer, nullable=False), + sa.Column('description', sa.String(500), nullable=False), + sa.Column('measurement_type', sa.String(100), nullable=True), + sa.Column('nominal', sa.DECIMAL(12, 6), nullable=True), + sa.Column('utl', sa.DECIMAL(12, 6), nullable=True), + sa.Column('uwl', sa.DECIMAL(12, 6), nullable=True), + sa.Column('lwl', sa.DECIMAL(12, 6), nullable=True), + sa.Column('ltl', sa.DECIMAL(12, 6), nullable=True), + sa.Column('unit', sa.String(20), nullable=False, server_default='mm'), + sa.UniqueConstraint('task_id', 'marker_number', name='uq_task_marker'), + sa.Index('ix_subtask_task_id', 'task_id'), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'measurements', + sa.Column('id', sa.Integer, primary_key=True, autoincrement=True), + sa.Column( + 'subtask_id', sa.Integer, sa.ForeignKey('recipe_subtasks.id'), + nullable=False, + ), + sa.Column( + 'version_id', sa.Integer, sa.ForeignKey('recipe_versions.id'), + nullable=False, + ), + sa.Column( + 'measured_by', sa.Integer, sa.ForeignKey('users.id'), nullable=False, + ), + sa.Column('value', sa.DECIMAL(12, 6), nullable=False), + sa.Column( + 'pass_fail', sa.Enum('pass', 'warning', 'fail', name='pass_fail_enum'), + nullable=False, + ), + sa.Column('deviation', sa.DECIMAL(12, 6), nullable=True), + sa.Column('lot_number', sa.String(100), nullable=True), + sa.Column('serial_number', sa.String(100), nullable=True), + sa.Column( + 'input_method', + sa.Enum('usb_caliper', 'manual', name='input_method_enum'), + nullable=False, server_default='manual', + ), + sa.Column( + 'measured_at', sa.DateTime, nullable=False, server_default=sa.func.now(), + ), + sa.Column('synced_to_csv', sa.Boolean, nullable=True, server_default='0'), + sa.Index('ix_measurements_subtask_id', 'subtask_id'), + sa.Index('ix_measurements_version_id', 'version_id'), + sa.Index('ix_measurements_measured_by', 'measured_by'), + sa.Index('ix_measurements_lot_number', 'lot_number'), + sa.Index('ix_measurements_serial_number', 'serial_number'), + sa.Index('ix_measurements_measured_at', 'measured_at'), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'access_logs', + sa.Column('id', sa.BigInteger, primary_key=True, autoincrement=True), + sa.Column('user_id', sa.Integer, sa.ForeignKey('users.id'), nullable=True), + sa.Column('action', sa.String(100), nullable=False), + sa.Column('details', sa.JSON, nullable=True), + sa.Column('ip_address', sa.String(45), nullable=True), + sa.Column('user_agent', sa.String(500), nullable=True), + sa.Column('created_at', sa.DateTime, nullable=False, server_default=sa.func.now()), + sa.Index('ix_access_logs_action', 'action'), + sa.Index('ix_access_log_user_time', 'user_id', 'created_at'), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'system_settings', + sa.Column('setting_key', sa.String(100), primary_key=True), + sa.Column('setting_value', sa.Text, nullable=False), + sa.Column( + 'setting_type', + sa.Enum('string', 'number', 'boolean', 'json', name='setting_type_enum'), + nullable=False, server_default='string', + ), + sa.Column('description', sa.String(500), nullable=True), + sa.Column('updated_at', sa.DateTime, nullable=False, server_default=sa.func.now()), + sa.Column('updated_by', sa.Integer, sa.ForeignKey('users.id'), nullable=True), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + op.create_table( + 'recipe_version_audit', + sa.Column('id', sa.Integer, primary_key=True, autoincrement=True), + sa.Column('recipe_id', sa.Integer, sa.ForeignKey('recipes.id'), nullable=False), + sa.Column('old_version_id', sa.Integer, nullable=True), + sa.Column('new_version_id', sa.Integer, nullable=False), + sa.Column('changed_by', sa.Integer, sa.ForeignKey('users.id'), nullable=False), + sa.Column( + 'change_type', + sa.Enum('CREATE', 'UPDATE', 'ACTIVATE', 'RETIRE', name='change_type_enum'), + nullable=False, + ), + sa.Column('change_reason', sa.Text, nullable=True), + sa.Column('created_at', sa.DateTime, nullable=False, server_default=sa.func.now()), + sa.Index('ix_recipe_audit_recipe_time', 'recipe_id', 'created_at'), + mysql_engine='InnoDB', + mysql_charset='utf8mb4', + ) + + +def downgrade() -> None: + bind = op.get_bind() + inspector = sa_inspect(bind) + for table in _TABLES_NEWEST_FIRST: + if inspector.has_table(table): + op.drop_table(table) diff --git a/src/backend/migrations/versions/001_add_image_path_to_recipe_and_subtask.py b/src/backend/migrations/versions/001_add_image_path_to_recipe_and_subtask.py index c1a5d39..b62b68e 100644 --- a/src/backend/migrations/versions/001_add_image_path_to_recipe_and_subtask.py +++ b/src/backend/migrations/versions/001_add_image_path_to_recipe_and_subtask.py @@ -12,7 +12,7 @@ import sqlalchemy as sa # revision identifiers, used by Alembic. revision: str = '001_image_path' -down_revision: Union[str, None] = None +down_revision: Union[str, None] = '000_base' branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None diff --git a/src/backend/tests/test_migrations.py b/src/backend/tests/test_migrations.py new file mode 100644 index 0000000..4df78a3 --- /dev/null +++ b/src/backend/tests/test_migrations.py @@ -0,0 +1,109 @@ +"""Tests for the Alembic migration chain, and specifically for +000_base_schema.py (issue #6): a genuinely empty database has no starting +point before it, since 001-013 all assume tables 001 was written against +already exist. + +These run the real `alembic upgrade head` (via `alembic.command`), not the +async app engine: `migrations/env.py` builds its own engine from +`settings.database_url`, so `Settings.database_url` is patched at the class +level (a plain instance monkeypatch cannot override a `@property`) to point +at a throwaway SQLite file for the duration of each test. +""" +from pathlib import Path + +import pytest +from alembic import command +from alembic.config import Config +from sqlalchemy import create_engine, inspect as sa_inspect, text + +from src.backend.config import Settings +from src.backend.database import Base + +_ALEMBIC_INI = ( + Path(__file__).resolve().parents[1] / "migrations" / "alembic.ini" +) + + +@pytest.fixture +def sqlite_database_url(tmp_path, monkeypatch): + """Point settings.database_url (and therefore migrations/env.py) at a + throwaway SQLite file, and return its async URL.""" + db_path = tmp_path / "migration_test.db" + async_url = f"sqlite+aiosqlite:///{db_path}" + monkeypatch.setattr(Settings, "database_url", property(lambda self: async_url)) + return db_path, async_url + + +def _alembic_config() -> Config: + return Config(str(_ALEMBIC_INI)) + + +def _inspect(db_path: Path): + engine = create_engine(f"sqlite:///{db_path}") + try: + return sa_inspect(engine) + finally: + engine.dispose() + + +class TestUpgradeHeadFromEmpty: + """Full chain on a genuinely empty SQLite database (#6).""" + + def test_upgrade_head_succeeds_on_empty_database(self, sqlite_database_url): + db_path, _ = sqlite_database_url + command.upgrade(_alembic_config(), "head") # must not raise + + insp = _inspect(db_path) + assert set(Base.metadata.tables) <= set(insp.get_table_names()) + + def test_tables_and_columns_match_base_metadata(self, sqlite_database_url): + """Every table and column ORM models declare must exist after + `upgrade head` - the exact assertion the analysis asks for.""" + db_path, _ = sqlite_database_url + command.upgrade(_alembic_config(), "head") + + insp = _inspect(db_path) + db_tables = set(insp.get_table_names()) + + for table_name, table in Base.metadata.tables.items(): + assert table_name in db_tables, f"missing table {table_name}" + db_columns = {c["name"] for c in insp.get_columns(table_name)} + model_columns = {c.name for c in table.columns} + missing = model_columns - db_columns + assert not missing, f"{table_name} missing columns: {missing}" + + +class TestBaseMigrationNoopsOnExistingSchema: + """000_base_schema.py does nothing when `recipes` already exists (#6).""" + + def test_upgrade_head_noops_000_when_pre_001_schema_already_there( + self, sqlite_database_url + ): + db_path, _ = sqlite_database_url + cfg = _alembic_config() + + # Build the pre-001 schema and record it as migrated. + command.upgrade(cfg, "000_base") + insp = _inspect(db_path) + assert "recipes" in insp.get_table_names() + assert "image_path" not in { + c["name"] for c in insp.get_columns("recipes") + } + + # Erase the tracking: alembic now believes nothing has run, exactly + # like a database that was born from Base.metadata.create_all() and + # never stamped. Running 000's upgrade() again on a database that + # already has `recipes` must be a no-op, not a "table already + # exists" failure. + sync_engine = create_engine(f"sqlite:///{db_path}") + with sync_engine.begin() as conn: + conn.execute(text("DELETE FROM alembic_version")) + sync_engine.dispose() + + command.upgrade(cfg, "head") # must not raise (no duplicate table) + + insp = _inspect(db_path) + assert set(Base.metadata.tables) <= set(insp.get_table_names()) + assert "image_path" in { + c["name"] for c in insp.get_columns("recipes") + } diff --git a/src/backend/tests/test_setup.py b/src/backend/tests/test_setup.py index 5746c22..673e22d 100644 --- a/src/backend/tests/test_setup.py +++ b/src/backend/tests/test_setup.py @@ -23,6 +23,11 @@ def enable_setup(monkeypatch): import src.backend.api.routers.setup as setup_mod monkeypatch.setattr(setup_mod, "engine", test_engine) monkeypatch.setattr(setup_mod, "async_session_factory", TestSessionFactory) + # _stamp_head runs `alembic stamp head`, which builds its own connection + # from settings.database_url via migrations/env.py - the real MySQL URL, + # not the patched test engine above. No-op it here so init-db/reset-db + # tests never try to reach a MySQL server that does not exist in CI/dev. + monkeypatch.setattr(setup_mod, "_stamp_head", lambda: None) async def _seed(client: AsyncClient) -> dict: diff --git a/src/backend/tests/test_station_seed.py b/src/backend/tests/test_station_seed.py index acb6099..b3848dc 100644 --- a/src/backend/tests/test_station_seed.py +++ b/src/backend/tests/test_station_seed.py @@ -34,6 +34,9 @@ def enable_setup(monkeypatch): import src.backend.api.routers.setup as setup_mod monkeypatch.setattr(setup_mod, "engine", test_engine) monkeypatch.setattr(setup_mod, "async_session_factory", TestSessionFactory) + # See test_setup.py: _stamp_head would otherwise try to reach a real + # MySQL server via migrations/env.py. + monkeypatch.setattr(setup_mod, "_stamp_head", lambda: None) async def _seed(client: AsyncClient) -> dict: