From b03f88e5567aac5b9d1704c9a3f119caa0ebc505 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:37:26 +0400 Subject: [PATCH] Drop the x and y columns from API v2's UK area impact tables `constituency_impacts` and `local_authority_impacts` stored each area's hex-map position in required `x` and `y` integer columns. Nothing reads or writes either table, and the app draws its maps from its own layout files, so the positions are unused. - `ConstituencyImpact` and `LocalAuthorityImpact` no longer define `x` and `y`. - v2 revision `e86f19a6c4da` was autogenerated from that metadata with `alembic -c alembic-v2.ini revision --autogenerate --rev-id e86f19a6c4da`, with no post-generation corrections. Its upgrade drops the four columns; its downgrade adds them back as required integers, which works only while both tables are empty. It is the new `V2_ALEMBIC_HEAD_REVISION`. - The previous Public API revision still maps these columns, but no managed database role holds any privilege on either table, so it can't read or write them during the deploy window or after a rollback. A new lifecycle test checks that the four columns are the only difference from the Stage 12A schema. The Stage 12A previous-revision test now upgrades to the Stage 12A revision, which it was written for, instead of head. - The Alembic guide documents the revision, and the Stage 12A guide names its own revision where it said head. Tests were written first and seen failing: the model test found `x` and `y` on both tables, the revision chain and revision-text tests found no `e86f19a6c4da`, and the lifecycle tests found the columns still present at head. Co-Authored-By: Claude Opus 5.5 --- .../v2-area-impacts-without-xy.removed.md | 1 + docs/engineering/skills/alembic-migrations.md | 24 ++++ docs/migration/stage-12a-user-login.md | 26 ++--- ...4da_drop_uk_area_impact_map_coordinates.py | 49 ++++++++ policyengine_api/data/v2/migration_target.py | 2 +- policyengine_api/data/v2/models/reports.py | 8 +- .../integration/test_alembic_v2_lifecycle.py | 106 ++++++++++++++---- tests/unit/v2/test_alembic_v2.py | 43 ++++++- tests/unit/v2/test_models.py | 5 + 9 files changed, 223 insertions(+), 41 deletions(-) create mode 100644 changelog.d/v2-area-impacts-without-xy.removed.md create mode 100644 migrations/v2/versions/e86f19a6c4da_drop_uk_area_impact_map_coordinates.py 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 9bcf0139a..86e54a89d 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -382,3 +382,27 @@ last dependent table is dropped. Its only post-generation correction drops the nine generated `v2_*` enum types at the end of the baseline downgrade. The v2 Postgres lifecycle test covers empty upgrade, downgrade to base, and re-upgrade so stale enum types cannot make the generated baseline non-reversible. + +### 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. 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 Stage 12A 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 c01657bc3..c75720548 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -314,7 +314,7 @@ from the final SQLModel metadata. It has no post-generation corrections. "Rollback"). The models drop the `user_associations` relationships from v2 simulations and -reports. `V2_ALEMBIC_HEAD_REVISION` is `fbb7488ebe8a`. +reports. Stage 12A set `V2_ALEMBIC_HEAD_REVISION` to `fbb7488ebe8a`. ### Reshape safety @@ -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..4151ffd6e --- /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: fbb7488ebe8a +Create Date: 2026-10-28 15:31:32.507854 +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] = "fbb7488ebe8a" +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", "x") + op.drop_column("constituency_impacts", "y") + op.drop_column("local_authority_impacts", "x") + op.drop_column("local_authority_impacts", "y") + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.add_column( + "local_authority_impacts", + sa.Column("y", sa.INTEGER(), autoincrement=False, nullable=False), + ) + op.add_column( + "local_authority_impacts", + sa.Column("x", sa.INTEGER(), autoincrement=False, nullable=False), + ) + op.add_column( + "constituency_impacts", + sa.Column("y", sa.INTEGER(), autoincrement=False, nullable=False), + ) + op.add_column( + "constituency_impacts", + sa.Column("x", 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 fd4c05fdf..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 = "fbb7488ebe8a" +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 814971236..b850fc2c5 100644 --- a/tests/integration/test_alembic_v2_lifecycle.py +++ b/tests/integration/test_alembic_v2_lifecycle.py @@ -30,7 +30,10 @@ STAGE_12_PREVIOUS_REVISION = "724b1b11a33e" STAGE_12_COMPARISON_PREVIOUS_REVISION = "439303be14fe" STAGE_12A_PREVIOUS_REVISION = "91af4fc8d8c1" +STAGE_12A_REVISION = "fbb7488ebe8a" 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: @@ -98,6 +101,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]: @@ -107,6 +112,17 @@ def _columns(engine, table_name: str) -> dict[str, dict]: } +def _assert_area_impact_coordinates_restored(engine: Engine) -> None: + """Check the Stage 12A revision's map coordinates are required integers.""" + _assert_revision(engine, STAGE_12A_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 ( @@ -365,6 +381,10 @@ def test_empty_upgrade_check_base_downgrade_and_reupgrade() -> None: _upgrade_and_assert_head(config, engine) + command.downgrade(config, STAGE_12A_REVISION) + _assert_area_impact_coordinates_restored(engine) + _upgrade_and_assert_head(config, engine) + command.downgrade(config, STAGE_12A_PREVIOUS_REVISION) _assert_revision(engine, STAGE_12A_PREVIOUS_REVISION) _assert_pre_stage_12a_association_shape(engine) @@ -597,14 +617,14 @@ def _assert_user_deletion_cascades( def test_stage_12a_schema_serves_the_previous_revision() -> None: - """Run the previous Public API revision's table usage against head. - - The parent revision's schema is reflected and then used, after upgrading, - 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 Stage 12A rows so the - cascades run. Head may not give any table the parent revision writes a - new required column without a server default. + """Run the previous Public API revision's table usage against Stage 12A. + + The parent revision's schema is reflected and then used, after upgrading + to the Stage 12A revision, 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 Stage + 12A rows so the cascades run. Stage 12A may not give any table the parent + revision writes a new required column without a server default. """ database_url = _disposable_url() config = _config() @@ -614,9 +634,9 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: command.downgrade(config, STAGE_12A_PREVIOUS_REVISION) parent = sa.MetaData() parent.reflect(engine) - command.upgrade(config, "head") - head = sa.MetaData() - head.reflect(engine) + command.upgrade(config, STAGE_12A_REVISION) + stage_12a = sa.MetaData() + stage_12a.reflect(engine) parent_tables = [ table for table in parent.sorted_tables if table.name != "alembic_version" ] @@ -625,14 +645,16 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: f"{table.name}.{column.name}" for table in parent_tables for column in table.columns - if table.name not in head.tables - or column.name not in head.tables[table.name].columns + if table.name not in stage_12a.tables + or column.name not in stage_12a.tables[table.name].columns ) assert dropped_columns == [] tables_with_new_required_columns = { - table.name: sorted(_new_required_columns(table, head.tables[table.name])) + table.name: sorted( + _new_required_columns(table, stage_12a.tables[table.name]) + ) for table in parent_tables - if _new_required_columns(table, head.tables[table.name]) + if _new_required_columns(table, stage_12a.tables[table.name]) } assert set(tables_with_new_required_columns) <= ( PARENT_REVISION_READ_ONLY_TABLES @@ -644,8 +666,8 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: try: stored_rows: dict[str, dict] = {} for table in parent_tables: - head_table = ( - head.tables[table.name] + stage_12a_table = ( + stage_12a.tables[table.name] if table.name in PARENT_REVISION_READ_ONLY_TABLES else None ) @@ -653,14 +675,14 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: connection, table, _row_values( - table, stored_rows, sequence, head_table=head_table + table, stored_rows, sequence, head_table=stage_12a_table ), - insert_table=head_table, + insert_table=stage_12a_table, ) _rewrite_mapped_columns(connection, table, stored_rows[table.name]) _assert_user_deletion_cascades( - connection, parent, head, stored_rows, sequence + connection, parent, stage_12a, stored_rows, sequence ) for table in reversed(parent_tables): @@ -677,6 +699,50 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: engine.dispose() +def test_area_impact_coordinate_removal_changes_nothing_else() -> None: + """Check that head's only change from Stage 12A drops the map coordinates. + + 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, STAGE_12A_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 4c0e6ee09..df5acfe37 100644 --- a/tests/unit/v2/test_alembic_v2.py +++ b/tests/unit/v2/test_alembic_v2.py @@ -235,6 +235,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, + "fbb7488ebe8a", "91af4fc8d8c1", "60d6518b6a98", "439303be14fe", @@ -495,7 +496,6 @@ def test_stage_12a_user_identity_and_association_revision_is_generated() -> None / "migrations/v2/versions/fbb7488ebe8a_link_signed_in_users_to_v1_anchored_.py" ).read_text(encoding="utf-8") - assert V2_ALEMBIC_HEAD_REVISION == "fbb7488ebe8a" assert ( "Generation: uv run alembic -c alembic-v2.ini revision --autogenerate" in revision @@ -565,6 +565,47 @@ def test_stage_12a_user_identity_and_association_revision_is_generated() -> None ) 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] = "fbb7488ebe8a"' 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 20947ffcb..fbb04b9fa 100644 --- a/tests/unit/v2/test_models.py +++ b/tests/unit/v2/test_models.py @@ -440,6 +440,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 ):