From 07bc6353f1fbd7c142cb0a29f2b597f67bfb301b Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:43:00 +0400 Subject: [PATCH] Add a guard that database code uses only SQLAlchemy and Alembic Register 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. The guard in the entry project's tests parses every Python file under projects/*/src, libs/*/src and .github/scripts with ast. It rejects database driver imports, alembic imports outside a migrations directory, create_all and drop_all calls, create() and drop() calls on objects it can identify as tables, SQLAlchemy DDL statements, and DDL text in string literals other than docstrings. The DDL text check reuses the Stage 12 DDL_PATTERN, now in database_access_support.py, so it also covers the gateway and every library. test_stage12_schema_authority.py imports the pattern from there. The proof's bad fixture is the psycopg connection code of the Stage 12 adapter added in 08ebe1ca, plus an illustrative metadata.create_all call. docs/engineering/skills/database-access.md explains the rule, which schema each migration chain owns, and that non-relational stores are out of scope. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 4 + changelog_entry.yaml | 1 + docs/engineering/skills/README.md | 2 + docs/engineering/skills/database-access.md | 80 +++++ docs/generated/rules.md | 10 +- .../tests/database_access_support.py | 273 ++++++++++++++++++ .../tests/test_database_access_boundaries.py | 98 +++++++ .../tests/test_database_access_support.py | 180 ++++++++++++ .../tests/test_stage12_schema_authority.py | 15 +- .../bad_database_store.py | 57 ++++ .../good_database_store.py | 40 +++ rules/registry.toml | 26 ++ 12 files changed, 777 insertions(+), 9 deletions(-) create mode 100644 docs/engineering/skills/database-access.md create mode 100644 projects/policyengine-simulation-entry/tests/database_access_support.py create mode 100644 projects/policyengine-simulation-entry/tests/test_database_access_boundaries.py create mode 100644 projects/policyengine-simulation-entry/tests/test_database_access_support.py create mode 100644 rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/bad_database_store.py create mode 100644 rules/fixtures/sim.boundaries.sqlalchemy-and-alembic-only/good_database_store.py 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]]