diff --git a/.github/workflows/auto_tests.yml b/.github/workflows/auto_tests.yml index 4d78bd65..0f7596e3 100644 --- a/.github/workflows/auto_tests.yml +++ b/.github/workflows/auto_tests.yml @@ -3,9 +3,18 @@ name: Auto Tests on: push jobs: - server_tests: + tests: runs-on: ubuntu-24.04 + strategy: + fail-fast: false + matrix: + include: + - suite: server + pytest_args: "-v --cov=mergin --cov-report=lcov mergin/tests" + - suite: migration + pytest_args: "-v mergin/test_migrations" + services: postgres: image: postgres:14 @@ -15,13 +24,18 @@ jobs: POSTGRES_USER: postgres ports: - 5435:5432 - # Set health checks to wait until postgres has started options: >- --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 + env: + DB_USER: postgres + DB_PASSWORD: postgres + DB_HOST: localhost + DB_PORT: 5435 + steps: - name: Check out repository uses: actions/checkout@v3 @@ -36,9 +50,10 @@ jobs: - name: Run tests run: | cd server - pipenv run pytest -v --cov=mergin --cov-report=lcov mergin/tests + pipenv run pytest ${{ matrix.pytest_args }} - name: Coveralls + if: matrix.suite == 'server' uses: coverallsapp/github-action@v2 with: base-path: server diff --git a/server/mergin/test_migrations/__init__.py b/server/mergin/test_migrations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/server/mergin/test_migrations/test_migrations.py b/server/mergin/test_migrations/test_migrations.py new file mode 100644 index 00000000..8bfa1154 --- /dev/null +++ b/server/mergin/test_migrations/test_migrations.py @@ -0,0 +1,38 @@ +# Copyright (C) Lutra Consulting Limited +# +# SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial + +from pathlib import Path + +from ..app import db +from .utils import ( + make_migration_app, + make_migration_engine, + ordered_revisions, + run_migration_lifecycle, +) + +MIGRATIONS_DIR = str(Path(__file__).parents[2] / "migrations") +# 1fcbea2a0f2c (drop_namespace_related_objects) has an intentional no-op downgrade +# because removing namespace tables is irreversible. We stop there. +DOWNGRADE_TARGET = "1fcbea2a0f2c" + +migration_engine = make_migration_engine("mergin_migration_test") +migration_app = make_migration_app( + model_modules=[ + "mergin.auth.models", + "mergin.stats.models", + "mergin.sync.models", + ] +) + + +def test_migration_lifecycle(migration_app, migration_engine): + """Exercise the full migration chain: empty DB → head (one step at a time) → schema check → partial downgrade.""" + run_migration_lifecycle( + migration_engine, + MIGRATIONS_DIR, + ordered_revisions(MIGRATIONS_DIR), + db.metadata, + DOWNGRADE_TARGET, + ) diff --git a/server/mergin/test_migrations/utils.py b/server/mergin/test_migrations/utils.py new file mode 100644 index 00000000..8f174a74 --- /dev/null +++ b/server/mergin/test_migrations/utils.py @@ -0,0 +1,241 @@ +# Copyright (C) Lutra Consulting Limited +# +# SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial + +import pytest +from alembic.autogenerate import compare_metadata +from alembic.config import Config as AlembicConfig +from alembic.runtime.migration import MigrationContext +from alembic.script import ScriptDirectory +from flask_migrate import downgrade, upgrade +from pathlib import Path +from sqlalchemy import create_engine, make_url, text + +# create_simple_app and Configuration are imported lazily inside their respective +# factory functions to avoid triggering Configuration evaluation at module import +# time (before pytest-dotenv has loaded .test.env into os.environ). + +_STRUCTURAL_DIFF_TYPES = ( + "add_table", + "remove_table", + "add_column", + "remove_column", + "modify_nullable", + "add_index", + "remove_index", + "add_constraint", + "remove_constraint", +) + + +def assert_schema_consistent(engine, metadata, include_schemas=()): + """Assert ORM metadata matches the migrated schema — structural checks only. + + Column types and server defaults are excluded: alembic's reflection of + PostgreSQL-specific types (JSONB, UUID, ARRAY …) and default expressions + produces false positives without an explicit allowlist. + + include_schemas: additional PostgreSQL schemas beyond 'public' to verify. + Extra schemas are checked via direct inspection (table existence only) rather + than compare_metadata, because include_schemas=True in MigrationContext causes + public-schema tables to be reflected with an explicit "public" qualifier that + doesn't match ORM models with schema=None — producing spurious add_table diffs. + """ + from sqlalchemy import inspect as sa_inspect + + extra_schema_set = set(include_schemas) + + def _table_schema(d): + """Extract the schema from a diff entry, or None.""" + if len(d) < 2: + return None + obj = d[1] + # add_table / remove_table: obj is the Table + schema = getattr(obj, "schema", None) + if schema is not None: + return schema + # add_index / remove_index: obj is an Index whose .table has the schema + table = getattr(obj, "table", None) + return getattr(table, "schema", None) + + # Standard structural check for the public (default) schema. + # Diffs involving tables in extra schemas are excluded here — they are + # checked separately below via direct inspection to avoid the schema-name + # mismatch that include_schemas=True causes in compare_metadata. + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + diff = compare_metadata(ctx, metadata) + checked = [ + d + for d in diff + if d[0] in _STRUCTURAL_DIFF_TYPES and _table_schema(d) not in extra_schema_set + ] + + # Table-existence check for each additional schema + if include_schemas: + inspector = sa_inspect(engine) + for schema in include_schemas: + existing = set(inspector.get_table_names(schema=schema)) + expected = { + table.name + for table in metadata.tables.values() + if table.schema == schema + } + for name in sorted(expected - existing): + checked.append(("add_table", f"{schema}.{name}")) + for name in sorted(existing - expected): + checked.append(("remove_table", f"{schema}.{name}")) + + assert not checked, ( + "ORM models differ from the migrated schema — add a migration or update the filter:\n" + + "\n".join(str(d) for d in checked) + ) + + +def ordered_revisions(migrations_dir, head="head", version_locations=None): + """Return revision IDs in upgrade order (base → head). + + Works for single-head (CE), single branch label (enterprise@head), and + multi-head tuples (("enterprise@head", "service@head")) alembic setups. + Pass version_locations as a colon-separated string when the script directory + has more than one branch folder. + """ + cfg = AlembicConfig() + cfg.set_main_option("script_location", migrations_dir) + cfg.set_main_option( + "version_locations", + version_locations or str(Path(migrations_dir) / "community"), + ) + cfg.set_main_option("path_separator", "os") + script = ScriptDirectory.from_config(cfg) + upper = tuple(head) if isinstance(head, (list, tuple)) else head + return [ + rev.revision + for rev in reversed(list(script.iterate_revisions(upper=upper, lower="base"))) + ] + + +def make_migration_app(extra_config=None, model_modules=()): + """Return a module-scoped pytest fixture that provides a minimal Flask app context. + + The app context is all alembic's env.py needs: it reads SQLALCHEMY_DATABASE_URI + from current_app.config and db metadata from Flask-Migrate's extension. + + model_modules: sequence of dotted module paths to import when the fixture runs, + e.g. ["mergin.sync.models", "src.workspace.models"]. Importing here (inside the + fixture) rather than at test-module level avoids db.metadata cross-contamination + when multiple migration test files are collected in the same pytest session: models + are only added to db.metadata when the fixture first executes, not at collection time. + + extra_config: optional dict of additional config values to set on the app. + Use this when a migration reads from current_app.config for non-DB settings. + """ + + @pytest.fixture(scope="module") + def migration_app(migration_engine): + import importlib + + from ..app import create_simple_app + + for module_path in model_modules: + importlib.import_module(module_path) + + app = create_simple_app() + app.config["SQLALCHEMY_DATABASE_URI"] = migration_engine.url.render_as_string( + hide_password=False + ) + if extra_config: + app.config.update(extra_config) + ctx = app.app_context() + ctx.push() + try: + yield app + finally: + ctx.pop() + + return migration_app + + +def make_migration_engine(test_db_name, pre_migration_sql=()): + """Return a module-scoped pytest fixture that creates and tears down a migration test DB. + + pre_migration_sql: optional sequence of SQL statements executed once after the DB + is created but before any migration runs (e.g. CREATE SCHEMA, CREATE EXTENSION). + """ + + @pytest.fixture(scope="module") + def migration_engine(): + from ..config import Configuration + + base_url = make_url(Configuration.SQLALCHEMY_DATABASE_URI) + admin_url = base_url.set(database="postgres") + test_db_url = base_url.set(database=test_db_name) + + admin_engine = create_engine(admin_url, isolation_level="AUTOCOMMIT") + with admin_engine.connect() as conn: + conn.execute(text(f"DROP DATABASE IF EXISTS {test_db_name}")) + conn.execute(text(f"CREATE DATABASE {test_db_name}")) + admin_engine.dispose() + + engine = create_engine(test_db_url) + if pre_migration_sql: + with engine.connect() as conn: + for sql in pre_migration_sql: + conn.execute(text(sql)) + conn.commit() + + yield engine + engine.dispose() + + admin_engine = create_engine(admin_url, isolation_level="AUTOCOMMIT") + with admin_engine.connect() as conn: + conn.execute(text(f"DROP DATABASE IF EXISTS {test_db_name}")) + admin_engine.dispose() + + return migration_engine + + +def run_migration_lifecycle( + engine, migrations_dir, revisions, metadata, downgrade_targets, include_schemas=() +): + """Run the three-phase migration lifecycle used by all migration test suites. + + Phase 1 — upgrade one revision at a time, asserting alembic_version after each. + Works for both single-head and multi-head chains: the applied-versions + set is checked with `in` so interleaved branch revisions pass correctly. + Phase 2 — structural schema consistency check between ORM metadata and the migrated DB. + Phase 3 — downgrade to each target in downgrade_targets (str or list[str]), + asserting the final alembic_version set matches exactly. + """ + assert ( + revisions + ), "ordered_revisions returned an empty list — check migrations_dir and version_locations" + for rev in revisions: + upgrade(directory=migrations_dir, revision=rev) + with engine.connect() as conn: + applied = { + row[0] + for row in conn.execute( + text("SELECT version_num FROM alembic_version") + ).fetchall() + } + assert ( + rev in applied + ), f"Migration {rev} did not apply correctly: alembic_version is {applied!r}" + + assert_schema_consistent(engine, metadata, include_schemas) + + if isinstance(downgrade_targets, str): + downgrade_targets = [downgrade_targets] + for target in downgrade_targets: + downgrade(directory=migrations_dir, revision=target) + with engine.connect() as conn: + final = { + row[0] + for row in conn.execute( + text("SELECT version_num FROM alembic_version") + ).fetchall() + } + assert final == set( + downgrade_targets + ), f"Unexpected state after downgrade: {final!r}" diff --git a/server/migrations/community/07f2185e2428_case_insensitive_unique_idx.py b/server/migrations/community/07f2185e2428_case_insensitive_unique_idx.py index 8c966a9c..985c306a 100644 --- a/server/migrations/community/07f2185e2428_case_insensitive_unique_idx.py +++ b/server/migrations/community/07f2185e2428_case_insensitive_unique_idx.py @@ -20,18 +20,26 @@ def upgrade(): conn = op.get_bind() conn.execute( - "CREATE UNIQUE INDEX ix_user_username ON public.user (LOWER(username));" + sa.text( + "CREATE UNIQUE INDEX ix_user_username ON public.user (LOWER(username));" + ) ) - conn.execute("CREATE UNIQUE INDEX ix_user_email ON public.user (LOWER(email));") - conn.execute("ALTER TABLE public.user DROP CONSTRAINT uq_user_email;") - conn.execute("ALTER TABLE public.user DROP CONSTRAINT uq_user_username;") + conn.execute( + sa.text("CREATE UNIQUE INDEX ix_user_email ON public.user (LOWER(email));") + ) + conn.execute(sa.text("ALTER TABLE public.user DROP CONSTRAINT uq_user_email;")) + conn.execute(sa.text("ALTER TABLE public.user DROP CONSTRAINT uq_user_username;")) def downgrade(): conn = op.get_bind() - conn.execute("DROP INDEX IF EXISTS ix_user_username;") - conn.execute("DROP INDEX IF EXISTS ix_user_email;") - conn.execute("ALTER TABLE public.user ADD CONSTRAINT uq_user_email UNIQUE (email);") + conn.execute(sa.text("DROP INDEX IF EXISTS ix_user_username;")) + conn.execute(sa.text("DROP INDEX IF EXISTS ix_user_email;")) + conn.execute( + sa.text("ALTER TABLE public.user ADD CONSTRAINT uq_user_email UNIQUE (email);") + ) conn.execute( - "ALTER TABLE public.user ADD CONSTRAINT uq_user_username UNIQUE (username);" + sa.text( + "ALTER TABLE public.user ADD CONSTRAINT uq_user_username UNIQUE (username);" + ) ) diff --git a/server/migrations/community/3daefa84ce67_add_deployment_info.py b/server/migrations/community/3daefa84ce67_add_deployment_info.py index 6b7e3f64..4014ecc8 100644 --- a/server/migrations/community/3daefa84ce67_add_deployment_info.py +++ b/server/migrations/community/3daefa84ce67_add_deployment_info.py @@ -34,7 +34,9 @@ def upgrade(): uuid.UUID(os.getenv("SERVICE_ID")) if os.getenv("SERVICE_ID") else uuid.uuid4() ) conn = op.get_bind() - conn.execute(f"INSERT INTO mergin_info VALUES ('{key}')") + conn.execute( + sa.text("INSERT INTO mergin_info VALUES (:service_id)"), {"service_id": key} + ) def downgrade(): diff --git a/server/migrations/community/c13819c566e7_migrate_away_from_jsonb_columns.py b/server/migrations/community/c13819c566e7_migrate_away_from_jsonb_columns.py index 81bbddf3..55acc9d5 100644 --- a/server/migrations/community/c13819c566e7_migrate_away_from_jsonb_columns.py +++ b/server/migrations/community/c13819c566e7_migrate_away_from_jsonb_columns.py @@ -226,7 +226,7 @@ def data_downgrade(): WHERE pv.name = 1 ) UPDATE project_version pv - SET files = first_pushes.files + SET files = first_pushes.file FROM first_pushes WHERE first_pushes.version_id = pv.id; """ diff --git a/server/migrations/community/dbd428cda965_migrate_to_jsonb.py b/server/migrations/community/dbd428cda965_migrate_to_jsonb.py index d77b540f..62248d5a 100644 --- a/server/migrations/community/dbd428cda965_migrate_to_jsonb.py +++ b/server/migrations/community/dbd428cda965_migrate_to_jsonb.py @@ -10,6 +10,7 @@ """ from alembic import op +from sqlalchemy import text # revision identifiers, used by Alembic. revision = "dbd428cda965" @@ -21,28 +22,40 @@ def upgrade(): conn = op.get_bind() conn.execute( - "ALTER TABLE project_version ALTER COLUMN files SET DATA TYPE jsonb USING files::jsonb;" + text( + "ALTER TABLE project_version ALTER COLUMN files SET DATA TYPE jsonb USING files::jsonb;" + ) ) conn.execute( - "ALTER TABLE project_version ALTER COLUMN changes SET DATA TYPE jsonb USING changes::jsonb;" + text( + "ALTER TABLE project_version ALTER COLUMN changes SET DATA TYPE jsonb USING changes::jsonb;" + ) ) conn.execute( - "ALTER TABLE project ALTER COLUMN files SET DATA TYPE jsonb USING files::jsonb;" + text( + "ALTER TABLE project ALTER COLUMN files SET DATA TYPE jsonb USING files::jsonb;" + ) ) conn.execute( - "CREATE INDEX ix_project_version_files_gin ON project_version USING gin (files);" + text( + "CREATE INDEX ix_project_version_files_gin ON project_version USING gin (files);" + ) ) conn.execute( - "CREATE INDEX ix_project_version_changes_gin ON project_version USING gin (changes);" + text( + "CREATE INDEX ix_project_version_changes_gin ON project_version USING gin (changes);" + ) + ) + conn.execute( + text("CREATE INDEX ix_project_files_gin ON project USING gin (files);") ) - conn.execute("CREATE INDEX ix_project_files_gin ON project USING gin (files);") def downgrade(): conn = op.get_bind() - conn.execute("DROP INDEX IF EXISTS ix_project_version_files_gin;") - conn.execute("DROP INDEX IF EXISTS ix_project_version_changes_gin;") - conn.execute("DROP INDEX IF EXISTS ix_project_files_gin;") - conn.execute("ALTER TABLE project_version ALTER COLUMN files TYPE json;") - conn.execute("ALTER TABLE project_version ALTER COLUMN changes TYPE json;") - conn.execute("ALTER TABLE project ALTER COLUMN files TYPE json;") + conn.execute(text("DROP INDEX IF EXISTS ix_project_version_files_gin;")) + conn.execute(text("DROP INDEX IF EXISTS ix_project_version_changes_gin;")) + conn.execute(text("DROP INDEX IF EXISTS ix_project_files_gin;")) + conn.execute(text("ALTER TABLE project_version ALTER COLUMN files TYPE json;")) + conn.execute(text("ALTER TABLE project_version ALTER COLUMN changes TYPE json;")) + conn.execute(text("ALTER TABLE project ALTER COLUMN files TYPE json;"))