diff --git a/changelog.d/v2-area-impacts-without-xy.removed.md b/changelog.d/v2-area-impacts-without-xy.removed.md new file mode 100644 index 000000000..a2fd35de3 --- /dev/null +++ b/changelog.d/v2-area-impacts-without-xy.removed.md @@ -0,0 +1 @@ +API v2's `constituency_impacts` and `local_authority_impacts` tables no longer have the `x` and `y` hex-map position columns; v2 Alembic revision `e86f19a6c4da` drops them. Nothing read or wrote these tables, and the app draws its maps from its own layout files. diff --git a/docs/engineering/skills/alembic-migrations.md b/docs/engineering/skills/alembic-migrations.md index 0fa123919..136467970 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -422,3 +422,31 @@ the new release, so instances of the previous release that start afterwards, and a rollback to it, refuse to start until the revision is downgraded. The module docstring of `supported_countries.py` lists the other checks a new country must widen. + +### API v2 UK area impact map coordinate removal + +Revision `e86f19a6c4da` was autogenerated, with `--rev-id e86f19a6c4da`, after +the `x` and `y` fields were removed from the `ConstituencyImpact` and +`LocalAuthorityImpact` models, and generated again on top of each revision +that reached `master` first: the countries revision `fb1a3e40d3d6`, then the +catalog format 2 revision `f5c1d9b4685b`. It has no post-generation +corrections. Its +upgrade drops `x` and `y` from `constituency_impacts` and +`local_authority_impacts`; its downgrade adds them back as required integers +without a server default, as the baseline created them. + +The columns held hex-map positions, which nothing needs: the app draws its +maps from its own layout files. The Stage 12A revision kept every column that +the previous Public API revision maps, so that revision keeps working during +the deploy window and after an application rollback +(`docs/migration/stage-12a-user-login.md`). This revision drops four columns +that the previous revision maps, which is safe because that revision can't +read or write either table: no managed database role holds any privilege on +them (`policyengine_api/data/v2/privileges.py`), and no Public API code reads +or writes them. The lifecycle test +`test_area_impact_coordinate_removal_changes_nothing_else` checks that these +four columns are the only difference from the catalog format 2 revision's +schema. + +The dropped values are not recoverable. Adding a required column fails on a +table that has rows, so the downgrade works only while both tables are empty. diff --git a/docs/migration/stage-12a-user-login.md b/docs/migration/stage-12a-user-login.md index 3a34626ab..ee1b32f30 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -402,9 +402,9 @@ row with a NULL `primary_country` does not fail there. The lifecycle test `test_stage_12a_schema_serves_the_previous_revision` exercises this rather than only comparing columns. It reflects the schema at -the parent revision with SQLAlchemy and upgrades to head. Then, through the -reflected parent tables, it does the following for every table the parent -revision maps: +the parent revision with SQLAlchemy and upgrades to `fbb7488ebe8a`. Then, +through the reflected parent tables, it does the following for every table the +parent revision maps: - inserts a row that supplies only the parent's required columns (plus the nullable values that the parent's own check constraints demand); @@ -412,13 +412,13 @@ revision maps: - rewrites every non-key column; - deletes the row. -Rows of the two dormant tables are inserted through head's tables, since only -new code writes them, and are then read, rewritten, and deleted through the -parent's. The test also deletes, through the parent's `users` table, a user who -owns rows in all four association tables and a `user_identities` row, and it -checks that every one of them cascades away. It fails if head drops a parent -column, or adds a required column without a server default to a table the -parent revision writes. +Rows of the two dormant tables are inserted through the Stage 12A tables, +since only new code writes them, and are then read, rewritten, and deleted +through the parent's. The test also deletes, through the parent's `users` +table, a user who owns rows in all four association tables and a +`user_identities` row, and it checks that every one of them cascades away. It +fails if Stage 12A drops a parent column, or adds a required column without a +server default to a table the parent revision writes. ## Settings @@ -571,8 +571,8 @@ The disposable-Postgres suite proves the following: - `MATCH SIMPLE` composite keys accept NULL v2 references; - account deletion cascades after removing legacy mappings; - NaN and Infinity never reach JSONB; -- the parent revision's table usage works against head (see "Schema - compatibility"); +- the parent revision's table usage works against the Stage 12A schema (see + "Schema compatibility"); - the qualification transaction is read-only. ## Deferred diff --git a/migrations/v2/versions/e86f19a6c4da_drop_uk_area_impact_map_coordinates.py b/migrations/v2/versions/e86f19a6c4da_drop_uk_area_impact_map_coordinates.py new file mode 100644 index 000000000..1a323b9ef --- /dev/null +++ b/migrations/v2/versions/e86f19a6c4da_drop_uk_area_impact_map_coordinates.py @@ -0,0 +1,49 @@ +"""drop UK area impact map coordinates + +Revision ID: e86f19a6c4da +Revises: f5c1d9b4685b +Create Date: 2026-11-03 23:42:47.925451 +Generation: uv run alembic -c alembic-v2.ini revision --autogenerate +""" + +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +import sqlmodel + + +revision: str = "e86f19a6c4da" +down_revision: Union[str, None] = "f5c1d9b4685b" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("constituency_impacts", "y") + op.drop_column("constituency_impacts", "x") + op.drop_column("local_authority_impacts", "y") + op.drop_column("local_authority_impacts", "x") + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.add_column( + "local_authority_impacts", + sa.Column("x", sa.INTEGER(), autoincrement=False, nullable=False), + ) + op.add_column( + "local_authority_impacts", + sa.Column("y", sa.INTEGER(), autoincrement=False, nullable=False), + ) + op.add_column( + "constituency_impacts", + sa.Column("x", sa.INTEGER(), autoincrement=False, nullable=False), + ) + op.add_column( + "constituency_impacts", + sa.Column("y", sa.INTEGER(), autoincrement=False, nullable=False), + ) + # ### end Alembic commands ### diff --git a/policyengine_api/data/v2/migration_target.py b/policyengine_api/data/v2/migration_target.py index d463fad16..3e3c09a8a 100644 --- a/policyengine_api/data/v2/migration_target.py +++ b/policyengine_api/data/v2/migration_target.py @@ -24,7 +24,7 @@ from collections.abc import Mapping V2_ALEMBIC_DISPOSABLE_TEST = "V2_ALEMBIC_DISPOSABLE_TEST" -V2_ALEMBIC_HEAD_REVISION = "f5c1d9b4685b" +V2_ALEMBIC_HEAD_REVISION = "e86f19a6c4da" DISPOSABLE_DATABASE_NAME = "policyengine_v2_alembic_test" DISPOSABLE_HOSTS = frozenset({"localhost", "127.0.0.1", "::1", "postgres"}) MIGRATION_ROLE = "policyengine_v2_migrator" diff --git a/policyengine_api/data/v2/models/reports.py b/policyengine_api/data/v2/models/reports.py index d10c36947..5c73947c1 100644 --- a/policyengine_api/data/v2/models/reports.py +++ b/policyengine_api/data/v2/models/reports.py @@ -606,7 +606,7 @@ class CongressionalDistrictImpact(GeographicImpactBase, table=True): class ConstituencyImpact(GeographicImpactBase, table=True): - """Household income change in one UK constituency, with its map coordinates. + """Household income change in one UK constituency. One row per run and constituency code. """ @@ -622,14 +622,12 @@ class ConstituencyImpact(GeographicImpactBase, table=True): constituency_code: str = Field(max_length=64) constituency_name: str = Field(max_length=255) - x: int - y: int report_run: ReportRun = Relationship(back_populates="constituency_impacts") class LocalAuthorityImpact(GeographicImpactBase, table=True): - """Household income change in one UK local authority, with its map coordinates. + """Household income change in one UK local authority. One row per run and local authority code. """ @@ -645,7 +643,5 @@ class LocalAuthorityImpact(GeographicImpactBase, table=True): local_authority_code: str = Field(max_length=64) local_authority_name: str = Field(max_length=255) - x: int - y: int report_run: ReportRun = Relationship(back_populates="local_authority_impacts") diff --git a/tests/integration/test_alembic_v2_lifecycle.py b/tests/integration/test_alembic_v2_lifecycle.py index a5bd7d20c..efe292331 100644 --- a/tests/integration/test_alembic_v2_lifecycle.py +++ b/tests/integration/test_alembic_v2_lifecycle.py @@ -31,8 +31,14 @@ STAGE_12_PREVIOUS_REVISION = "724b1b11a33e" STAGE_12_COMPARISON_PREVIOUS_REVISION = "439303be14fe" STAGE_12A_PREVIOUS_REVISION = "91af4fc8d8c1" -COUNTRIES_PREVIOUS_REVISION = "fbb7488ebe8a" +STAGE_12A_REVISION = "fbb7488ebe8a" +COUNTRIES_PREVIOUS_REVISION = STAGE_12A_REVISION +COUNTRIES_REVISION = "fb1a3e40d3d6" +# The catalog format 2 revision, the parent of the area impact coordinate removal. +CATALOG_FORMAT_2_REVISION = "f5c1d9b4685b" V2_TABLE_NAMES = frozenset(table.name for table in V2_METADATA.tables.values()) +AREA_IMPACT_TABLES = ("constituency_impacts", "local_authority_impacts") +MAP_COORDINATE_COLUMNS = ("x", "y") def _disposable_url() -> str: @@ -100,6 +106,8 @@ def _assert_head(engine) -> None: "legacy_user_mappings", "legacy_user_policy_mappings", } <= set(inspect(engine).get_table_names(schema="public")) + for table_name in AREA_IMPACT_TABLES: + assert set(MAP_COORDINATE_COLUMNS).isdisjoint(_columns(engine, table_name)) def _columns(engine, table_name: str) -> dict[str, dict]: @@ -109,6 +117,17 @@ def _columns(engine, table_name: str) -> dict[str, dict]: } +def _assert_area_impact_coordinates_restored(engine: Engine) -> None: + """Check the catalog format 2 revision's map coordinates are required integers.""" + _assert_revision(engine, CATALOG_FORMAT_2_REVISION) + for table_name in AREA_IMPACT_TABLES: + columns = _columns(engine, table_name) + for column_name in MAP_COORDINATE_COLUMNS: + assert isinstance(columns[column_name]["type"], sa.Integer) + assert not columns[column_name]["nullable"] + assert columns[column_name]["default"] is None + + def _assert_pre_stage_12a_association_shape(engine) -> None: assert "user_identities" not in inspect(engine).get_table_names(schema="public") for table_name, reference in ( @@ -436,6 +455,10 @@ def test_empty_upgrade_check_base_downgrade_and_reupgrade() -> None: _upgrade_and_assert_head(config, engine) _assert_countries_shape(engine) + command.downgrade(config, CATALOG_FORMAT_2_REVISION) + _assert_area_impact_coordinates_restored(engine) + _upgrade_and_assert_head(config, engine) + command.downgrade(config, COUNTRIES_PREVIOUS_REVISION) _assert_revision(engine, COUNTRIES_PREVIOUS_REVISION) _assert_pre_countries_shape(engine) @@ -636,10 +659,10 @@ def _rewrite_mapped_columns(connection, table: sa.Table, row: dict) -> None: @dataclass(frozen=True) class PreviousRevisionSchemas: - """A previous revision's reflected schema and head's, after the upgrade. + """A previous revision's reflected schema and the one it was upgraded to. ``read_only_tables`` are the previous revision's tables that only new code - writes, so their test rows are inserted through head's table. + writes, so their test rows are inserted through the upgraded table. """ parent: sa.MetaData @@ -647,7 +670,7 @@ class PreviousRevisionSchemas: read_only_tables: frozenset[str] def insert_table(self, table_name: str) -> sa.Table | None: - """Head's table for a read-only table, otherwise ``None``.""" + """Return the upgraded table for a read-only table, otherwise ``None``.""" if table_name in self.read_only_tables: return self.head.tables[table_name] return None @@ -698,19 +721,21 @@ def _assert_user_deletion_cascades( assert remaining == 0, table_name -def _assert_head_serves_previous_revision( +def _assert_revision_serves_previous_revision( previous_revision: str, + revision: str, read_only_tables: frozenset[str], ) -> None: - """Run a previous Public API revision's table usage against head. + """Run a previous Public API revision's table usage against ``revision``. - The parent revision's schema is reflected and then used, after upgrading, + The parent revision's schema is reflected and then used, after upgrading + to ``revision``, the revision it ran beside during that deploy, to insert a row into every table it maps (supplying only what it requires), read the row back with every mapped column, rewrite those columns, delete it, and delete a user who owns association rows so the - cascades run. Head may not give any table the parent revision writes a - new required column without a server default; ``read_only_tables`` are the - parent's tables that only new code writes. + cascades run. ``revision`` may not give any table the parent revision + writes a new required column without a server default; ``read_only_tables`` + are the parent's tables that only new code writes. """ database_url = _disposable_url() config = _config() @@ -720,7 +745,7 @@ def _assert_head_serves_previous_revision( command.downgrade(config, previous_revision) parent = sa.MetaData() parent.reflect(engine) - command.upgrade(config, "head") + command.upgrade(config, revision) head = sa.MetaData() head.reflect(engine) parent_tables = [ @@ -781,20 +806,67 @@ def _assert_head_serves_previous_revision( def test_stage_12a_schema_serves_the_previous_revision() -> None: - _assert_head_serves_previous_revision( + _assert_revision_serves_previous_revision( STAGE_12A_PREVIOUS_REVISION, + STAGE_12A_REVISION, STAGE_12A_PARENT_READ_ONLY_TABLES, ) def test_countries_schema_serves_the_previous_revision() -> None: """The release before the countries table keeps working during a deploy.""" - _assert_head_serves_previous_revision( + _assert_revision_serves_previous_revision( COUNTRIES_PREVIOUS_REVISION, + COUNTRIES_REVISION, frozenset(), ) +def test_area_impact_coordinate_removal_changes_nothing_else() -> None: + """Check that head drops the map coordinates and changes nothing else. + + Head is compared with its parent, the catalog format 2 revision. The + previous Public API revision still maps those four columns, but it can't + read or write either table: no managed database role holds any privilege + on them (``policyengine_api/data/v2/privileges.py``). Every table it can + use keeps every column. + """ + database_url = _disposable_url() + config = _config() + engine = create_engine(database_url) + + try: + command.downgrade(config, CATALOG_FORMAT_2_REVISION) + parent = sa.MetaData() + parent.reflect(engine) + command.upgrade(config, "head") + head = sa.MetaData() + head.reflect(engine) + + assert set(head.tables) == set(parent.tables) + dropped_columns = sorted( + f"{table.name}.{column.name}" + for table in parent.tables.values() + for column in table.columns + if column.name not in head.tables[table.name].columns + ) + added_columns = sorted( + f"{table.name}.{column.name}" + for table in head.tables.values() + for column in table.columns + if column.name not in parent.tables[table.name].columns + ) + assert dropped_columns == sorted( + f"{table_name}.{column_name}" + for table_name in AREA_IMPACT_TABLES + for column_name in MAP_COORDINATE_COLUMNS + ) + assert added_columns == [] + finally: + command.upgrade(config, "head") + engine.dispose() + + def test_upgrade_to_head_validates_the_resulting_schema_against_metadata() -> None: database_url = _disposable_url() config = _config() diff --git a/tests/unit/v2/test_alembic_v2.py b/tests/unit/v2/test_alembic_v2.py index 9eca8f5b3..8b2d2c710 100644 --- a/tests/unit/v2/test_alembic_v2.py +++ b/tests/unit/v2/test_alembic_v2.py @@ -238,6 +238,7 @@ def test_v2_revision_chain_has_generated_policy_and_user_identity_changes() -> N assert script.get_heads() == [V2_ALEMBIC_HEAD_REVISION] assert [revision.revision for revision in script.walk_revisions()] == [ V2_ALEMBIC_HEAD_REVISION, + "f5c1d9b4685b", CATALOG_FORMAT_2_PARENT_REVISION, "fbb7488ebe8a", "91af4fc8d8c1", @@ -657,6 +658,47 @@ def test_countries_revision_inserts_us_and_uk_and_replaces_country_checks() -> N assert "op.add_column(" not in upgrade +AREA_IMPACT_COORDINATE_COLUMNS = frozenset( + (table_name, column_name) + for table_name in ("constituency_impacts", "local_authority_impacts") + for column_name in ("x", "y") +) + + +def test_area_impact_coordinate_removal_is_generated_and_reversible() -> None: + revision = ( + REPO + / "migrations/v2/versions/e86f19a6c4da_drop_uk_area_impact_map_coordinates.py" + ).read_text(encoding="utf-8") + + assert V2_ALEMBIC_HEAD_REVISION == "e86f19a6c4da" + assert ( + "Generation: uv run alembic -c alembic-v2.ini revision --autogenerate" + in revision + ) + assert 'down_revision: Union[str, None] = "f5c1d9b4685b"' in revision + assert "Post-generation" not in revision + upgrade = revision[ + revision.index("def upgrade()") : revision.index("def downgrade()") + ] + downgrade = revision[revision.index("def downgrade()") :] + # The upgrade drops the four map coordinate columns and does nothing else. + assert len(re.findall(r"\bop\.\w+\(", upgrade)) == 4 + dropped_columns = re.findall( + r'op\.drop_column\(\s*"(\w+)",\s*"(\w+)"\s*\)', upgrade + ) + assert frozenset(dropped_columns) == AREA_IMPACT_COORDINATE_COLUMNS + # The downgrade adds each one back as the baseline created it: a required + # integer without a server default. + assert len(re.findall(r"\bop\.\w+\(", downgrade)) == 4 + restored_columns = re.findall( + r'op\.add_column\(\s*"(\w+)",\s*sa\.Column\(\s*"(\w+)",\s*' + r"sa\.INTEGER\(\),\s*autoincrement=False,\s*nullable=False,?\s*\),?\s*\)", + downgrade, + ) + assert frozenset(restored_columns) == AREA_IMPACT_COORDINATE_COLUMNS + + def test_alembic_rejects_unknown_missing_and_divergent_history(tmp_path: Path) -> None: original = REPO / "migrations/v2" missing = tmp_path / "missing" diff --git a/tests/unit/v2/test_models.py b/tests/unit/v2/test_models.py index 72a986c00..806b9d182 100644 --- a/tests/unit/v2/test_models.py +++ b/tests/unit/v2/test_models.py @@ -455,6 +455,11 @@ def test_run_outputs_reference_report_runs_not_base_reports() -> None: assert foreign_key.ondelete == "CASCADE" +def test_uk_area_impacts_store_no_map_coordinates() -> None: + for table_name in ("constituency_impacts", "local_authority_impacts"): + assert {"x", "y"}.isdisjoint(V2_METADATA.tables[table_name].c.keys()) + + def test_report_rerun_constraints_allow_same_versions_but_deduplicate_requests() -> ( None ):