diff --git a/AGENTS.md b/AGENTS.md index a35aeedda..631b454b7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,6 +22,9 @@ When changing code that aggregates simulation microdata (the output builders, the national reduce, Stage 12 aggregation, or anything that reads `output_dataset.data`), read `docs/engineering/skills/microdata-weights.md`. +Before adding code that connects to a database, declares tables, or changes a +database schema, read `docs/engineering/skills/database-access.md`. + When adding or changing a GitHub Actions workflow step or a script under `.github/scripts/`, read `docs/engineering/skills/ci-workflows.md`. @@ -97,6 +100,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Service and package boundaries - `sim.boundaries.gateway-executor-decoupled`: The gateway and executor never import each other; they communicate only through the models and state helpers in policyengine-simulation-contract. (doc-only, not checked: follow it yourself) +- `sim.boundaries.sqlalchemy-and-alembic-only`: Code connects to databases only through SQLAlchemy engines and connections, never by importing a database driver directly, and runtime code never changes the schema, which changes only through Alembic migrations. (blocking: pytest `projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py`) - `sim.boundaries.unit-tests-need-no-services`: Unit tests never require real network credentials, Modal, Hugging Face, GCP or deployed services; those seams are mocked. (doc-only, not checked: follow it yourself) ### Stage 12 persistence diff --git a/changelog_entry.yaml b/changelog_entry.yaml index 30ac14cfe..ffa169491 100644 --- a/changelog_entry.yaml +++ b/changelog_entry.yaml @@ -1,6 +1,7 @@ - bump: patch changes: added: + - Added the rule sim.boundaries.sqlalchemy-and-alembic-only, with proof from the psycopg adapter of 08ebe1ca; code connects to databases only through SQLAlchemy, never by importing a database driver, and runtime code never changes the schema, which changes only through Alembic migrations; a guard parses every file under projects/*/src, libs/*/src and .github/scripts and rejects driver imports, alembic imports outside a migrations directory, create_all and drop_all, create and drop on tables, SQLAlchemy DDL statements and DDL text in string literals, which extends the Stage 12 DDL check's pattern to the gateway and every library - Made the budget's reform-minus-baseline totals log a warning when the baseline and reform household frames carry different weights, since the change is then weighted by the reform's weights alone - Added weighted-microdata rules to the registry, with proof from the #767 household count; the executor's code never aggregates a weight column as a value, reads raw values from weighted frames only in simulation_output_common.py (the UK local-authority breakdown's plain household frame is its one recorded exception there, in a list that may only shrink), and its output-builder tests build entity frames through one shared factory with non-uniform weights, while a doc-only rule records that weights, values, labels and counts each allow different operations - Made CLAUDE.md and the Copilot instructions thin pointers to AGENTS.md, required workflow logic longer than one line to live in tested and shellchecked scripts, and registered the tracking-issue rule diff --git a/docs/engineering/skills/README.md b/docs/engineering/skills/README.md index 5a6061631..bc64328ee 100644 --- a/docs/engineering/skills/README.md +++ b/docs/engineering/skills/README.md @@ -11,6 +11,8 @@ Current skills: - `ci-workflows.md`: one line of code per workflow `run:` block, tested workflow scripts, and shellcheck. +- `database-access.md`: SQLAlchemy-only database connections, schema changes + only through Alembic migrations, and the guard that checks both. - `github-prs.md`: same-repository draft PR workflow, issue linkage, PR head verification, and title conventions. - `testing.md`: test placement, dependency boundaries, and expected validation diff --git a/docs/engineering/skills/database-access.md b/docs/engineering/skills/database-access.md new file mode 100644 index 000000000..1ed83a886 --- /dev/null +++ b/docs/engineering/skills/database-access.md @@ -0,0 +1,80 @@ +# Database Access Skill + +Read this before adding code that connects to a relational database, declares +tables, or changes a database schema. + +## Rules + +`sim.boundaries.sqlalchemy-and-alembic-only` has two parts: + +- **Connections go through SQLAlchemy.** Code connects to databases only + through SQLAlchemy engines and connections, never by importing a database + driver directly. Select the driver in the URL's driver name instead, as the + Stage 12 store does with `postgresql+psycopg`. +- **Runtime code never changes the schema.** Schema changes happen only + through Alembic migrations, never through `create_all`, `drop_all`, + `Table.create`, `Table.drop`, SQLAlchemy DDL statements or DDL text in + runtime code. + +It builds on the Stage 12 rules: + +- `sim.stage12.no-schema-ddl`: the simulation runtimes and the Stage 12 + persistence library contain no schema DDL. Its `DDL_PATTERN` regex is the + one this rule matches against every source directory's string literals. +- `sim.stage12.no-migration-chain`: this repository has no migration chain + of its own; policyengine-api migrates the Stage 12 tables. +- `sim.stage12.persistence-typed-dml-only` and + `sim.stage12.deployment-uses-api-schema`: the Stage 12 persistence + library uses typed SQLAlchemy Core DML only, with no raw SQL text. + +## Who owns the schema + +- **Today**, policyengine-api's Alembic migrations own the only tables this + repository uses, the Stage 12 comparison tables + (`sim.stage12.no-migration-chain`). This repository's code reads and + writes their rows, and deployment validation compares their live schema + with its SQLAlchemy mappings; nothing here changes them. +- **Once #847 merges**, this repository's own Alembic chain at `migrations/` + owns the `simulation` schema, and the Stage 12 tables stay with + policyengine-api. + +The rule holds either way: whichever chain owns a table, only that chain's +migrations change it. + +## What the guard checks + +`projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py` +parses every Python file under `projects/*/src` (including the executor's +`src/modal`), `libs/*/src` and `.github/scripts`, and runs in the entry +project's `Test` job. The checks live in `database_access_support.py` next to +it. It fails on: + +- an import of a database driver: `psycopg`, `psycopg2`, `pymysql`, + `MySQLdb`, `mysql.connector`, `sqlite3`, `asyncpg`, `pg8000`, `aiosqlite` + or `aiomysql`, or any of their submodules; +- an import of `alembic` outside a `migrations` directory; +- a `create_all` or `drop_all` call; +- a `create()` or `drop()` call on a table: a `Table(...)` call, a name or + attribute bound to one or annotated as `Table` in any scanned file, a + `__table__` attribute or an item of `.tables`, or any `create()` or `drop()` + call passing `checkfirst=`. Other `drop()` calls, such as pandas' + `frame.drop(columns=...)`, are not schema changes; +- SQLAlchemy's DDL statements (`CreateTable`, `DropTable`, `CreateSchema`, + `DDL` and the rest); +- DDL text in a string literal: `CREATE`, `ALTER`, `DROP` or `TRUNCATE` of a + table, type, schema or database. Docstrings and comments don't count. + +Files under a `migrations` directory may import alembic and change the schema, +but still connect through SQLAlchemy. + +The guard reads source, not types. It misses imports through `importlib` or +`__import__`, a table reached through some other local variable, and SQL text +assembled from separate literals. It doesn't read shell scripts or SQL files; +today no shell script calls a database client, and the repository has no SQL +files. + +## Out of scope + +The rule covers relational databases only. Non-relational stores are out of +scope: Modal Dicts, object storage such as the Google Cloud Storage artifact +and cache buckets, and in-process or on-disk caches. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index ef470c45b..7d2a09995 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 42 | 2 | 0 | 15 | 59 | +| mechanical | 43 | 2 | 0 | 15 | 60 | | metric | 0 | 0 | 0 | 0 | 0 | | judgment | 0 | 0 | 0 | 0 | 0 | -**Blocking:** 44 of 59 rules. +**Blocking:** 45 of 60 rules. ## Gaps @@ -77,14 +77,20 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | | `sim.boundaries.gateway-executor-decoupled` | The gateway and executor never import each other; they communicate only through the models and state helpers in policyengine-simulation-contract. | mechanical | doc-only | no | nothing | — | — | +| `sim.boundaries.sqlalchemy-and-alembic-only` | Code connects to databases only through SQLAlchemy engines and connections, never by importing a database driver directly, and runtime code never changes the schema, which changes only through Alembic migrations. | mechanical | enforced | yes | pytest `projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py` | `pr.yml:test` | yes | | `sim.boundaries.unit-tests-need-no-services` | Unit tests never require real network credentials, Modal, Hugging Face, GCP or deployed services; those seams are mocked. | mechanical | doc-only | no | nothing | — | — | ```mermaid flowchart LR r_sim_boundaries_gateway_executor_decoupled("sim.boundaries.gateway-executor-decoupled (doc-only)") gap_unenforced>"not enforced"] + r_sim_boundaries_sqlalchemy_and_alembic_only("sim.boundaries.sqlalchemy-and-alembic-only (enforced)") + e_pytest_projects_policyengine_simulation_entry_tests_test_database_access_boundaries_py["pytest projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py"] + j_CI_pr_yml_test["CI pr.yml:test"] r_sim_boundaries_unit_tests_need_no_services("sim.boundaries.unit-tests-need-no-services (doc-only)") r_sim_boundaries_gateway_executor_decoupled -.-> gap_unenforced + r_sim_boundaries_sqlalchemy_and_alembic_only --> e_pytest_projects_policyengine_simulation_entry_tests_test_database_access_boundaries_py + e_pytest_projects_policyengine_simulation_entry_tests_test_database_access_boundaries_py --> j_CI_pr_yml_test r_sim_boundaries_unit_tests_need_no_services -.-> gap_unenforced ``` diff --git a/projects/policyengine-simulation-entry/tests/database_access_support.py b/projects/policyengine-simulation-entry/tests/database_access_support.py new file mode 100644 index 000000000..752a4d9e6 --- /dev/null +++ b/projects/policyengine-simulation-entry/tests/database_access_support.py @@ -0,0 +1,273 @@ +"""Checks behind the repository's database access rules in rules/registry.toml. + +``DDL_PATTERN`` is shared: sim.stage12.no-schema-ddl matches it against whole +Stage 12 runtime files, and sim.boundaries.sqlalchemy-and-alembic-only matches +it against the string literals of every source file. ``database_access_findings`` +holds the second rule's checks, so the rule test runs it on the repository and +the proof test runs it on the fixtures under ``rules/fixtures//``. + +The checks read source, not types. They recognise a table as a ``Table(...)`` +call, a name or attribute bound to one or annotated as ``Table`` in any of the +scanned files, a ``__table__`` attribute or an item of ``.tables``. They don't +follow imports made through ``importlib`` or ``__import__``, tables reached +through other local variables, or SQL text assembled from separate literals. +""" + +from __future__ import annotations + +import ast +import re +from dataclasses import dataclass +from pathlib import PurePosixPath +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Iterator, Mapping + +# Schema DDL as SQL text, and metadata creation. +DDL_PATTERN = re.compile( + r"\b(?:CREATE|ALTER|DROP|TRUNCATE)\s+" + r"(?:TABLE|TYPE|SCHEMA|DATABASE)\b|\bcreate_all\s*\(", + flags=re.IGNORECASE, +) + +# Modules that open database connections without SQLAlchemy. A module matches +# itself and every submodule, so psycopg covers psycopg.rows. +DATABASE_DRIVER_MODULES = frozenset( + { + "aiomysql", + "aiosqlite", + "asyncpg", + "MySQLdb", + "mysql.connector", + "pg8000", + "psycopg", + "psycopg2", + "pymysql", + "sqlite3", + } +) +SCHEMA_MIGRATION_MODULE = "alembic" +# A directory holding an Alembic chain; only files under it may import alembic +# or change the schema. +MIGRATIONS_DIRECTORY = "migrations" +METADATA_SCHEMA_METHODS = frozenset({"create_all", "drop_all"}) +TABLE_SCHEMA_METHODS = frozenset({"create", "drop"}) +# SQLAlchemy's create and drop methods take checkfirst; pandas' don't. +SCHEMA_METHOD_KEYWORD = "checkfirst" +TABLE_CLASS = "Table" +TABLE_ATTRIBUTE = "__table__" +METADATA_TABLES_ATTRIBUTE = "tables" +# SQLAlchemy statements that create, drop or alter schema objects when executed. +SQLALCHEMY_DDL_CONSTRUCTS = frozenset( + { + "AddConstraint", + "CreateIndex", + "CreateSchema", + "CreateSequence", + "CreateTable", + "DDL", + "DropConstraint", + "DropIndex", + "DropSchema", + "DropSequence", + "DropTable", + } +) + +DRIVER_IMPORT = "database driver import" +ALEMBIC_IMPORT = "alembic import outside a migrations directory" +SCHEMA_CHANGE_CALL = "schema change call" +DDL_CONSTRUCT = "SQLAlchemy DDL construct" +DDL_TEXT = "DDL text" + + +@dataclass(frozen=True) +class Finding: + """One way a source file reaches a database or changes its schema.""" + + file: str + line: int + check: str + source: str + + def __str__(self) -> str: + """Describe the finding as ``file:line: check: source``.""" + return f"{self.file}:{self.line}: {self.check}: {self.source}" + + +def _matches_module(module_name: str, forbidden_module: str) -> bool: + return module_name == forbidden_module or module_name.startswith( + f"{forbidden_module}." + ) + + +def _imported_modules(node: ast.Import | ast.ImportFrom) -> list[str]: + if isinstance(node, ast.Import): + return [alias.name for alias in node.names] + if node.level or node.module is None: + return [] + # ``from mysql import connector`` imports mysql.connector. + return [node.module, *(f"{node.module}.{alias.name}" for alias in node.names)] + + +def _called_attribute(node: ast.expr) -> str: + if isinstance(node, ast.Name): + return node.id + if isinstance(node, ast.Attribute): + return node.attr + return "" + + +def _annotates_table(annotation: ast.expr | None) -> bool: + if annotation is None: + return False + if isinstance(annotation, ast.Constant) and isinstance(annotation.value, str): + return annotation.value.rsplit(".", 1)[-1] == TABLE_CLASS + return _called_attribute(annotation) == TABLE_CLASS + + +def _is_table_call(node: ast.expr) -> bool: + return isinstance(node, ast.Call) and _called_attribute(node.func) == TABLE_CLASS + + +def _bound_names(target: ast.expr) -> list[str]: + if isinstance(target, (ast.Name, ast.Attribute)): + return [_called_attribute(target)] + if isinstance(target, (ast.Tuple, ast.List)): + return [name for element in target.elts for name in _bound_names(element)] + return [] + + +def table_names(tree: ast.Module) -> frozenset[str]: + """Return the names a module binds to a ``Table(...)`` or annotates as one.""" + names: set[str] = set() + for node in ast.walk(tree): + if isinstance(node, ast.Assign) and _is_table_call(node.value): + names.update( + name for target in node.targets for name in _bound_names(target) + ) + elif isinstance(node, ast.AnnAssign) and ( + _annotates_table(node.annotation) + or (node.value is not None and _is_table_call(node.value)) + ): + names.update(_bound_names(node.target)) + elif isinstance(node, ast.arg) and _annotates_table(node.annotation): + names.add(node.arg) + return frozenset(names) + + +def _is_table(node: ast.expr, known_tables: frozenset[str]) -> bool: + if _is_table_call(node): + return True + if isinstance(node, ast.Attribute) and node.attr == TABLE_ATTRIBUTE: + return True + if isinstance(node, ast.Subscript): + return _called_attribute(node.value) == METADATA_TABLES_ATTRIBUTE + return _called_attribute(node) in known_tables + + +def _changes_schema(call: ast.Call, known_tables: frozenset[str]) -> bool: + if not isinstance(call.func, ast.Attribute): + return False + method = call.func.attr + if method in METADATA_SCHEMA_METHODS: + return True + if method not in TABLE_SCHEMA_METHODS: + return False + return _is_table(call.func.value, known_tables) or any( + keyword.arg == SCHEMA_METHOD_KEYWORD for keyword in call.keywords + ) + + +def _docstring_nodes(tree: ast.Module) -> set[int]: + documented = (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef) + docstrings: set[int] = set() + for node in ast.walk(tree): + if not isinstance(node, documented) or not node.body: + continue + first_statement = node.body[0] + if isinstance(first_statement, ast.Expr) and isinstance( + first_statement.value, ast.Constant + ): + docstrings.add(id(first_statement.value)) + return docstrings + + +def _is_in_migrations(file: str) -> bool: + return MIGRATIONS_DIRECTORY in PurePosixPath(file).parts[:-1] + + +def _import_findings( + node: ast.Import | ast.ImportFrom, *, in_migrations: bool +) -> Iterator[str]: + modules = _imported_modules(node) + if any( + _matches_module(module, driver) + for module in modules + for driver in DATABASE_DRIVER_MODULES + ): + yield DRIVER_IMPORT + if not in_migrations and any( + _matches_module(module, SCHEMA_MIGRATION_MODULE) for module in modules + ): + yield ALEMBIC_IMPORT + if ( + not in_migrations + and isinstance(node, ast.ImportFrom) + and node.module is not None + and _matches_module(node.module, "sqlalchemy") + and any(alias.name in SQLALCHEMY_DDL_CONSTRUCTS for alias in node.names) + ): + yield DDL_CONSTRUCT + + +def _schema_findings( + node: ast.AST, docstrings: set[int], known_tables: frozenset[str] +) -> Iterator[str]: + if isinstance(node, ast.Call) and _changes_schema(node, known_tables): + yield SCHEMA_CHANGE_CALL + elif isinstance(node, ast.Attribute) and node.attr in SQLALCHEMY_DDL_CONSTRUCTS: + yield DDL_CONSTRUCT + elif ( + isinstance(node, ast.Constant) + and isinstance(node.value, str) + and id(node) not in docstrings + and DDL_PATTERN.search(node.value) + ): + yield DDL_TEXT + + +def _module_findings( + file: str, tree: ast.Module, known_tables: frozenset[str] +) -> Iterator[Finding]: + in_migrations = _is_in_migrations(file) + docstrings = _docstring_nodes(tree) + for node in ast.walk(tree): + if isinstance(node, (ast.Import, ast.ImportFrom)): + checks = list(_import_findings(node, in_migrations=in_migrations)) + elif in_migrations: + checks = [] + else: + checks = list(_schema_findings(node, docstrings, known_tables)) + for check in checks: + yield Finding(file, getattr(node, "lineno", 0), check, ast.unparse(node)) + + +def database_access_findings(sources: Mapping[str, str]) -> list[Finding]: + """Return every driver import, stray alembic import and schema change. + + ``sources`` maps a repository-relative file name to its source. Tables + bound in one file are recognised in all of them, since modules import their + tables from a shared module. Files under a ``migrations`` directory may + import alembic and change the schema; every file must reach the database + through SQLAlchemy. + """ + trees = {file: ast.parse(source, filename=file) for file, source in sources.items()} + known_tables = frozenset().union(*(table_names(tree) for tree in trees.values())) + findings = [ + finding + for file, tree in trees.items() + for finding in _module_findings(file, tree, known_tables) + ] + return sorted(findings, key=lambda finding: (finding.file, finding.line)) diff --git a/projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py b/projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py new file mode 100644 index 000000000..81d7ad590 --- /dev/null +++ b/projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py @@ -0,0 +1,98 @@ +"""Guard sim.boundaries.sqlalchemy-and-alembic-only over every source directory. + +Code connects to databases only through SQLAlchemy, and runtime code never +changes the schema; docs/engineering/skills/database-access.md explains the +rule. The checks live in database_access_support.py. +""" + +from __future__ import annotations + +from pathlib import Path + +from database_access_support import ( + DRIVER_IMPORT, + SCHEMA_CHANGE_CALL, + database_access_findings, +) + +REPOSITORY_ROOT = Path(__file__).resolve().parents[3] +FIXTURES_ROOT = ( + REPOSITORY_ROOT + / "rules" + / "fixtures" + / "sim.boundaries.sqlalchemy-and-alembic-only" +) +# Every directory that holds non-test source. The executor's src/modal is +# under projects/*/src. +SOURCE_ROOT_PATTERNS = ("projects/*/src", "libs/*/src", ".github/scripts") +# Directories the rule names, each of which must contribute scanned files. +REQUIRED_SOURCE_DIRECTORIES = ( + "projects/policyengine-simulation-entry/src", + "projects/policyengine-simulation-executor/src/modal", + "projects/policyengine-simulation-executor/src/policyengine_simulation_executor", + "projects/policyengine-simulation-gateway/src", + "libs/policyengine-fastapi/src", + "libs/policyengine-simulation-contract/src", + "libs/policyengine-simulation-observability/src", + "libs/policyengine-stage12-persistence/src", + ".github/scripts", +) + + +def _source_files() -> list[Path]: + return sorted( + source_file + for pattern in SOURCE_ROOT_PATTERNS + for source_root in REPOSITORY_ROOT.glob(pattern) + for source_file in source_root.rglob("*.py") + ) + + +def _relative_name(source_file: Path) -> str: + return source_file.relative_to(REPOSITORY_ROOT).as_posix() + + +def _fixture_findings(fixture_name: str) -> list[tuple[int, str, str]]: + fixture = FIXTURES_ROOT / fixture_name + findings = database_access_findings( + {fixture_name: fixture.read_text(encoding="utf-8")} + ) + return [(finding.line, finding.check, finding.source) for finding in findings] + + +def test_source_code_uses_sqlalchemy_and_never_changes_the_schema() -> None: + sources = { + _relative_name(source_file): source_file.read_text(encoding="utf-8") + for source_file in _source_files() + } + + findings = database_access_findings(sources) + + assert [str(finding) for finding in findings] == [] + + +def test_every_source_directory_is_scanned() -> None: + scanned = [_relative_name(source_file) for source_file in _source_files()] + globbed_roots = [ + _relative_name(source_root) + for pattern in SOURCE_ROOT_PATTERNS + for source_root in REPOSITORY_ROOT.glob(pattern) + ] + + unscanned = [ + directory + for directory in sorted({*REQUIRED_SOURCE_DIRECTORIES, *globbed_roots}) + if not any(name.startswith(f"{directory}/") for name in scanned) + ] + + assert unscanned == [] + + +def test_rule_proof_sqlalchemy_and_alembic_only() -> None: + """Proof for rule sim.boundaries.sqlalchemy-and-alembic-only.""" + assert _fixture_findings("bad_database_store.py") == [ + (16, DRIVER_IMPORT, "import psycopg"), + (17, DRIVER_IMPORT, "from psycopg.rows import dict_row"), + (57, SCHEMA_CHANGE_CALL, "metadata.create_all(engine)"), + ] + assert _fixture_findings("good_database_store.py") == [] diff --git a/projects/policyengine-simulation-entry/tests/test_database_access_support.py b/projects/policyengine-simulation-entry/tests/test_database_access_support.py new file mode 100644 index 000000000..39443eaca --- /dev/null +++ b/projects/policyengine-simulation-entry/tests/test_database_access_support.py @@ -0,0 +1,180 @@ +"""Check how database_access_support recognises database access and schema changes.""" + +from __future__ import annotations + +import textwrap + +import pytest +from database_access_support import ( + ALEMBIC_IMPORT, + DDL_CONSTRUCT, + DDL_TEXT, + DRIVER_IMPORT, + SCHEMA_CHANGE_CALL, + database_access_findings, +) + +RUNTIME_MODULE = "libs/example/src/example/store.py" +TABLES_MODULE = "libs/example/src/example/tables.py" +MIGRATION_MODULE = "libs/example/src/example/migrations/versions/0001_initial.py" +TABLES_SOURCE = """ +from sqlalchemy import Column, MetaData, String, Table + +metadata = MetaData() +comparison_reports = Table("reports", metadata, Column("id", String)) +""" + + +def _checks(source: str, file: str = RUNTIME_MODULE) -> list[tuple[str, str]]: + sources = {TABLES_MODULE: TABLES_SOURCE, file: textwrap.dedent(source)} + return [ + (finding.check, finding.source) + for finding in database_access_findings(sources) + if finding.file == file + ] + + +@pytest.mark.parametrize( + ("import_statement", "expected_source"), + [ + ("import psycopg", "import psycopg"), + ("import psycopg2.extras", "import psycopg2.extras"), + ( + "from psycopg.types.json import Jsonb", + "from psycopg.types.json import Jsonb", + ), + ("import mysql.connector", "import mysql.connector"), + ("from mysql import connector", "from mysql import connector"), + ("import sqlite3 as local_database", "import sqlite3 as local_database"), + ("from asyncpg import connect", "from asyncpg import connect"), + ("import MySQLdb", "import MySQLdb"), + ], +) +def test_driver_imports_are_findings(import_statement, expected_source) -> None: + assert _checks(import_statement) == [(DRIVER_IMPORT, expected_source)] + + +@pytest.mark.parametrize( + "import_statement", + [ + "from sqlalchemy.dialects import sqlite", + "from sqlalchemy.dialects.postgresql import insert", + "import sqlalchemy.dialects.sqlite", + "from . import sqlite3", + "import mysqlclient_settings", + ], +) +def test_sqlalchemy_and_unrelated_imports_are_not_findings(import_statement) -> None: + assert _checks(import_statement) == [] + + +def test_alembic_imports_only_inside_a_migrations_directory() -> None: + migration = """ + from alembic import op + + def upgrade(): + op.execute("CREATE SCHEMA simulation") + """ + + assert _checks("from alembic import command") == [ + (ALEMBIC_IMPORT, "from alembic import command") + ] + assert _checks("import alembic.config") == [ + (ALEMBIC_IMPORT, "import alembic.config") + ] + assert _checks(migration, MIGRATION_MODULE) == [] + + +def test_migrations_still_reach_the_database_through_sqlalchemy() -> None: + assert _checks("import psycopg", MIGRATION_MODULE) == [ + (DRIVER_IMPORT, "import psycopg") + ] + + +@pytest.mark.parametrize( + ("statement", "expected_source"), + [ + ("metadata.create_all(engine)", "metadata.create_all(engine)"), + ( + "SQLModel.metadata.drop_all(bind=engine)", + "SQLModel.metadata.drop_all(bind=engine)", + ), + ("comparison_reports.create(engine)", "comparison_reports.create(engine)"), + ( + "tables.comparison_reports.drop(connection)", + "tables.comparison_reports.drop(connection)", + ), + ("Report.__table__.create(engine)", "Report.__table__.create(engine)"), + ( + "metadata.tables['reports'].drop(engine)", + "metadata.tables['reports'].drop(engine)", + ), + ( + "Table('reports', metadata).create(engine)", + "Table('reports', metadata).create(engine)", + ), + ( + "report_index.create(engine, checkfirst=True)", + "report_index.create(engine, checkfirst=True)", + ), + ], +) +def test_schema_change_calls_are_findings(statement, expected_source) -> None: + assert _checks(statement) == [(SCHEMA_CHANGE_CALL, expected_source)] + + +def test_a_parameter_annotated_as_a_table_is_a_table() -> None: + source = """ + from sqlalchemy import Table + + def reset(table: Table, connection): + table.drop(connection) + """ + + assert _checks(source) == [(SCHEMA_CHANGE_CALL, "table.drop(connection)")] + + +@pytest.mark.parametrize( + "statement", + [ + "frame.drop(columns=['household_weight'])", + "frame.drop('household_weight', axis=1)", + "client.create(name='report')", + "modal.Dict.from_name('reports', create_if_missing=True)", + "create_engine('postgresql+psycopg://user@host/database')", + ], +) +def test_calls_on_other_objects_are_not_findings(statement) -> None: + assert _checks(statement) == [] + + +def test_ddl_text_in_literals_but_not_docstrings() -> None: + source = ''' + """Never run CREATE TABLE here; policyengine-api owns the schema.""" + + def create_reports(connection, name): + """Do not DROP TABLE either.""" + connection.exec_driver_sql("create table reports (id text)") + connection.exec_driver_sql(f"ALTER TYPE {name} ADD VALUE 'done'") + ''' + + assert _checks(source) == [ + (DDL_TEXT, "'create table reports (id text)'"), + (DDL_TEXT, "'ALTER TYPE '"), + ] + + +def test_sqlalchemy_ddl_constructs_are_findings() -> None: + source = """ + import sqlalchemy as sa + from sqlalchemy.schema import CreateTable + + def build(connection, table): + connection.execute(CreateTable(table)) + connection.execute(sa.schema.DropTable(table)) + """ + + assert _checks(source) == [ + (DDL_CONSTRUCT, "from sqlalchemy.schema import CreateTable"), + (DDL_CONSTRUCT, "sa.schema.DropTable"), + ] diff --git a/projects/policyengine-simulation-entry/tests/test_stage12_schema_authority.py b/projects/policyengine-simulation-entry/tests/test_stage12_schema_authority.py index b3e84d470..daf0a01c1 100644 --- a/projects/policyengine-simulation-entry/tests/test_stage12_schema_authority.py +++ b/projects/policyengine-simulation-entry/tests/test_stage12_schema_authority.py @@ -1,21 +1,22 @@ -"""Keep Stage 12 database schema ownership outside simulation runtimes.""" +"""Keep Stage 12 database schema ownership outside simulation runtimes. + +DDL_PATTERN is shared with sim.boundaries.sqlalchemy-and-alembic-only, which +matches it against the string literals of every source directory; see +database_access_support.py. +""" from __future__ import annotations -import re from pathlib import Path +from database_access_support import DDL_PATTERN + REPOSITORY_ROOT = Path(__file__).resolve().parents[3] RUNTIME_SOURCE_ROOTS = ( REPOSITORY_ROOT / "libs/policyengine-stage12-persistence/src", REPOSITORY_ROOT / "projects/policyengine-simulation-entry/src", REPOSITORY_ROOT / "projects/policyengine-simulation-executor/src", ) -DDL_PATTERN = re.compile( - r"\b(?:CREATE|ALTER|DROP|TRUNCATE)\s+" - r"(?:TABLE|TYPE|SCHEMA|DATABASE)\b|\bcreate_all\s*\(", - flags=re.IGNORECASE, -) def test_simulation_runtimes_contain_no_schema_ddl() -> None: diff --git a/rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/bad_database_store.py b/rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/bad_database_store.py new file mode 100644 index 000000000..c7f3a92cf --- /dev/null +++ b/rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/bad_database_store.py @@ -0,0 +1,57 @@ +"""Proof fixture: a store that bypasses SQLAlchemy and creates its own tables. + +The rule must reject it. The driver connection is a real past mistake: commit +08ebe1ca added a Stage 12 persistence adapter to policyengine-simulation-contract +that imported psycopg and opened its connections with psycopg.connect, until +commit 7e7fa003 replaced it with a SQLAlchemy engine. The store below is that +adapter's connection code, shortened. The metadata.create_all call is +illustrative, not taken from history: no commit in this repository has called +it. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +import psycopg +from psycopg.rows import dict_row +from sqlalchemy import MetaData, create_engine + +if TYPE_CHECKING: + from collections.abc import Callable + from uuid import UUID + +REPORT_SELECT_SQL = ( + "SELECT * FROM public.stage12_evaluation_reports WHERE evaluation_id = %(id)s" +) + +metadata = MetaData() + + +class PostgresEvaluationStore: + """Read temporary records over a connection the driver opens directly.""" + + def __init__( + self, + database_url: str, + *, + connect: Callable[..., Any] = psycopg.connect, + ) -> None: + """Keep the database URL and the driver function that connects to it.""" + self._database_url = database_url + self._connect = connect + + def _connection(self) -> psycopg.Connection[dict[str, Any]]: + return self._connect(self._database_url, row_factory=dict_row) + + def get_report(self, evaluation_id: UUID) -> dict[str, Any] | None: + """Read one report row through a driver cursor.""" + with self._connection() as connection, connection.cursor() as cursor: + cursor.execute(REPORT_SELECT_SQL, {"id": evaluation_id}) + return cursor.fetchone() + + +def ensure_tables(database_url: str) -> None: + """Create every mapped table from runtime code, the schema change to catch.""" + engine = create_engine(database_url) + metadata.create_all(engine) diff --git a/rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/good_database_store.py b/rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/good_database_store.py new file mode 100644 index 000000000..0864e853a --- /dev/null +++ b/rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/good_database_store.py @@ -0,0 +1,40 @@ +"""Proof fixture: a store that connects through SQLAlchemy and only reads rows. + +The rule must accept it. It follows today's Stage 12 store: a SQLAlchemy engine +selects the psycopg 3 driver through the URL's driver name, and statements are +typed SQLAlchemy Core. It never runs CREATE TABLE or metadata.create_all; the +schema changes only through Alembic migrations. Dropping a pandas column is not +a schema change. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pandas as pd +from sqlalchemy import Column, MetaData, String, Table, create_engine, select +from sqlalchemy.engine import make_url + +if TYPE_CHECKING: + from sqlalchemy import Engine + +metadata = MetaData() +comparison_reports = Table( + "stage12_evaluation_reports", + metadata, + Column("evaluation_id", String, primary_key=True), + Column("status", String), +) + + +def create_store_engine(database_url: str) -> Engine: + """Build an engine that connects with psycopg 3 through SQLAlchemy.""" + return create_engine(make_url(database_url).set(drivername="postgresql+psycopg")) + + +def report_statuses(engine: Engine) -> pd.DataFrame: + """Read report statuses with a typed select, without the identifier column.""" + with engine.connect() as connection: + rows = connection.execute(select(comparison_reports)).mappings().all() + frame = pd.DataFrame(rows) + return frame.drop(columns=["evaluation_id"]) diff --git a/rules/registry.toml b/rules/registry.toml index 08e3c22db..d1d305ea8 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -101,6 +101,32 @@ autofix = false [rule.enforcer] kind = "none" +[[rule]] +id = "sim.boundaries.sqlalchemy-and-alembic-only" +category = "boundaries" +statement = "Code connects to databases only through SQLAlchemy engines and connections, never by importing a database driver directly, and runtime code never changes the schema, which changes only through Alembic migrations." +rationale = "docs/engineering/skills/database-access.md#rules" +scope = ["projects/*/src/**/*.py", "libs/*/src/**/*.py", ".github/scripts/**/*.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "pytest" +project = "projects/policyengine-simulation-entry" +nodes = [ + "tests/test_database_access_boundaries.py::test_source_code_uses_sqlalchemy_and_never_changes_the_schema", + "tests/test_database_access_boundaries.py::test_every_source_directory_is_scanned", +] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "uv run pytest tests/" +[rule.proof] +bad_fixture = "rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/bad_database_store.py" +good_fixture = "rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/good_database_store.py" +project = "projects/policyengine-simulation-entry" +test = "tests/test_database_access_boundaries.py::test_rule_proof_sqlalchemy_and_alembic_only" + # --- Stage 12 persistence -------------------------------------------------- [[rule]]