Skip to content

Commit 33d5c85

Browse files
anth-volkclaude
andcommitted
Test every definition check in the database and correct the roles table
The database repeats each check NewSimulation and NewReport make, for any other writer, but only the report year and options checks had a test, so a mistake in another check's expression would have gone unnoticed. New tests insert rows directly as the runtime role and expect each of the other checks to refuse them: the kinds, countries, region code, dataset name, both v1 IDs, and an economy report without a reform. The README's roles table still gave the runtime role UPDATE on every table, and did not say the migrator needs REFERENCES on the Public API tables before revision 0002 deploys; both are corrected. The unused REFERENCED_PUBLIC_API_TABLES constant is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 259c390 commit 33d5c85

4 files changed

Lines changed: 86 additions & 8 deletions

File tree

‎libs/policyengine-simulation-persistence/README.md‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,12 +57,18 @@ These rules are registered in `rules/registry.toml` and checked by
5757

5858
| Role | Used by | Privileges |
5959
| --- | --- | --- |
60-
| `simulation_migrator` | The deploy workflow's migration job, through `SIMULATION_MIGRATION_DATABASE_URL` | Creates the `simulation` schema if it is missing, and owns every object in it |
61-
| `simulation_runtime` | The Simulation Entrypoint, through `SIMULATION_DATABASE_URL` | `USAGE` on the schema, `SELECT` on the version table, and `SELECT`, `INSERT` and `UPDATE` on the tables the migrations create |
60+
| `simulation_migrator` | The deploy workflow's migration job, through `SIMULATION_MIGRATION_DATABASE_URL` | Creates the `simulation` schema if it is missing, and owns every object in it; `REFERENCES` on the Public API tables its foreign keys point to |
61+
| `simulation_runtime` | The Simulation Entrypoint, through `SIMULATION_DATABASE_URL` | `USAGE` on the schema, `SELECT` on the version table, and `SELECT`, `INSERT` and `UPDATE` on the tables the migrations create, except `UPDATE` on the definition tables (`simulations`, `reports`); never `DELETE`, and nothing on the Public API tables |
6262

6363
The initial revision grants the runtime role its privileges, including default
64-
privileges on tables the migrator creates later. Both roles must exist before
65-
the first migration runs; an operator creates them. The migrator needs either
64+
privileges on tables the migrator creates later; revision 0002 revokes
65+
`UPDATE` on the definition tables, which never change. Both roles must exist
66+
before the first migration runs; an operator creates them. Before revision
67+
0002 is deployed, the owner of the Public API's tables must also grant the
68+
migrator `REFERENCES` on `policies`, `households` and `dynamics` (the
69+
operator step grants it on `tax_benefit_model_versions`, `datasets` and
70+
`regions` too, for later revisions); without it the migration fails and
71+
applies nothing. The migrator needs either
6672
`CREATE` on the database or a `simulation` schema created in advance and owned
6773
by it: the environment creates the schema only when it is missing, because
6874
PostgreSQL checks `CREATE` on the database even for

‎libs/policyengine-simulation-persistence/src/policyengine_simulation_persistence/public_api.py‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,5 +41,3 @@
4141
public_api_metadata,
4242
Column("id", Uuid(as_uuid=True), primary_key=True),
4343
)
44-
45-
REFERENCED_PUBLIC_API_TABLES = tuple(sorted(public_api_metadata.tables))

‎libs/policyengine-simulation-persistence/tests/test_reports.py‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@
55
from uuid import uuid4
66

77
import pytest
8-
from policyengine_simulation_persistence import ReportKind, ReportRecord
8+
from policyengine_simulation_persistence import (
9+
ReportKind,
10+
ReportRecord,
11+
create_simulation_engine,
12+
)
13+
from policyengine_simulation_persistence.reports import reports
914
from public_api_support import superuser_engine
1015
from report_support import (
1116
economy_pair,
@@ -14,7 +19,7 @@
1419
report_store,
1520
)
1621
from simulation_support import economy_simulation, simulation_store
17-
from sqlalchemy import text
22+
from sqlalchemy import insert, text
1823
from sqlalchemy.exc import IntegrityError
1924

2025
pytestmark = pytest.mark.postgres
@@ -188,3 +193,37 @@ def test_a_migrated_report_needs_its_legacy_id(
188193

189194
def test_an_unknown_report_reads_as_none(migrated_database) -> None:
190195
assert report_store(migrated_database).get(uuid4()) is None
196+
197+
198+
@pytest.mark.parametrize(
199+
("column", "value", "constraint"),
200+
[
201+
("kind", "microsimulation", "ck_reports_kind"),
202+
("country", "GB", "ck_reports_country"),
203+
("legacy_report_id", -4, "ck_reports_legacy_report_id"),
204+
("reform_simulation_id", None, "ck_reports_economy_has_reform"),
205+
],
206+
)
207+
def test_the_database_checks_each_report_field(
208+
migrated_database, public_api_rows, column, value, constraint
209+
) -> None:
210+
"""The database checks what NewReport also checks, for any other writer."""
211+
baseline, reform = economy_pair(migrated_database, public_api_rows)
212+
values = {
213+
"id": uuid4(),
214+
"kind": "economy",
215+
"country": "us",
216+
"baseline_simulation_id": baseline.id,
217+
"reform_simulation_id": reform.id,
218+
"year": 2026,
219+
column: value,
220+
}
221+
engine = create_simulation_engine(migrated_database.runtime_url)
222+
try:
223+
with (
224+
pytest.raises(IntegrityError, match=constraint),
225+
engine.begin() as connection,
226+
):
227+
connection.execute(insert(reports).values(**values))
228+
finally:
229+
engine.dispose()

‎libs/policyengine-simulation-persistence/tests/test_simulations.py‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,16 @@
99
NewSimulation,
1010
SimulationKind,
1111
SimulationRecord,
12+
create_simulation_engine,
1213
)
14+
from policyengine_simulation_persistence.simulations import simulations
1315
from public_api_support import delete_public_api_row
1416
from simulation_support import (
1517
economy_simulation,
1618
household_simulation,
1719
simulation_store,
1820
)
21+
from sqlalchemy import insert
1922
from sqlalchemy.exc import IntegrityError
2023

2124
pytestmark = pytest.mark.postgres
@@ -210,3 +213,35 @@ def test_two_simulations_cannot_share_a_legacy_id(
210213

211214
def test_an_unknown_simulation_reads_as_none(migrated_database) -> None:
212215
assert simulation_store(migrated_database).get(uuid4()) is None
216+
217+
218+
@pytest.mark.parametrize(
219+
("column", "value", "constraint"),
220+
[
221+
("kind", "microsimulation", "ck_simulations_kind"),
222+
("country", "USA", "ck_simulations_country"),
223+
("region_code", "state ca", "ck_simulations_region_code"),
224+
("dataset_name", "populace us", "ck_simulations_dataset_name"),
225+
("legacy_simulation_id", 0, "ck_simulations_legacy_simulation_id"),
226+
],
227+
)
228+
def test_the_database_checks_each_simulation_field(
229+
migrated_database, public_api_rows, column, value, constraint
230+
) -> None:
231+
"""The database checks what NewSimulation also checks, for any other writer."""
232+
values = {
233+
"id": uuid4(),
234+
"kind": "economy",
235+
"country": "us",
236+
"region_code": "state/ca",
237+
column: value,
238+
}
239+
engine = create_simulation_engine(migrated_database.runtime_url)
240+
try:
241+
with (
242+
pytest.raises(IntegrityError, match=constraint),
243+
engine.begin() as connection,
244+
):
245+
connection.execute(insert(simulations).values(**values))
246+
finally:
247+
engine.dispose()

0 commit comments

Comments
 (0)