From 3d0b10533f922e4d9d9998c737da28e480217cf8 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:05:51 +0400 Subject: [PATCH 1/8] Count saved policies by table name in the migration qualification commands The policy and user-association qualification commands counted saved policies through the UserPolicy model. Both run before every v2 upgrade, so once the model takes a new table name they would query a table the database does not have yet. The policy qualification counts only while its revision is pending, when the table is still named user_policies, so it names that table. The user-association qualification counts on every run, so it counts the table under whichever name the database has: user_policy_associations once renamed, otherwise user_policies. Co-Authored-By: Claude Opus 5.5 --- .../data/v2/policy_migration_qualification.py | 19 ++++++-- ...ser_association_migration_qualification.py | 23 +++++++--- .../v2/test_policy_migration_qualification.py | 26 +++++++++++ ...ser_association_migration_qualification.py | 45 +++++++++++++++++++ 4 files changed, 104 insertions(+), 9 deletions(-) diff --git a/policyengine_api/data/v2/policy_migration_qualification.py b/policyengine_api/data/v2/policy_migration_qualification.py index beb03db24..d353226b1 100644 --- a/policyengine_api/data/v2/policy_migration_qualification.py +++ b/policyengine_api/data/v2/policy_migration_qualification.py @@ -10,12 +10,12 @@ from alembic.config import Config from alembic.migration import MigrationContext from alembic.script import ScriptDirectory -from sqlalchemy import Connection, Engine, func, inspect, select +from sqlalchemy import Connection, Engine, func, inspect, select, table from sqlalchemy.exc import NoInspectionAvailable from sqlalchemy.pool import NullPool from sqlmodel import create_engine -from policyengine_api.data.v2.models import ParameterValue, Policy, UserPolicy +from policyengine_api.data.v2.models import ParameterValue, Policy from policyengine_api.data.v2.settings import ( V2ConfigurationError, V2DatabaseSettings, @@ -26,6 +26,10 @@ from collections.abc import Callable, Collection, Mapping POLICY_MIGRATION_REVISION = "711ec2f0a5a5" +# The saved-policy table's name while the policy revision is pending. A later +# revision renames it to ``user_policy_associations``, so counting through +# the current model would query a table such a database does not have yet. +SAVED_POLICY_TABLE_AT_POLICY_REVISION = "user_policies" V2_ALEMBIC_CONFIG = "alembic-v2.ini" @@ -123,8 +127,15 @@ def table_exists(name: str) -> bool: if table_exists("parameter_values") else 0, user_policies=( - int(executor.scalar(select(func.count()).select_from(UserPolicy)) or 0) - if table_exists("user_policies") + int( + executor.scalar( + select(func.count()).select_from( + table(SAVED_POLICY_TABLE_AT_POLICY_REVISION) + ) + ) + or 0 + ) + if table_exists(SAVED_POLICY_TABLE_AT_POLICY_REVISION) else 0 ), ) diff --git a/policyengine_api/data/v2/user_association_migration_qualification.py b/policyengine_api/data/v2/user_association_migration_qualification.py index 9d7aad2a5..fac2b0d00 100644 --- a/policyengine_api/data/v2/user_association_migration_qualification.py +++ b/policyengine_api/data/v2/user_association_migration_qualification.py @@ -24,14 +24,13 @@ from alembic.config import Config from alembic.migration import MigrationContext from alembic.script import ScriptDirectory -from sqlalchemy import Connection, Engine, func, inspect, select +from sqlalchemy import Connection, Engine, func, inspect, select, table from sqlalchemy.exc import NoInspectionAvailable from sqlalchemy.pool import NullPool from sqlmodel import create_engine from policyengine_api.data.v2.models import ( UserHouseholdAssociation, - UserPolicy, UserReportAssociation, UserSimulationAssociation, ) @@ -44,7 +43,14 @@ if TYPE_CHECKING: from collections.abc import Callable, Collection, Mapping + from sqlalchemy.sql.expression import TableClause + USER_ASSOCIATION_MIGRATION_REVISION = "fbb7488ebe8a" +# The saved-policy table's name at this revision, and the name a later +# revision gives it. The command runs before every upgrade, so it counts the +# table under whichever name the database has. +SAVED_POLICY_TABLE_AT_RESHAPE = "user_policies" +SAVED_POLICY_TABLE_AFTER_RENAME = "user_policy_associations" V2_ALEMBIC_CONFIG = "alembic-v2.ini" @@ -122,13 +128,20 @@ def read_user_association_counts( ) -> UserAssociationCounts: """Count every row in the four user-association tables.""" - def count(model: type, table_name: str) -> int: + def count(source: type | TableClause, table_name: str) -> int: if existing_tables is not None and table_name not in existing_tables: return 0 - return int(executor.scalar(select(func.count()).select_from(model)) or 0) + return int(executor.scalar(select(func.count()).select_from(source)) or 0) + + saved_policy_table = ( + SAVED_POLICY_TABLE_AFTER_RENAME + if existing_tables is not None + and SAVED_POLICY_TABLE_AFTER_RENAME in existing_tables + else SAVED_POLICY_TABLE_AT_RESHAPE + ) return UserAssociationCounts( - user_policies=count(UserPolicy, "user_policies"), + user_policies=count(table(saved_policy_table), saved_policy_table), user_household_associations=count( UserHouseholdAssociation, "user_household_associations" ), diff --git a/tests/unit/v2/test_policy_migration_qualification.py b/tests/unit/v2/test_policy_migration_qualification.py index 54a711ef4..dc28306a5 100644 --- a/tests/unit/v2/test_policy_migration_qualification.py +++ b/tests/unit/v2/test_policy_migration_qualification.py @@ -175,3 +175,29 @@ def fail() -> None: assert "qualification failed unexpectedly" in captured.err assert "secret" not in captured.err assert "private-host" not in captured.err + + +class RecordingExecutor: + def __init__(self) -> None: + self.statements: list[str] = [] + + def scalar(self, statement: object) -> int: + self.statements.append(str(statement)) + return 0 + + +def test_saved_policies_are_counted_under_the_name_of_the_pending_revision() -> None: + """The policy revision is pending only before the rename of ``user_policies``. + + Counting through the current model would query ``user_policy_associations``, + which a database still waiting for the policy revision does not have yet. + """ + executor = RecordingExecutor() + + qualification.read_policy_data_counts( + executor, existing_tables={"policies", "parameter_values", "user_policies"} + ) + + saved_policy_count = executor.statements[2] + assert "FROM user_policies" in saved_policy_count + assert "user_policy_associations" not in saved_policy_count diff --git a/tests/unit/v2/test_user_association_migration_qualification.py b/tests/unit/v2/test_user_association_migration_qualification.py index 463a073f9..ac3997cac 100644 --- a/tests/unit/v2/test_user_association_migration_qualification.py +++ b/tests/unit/v2/test_user_association_migration_qualification.py @@ -143,3 +143,48 @@ def fail(): payload = json.loads(capsys.readouterr().err) assert payload["outcome"] == "error" assert "secret" not in json.dumps(payload) + + +def test_saved_policies_are_counted_under_the_name_of_the_pending_revision() -> None: + """The reshape is pending only before the later rename of ``user_policies``. + + Counting through the current model would query ``user_policy_associations``, + which a database still waiting for the reshape does not have yet. + """ + _evidence, connection, _engine = _qualify((0, 0, 0, 0), pending=True) + + saved_policy_count = connection.statements[0] + assert "FROM user_policies" in saved_policy_count + assert "user_policy_associations" not in saved_policy_count + + +class RecordingExecutor: + def __init__(self) -> None: + self.statements: list[str] = [] + + def scalar(self, statement: object) -> int: + self.statements.append(str(statement)) + return 3 + + +def test_saved_policies_are_counted_under_their_name_after_the_rename() -> None: + """Once ``user_policies`` is renamed, its rows are counted under the new name. + + The command runs before every upgrade, so it meets the table under either + name; reading only the old name would report no saved policies. + """ + executor = RecordingExecutor() + + counts = qualification.read_user_association_counts( + executor, + existing_tables={ + "user_policy_associations", + "user_household_associations", + "user_simulation_associations", + "user_report_associations", + }, + ) + + assert counts.user_policies == 3 + saved_policy_count = executor.statements[0] + assert "FROM user_policy_associations" in saved_policy_count From 320c5d20d85086d6c46d025163c2c9124829c023 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:32:39 +0400 Subject: [PATCH 2/8] Rename the v2 table user_policies to user_policy_associations The saved-policy table now follows the naming of the other user-association tables. Revision fa7fa1035444 was autogenerated from the renamed model. Its one documented post-generation correction replaces the generated create-and-drop pair, which would have deleted every saved policy, with an in-place rename; each constraint named after the table is dropped under its old name and created under the generated name, and the generated changes to the two convention-named indexes and the three foreign keys that refer to the table are kept around the rename. The composite listing indexes keep their ix_user_policies_* names, as the other association tables keep their short index names. The privilege declarations and the head revision constant use the new name. Dual-write's saved-policy copy goes through the model and follows it. Unchanged: the route /v2/user-policies, the user_policy_id columns, legacy_user_policy_mappings and the v1 Cloud SQL table user_policies. Not included: a temporary view named user_policies for the release still running during the deploy. A view is not autogenerated, so it needs a human migration decision; a strict expected-failure test records the gap. Co-Authored-By: Claude Opus 5.5 --- ...lan-1f-122-rename-user-policies.changed.md | 1 + docs/engineering/skills/alembic-migrations.md | 27 ++ docs/migration/stage-12a-user-login.md | 8 +- ...44_rename_user_policies_to_user_policy_.py | 292 ++++++++++++++ .../data/v2/metadata_validation.py | 4 +- policyengine_api/data/v2/migration_target.py | 2 +- .../data/v2/models/associations.py | 16 +- .../data/v2/models/policy_mappings.py | 2 +- policyengine_api/data/v2/privileges.py | 2 +- .../reads_good.py | 4 +- .../integration/test_alembic_v2_lifecycle.py | 356 +++++++++++++++++- tests/unit/v2/test_alembic_v2.py | 40 +- tests/unit/v2/test_models.py | 55 ++- tests/unit/v2/test_privilege_manifest.py | 2 +- tests/unit/v2/test_privilege_requirements.py | 8 +- .../v2/test_user_policy_legacy_persistence.py | 16 +- 16 files changed, 795 insertions(+), 40 deletions(-) create mode 100644 changelog.d/v2plan-1f-122-rename-user-policies.changed.md create mode 100644 migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py diff --git a/changelog.d/v2plan-1f-122-rename-user-policies.changed.md b/changelog.d/v2plan-1f-122-rename-user-policies.changed.md new file mode 100644 index 000000000..1141ffd75 --- /dev/null +++ b/changelog.d/v2plan-1f-122-rename-user-policies.changed.md @@ -0,0 +1 @@ +The v2 table user_policies is renamed to user_policy_associations, matching the other user-association tables; its constraints and convention-named indexes take the new name. Saved policies, their v1 mappings and the reports that name them are kept. The route /v2/user-policies, the user_policy_id columns, legacy_user_policy_mappings and the v1 Cloud SQL table user_policies are unchanged. diff --git a/docs/engineering/skills/alembic-migrations.md b/docs/engineering/skills/alembic-migrations.md index d4a043d91..3b7a13f36 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -294,3 +294,30 @@ 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 saved-policy table rename + +Revision `fa7fa1035444` was autogenerated after renaming the `UserPolicy` +model's table from `user_policies` to `user_policy_associations`, with its +constraints and convention-named indexes. Autogeneration cannot detect a +rename: it created an empty `user_policy_associations` table and dropped +`user_policies`, which would delete every saved policy, and its copies of the +three composite indexes collided with the old table's indexes of the same +names. Its only post-generation correction replaces that pair with +`op.rename_table` and, for each constraint the generated table named after +`user_policy_associations` (primary key, unique, three checks and two foreign +keys), drops it under its old name and creates it under the generated name +with the generated definition. The generated drops and re-creations of the +two convention-named indexes and of the three foreign keys that refer to the +table are kept, ordered around the rename. The downgrade is the exact reverse. +The v2 Postgres lifecycle test stores a saved policy with a v1 mapping and a +report that names it, and checks that the rows, the references, the +cascades, a table grant and every constraint and index name survive the +upgrade and the downgrade. + +The plan also asks for a temporary view named `user_policies` over the renamed +table, so the release still running during the deploy keeps working. A view +is not autogenerated, and adding one would be a hand-written schema operation, +so the revision leaves it out pending a human migration decision. +`test_previous_release_still_finds_user_policies` records the gap as a strict +expected failure. diff --git a/docs/migration/stage-12a-user-login.md b/docs/migration/stage-12a-user-login.md index 3fc391392..2831a4a60 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -324,7 +324,13 @@ 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. `V2_ALEMBIC_HEAD_REVISION` was `fbb7488ebe8a` when Stage 12A shipped. + +Revision `fa7fa1035444` later renamed `user_policies` to +`user_policy_associations`, with its constraints and its convention-named +indexes, matching the other association tables; the composite listing +indexes keep their `ix_user_policies_*` names. The route `/v2/user-policies`, +the `user_policy_id` columns and `legacy_user_policy_mappings` are unchanged. ### Reshape safety diff --git a/migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py b/migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py new file mode 100644 index 000000000..d2aa619c5 --- /dev/null +++ b/migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py @@ -0,0 +1,292 @@ +"""rename user_policies to user_policy_associations + +Revision ID: fa7fa1035444 +Revises: fbb7488ebe8a +Create Date: 2026-10-22 17:07:03.720031 +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 +from sqlalchemy.dialects import postgresql + +revision: str = "fa7fa1035444" +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_constraint( + op.f("fk_legacy_user_policy_mappings_association_country"), + "legacy_user_policy_mappings", + type_="foreignkey", + ) + op.drop_constraint( + op.f("fk_user_report_associations_baseline_user_policy_id_use_7740"), + "user_report_associations", + type_="foreignkey", + ) + op.drop_constraint( + op.f("fk_user_report_associations_user_policy_id_user_policies"), + "user_report_associations", + type_="foreignkey", + ) + op.drop_index(op.f("ix_user_policies_policy_id"), table_name="user_policies") + op.drop_index(op.f("ix_user_policies_user_id"), table_name="user_policies") + # Post-generation correction: autogeneration cannot detect a rename. It + # created an empty user_policy_associations table and dropped user_policies, + # which would delete every saved policy (its three composite indexes would + # also have collided with the old table's indexes of the same names). The + # table is renamed in place instead, keeping its rows, grants and composite + # indexes. Each constraint the generated table named after + # user_policy_associations is dropped under its old name and created under + # the generated name with the generated definition. The foreign keys that + # refer to this table were dropped above, before its primary key and unique + # constraint. + op.rename_table("user_policies", "user_policy_associations") + op.drop_constraint("pk_user_policies", "user_policy_associations", type_="primary") + op.create_primary_key( + op.f("pk_user_policy_associations"), "user_policy_associations", ["id"] + ) + op.drop_constraint( + "uq_user_policies_id_country", "user_policy_associations", type_="unique" + ) + op.create_unique_constraint( + "uq_user_policy_associations_id_country", + "user_policy_associations", + ["id", "country_id"], + ) + op.drop_constraint( + "ck_user_policies_baseline", "user_policy_associations", type_="check" + ) + op.create_check_constraint( + op.f("ck_user_policy_associations_baseline"), + "user_policy_associations", + "baseline IS NULL OR baseline IN ('current-law', 'current-policy')", + ) + op.drop_constraint( + "ck_user_policies_country", "user_policy_associations", type_="check" + ) + op.create_check_constraint( + op.f("ck_user_policy_associations_country"), + "user_policy_associations", + "country_id IN ('us', 'uk')", + ) + op.drop_constraint( + "ck_user_policies_policy_ref", "user_policy_associations", type_="check" + ) + op.create_check_constraint( + op.f("ck_user_policy_associations_policy_ref"), + "user_policy_associations", + "policy_id IS NOT NULL OR legacy_policy_id IS NOT NULL", + ) + op.drop_constraint( + "fk_user_policies_policy_country", + "user_policy_associations", + type_="foreignkey", + ) + op.create_foreign_key( + "fk_user_policy_associations_policy_country", + "user_policy_associations", + "policies", + ["policy_id", "country_id"], + ["id", "country_id"], + ondelete="RESTRICT", + ) + op.drop_constraint( + "fk_user_policies_user_id_users", + "user_policy_associations", + type_="foreignkey", + ) + op.create_foreign_key( + op.f("fk_user_policy_associations_user_id_users"), + "user_policy_associations", + "users", + ["user_id"], + ["id"], + ondelete="CASCADE", + ) + op.create_index( + op.f("ix_user_policy_associations_policy_id"), + "user_policy_associations", + ["policy_id"], + unique=False, + ) + op.create_index( + op.f("ix_user_policy_associations_user_id"), + "user_policy_associations", + ["user_id"], + unique=False, + ) + op.create_foreign_key( + "fk_legacy_user_policy_mappings_association_country", + "legacy_user_policy_mappings", + "user_policy_associations", + ["user_policy_id", "country_id"], + ["id", "country_id"], + ondelete="CASCADE", + ) + op.create_foreign_key( + op.f("fk_user_report_associations_user_policy_id_user_policy_associations"), + "user_report_associations", + "user_policy_associations", + ["user_policy_id"], + ["id"], + ondelete="SET NULL", + ) + op.create_foreign_key( + op.f( + "fk_user_report_associations_baseline_user_policy_id_user_policy_associations" + ), + "user_report_associations", + "user_policy_associations", + ["baseline_user_policy_id"], + ["id"], + ondelete="SET NULL", + ) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint( + op.f( + "fk_user_report_associations_baseline_user_policy_id_user_policy_associations" + ), + "user_report_associations", + type_="foreignkey", + ) + op.drop_constraint( + op.f("fk_user_report_associations_user_policy_id_user_policy_associations"), + "user_report_associations", + type_="foreignkey", + ) + op.drop_constraint( + "fk_legacy_user_policy_mappings_association_country", + "legacy_user_policy_mappings", + type_="foreignkey", + ) + op.drop_index( + op.f("ix_user_policy_associations_user_id"), + table_name="user_policy_associations", + ) + op.drop_index( + op.f("ix_user_policy_associations_policy_id"), + table_name="user_policy_associations", + ) + # Post-generation correction: the reverse of the upgrade's rename. The + # generated downgrade created an empty user_policies table and dropped + # user_policy_associations, which would delete every saved policy. The + # table is renamed back in place, and each constraint is dropped under its + # new name and created under the name and definition it has at revision + # fbb7488ebe8a. + op.drop_constraint( + op.f("fk_user_policy_associations_user_id_users"), + "user_policy_associations", + type_="foreignkey", + ) + op.drop_constraint( + "fk_user_policy_associations_policy_country", + "user_policy_associations", + type_="foreignkey", + ) + op.drop_constraint( + op.f("ck_user_policy_associations_policy_ref"), + "user_policy_associations", + type_="check", + ) + op.drop_constraint( + op.f("ck_user_policy_associations_country"), + "user_policy_associations", + type_="check", + ) + op.drop_constraint( + op.f("ck_user_policy_associations_baseline"), + "user_policy_associations", + type_="check", + ) + op.drop_constraint( + "uq_user_policy_associations_id_country", + "user_policy_associations", + type_="unique", + ) + op.drop_constraint( + op.f("pk_user_policy_associations"), + "user_policy_associations", + type_="primary", + ) + op.rename_table("user_policy_associations", "user_policies") + op.create_primary_key(op.f("pk_user_policies"), "user_policies", ["id"]) + op.create_unique_constraint( + op.f("uq_user_policies_id_country"), "user_policies", ["id", "country_id"] + ) + op.create_check_constraint( + op.f("ck_user_policies_baseline"), + "user_policies", + "baseline IS NULL OR baseline IN ('current-law', 'current-policy')", + ) + op.create_check_constraint( + op.f("ck_user_policies_country"), + "user_policies", + "country_id IN ('us', 'uk')", + ) + op.create_check_constraint( + op.f("ck_user_policies_policy_ref"), + "user_policies", + "policy_id IS NOT NULL OR legacy_policy_id IS NOT NULL", + ) + op.create_foreign_key( + op.f("fk_user_policies_policy_country"), + "user_policies", + "policies", + ["policy_id", "country_id"], + ["id", "country_id"], + ondelete="RESTRICT", + ) + op.create_foreign_key( + op.f("fk_user_policies_user_id_users"), + "user_policies", + "users", + ["user_id"], + ["id"], + ondelete="CASCADE", + ) + op.create_index( + op.f("ix_user_policies_user_id"), "user_policies", ["user_id"], unique=False + ) + op.create_index( + op.f("ix_user_policies_policy_id"), + "user_policies", + ["policy_id"], + unique=False, + ) + op.create_foreign_key( + op.f("fk_user_report_associations_user_policy_id_user_policies"), + "user_report_associations", + "user_policies", + ["user_policy_id"], + ["id"], + ondelete="SET NULL", + ) + op.create_foreign_key( + op.f("fk_user_report_associations_baseline_user_policy_id_use_7740"), + "user_report_associations", + "user_policies", + ["baseline_user_policy_id"], + ["id"], + ondelete="SET NULL", + ) + op.create_foreign_key( + op.f("fk_legacy_user_policy_mappings_association_country"), + "legacy_user_policy_mappings", + "user_policies", + ["user_policy_id", "country_id"], + ["id", "country_id"], + ondelete="CASCADE", + ) + # ### end Alembic commands ### diff --git a/policyengine_api/data/v2/metadata_validation.py b/policyengine_api/data/v2/metadata_validation.py index 7445269f1..ab60ab93a 100644 --- a/policyengine_api/data/v2/metadata_validation.py +++ b/policyengine_api/data/v2/metadata_validation.py @@ -3,8 +3,8 @@ from collections.abc import Iterable # These names belong only to the existing v1 database domain. Names that are -# intentionally shared between v1 and v2, such as `simulations` and -# `user_policies`, are excluded from this targeted rejection set. +# intentionally shared between v1 and v2, such as `simulations`, are excluded +# from this targeted rejection set. V1_ONLY_TABLES = frozenset( { "analysis", diff --git a/policyengine_api/data/v2/migration_target.py b/policyengine_api/data/v2/migration_target.py index fd4c05fdf..a57c2419c 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 = "fa7fa1035444" 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/associations.py b/policyengine_api/data/v2/models/associations.py index 2c5cd21b1..4823e87fb 100644 --- a/policyengine_api/data/v2/models/associations.py +++ b/policyengine_api/data/v2/models/associations.py @@ -106,29 +106,29 @@ class UserPolicy(TimestampedModel, table=True): policy that users have saved cannot be deleted. """ - __tablename__ = "user_policies" + __tablename__ = "user_policy_associations" __table_args__ = ( sa.UniqueConstraint( "id", "country_id", - name="uq_user_policies_id_country", + name="uq_user_policy_associations_id_country", ), sa.CheckConstraint( "country_id IN ('us', 'uk')", - name="ck_user_policies_country", + name="ck_user_policy_associations_country", ), sa.CheckConstraint( "policy_id IS NOT NULL OR legacy_policy_id IS NOT NULL", - name="ck_user_policies_policy_ref", + name="ck_user_policy_associations_policy_ref", ), sa.CheckConstraint( "baseline IS NULL OR baseline IN ('current-law', 'current-policy')", - name="ck_user_policies_baseline", + name="ck_user_policy_associations_baseline", ), sa.ForeignKeyConstraint( ["policy_id", "country_id"], ["policies.id", "policies.country_id"], - name="fk_user_policies_policy_country", + name="fk_user_policy_associations_policy_country", ondelete="RESTRICT", ), sa.Index( @@ -278,7 +278,7 @@ class UserReportAssociation(TimestampedModel, table=True): description: str | None = Field(default=None, sa_type=sa.Text) user_policy_id: UUID | None = Field( default=None, - foreign_key="user_policies.id", + foreign_key="user_policy_associations.id", ondelete="SET NULL", index=True, ) @@ -286,7 +286,7 @@ class UserReportAssociation(TimestampedModel, table=True): # comparison was not current law; its name is the baseline's label. baseline_user_policy_id: UUID | None = Field( default=None, - foreign_key="user_policies.id", + foreign_key="user_policy_associations.id", ondelete="SET NULL", index=True, ) diff --git a/policyengine_api/data/v2/models/policy_mappings.py b/policyengine_api/data/v2/models/policy_mappings.py index 7cdbc91b9..69aef5642 100644 --- a/policyengine_api/data/v2/models/policy_mappings.py +++ b/policyengine_api/data/v2/models/policy_mappings.py @@ -104,7 +104,7 @@ class LegacyUserPolicyMapping(TimestampedModel, table=True): ), sa.ForeignKeyConstraint( ["user_policy_id", "country_id"], - ["user_policies.id", "user_policies.country_id"], + ["user_policy_associations.id", "user_policy_associations.country_id"], name="fk_legacy_user_policy_mappings_association_country", ondelete="CASCADE", ), diff --git a/policyengine_api/data/v2/privileges.py b/policyengine_api/data/v2/privileges.py index 8f8c34515..dc965e36e 100644 --- a/policyengine_api/data/v2/privileges.py +++ b/policyengine_api/data/v2/privileges.py @@ -135,7 +135,7 @@ def _access( # Users and their saved work (Stage 12A). "user_household_associations": _access(runtime=READ_WRITE), "user_identities": _access(runtime=READ_WRITE), - "user_policies": _access(runtime=READ_WRITE), + "user_policy_associations": _access(runtime=READ_WRITE), "user_report_associations": _access(runtime=READ_WRITE), "user_simulation_associations": _access(runtime=READ_WRITE), "users": _access(runtime=READ_WRITE), diff --git a/rules/fixtures/api.v2.runtime-privileges-declared/reads_good.py b/rules/fixtures/api.v2.runtime-privileges-declared/reads_good.py index 5437a95c0..0f7567704 100644 --- a/rules/fixtures/api.v2.runtime-privileges-declared/reads_good.py +++ b/rules/fixtures/api.v2.runtime-privileges-declared/reads_good.py @@ -1,7 +1,7 @@ """Good fixture: a runtime read of only the tables the runtime role is granted. -The user-policy lookup reads ``user_policies``, which the runtime role holds -SELECT on, and nothing else. +The user-policy lookup reads ``user_policy_associations``, which the runtime +role holds SELECT on, and nothing else. """ from __future__ import annotations diff --git a/tests/integration/test_alembic_v2_lifecycle.py b/tests/integration/test_alembic_v2_lifecycle.py index 814971236..694fe0363 100644 --- a/tests/integration/test_alembic_v2_lifecycle.py +++ b/tests/integration/test_alembic_v2_lifecycle.py @@ -30,6 +30,7 @@ STAGE_12_PREVIOUS_REVISION = "724b1b11a33e" STAGE_12_COMPARISON_PREVIOUS_REVISION = "439303be14fe" STAGE_12A_PREVIOUS_REVISION = "91af4fc8d8c1" +SAVED_POLICY_RENAME_PARENT_REVISION = "fbb7488ebe8a" V2_TABLE_NAMES = frozenset(table.name for table in V2_METADATA.tables.values()) @@ -147,7 +148,7 @@ def _assert_stage_12a_association_shape(engine) -> None: "subject", ] for table_name, reference in ( - ("user_policies", "policy"), + ("user_policy_associations", "policy"), ("user_household_associations", "household"), ("user_simulation_associations", "simulation"), ("user_report_associations", "report"), @@ -171,7 +172,7 @@ def _assert_stage_12a_association_shape(engine) -> None: report_policy_key = next( foreign_key for foreign_key in inspect(engine).get_foreign_keys("user_report_associations") - if foreign_key["referred_table"] == "user_policies" + if foreign_key["referred_table"] == "user_policy_associations" ) assert report_policy_key["options"]["ondelete"] == "SET NULL" for table_name in ("user_simulation_associations", "user_report_associations"): @@ -408,6 +409,11 @@ def test_empty_upgrade_check_base_downgrade_and_reupgrade() -> None: "user_simulation_associations", "user_report_associations", ) +# Tables head renamed, keyed by their name at the parent revision. The +# previous release's statements name the old table, which head does not have +# until a view under that name exists (test_previous_release_still_finds_ +# user_policies), so its column usage runs against the renamed table here. +RENAMED_AT_HEAD = {"user_policies": "user_policy_associations"} COUNTRY_COLUMN_NAMES = frozenset({"country", "country_id", "primary_country"}) # Nullable references that a parent-revision check constraint requires. REFERENCES_REQUIRED_BY_CHECKS = frozenset({("simulations", "household_id")}) @@ -587,7 +593,7 @@ def _assert_user_deletion_cascades( assert result.rowcount == 1 for table_name in (*USER_ASSOCIATION_TABLES, "user_identities"): - table = head.tables[table_name] + table = head.tables[RENAMED_AT_HEAD.get(table_name, table_name)] remaining = connection.execute( sa.select(sa.func.count()) .select_from(table) @@ -604,7 +610,8 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: 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. + new required column without a server default. A table head renamed is + used under its new name (``RENAMED_AT_HEAD``). """ database_url = _disposable_url() config = _config() @@ -614,6 +621,8 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: command.downgrade(config, STAGE_12A_PREVIOUS_REVISION) parent = sa.MetaData() parent.reflect(engine) + for parent_name, head_name in RENAMED_AT_HEAD.items(): + parent.tables[parent_name].name = head_name command.upgrade(config, "head") head = sa.MetaData() head.reflect(engine) @@ -957,3 +966,342 @@ def reflected(name: str) -> sa.Table: connection.execute(sa.delete(table).where(table.c.id == row_id)) command.upgrade(config, "head") engine.dispose() + + +@pytest.mark.xfail( + strict=True, + raises=sa.exc.ProgrammingError, + reason=( + "Head has no view named user_policies yet. Autogeneration cannot " + "create one, and a hand-written view needs a human migration decision " + "(see the saved-policy table rename in the Alembic migrations skill); " + "until then the release running during a deploy cannot find the table." + ), +) +def test_previous_release_still_finds_user_policies() -> None: + """The release running during the deploy reads saved policies by the old name.""" + engine = create_engine(_disposable_url()) + try: + with engine.connect() as connection: + connection.execute(text("SELECT count(*) FROM user_policies")).scalar_one() + finally: + engine.dispose() + + +def _constraint_names(engine, table_name: str) -> set[str]: + with engine.connect() as connection: + return set( + connection.execute( + text( + "SELECT conname FROM pg_constraint " + "WHERE conrelid = to_regclass(:table_name)" + ), + {"table_name": f"public.{table_name}"}, + ).scalars() + ) + + +def _index_names(engine, table_name: str) -> set[str]: + with engine.connect() as connection: + return set( + connection.execute( + text( + "SELECT indexname FROM pg_indexes " + "WHERE schemaname = 'public' AND tablename = :table_name" + ), + {"table_name": table_name}, + ).scalars() + ) + + +def _referring_foreign_keys(engine, table_name: str) -> set[tuple[str, str]]: + with engine.connect() as connection: + return { + (row.on_table, row.conname) + for row in connection.execute( + text( + "SELECT conrelid::regclass::text AS on_table, conname " + "FROM pg_constraint WHERE contype = 'f' " + "AND confrelid = to_regclass(:table_name)" + ), + {"table_name": f"public.{table_name}"}, + ) + } + + +def _closure_of_referred_tables( + metadata: sa.MetaData, table_names: set[str] +) -> set[str]: + closure: set[str] = set() + pending = list(table_names) + while pending: + table_name = pending.pop() + if table_name in closure: + continue + closure.add(table_name) + pending.extend( + foreign_key.referred_table.name + for foreign_key in metadata.tables[table_name].foreign_key_constraints + ) + return closure + + +def _saved_policy_with_references(connection, parent: sa.MetaData) -> dict: + """Store a user, a policy, a saved policy, its v1 mapping and a report.""" + sequence = itertools.count() + stored_rows: dict[str, dict] = {} + owner_tables = _closure_of_referred_tables(parent, {"users", "policies"}) + for table in parent.sorted_tables: + if table.name in owner_tables: + values = _row_values(table, stored_rows, sequence) + if table.name == "users": + # A standard user needs a country (ck_users_standard_primary_country). + values["primary_country"] = "us" + stored_rows[table.name] = _insert_and_read_back(connection, table, values) + saved_policy_id = uuid4() + connection.execute( + parent.tables["user_policies"] + .insert() + .values( + id=saved_policy_id, + user_id=stored_rows["users"]["id"], + policy_id=stored_rows["policies"]["id"], + country_id=stored_rows["policies"]["country_id"], + name="Saved reform", + baseline="current-law", + ) + ) + connection.execute( + parent.tables["legacy_user_policy_mappings"] + .insert() + .values( + id=uuid4(), + country_id=stored_rows["policies"]["country_id"], + legacy_user_policy_id=41, + user_policy_id=saved_policy_id, + fingerprint_version=1, + fingerprint_sha256="a" * 64, + ) + ) + report_id = uuid4() + connection.execute( + parent.tables["user_report_associations"] + .insert() + .values( + id=report_id, + user_id=stored_rows["users"]["id"], + country_id=stored_rows["policies"]["country_id"], + legacy_report_id=42, + user_policy_id=saved_policy_id, + baseline_user_policy_id=saved_policy_id, + ) + ) + return { + "stored_rows": stored_rows, + "saved_policy_id": saved_policy_id, + "report_id": report_id, + } + + +def _saved_policy_row(connection, table_name: str, saved_policy_id: UUID) -> dict: + return dict( + connection.execute( + sa.select(sa.text("*")) + .select_from(sa.table(table_name)) + .where(sa.column("id") == saved_policy_id) + ) + .mappings() + .one() + ) + + +def _delete_saved_policy_rows(engine, parent: sa.MetaData, saved: dict) -> None: + """Remove the stored rows at head, children first, so later tests start clean.""" + with engine.begin() as connection: + connection.execute( + text("DELETE FROM user_report_associations WHERE id = :id"), + {"id": saved["report_id"]}, + ) + # Deleting the saved policy cascades to its v1 mapping. + connection.execute( + text("DELETE FROM user_policy_associations WHERE id = :id"), + {"id": saved["saved_policy_id"]}, + ) + for table in reversed(parent.sorted_tables): + stored_row = saved["stored_rows"].get(table.name) + if stored_row is not None: + connection.execute( + table.delete().where(_primary_key_clause(table, stored_row)) + ) + + +# The names head gives the renamed table's constraints, indexes and the +# foreign keys that refer to it. Names longer than PostgreSQL's 63 characters +# end in a hash, as SQLAlchemy shortens the model's convention names. +SAVED_POLICY_SCHEMA_NAMES_AT_HEAD = { + "constraints": { + "pk_user_policy_associations", + "uq_user_policy_associations_id_country", + "ck_user_policy_associations_country", + "ck_user_policy_associations_policy_ref", + "ck_user_policy_associations_baseline", + "fk_user_policy_associations_policy_country", + "fk_user_policy_associations_user_id_users", + }, + "indexes": { + "pk_user_policy_associations", + "uq_user_policy_associations_id_country", + "ix_user_policy_associations_user_id", + "ix_user_policy_associations_policy_id", + "ix_user_policies_country_user_created_id", + "ix_user_policies_country_policy", + "ix_user_policies_country_legacy_policy", + }, + "referring_foreign_keys": { + ( + "legacy_user_policy_mappings", + "fk_legacy_user_policy_mappings_association_country", + ), + ( + "user_report_associations", + "fk_user_report_associations_user_policy_id_user_policy__0820", + ), + ( + "user_report_associations", + "fk_user_report_associations_baseline_user_policy_id_use_0a3d", + ), + }, +} +RENAME_PRIVILEGE_PROBE_ROLE = "rename_privilege_probe" + + +def _schema_names(engine, table_name: str) -> dict[str, set]: + return { + "constraints": _constraint_names(engine, table_name), + "indexes": _index_names(engine, table_name), + "referring_foreign_keys": _referring_foreign_keys(engine, table_name), + } + + +def _saved_policy_references(connection, saved: dict) -> tuple: + """Return the saved policy a v1 mapping and a report's two keys point at.""" + mapped = connection.execute( + text( + "SELECT user_policy_id FROM legacy_user_policy_mappings " + "WHERE legacy_user_policy_id = 41" + ) + ).scalar_one_or_none() + report_references = connection.execute( + text( + "SELECT user_policy_id, baseline_user_policy_id " + "FROM user_report_associations WHERE id = :id" + ), + {"id": saved["report_id"]}, + ).one() + return (mapped, *report_references) + + +def _references_after_deleting_saved_policy(engine, saved: dict) -> tuple: + """Delete the saved policy in a rolled-back transaction; return the references. + + The v1 mapping cascades away and the report's keys are set to null. + """ + with engine.connect() as connection: + transaction = connection.begin() + try: + connection.execute( + text("DELETE FROM user_policy_associations WHERE id = :id"), + {"id": saved["saved_policy_id"]}, + ) + return _saved_policy_references(connection, saved) + finally: + transaction.rollback() + + +def _drop_rename_privilege_probe(engine) -> None: + with engine.begin() as connection: + connection.execute( + text( + "DO $$ BEGIN IF EXISTS (SELECT FROM pg_roles WHERE rolname = " + f"'{RENAME_PRIVILEGE_PROBE_ROLE}') THEN " + f"DROP OWNED BY {RENAME_PRIVILEGE_PROBE_ROLE}; " + f"DROP ROLE {RENAME_PRIVILEGE_PROBE_ROLE}; END IF; END $$" + ) + ) + + +def test_saved_policy_rename_keeps_rows_references_and_privileges() -> None: + """Upgrading renames the table in place, and downgrading renames it back. + + Saved policies, their v1 mappings and the reports that name them survive + both directions, the references and a table grant follow the table, and + the constraints and indexes named after the table take its name. The + downgrade restores every name the parent revision had. + """ + config = _config() + engine = create_engine(_disposable_url()) + parent = sa.MetaData() + saved = None + try: + command.downgrade(config, SAVED_POLICY_RENAME_PARENT_REVISION) + parent.reflect(engine) + names_at_parent = _schema_names(engine, "user_policies") + _drop_rename_privilege_probe(engine) + with engine.begin() as connection: + saved = _saved_policy_with_references(connection, parent) + connection.execute(text(f"CREATE ROLE {RENAME_PRIVILEGE_PROBE_ROLE}")) + connection.execute( + text( + "GRANT SELECT, UPDATE ON user_policies TO " + f"{RENAME_PRIVILEGE_PROBE_ROLE}" + ) + ) + before = _saved_policy_row( + connection, "user_policies", saved["saved_policy_id"] + ) + saved_policy_id = saved["saved_policy_id"] + + command.upgrade(config, "head") + + table_names = set(inspect(engine).get_table_names(schema="public")) + assert "user_policies" not in table_names + with engine.connect() as connection: + after = _saved_policy_row( + connection, "user_policy_associations", saved_policy_id + ) + references = _saved_policy_references(connection, saved) + granted = connection.execute( + text( + f"SELECT has_table_privilege('{RENAME_PRIVILEGE_PROBE_ROLE}', " + "'public.user_policy_associations', 'SELECT, UPDATE')" + ) + ).scalar_one() + assert after == before + assert references == (saved_policy_id, saved_policy_id, saved_policy_id) + assert granted + assert ( + _schema_names(engine, "user_policy_associations") + == SAVED_POLICY_SCHEMA_NAMES_AT_HEAD + ) + assert _references_after_deleting_saved_policy(engine, saved) == ( + None, + None, + None, + ) + + command.downgrade(config, SAVED_POLICY_RENAME_PARENT_REVISION) + + table_names = set(inspect(engine).get_table_names(schema="public")) + assert "user_policy_associations" not in table_names + with engine.connect() as connection: + restored = _saved_policy_row(connection, "user_policies", saved_policy_id) + restored_references = _saved_policy_references(connection, saved) + assert restored == before + assert restored_references == references + assert _schema_names(engine, "user_policies") == names_at_parent + finally: + command.upgrade(config, "head") + if saved is not None: + _delete_saved_policy_rows(engine, parent, saved) + _drop_rename_privilege_probe(engine) + engine.dispose() diff --git a/tests/unit/v2/test_alembic_v2.py b/tests/unit/v2/test_alembic_v2.py index 4c0e6ee09..f031d04b3 100644 --- a/tests/unit/v2/test_alembic_v2.py +++ b/tests/unit/v2/test_alembic_v2.py @@ -54,6 +54,8 @@ f"{PROJECT_REF}:test-password@aws-0-us-east-2.pooler.supabase.com:5432/" "postgres?sslmode=require" ) +# The Stage 12A revision; the saved-policy rename follows it. +SAVED_POLICY_RENAME_PARENT_REVISION = "fbb7488ebe8a" DISPOSABLE_URL = ( "postgresql+psycopg://postgres:test-password@127.0.0.1:5432/" f"{DISPOSABLE_DATABASE_NAME}" @@ -235,6 +237,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, + SAVED_POLICY_RENAME_PARENT_REVISION, "91af4fc8d8c1", "60d6518b6a98", "439303be14fe", @@ -495,7 +498,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 @@ -739,3 +741,39 @@ def test_v2_environment_commits_after_persistent_target_qualification() -> None: assert "current_heads != previous_heads" in env_source assert "current_heads == script_heads" in env_source assert "validate_v2_head_schema(connection, target_metadata)" in env_source + + +def _revision_source(revision_id: str) -> str: + (path,) = (REPO / "migrations/v2/versions").glob(f"{revision_id}_*.py") + return path.read_text(encoding="utf-8") + + +def test_saved_policy_rename_revision_renames_the_table_in_place() -> None: + """The rename keeps every saved reform: no table is created or dropped. + + Autogeneration replaces a renamed table with a new empty one; the revision + records its corrections, and every other operation is generated. + """ + revision = _revision_source(V2_ALEMBIC_HEAD_REVISION) + upgrade = revision[ + revision.index("def upgrade()") : revision.index("def downgrade()") + ] + downgrade = revision[revision.index("def downgrade()") :] + + assert V2_ALEMBIC_HEAD_REVISION != SAVED_POLICY_RENAME_PARENT_REVISION + assert ( + "Generation: uv run alembic -c alembic-v2.ini revision --autogenerate" + in revision + ) + assert ( + f'down_revision: Union[str, None] = "{SAVED_POLICY_RENAME_PARENT_REVISION}"' + ) in revision + assert 'op.rename_table("user_policies", "user_policy_associations")' in upgrade + assert 'op.rename_table("user_policy_associations", "user_policies")' in downgrade + for body in (upgrade, downgrade): + assert "op.create_table(" not in body + assert "op.drop_table(" not in body + assert "op.execute(" not in body + assert "op.bulk_insert(" not in body + assert upgrade.count("Post-generation correction:") == 1 + assert downgrade.count("Post-generation correction:") == 1 diff --git a/tests/unit/v2/test_models.py b/tests/unit/v2/test_models.py index 20947ffcb..17b87ec03 100644 --- a/tests/unit/v2/test_models.py +++ b/tests/unit/v2/test_models.py @@ -130,7 +130,7 @@ def test_user_owned_associations_have_relational_integrity() -> None: "user_simulation_associations", "simulation_associations", ), - (UserPolicy, "user_policies", "policy_associations"), + (UserPolicy, "user_policy_associations", "policy_associations"), (UserReportAssociation, "user_report_associations", "report_associations"), ) @@ -195,7 +195,7 @@ def test_policy_parameter_values_use_jsonb_and_enforce_period_identity() -> None def test_user_policy_is_an_independent_country_scoped_association() -> None: - associations = V2_METADATA.tables["user_policies"] + associations = V2_METADATA.tables["user_policy_associations"] assert isinstance(associations.c.user_id.type, sa.Uuid) user_foreign_key = next(iter(associations.c.user_id.foreign_keys)) @@ -215,7 +215,7 @@ def test_user_policy_is_an_independent_country_scoped_association() -> None: policy_country = next( constraint for constraint in associations.foreign_key_constraints - if constraint.name == "fk_user_policies_policy_country" + if constraint.name == "fk_user_policy_associations_policy_country" ) assert [column.name for column in policy_country.columns] == [ "policy_id", @@ -571,7 +571,7 @@ def test_user_identity_maps_one_issuer_subject_to_one_user() -> None: def test_associations_are_v1_anchored_without_client_refs() -> None: for table_name, reference in ( - ("user_policies", "policy"), + ("user_policy_associations", "policy"), ("user_household_associations", "household"), ("user_simulation_associations", "simulation"), ("user_report_associations", "report"), @@ -610,7 +610,7 @@ def test_simulation_and_report_associations_have_no_cross_service_keys() -> None } == {"users.id"} assert {foreign_key.target_fullname for foreign_key in reports.foreign_keys} == { "users.id", - "user_policies.id", + "user_policy_associations.id", } user_policy_key = next(iter(reports.c.user_policy_id.foreign_keys)) assert user_policy_key.ondelete == "SET NULL" @@ -623,15 +623,56 @@ def test_simulation_and_report_associations_have_no_cross_service_keys() -> None def test_user_policy_records_v3_reform_fields() -> None: - policies = V2_METADATA.tables["user_policies"] + policies = V2_METADATA.tables["user_policy_associations"] assert policies.c.baseline.type.length == 16 baseline_check = next( constraint for constraint in policies.constraints - if constraint.name == "ck_user_policies_baseline" + if constraint.name == "ck_user_policy_associations_baseline" ) assert "current-law" in str(baseline_check.sqltext) assert "current-policy" in str(baseline_check.sqltext) postgres_type = policies.c.provenance.type.dialect_impl(postgresql.dialect()) assert isinstance(postgres_type, postgresql.JSONB) + + +def test_saved_policy_table_is_named_like_the_other_association_tables() -> None: + """The saved-reform table and its constraints follow the association pattern. + + Composite indexes keep their short ``ix_user_policies_*`` names, as the + household, simulation and report associations keep ``ix_user_households_*`` + and the like; constraints and convention-named indexes take the table name. + """ + assert UserPolicy.__tablename__ == "user_policy_associations" + assert "user_policies" not in V2_METADATA.tables + table = V2_METADATA.tables["user_policy_associations"] + + assert {constraint.name for constraint in table.constraints} == { + "pk_user_policy_associations", + "uq_user_policy_associations_id_country", + "ck_user_policy_associations_country", + "ck_user_policy_associations_policy_ref", + "ck_user_policy_associations_baseline", + "fk_user_policy_associations_policy_country", + "fk_user_policy_associations_user_id_users", + } + assert {index.name for index in table.indexes} == { + "ix_user_policy_associations_user_id", + "ix_user_policy_associations_policy_id", + "ix_user_policies_country_user_created_id", + "ix_user_policies_country_policy", + "ix_user_policies_country_legacy_policy", + } + referring_keys = { + (foreign_key.parent.table.name, foreign_key.parent.name) + for referring_table in V2_METADATA.tables.values() + for foreign_key in referring_table.foreign_keys + if foreign_key.column.table is table + } + assert referring_keys == { + ("legacy_user_policy_mappings", "user_policy_id"), + ("legacy_user_policy_mappings", "country_id"), + ("user_report_associations", "user_policy_id"), + ("user_report_associations", "baseline_user_policy_id"), + } diff --git a/tests/unit/v2/test_privilege_manifest.py b/tests/unit/v2/test_privilege_manifest.py index 7a3a76b44..1168d4888 100644 --- a/tests/unit/v2/test_privilege_manifest.py +++ b/tests/unit/v2/test_privilege_manifest.py @@ -133,7 +133,7 @@ def test_rule_proof_runtime_privileges_declared() -> None: reads ``legacy_policy_mappings``, which the runtime role had no grant on. """ runtime_before_3977 = dict.fromkeys(TABLE_ACCESS, NO_ACCESS) | { - "user_policies": READ_WRITE + "user_policy_associations": READ_WRITE } table_by_model = _table_by_model() diff --git a/tests/unit/v2/test_privilege_requirements.py b/tests/unit/v2/test_privilege_requirements.py index 76ba0b5eb..314a85aff 100644 --- a/tests/unit/v2/test_privilege_requirements.py +++ b/tests/unit/v2/test_privilege_requirements.py @@ -20,7 +20,7 @@ TABLE_BY_MODEL = { "User": "users", - "UserPolicy": "user_policies", + "UserPolicy": "user_policy_associations", "LegacyPolicyMapping": "legacy_policy_mappings", } @@ -72,8 +72,8 @@ def create(session, values): ) assert _needed(path) == { - ("user_policies", TablePrivilege.INSERT), - ("user_policies", TablePrivilege.SELECT), + ("user_policy_associations", TablePrivilege.INSERT), + ("user_policy_associations", TablePrivilege.SELECT), } @@ -169,5 +169,5 @@ def read_with_owner(session): """, ) - assert ("user_policies", TablePrivilege.UPDATE) in _needed(path) + assert ("user_policy_associations", TablePrivilege.UPDATE) in _needed(path) assert ("users", TablePrivilege.UPDATE) not in _needed(path) diff --git a/tests/unit/v2/test_user_policy_legacy_persistence.py b/tests/unit/v2/test_user_policy_legacy_persistence.py index e52134e67..a6911a0db 100644 --- a/tests/unit/v2/test_user_policy_legacy_persistence.py +++ b/tests/unit/v2/test_user_policy_legacy_persistence.py @@ -44,13 +44,15 @@ SELECT_MAPPING_FOR_UPDATE = f"{SELECT_MAPPING} FOR UPDATE" MAPPING_LOOKUP = {"country_id_1": "us", "legacy_user_policy_id_1": 10} SELECT_MAPPED_ASSOCIATION = ( - "SELECT user_policies.id, user_policies.created_at, user_policies.updated_at, " - "user_policies.user_id, user_policies.policy_id, " - "user_policies.legacy_policy_id, user_policies.country_id, " - "user_policies.name, user_policies.description, user_policies.baseline, " - "user_policies.provenance \nFROM user_policies \n" - "WHERE user_policies.id = %(id_1)s::UUID AND " - "user_policies.country_id = %(country_id_1)s::VARCHAR" + "SELECT user_policy_associations.id, user_policy_associations.created_at, " + "user_policy_associations.updated_at, user_policy_associations.user_id, " + "user_policy_associations.policy_id, " + "user_policy_associations.legacy_policy_id, " + "user_policy_associations.country_id, user_policy_associations.name, " + "user_policy_associations.description, user_policy_associations.baseline, " + "user_policy_associations.provenance \nFROM user_policy_associations \n" + "WHERE user_policy_associations.id = %(id_1)s::UUID AND " + "user_policy_associations.country_id = %(country_id_1)s::VARCHAR" ) INSERT_MAPPING = ( "INSERT INTO legacy_user_policy_mappings (id, created_at, updated_at, " From 54bef66a19d81508f09daac354985824c1b8d39a Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:54:39 +0400 Subject: [PATCH 3/8] Tighten the saved-policy rename tests - The revision test finds fa7fa1035444 by its ID instead of through the head constant, so a later revision no longer makes it read the wrong file, and it checks the revision's parent through the script directory. - The lifecycle test checks each privilege of the probe role on its own (has_table_privilege with a list is true when any one is held), and checks grants and delete cascades after the downgrade as well as after the upgrade. - The test for the missing user_policies view now stores a saved policy and upgrades to head before reading it under the old name, and only a missing relation counts as the expected failure, so an empty database or a failure while setting up no longer satisfies it. Co-Authored-By: Claude Opus 5.5 --- .../integration/test_alembic_v2_lifecycle.py | 135 +++++++++++++----- tests/unit/v2/test_alembic_v2.py | 17 +-- 2 files changed, 107 insertions(+), 45 deletions(-) diff --git a/tests/integration/test_alembic_v2_lifecycle.py b/tests/integration/test_alembic_v2_lifecycle.py index 694fe0363..6a2b4b129 100644 --- a/tests/integration/test_alembic_v2_lifecycle.py +++ b/tests/integration/test_alembic_v2_lifecycle.py @@ -6,6 +6,7 @@ from datetime import UTC, datetime, timedelta from uuid import UUID, uuid4 +import psycopg import pytest import sqlalchemy as sa from alembic import command @@ -968,26 +969,6 @@ def reflected(name: str) -> sa.Table: engine.dispose() -@pytest.mark.xfail( - strict=True, - raises=sa.exc.ProgrammingError, - reason=( - "Head has no view named user_policies yet. Autogeneration cannot " - "create one, and a hand-written view needs a human migration decision " - "(see the saved-policy table rename in the Alembic migrations skill); " - "until then the release running during a deploy cannot find the table." - ), -) -def test_previous_release_still_finds_user_policies() -> None: - """The release running during the deploy reads saved policies by the old name.""" - engine = create_engine(_disposable_url()) - try: - with engine.connect() as connection: - connection.execute(text("SELECT count(*) FROM user_policies")).scalar_one() - finally: - engine.dispose() - - def _constraint_names(engine, table_name: str) -> set[str]: with engine.connect() as connection: return set( @@ -1201,7 +1182,9 @@ def _saved_policy_references(connection, saved: dict) -> tuple: return (mapped, *report_references) -def _references_after_deleting_saved_policy(engine, saved: dict) -> tuple: +def _references_after_deleting_saved_policy( + engine, table_name: str, saved: dict +) -> tuple: """Delete the saved policy in a rolled-back transaction; return the references. The v1 mapping cascades away and the report's keys are set to null. @@ -1210,14 +1193,38 @@ def _references_after_deleting_saved_policy(engine, saved: dict) -> tuple: transaction = connection.begin() try: connection.execute( - text("DELETE FROM user_policy_associations WHERE id = :id"), - {"id": saved["saved_policy_id"]}, + sa.delete(sa.table(table_name, sa.column("id"))).where( + sa.column("id") == saved["saved_policy_id"] + ) ) return _saved_policy_references(connection, saved) finally: transaction.rollback() +def _probe_privileges(connection, table_name: str) -> dict[str, bool]: + """Return which of three privileges the probe role holds, one at a time. + + ``has_table_privilege`` with a list of privileges is true when any one is + held, so each is checked separately. + """ + return { + privilege: connection.execute( + text("SELECT has_table_privilege(:role, :table, :privilege)"), + { + "role": RENAME_PRIVILEGE_PROBE_ROLE, + "table": f"public.{table_name}", + "privilege": privilege, + }, + ).scalar_one() + for privilege in ("SELECT", "UPDATE", "INSERT") + } + + +GRANTED_TO_PROBE = {"SELECT": True, "UPDATE": True, "INSERT": False} +CLEARED_REFERENCES = (None, None, None) + + def _drop_rename_privilege_probe(engine) -> None: with engine.begin() as connection: connection.execute( @@ -1270,23 +1277,19 @@ def test_saved_policy_rename_keeps_rows_references_and_privileges() -> None: connection, "user_policy_associations", saved_policy_id ) references = _saved_policy_references(connection, saved) - granted = connection.execute( - text( - f"SELECT has_table_privilege('{RENAME_PRIVILEGE_PROBE_ROLE}', " - "'public.user_policy_associations', 'SELECT, UPDATE')" - ) - ).scalar_one() + privileges = _probe_privileges(connection, "user_policy_associations") assert after == before assert references == (saved_policy_id, saved_policy_id, saved_policy_id) - assert granted + assert privileges == GRANTED_TO_PROBE assert ( _schema_names(engine, "user_policy_associations") == SAVED_POLICY_SCHEMA_NAMES_AT_HEAD ) - assert _references_after_deleting_saved_policy(engine, saved) == ( - None, - None, - None, + assert ( + _references_after_deleting_saved_policy( + engine, "user_policy_associations", saved + ) + == CLEARED_REFERENCES ) command.downgrade(config, SAVED_POLICY_RENAME_PARENT_REVISION) @@ -1296,12 +1299,76 @@ def test_saved_policy_rename_keeps_rows_references_and_privileges() -> None: with engine.connect() as connection: restored = _saved_policy_row(connection, "user_policies", saved_policy_id) restored_references = _saved_policy_references(connection, saved) + restored_privileges = _probe_privileges(connection, "user_policies") assert restored == before assert restored_references == references + assert restored_privileges == GRANTED_TO_PROBE assert _schema_names(engine, "user_policies") == names_at_parent + assert ( + _references_after_deleting_saved_policy(engine, "user_policies", saved) + == CLEARED_REFERENCES + ) finally: command.upgrade(config, "head") if saved is not None: _delete_saved_policy_rows(engine, parent, saved) _drop_rename_privilege_probe(engine) engine.dispose() + + +class OldTableNameMissingError(Exception): + """Raised when head has no relation named ``user_policies`` to read from.""" + + +def _saved_policy_row_by_old_name(connection, saved_policy_id: UUID) -> dict: + """Read a saved policy as the previous release does, under the old name. + + Only the missing relation is turned into ``OldTableNameMissingError``, so + any other failure in the test is reported as a failure. + """ + try: + return _saved_policy_row(connection, "user_policies", saved_policy_id) + except sa.exc.ProgrammingError as error: + if isinstance(error.orig, psycopg.errors.UndefinedTable): + raise OldTableNameMissingError from error + raise + + +@pytest.mark.xfail( + strict=True, + raises=OldTableNameMissingError, + reason=( + "Head has no view named user_policies yet. Autogeneration cannot " + "create one, and a hand-written view needs a human migration decision " + "(see the saved-policy table rename in the Alembic migrations skill); " + "until then the release running during a deploy cannot find the table." + ), +) +def test_previous_release_still_finds_user_policies() -> None: + """The release running during the deploy reads saved policies by the old name. + + A saved policy stored before the rename must still be found under + ``user_policies`` at head, as the previous release's statements name it. + """ + config = _config() + engine = create_engine(_disposable_url()) + parent = sa.MetaData() + saved = None + try: + command.downgrade(config, SAVED_POLICY_RENAME_PARENT_REVISION) + parent.reflect(engine) + with engine.begin() as connection: + saved = _saved_policy_with_references(connection, parent) + command.upgrade(config, "head") + with engine.connect() as connection: + stored = _saved_policy_row( + connection, "user_policy_associations", saved["saved_policy_id"] + ) + with engine.connect() as connection: + found = _saved_policy_row_by_old_name(connection, saved["saved_policy_id"]) + assert found == stored + finally: + command.upgrade(config, "head") + if saved is not None: + _delete_saved_policy_rows(engine, parent, saved) + engine.dispose() diff --git a/tests/unit/v2/test_alembic_v2.py b/tests/unit/v2/test_alembic_v2.py index f031d04b3..1be9bc669 100644 --- a/tests/unit/v2/test_alembic_v2.py +++ b/tests/unit/v2/test_alembic_v2.py @@ -54,7 +54,8 @@ f"{PROJECT_REF}:test-password@aws-0-us-east-2.pooler.supabase.com:5432/" "postgres?sslmode=require" ) -# The Stage 12A revision; the saved-policy rename follows it. +# The saved-policy rename and its parent, the Stage 12A revision. +SAVED_POLICY_RENAME_REVISION = "fa7fa1035444" SAVED_POLICY_RENAME_PARENT_REVISION = "fbb7488ebe8a" DISPOSABLE_URL = ( "postgresql+psycopg://postgres:test-password@127.0.0.1:5432/" @@ -743,31 +744,25 @@ def test_v2_environment_commits_after_persistent_target_qualification() -> None: assert "validate_v2_head_schema(connection, target_metadata)" in env_source -def _revision_source(revision_id: str) -> str: - (path,) = (REPO / "migrations/v2/versions").glob(f"{revision_id}_*.py") - return path.read_text(encoding="utf-8") - - def test_saved_policy_rename_revision_renames_the_table_in_place() -> None: """The rename keeps every saved reform: no table is created or dropped. Autogeneration replaces a renamed table with a new empty one; the revision records its corrections, and every other operation is generated. """ - revision = _revision_source(V2_ALEMBIC_HEAD_REVISION) + script = ScriptDirectory.from_config(Config(str(REPO / "alembic-v2.ini"))) + rename = script.get_revision(SAVED_POLICY_RENAME_REVISION) + revision = Path(rename.path).read_text(encoding="utf-8") upgrade = revision[ revision.index("def upgrade()") : revision.index("def downgrade()") ] downgrade = revision[revision.index("def downgrade()") :] - assert V2_ALEMBIC_HEAD_REVISION != SAVED_POLICY_RENAME_PARENT_REVISION + assert rename.down_revision == SAVED_POLICY_RENAME_PARENT_REVISION assert ( "Generation: uv run alembic -c alembic-v2.ini revision --autogenerate" in revision ) - assert ( - f'down_revision: Union[str, None] = "{SAVED_POLICY_RENAME_PARENT_REVISION}"' - ) in revision assert 'op.rename_table("user_policies", "user_policy_associations")' in upgrade assert 'op.rename_table("user_policy_associations", "user_policies")' in downgrade for body in (upgrade, downgrade): From c747f002ebd24c72df0112795e803800c9729aa4 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:54:39 +0400 Subject: [PATCH 4/8] Describe the saved-policy rename in the Stage 12A document The merge table and the rule for Stage 10 mirror rows name the table by its current name. The rollback and schema compatibility sections no longer say head lets earlier releases run: until a view under the old name exists, a release from before the rename fails every statement on saved policies, from the schema upgrade until the new release takes traffic, and after a rollback until the rename is downgraded. They also say that downgrading the rename alone keeps every row, and that the Stage 12A compatibility test now maps the renamed table and no longer proves the parent release runs against head. Co-Authored-By: Claude Opus 5.5 --- docs/migration/stage-12a-user-login.md | 72 ++++++++++++++++++-------- 1 file changed, 51 insertions(+), 21 deletions(-) diff --git a/docs/migration/stage-12a-user-login.md b/docs/migration/stage-12a-user-login.md index 2831a4a60..cd4251781 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -126,7 +126,7 @@ unauthenticated: a standard user's row is never locked. | State | Outcome | What happens | | --- | --- | --- | | The identity has no user yet; the browser's unauthenticated user exists | `converted` | The `user_identities` row (issuer, subject, verified email) is inserted pointing at the browser's `users.id`; the user becomes `standard` with `primary_country = country_id`. The ID is unchanged, so every association stays owned. | -| The identity already belongs to standard user S; the unauthenticated user exists | `merged` | Every row of `user_policies`, `user_household_associations`, `user_simulation_associations`, and `user_report_associations` owned by the unauthenticated user moves to S with its ID, contents, and timestamps unchanged; then the unauthenticated user is deleted. | +| The identity already belongs to standard user S; the unauthenticated user exists | `merged` | Every row of `user_policy_associations`, `user_household_associations`, `user_simulation_associations`, and `user_report_associations` owned by the unauthenticated user moves to S with its ID, contents, and timestamps unchanged; then the unauthenticated user is deleted. | | No unauthenticated user with that ID | `none` | The identity's user is resolved, or provisioned, as on any request. | | The ID names a standard user (already converted, or anyone else's) | `none` | That user is never touched; the identity's user is resolved as on any request. | @@ -194,8 +194,8 @@ Concurrency on Postgres: `legacy_household_id`; with `legacy_household_id`, both are stored, and it must be the copy of that v1 household when one exists. - `user-simulations` and `user-reports` accept only legacy IDs in 12A. -- **Stage 10 mirror rows.** A `user_policies` row with a - `legacy_user_policy_mappings` entry returns 409 on PATCH and DELETE. The +- **Stage 10 mirror rows.** A saved-policy row (`user_policy_associations`) + with a `legacy_user_policy_mappings` entry returns 409 on PATCH and DELETE. The mirror projection now also records `legacy_policy_id`. - **Request bounds.** Every `/v2/me*` and `/v2/user-*` write rejects a body over 256 KiB with 413. FastAPI reads and parses the whole JSON body before @@ -331,6 +331,9 @@ Revision `fa7fa1035444` later renamed `user_policies` to indexes, matching the other association tables; the composite listing indexes keep their `ix_user_policies_*` names. The route `/v2/user-policies`, the `user_policy_id` columns and `legacy_user_policy_mappings` are unchanged. +The bullet points above describe revision `fbb7488ebe8a`, when the table was +still named `user_policies`. "Schema compatibility" below says what the +rename means for earlier Public API releases. ### Reshape safety @@ -353,15 +356,22 @@ and production and confirm both dormant tables are empty. ### Rollback -Roll forward whenever possible. The schema lets the previous Public API -revision run (see "Schema compatibility" below), but that revision serves -`/v2/user-policies` and `/v2/user-households` without authentication: any -caller can list a user's saved items by `user_id`, read, rename or delete one -by record ID, and create rows for any user. Rolling the application back -would therefore hand users' saved work to anyone. +Roll forward whenever possible. Since revision `fa7fa1035444` (see +"Schema compatibility" below), no Public API release from before that +revision can read or write saved policies at head: they name the table +`user_policies`, which no longer exists. Every statement they make on it +fails, including `/v2/user-policies`, user-report reads that join saved +policies, the sign-in merge and the Stage 10 saved-policy copy. Rolling the +application back to such a release therefore also needs revision +`fa7fa1035444` downgraded, which keeps every row (see below). The release +before Stage 12A also serves `/v2/user-policies` and `/v2/user-households` +without authentication: any caller can list a user's saved items by +`user_id`, read, rename or delete one by record ID, and create rows for any +user. Rolling the application back would therefore hand users' saved work to +anyone. - **Before any user-owned record exists, an application rollback is safe - without a database downgrade.** Signed-out saving needs no configuration, so + without a Stage 12A database downgrade.** Signed-out saving needs no configuration, so treat user-owned records as existing from the moment this revision serves traffic in an environment. - **Once any user-owned record exists, signed in or not, an application @@ -395,17 +405,35 @@ would therefore hand users' saved work to anyone. `/v2/user-policies` and `/v2/user-households` responses require a non-null `policy_id` / `household_id`, so any response that includes a legacy-only row (a v1 reference with no v2 copy) fails with 500. -- **A database downgrade is lossy.** It restores NOT NULL on `policy_id`, - `household_id`, `simulation_id`, `report_id`, `country`, and - `users.primary_country`, so it fails while legacy-only rows, any simulation - or report associations, or any unauthenticated user exist. Removing them - first deletes users' saved work. +- **Downgrading the rename alone is not lossy.** Downgrading revision + `fa7fa1035444` renames `user_policy_associations` back to `user_policies` + with every row, reference and grant, so releases from before the rename + can use saved policies again. +- **A database downgrade of Stage 12A is lossy.** Downgrading revision + `fbb7488ebe8a` restores NOT NULL on `policy_id`, `household_id`, + `simulation_id`, `report_id`, `country`, and `users.primary_country`, so it + fails while legacy-only rows, any simulation or report associations, or any + unauthenticated user exist. Removing them first deletes users' saved work. ### Schema compatibility -The revision is additive (expand-only) for everything the previous Public API -revision maps: it drops no table or column, and it adds no required column -without a server default to any table that revision writes. The only new +**Since revision `fa7fa1035444`, head does not serve earlier releases.** That +revision renames `user_policies` to `user_policy_associations` without the +temporary view under the old name that the rename was planned with: a view is +not autogenerated, so it waits for a human migration decision (see the +saved-policy table rename in `docs/engineering/skills/alembic-migrations.md`). +Until a view exists, any release from before the rename fails every statement +on saved policies. During a deploy this lasts from the schema upgrade until +the new release takes traffic; after an application rollback it lasts until a +release from the rename or later serves traffic again, or until revision +`fa7fa1035444` is downgraded. +`test_previous_release_still_finds_user_policies` records the gap as a strict +expected failure. + +The rest of this section describes revision `fbb7488ebe8a` alone. It was +additive (expand-only) for everything the previous Public API revision maps: +it drops no table or column, and it adds no required column without a server +default to any table that revision writes. The only new required columns are `country_id` on the simulation and report associations, which that revision maps but never inserts into, and `users.user_type`, whose server default makes every user the previous revision creates (its Stage 10 @@ -418,9 +446,11 @@ 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 head. Since the rename, it +uses the parent's `user_policies` under its head name (`RENAMED_AT_HEAD`), so +it checks the parent's columns and constraints but no longer proves that the +parent release runs against head. 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); From d31f13df213495e7e0f0eb76a1c595f8866c7172 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:49:39 +0400 Subject: [PATCH 5/8] Let the privilege reconciliation find, grant and verify views The reconciliation read only ordinary and partitioned tables when it checked which declared relations exist, so a view declared in the privilege manifest counted as missing and blocked every grant. It now reads views too. GRANT ... ON TABLE and has_table_privilege already work on views, and the granted-privilege query already read them. Co-Authored-By: Claude Opus 5.5 --- .../data/v2/privilege_reconciliation.py | 9 +++-- .../test_v2_privilege_reconciliation.py | 36 ++++++++++++++++++- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/policyengine_api/data/v2/privilege_reconciliation.py b/policyengine_api/data/v2/privilege_reconciliation.py index d9d0cacec..bb5d673ab 100644 --- a/policyengine_api/data/v2/privilege_reconciliation.py +++ b/policyengine_api/data/v2/privilege_reconciliation.py @@ -276,7 +276,8 @@ def render_summary( FROM pg_class AS table_class JOIN pg_namespace AS table_namespace ON table_namespace.oid = table_class.relnamespace - WHERE table_namespace.nspname = :schema AND table_class.relkind IN ('r', 'p') + WHERE table_namespace.nspname = :schema + AND table_class.relkind IN ('r', 'p', 'v') """ ) @@ -312,7 +313,11 @@ def read_existing_roles(connection: Connection, roles: Collection[str]) -> set[s def read_public_tables(connection: Connection) -> set[str]: - """Return every table in the public schema.""" + """Return every table and view in the public schema. + + Views count because the manifest declares privileges on temporary views as + it does on tables, and ``GRANT ... ON TABLE`` grants on a view too. + """ return set(connection.execute(_PUBLIC_TABLES, {"schema": SCHEMA}).scalars()) diff --git a/tests/integration/test_v2_privilege_reconciliation.py b/tests/integration/test_v2_privilege_reconciliation.py index f720c3fb9..38db72d94 100644 --- a/tests/integration/test_v2_privilege_reconciliation.py +++ b/tests/integration/test_v2_privilege_reconciliation.py @@ -23,7 +23,7 @@ check_declared_privileges, run, ) -from policyengine_api.data.v2.privileges import READ, TablePrivilege +from policyengine_api.data.v2.privileges import READ, READ_WRITE, TablePrivilege from policyengine_api.data.v2.settings import V2_MIGRATION_DATABASE_URL if TYPE_CHECKING: @@ -62,6 +62,21 @@ def create_role() -> str: connection.execute(text(f'DROP ROLE "{role}"')) +@pytest.fixture +def temporary_view(superuser_engine) -> Iterator[str]: + """Create a view over ``regions`` for one test, then drop it.""" + view = f"policyengine_privilege_test_view_{uuid4().hex[:12]}" + with superuser_engine.begin() as connection: + connection.execute( + text(f'CREATE VIEW public."{view}" AS SELECT * FROM public.regions') + ) + try: + yield view + finally: + with superuser_engine.begin() as connection: + connection.execute(text(f'DROP VIEW public."{view}"')) + + def _has_privilege(engine: Engine, role: str, table: str, privilege: str) -> bool: with engine.connect() as connection: return bool( @@ -137,3 +152,22 @@ def test_a_missing_role_blocks_every_grant_and_fails_verification( PrivilegeEntry(role, "regions", "SELECT"), ] ) + + +def test_apply_grants_and_verify_checks_privileges_on_a_declared_view( + superuser_engine, temporary_role, temporary_view +) -> None: + """A view declared like a table is found, granted and verified.""" + role = temporary_role() + declared = {role: {temporary_view: READ_WRITE}} + with superuser_engine.begin() as connection: + plan, _defaults = apply_declared_privileges(connection, declared) + missing = check_declared_privileges(connection, declared) + + assert plan.missing_tables == () + assert {entry.privilege for entry in plan.grants} == { + privilege.value for privilege in READ_WRITE + } + assert missing == [] + for privilege in READ_WRITE: + assert _has_privilege(superuser_engine, role, temporary_view, privilege.value) From 035af26e0a96790fd3fcf305a36240cef399a5e3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:00:44 +0400 Subject: [PATCH 6/8] Keep a temporary user_policies view for the release before the rename Revision fa7fa1035444 now creates a view named user_policies with every column of user_policy_associations after the rename, and drops it before the downgrade renames the table back. These two statements are the only hand-written SQL in the v2 chain: a maintainer approved them on 2026-10-22 as an exception to the generated-only rule, and a later revision drops the view. The revision comments and the Alembic skill say so, and the revision test allows exactly these two statements. A single-table view is automatically updatable, so the Public API release still serving during the deploy, or after a rollback to it, keeps reading and writing saved policies through it. The privilege manifest declares the view with its table's privileges, so the reconciliation grants the runtime role SELECT, INSERT, UPDATE and DELETE on it. Autogeneration does not reflect views, so alembic check and the head schema validation stay clean. Tests: the lifecycle test checks the view's columns after the upgrade and its absence after the downgrade, runs the parent revision's inserts, reads, updates and deletes through it, and reads a saved policy stored before the rename under the old name. A new integration test runs the saved-policy routes' services, the sign-in merge, report creation and the Stage 10 saved-policy copy as the runtime role with every statement naming user_policies. The Stage 12A document and the changelog describe the view. Co-Authored-By: Claude Opus 5.5 --- ...lan-1f-122-rename-user-policies.changed.md | 2 +- docs/engineering/skills/alembic-migrations.md | 30 +- docs/migration/stage-12a-user-login.md | 54 ++- ...44_rename_user_policies_to_user_policy_.py | 14 + policyengine_api/data/v2/privileges.py | 34 +- .../integration/test_alembic_v2_lifecycle.py | 138 ++++--- .../integration/test_v2_saved_policy_view.py | 382 ++++++++++++++++++ tests/unit/v2/test_alembic_v2.py | 45 ++- tests/unit/v2/test_privilege_manifest.py | 24 +- 9 files changed, 630 insertions(+), 93 deletions(-) create mode 100644 tests/integration/test_v2_saved_policy_view.py diff --git a/changelog.d/v2plan-1f-122-rename-user-policies.changed.md b/changelog.d/v2plan-1f-122-rename-user-policies.changed.md index 1141ffd75..9c9a77ad8 100644 --- a/changelog.d/v2plan-1f-122-rename-user-policies.changed.md +++ b/changelog.d/v2plan-1f-122-rename-user-policies.changed.md @@ -1 +1 @@ -The v2 table user_policies is renamed to user_policy_associations, matching the other user-association tables; its constraints and convention-named indexes take the new name. Saved policies, their v1 mappings and the reports that name them are kept. The route /v2/user-policies, the user_policy_id columns, legacy_user_policy_mappings and the v1 Cloud SQL table user_policies are unchanged. +The v2 table user_policies is renamed to user_policy_associations, matching the other user-association tables; its constraints and convention-named indexes take the new name. Saved policies, their v1 mappings and the reports that name them are kept. A temporary view named user_policies, with the runtime role's privileges, lets the release from before the rename keep reading and writing saved policies during the deploy and after a rollback; a later change removes it. The privilege reconciliation now finds, grants and verifies privileges on views. The route /v2/user-policies, the user_policy_id columns, legacy_user_policy_mappings and the v1 Cloud SQL table user_policies are unchanged. diff --git a/docs/engineering/skills/alembic-migrations.md b/docs/engineering/skills/alembic-migrations.md index 3b7a13f36..a9f6ce77b 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -315,9 +315,27 @@ report that names it, and checks that the rows, the references, the cascades, a table grant and every constraint and index name survive the upgrade and the downgrade. -The plan also asks for a temporary view named `user_policies` over the renamed -table, so the release still running during the deploy keeps working. A view -is not autogenerated, and adding one would be a hand-written schema operation, -so the revision leaves it out pending a human migration decision. -`test_previous_release_still_finds_user_policies` records the gap as a strict -expected failure. +The revision also holds the only approved hand-written exception in the v2 +chain, approved by a maintainer on 2026-10-22: after the rename it creates a +view named `user_policies` with every column of `user_policy_associations`, +and the downgrade drops the view before renaming the table back. The Public +API release still serving during a deploy, or after a rollback to it, names +the table `user_policies`; a single-table view is automatically updatable, so +that release keeps reading and writing saved policies through it. The +privilege manifest declares the view with the table's privileges +(`TEMPORARY_VIEW_TABLES` in `policyengine_api/data/v2/privileges.py`), and the +privilege reconciliation grants and verifies privileges on views as on tables. +Autogeneration does not reflect views, so `alembic check` and the head schema +validation ignore it. A later revision drops the view once no deployable +release names `user_policies`; until then, a revision cannot drop or change +the type of a `user_policy_associations` column the view shows without +dropping the view first. + +Tests: `test_alembic_v2.py` allows exactly these two statements and no other +`op.execute` in any v2 revision. The lifecycle test checks the view's columns +after the upgrade and its absence after the downgrade, and runs the previous +revision's inserts, reads, updates and deletes through it. +`test_v2_saved_policy_view.py` runs today's saved-policy services, the +sign-in merge, report creation and the Stage 10 saved-policy copy as the +runtime role with the table named `user_policies`, so every statement goes +through the view. diff --git a/docs/migration/stage-12a-user-login.md b/docs/migration/stage-12a-user-login.md index cd4251781..ed952e189 100644 --- a/docs/migration/stage-12a-user-login.md +++ b/docs/migration/stage-12a-user-login.md @@ -357,14 +357,11 @@ and production and confirm both dormant tables are empty. ### Rollback Roll forward whenever possible. Since revision `fa7fa1035444` (see -"Schema compatibility" below), no Public API release from before that -revision can read or write saved policies at head: they name the table -`user_policies`, which no longer exists. Every statement they make on it -fails, including `/v2/user-policies`, user-report reads that join saved -policies, the sign-in merge and the Stage 10 saved-policy copy. Rolling the -application back to such a release therefore also needs revision -`fa7fa1035444` downgraded, which keeps every row (see below). The release -before Stage 12A also serves `/v2/user-policies` and `/v2/user-households` +"Schema compatibility" below), releases from before that revision use saved +policies through the temporary view `user_policies` until a later revision +drops it; after that, rolling the application back to such a release also +needs revision `fa7fa1035444` downgraded, which keeps every row (see below). +The release before Stage 12A also serves `/v2/user-policies` and `/v2/user-households` without authentication: any caller can list a user's saved items by `user_id`, read, rename or delete one by record ID, and create rows for any user. Rolling the application back would therefore hand users' saved work to @@ -406,9 +403,9 @@ anyone. `policy_id` / `household_id`, so any response that includes a legacy-only row (a v1 reference with no v2 copy) fails with 500. - **Downgrading the rename alone is not lossy.** Downgrading revision - `fa7fa1035444` renames `user_policy_associations` back to `user_policies` - with every row, reference and grant, so releases from before the rename - can use saved policies again. + `fa7fa1035444` drops the temporary view and renames + `user_policy_associations` back to `user_policies` with every row, + reference and table grant. - **A database downgrade of Stage 12A is lossy.** Downgrading revision `fbb7488ebe8a` restores NOT NULL on `policy_id`, `household_id`, `simulation_id`, `report_id`, `country`, and `users.primary_country`, so it @@ -417,18 +414,19 @@ anyone. ### Schema compatibility -**Since revision `fa7fa1035444`, head does not serve earlier releases.** That -revision renames `user_policies` to `user_policy_associations` without the -temporary view under the old name that the rename was planned with: a view is -not autogenerated, so it waits for a human migration decision (see the -saved-policy table rename in `docs/engineering/skills/alembic-migrations.md`). -Until a view exists, any release from before the rename fails every statement -on saved policies. During a deploy this lasts from the schema upgrade until -the new release takes traffic; after an application rollback it lasts until a -release from the rename or later serves traffic again, or until revision -`fa7fa1035444` is downgraded. -`test_previous_release_still_finds_user_policies` records the gap as a strict -expected failure. +**Since revision `fa7fa1035444`, earlier releases use saved policies through a +temporary view.** That revision renames `user_policies` to +`user_policy_associations` and creates a view named `user_policies` with every +column of the renamed table, the approved hand-written exception described in +`docs/engineering/skills/alembic-migrations.md` ("API v2 saved-policy table +rename"). A single-table view is automatically updatable, and the runtime role +holds on it what it holds on the table, so a release from before the rename +keeps reading and writing saved policies, during a deploy and after a +rollback: its `/v2/user-policies` routes, report creation that checks the saved +policies a report names, the sign-in merge, and the Stage 10 saved-policy +copy. `test_v2_saved_policy_view.py` runs those operations as the runtime role +through the view. Once a later revision drops the view, such a release fails +every statement on saved policies. The rest of this section describes revision `fbb7488ebe8a` alone. It was additive (expand-only) for everything the previous Public API revision maps: @@ -446,11 +444,11 @@ 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. Since the rename, it -uses the parent's `user_policies` under its head name (`RENAMED_AT_HEAD`), so -it checks the parent's columns and constraints but no longer proves that the -parent release runs against 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 head. Since the rename, +the parent's `user_policies` is head's temporary view of that name, so the +parent's statements on saved policies run through the view. 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); diff --git a/migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py b/migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py index d2aa619c5..d9dea42da 100644 --- a/migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py +++ b/migrations/v2/versions/fa7fa1035444_rename_user_policies_to_user_policy_.py @@ -149,11 +149,25 @@ def upgrade() -> None: ["id"], ondelete="SET NULL", ) + # Approved hand-written exception: a temporary view under the table's + # earlier name, so the Public API release still serving during the deploy, + # or after a rollback to it, keeps reading and writing saved policies. A + # maintainer approved this hand-written statement on 2026-10-22, and a later + # revision drops the view. A single-table view is automatically updatable; + # the privilege manifest grants on it what it grants on the table. + op.execute( + "CREATE VIEW user_policies AS SELECT id, created_at, updated_at, user_id, " + "policy_id, country_id, name, description, legacy_policy_id, baseline, " + "provenance FROM user_policy_associations" + ) # ### end Alembic commands ### def downgrade() -> None: # ### commands auto generated by Alembic - please adjust! ### + # Approved hand-written exception: the upgrade's temporary view is dropped + # first, so the table can take its earlier name back. + op.execute("DROP VIEW user_policies") op.drop_constraint( op.f( "fk_user_report_associations_baseline_user_policy_id_user_policy_associations" diff --git a/policyengine_api/data/v2/privileges.py b/policyengine_api/data/v2/privileges.py index dc965e36e..5014230f0 100644 --- a/policyengine_api/data/v2/privileges.py +++ b/policyengine_api/data/v2/privileges.py @@ -10,7 +10,9 @@ ``api.v2.runtime-privileges-tested-as-runtime-role``). Every table in the v2 SQLModel metadata, plus ``alembic_version``, has an entry -for every managed role, so "no access" is a decision, not an omission. +for every managed role, so "no access" is a decision, not an omission. A +temporary view under a renamed table's earlier name (``TEMPORARY_VIEW_TABLES``) +is declared with its table's privileges. Roles and what decides their privileges: @@ -145,6 +147,17 @@ def _access( } +# Temporary views under a renamed table's earlier name, each mapped to the +# table it shows. The Public API release that still serves during a deploy, or +# after a rollback to it, reads and writes the table through the view, so every +# role holds on the view what it holds on the table. A later revision drops +# each view, and its entry here goes with it. +TEMPORARY_VIEW_TABLES: Final[dict[str, str]] = { + # Revision fa7fa1035444 renamed user_policies to user_policy_associations. + "user_policies": "user_policy_associations", +} + + class InvalidDeploymentEnvironmentError(ValueError): """Raised for a deployment environment that can't name a Stage 12 role.""" @@ -167,12 +180,23 @@ def stage12_role(deployment_environment: str) -> str: def declared_privileges( deployment_environment: str, ) -> dict[str, dict[str, frozenset[TablePrivilege]]]: - """Return each managed role's declared privileges, keyed by role then table.""" + """Return each managed role's declared privileges, keyed by role then table. + + Each temporary view is declared with its table's privileges. + """ stage12 = stage12_role(deployment_environment) + access_by_relation = TABLE_ACCESS | { + view: TABLE_ACCESS[table] for view, table in TEMPORARY_VIEW_TABLES.items() + } return { - RUNTIME_ROLE: {table: access.runtime for table, access in TABLE_ACCESS.items()}, + RUNTIME_ROLE: { + relation: access.runtime for relation, access in access_by_relation.items() + }, CATALOG_WRITER_ROLE: { - table: access.catalog_writer for table, access in TABLE_ACCESS.items() + relation: access.catalog_writer + for relation, access in access_by_relation.items() + }, + stage12: { + relation: access.stage12 for relation, access in access_by_relation.items() }, - stage12: {table: access.stage12 for table, access in TABLE_ACCESS.items()}, } diff --git a/tests/integration/test_alembic_v2_lifecycle.py b/tests/integration/test_alembic_v2_lifecycle.py index 6a2b4b129..5ef7c6169 100644 --- a/tests/integration/test_alembic_v2_lifecycle.py +++ b/tests/integration/test_alembic_v2_lifecycle.py @@ -6,7 +6,6 @@ from datetime import UTC, datetime, timedelta from uuid import UUID, uuid4 -import psycopg import pytest import sqlalchemy as sa from alembic import command @@ -22,6 +21,7 @@ V2_ALEMBIC_HEAD_REVISION, V2MigrationTargetError, load_v2_alembic_settings, + validate_v2_head_schema, ) from policyengine_api.data.v2.models import V2_METADATA from policyengine_api.data.v2.settings import V2_MIGRATION_DATABASE_URL @@ -410,11 +410,6 @@ def test_empty_upgrade_check_base_downgrade_and_reupgrade() -> None: "user_simulation_associations", "user_report_associations", ) -# Tables head renamed, keyed by their name at the parent revision. The -# previous release's statements name the old table, which head does not have -# until a view under that name exists (test_previous_release_still_finds_ -# user_policies), so its column usage runs against the renamed table here. -RENAMED_AT_HEAD = {"user_policies": "user_policy_associations"} COUNTRY_COLUMN_NAMES = frozenset({"country", "country_id", "primary_country"}) # Nullable references that a parent-revision check constraint requires. REFERENCES_REQUIRED_BY_CHECKS = frozenset({("simulations", "household_id")}) @@ -594,7 +589,7 @@ def _assert_user_deletion_cascades( assert result.rowcount == 1 for table_name in (*USER_ASSOCIATION_TABLES, "user_identities"): - table = head.tables[RENAMED_AT_HEAD.get(table_name, table_name)] + table = head.tables[table_name] remaining = connection.execute( sa.select(sa.func.count()) .select_from(table) @@ -611,8 +606,9 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: 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. A table head renamed is - used under its new name (``RENAMED_AT_HEAD``). + new required column without a server default. Since the saved-policy + rename, the parent's ``user_policies`` is head's temporary view of that + name, so the parent's statements on it run through the view. """ database_url = _disposable_url() config = _config() @@ -622,11 +618,9 @@ def test_stage_12a_schema_serves_the_previous_revision() -> None: command.downgrade(config, STAGE_12A_PREVIOUS_REVISION) parent = sa.MetaData() parent.reflect(engine) - for parent_name, head_name in RENAMED_AT_HEAD.items(): - parent.tables[parent_name].name = head_name command.upgrade(config, "head") head = sa.MetaData() - head.reflect(engine) + head.reflect(engine, views=True) parent_tables = [ table for table in parent.sorted_tables if table.name != "alembic_version" ] @@ -1225,6 +1219,31 @@ def _probe_privileges(connection, table_name: str) -> dict[str, bool]: CLEARED_REFERENCES = (None, None, None) +def _view_names(engine) -> set[str]: + return set(inspect(engine).get_view_names(schema="public")) + + +def _column_names(engine, relation_name: str) -> list[str]: + return [ + column["name"] + for column in inspect(engine).get_columns(relation_name, schema="public") + ] + + +def _assert_view_under_old_name_at_head(engine, config) -> None: + """Head keeps a view named ``user_policies`` with the table's columns. + + Neither the head schema validation nor ``alembic check`` reports it. + """ + assert _view_names(engine) == {"user_policies"} + assert _column_names(engine, "user_policies") == _column_names( + engine, "user_policy_associations" + ) + with engine.connect() as connection: + validate_v2_head_schema(connection, V2_METADATA) + command.check(config) + + def _drop_rename_privilege_probe(engine) -> None: with engine.begin() as connection: connection.execute( @@ -1242,8 +1261,10 @@ def test_saved_policy_rename_keeps_rows_references_and_privileges() -> None: Saved policies, their v1 mappings and the reports that name them survive both directions, the references and a table grant follow the table, and - the constraints and indexes named after the table take its name. The - downgrade restores every name the parent revision had. + the constraints and indexes named after the table take its name. Head has + a view named ``user_policies`` with the table's columns, which neither the + schema validation nor ``alembic check`` reports. The downgrade drops the + view and restores every name the parent revision had. """ config = _config() engine = create_engine(_disposable_url()) @@ -1272,6 +1293,7 @@ def test_saved_policy_rename_keeps_rows_references_and_privileges() -> None: table_names = set(inspect(engine).get_table_names(schema="public")) assert "user_policies" not in table_names + _assert_view_under_old_name_at_head(engine, config) with engine.connect() as connection: after = _saved_policy_row( connection, "user_policy_associations", saved_policy_id @@ -1296,6 +1318,8 @@ def test_saved_policy_rename_keeps_rows_references_and_privileges() -> None: table_names = set(inspect(engine).get_table_names(schema="public")) assert "user_policy_associations" not in table_names + assert "user_policies" in table_names + assert _view_names(engine) == set() with engine.connect() as connection: restored = _saved_policy_row(connection, "user_policies", saved_policy_id) restored_references = _saved_policy_references(connection, saved) @@ -1316,39 +1340,13 @@ def test_saved_policy_rename_keeps_rows_references_and_privileges() -> None: engine.dispose() -class OldTableNameMissingError(Exception): - """Raised when head has no relation named ``user_policies`` to read from.""" - - -def _saved_policy_row_by_old_name(connection, saved_policy_id: UUID) -> dict: - """Read a saved policy as the previous release does, under the old name. - - Only the missing relation is turned into ``OldTableNameMissingError``, so - any other failure in the test is reported as a failure. - """ - try: - return _saved_policy_row(connection, "user_policies", saved_policy_id) - except sa.exc.ProgrammingError as error: - if isinstance(error.orig, psycopg.errors.UndefinedTable): - raise OldTableNameMissingError from error - raise - - -@pytest.mark.xfail( - strict=True, - raises=OldTableNameMissingError, - reason=( - "Head has no view named user_policies yet. Autogeneration cannot " - "create one, and a hand-written view needs a human migration decision " - "(see the saved-policy table rename in the Alembic migrations skill); " - "until then the release running during a deploy cannot find the table." - ), -) def test_previous_release_still_finds_user_policies() -> None: - """The release running during the deploy reads saved policies by the old name. + """The release running during the deploy uses saved policies by the old name. - A saved policy stored before the rename must still be found under - ``user_policies`` at head, as the previous release's statements name it. + A saved policy stored before the rename is found under ``user_policies`` + at head, and a row written, changed and deleted through the view is + written, changed and deleted in ``user_policy_associations``. The runtime + role's own use of the view is in ``test_v2_saved_policy_view``. """ config = _config() engine = create_engine(_disposable_url()) @@ -1360,13 +1358,57 @@ def test_previous_release_still_finds_user_policies() -> None: with engine.begin() as connection: saved = _saved_policy_with_references(connection, parent) command.upgrade(config, "head") + saved_policy_id = saved["saved_policy_id"] with engine.connect() as connection: stored = _saved_policy_row( - connection, "user_policy_associations", saved["saved_policy_id"] + connection, "user_policy_associations", saved_policy_id ) - with engine.connect() as connection: - found = _saved_policy_row_by_old_name(connection, saved["saved_policy_id"]) + found = _saved_policy_row(connection, "user_policies", saved_policy_id) assert found == stored + + written_id = uuid4() + with engine.connect() as connection: + transaction = connection.begin() + try: + connection.execute( + text( + "INSERT INTO user_policies " + "(id, user_id, policy_id, country_id, name) " + "VALUES (:id, :user_id, :policy_id, :country_id, 'Written')" + ), + { + "id": written_id, + "user_id": stored["user_id"], + "policy_id": stored["policy_id"], + "country_id": stored["country_id"], + }, + ) + written = _saved_policy_row( + connection, "user_policy_associations", written_id + ) + connection.execute( + text("UPDATE user_policies SET name = 'Changed' WHERE id = :id"), + {"id": written_id}, + ) + changed_name = _saved_policy_row( + connection, "user_policy_associations", written_id + )["name"] + connection.execute( + text("DELETE FROM user_policies WHERE id = :id"), + {"id": written_id}, + ) + remaining = connection.execute( + text( + "SELECT count(*) FROM user_policy_associations WHERE id = :id" + ), + {"id": written_id}, + ).scalar_one() + finally: + transaction.rollback() + assert written["name"] == "Written" + assert written["created_at"] is not None + assert changed_name == "Changed" + assert remaining == 0 finally: command.upgrade(config, "head") if saved is not None: diff --git a/tests/integration/test_v2_saved_policy_view.py b/tests/integration/test_v2_saved_policy_view.py new file mode 100644 index 000000000..7ffb666d5 --- /dev/null +++ b/tests/integration/test_v2_saved_policy_view.py @@ -0,0 +1,382 @@ +"""The release before the saved-policy rename keeps working through the view. + +Revision ``fa7fa1035444`` renames ``user_policies`` to +``user_policy_associations`` and keeps a temporary view under the old name. The +Public API release that still serves during the deploy, or after a rollback to +it, runs the same statements as today's services with the old table name. +These tests run today's services as the runtime role, rewrite each statement's +table name to ``user_policies`` before it is sent, as that release would +compile it, and check the result in the renamed table. +""" + +from __future__ import annotations + +import re +from typing import TYPE_CHECKING +from uuid import UUID, uuid4 + +import pytest +from sqlalchemy import create_engine, delete, event, select, text +from sqlalchemy.orm import sessionmaker +from sqlmodel import Session + +from policyengine_api.constants import COUNTRY_PACKAGE_VERSIONS, POLICYENGINE_VERSION +from policyengine_api.data.v2.models import ( + LegacyPolicyMapping, + LegacyUserMapping, + Parameter, + ParameterValue, + Policy, + TaxBenefitModel, + TaxBenefitModelVersion, + User, + UserIdentity, +) +from policyengine_api.services.v2.identity.types import ( + UnauthenticatedPrincipal, + UserPrincipal, +) +from policyengine_api.services.v2.policies.types import LegacyPolicySnapshot +from policyengine_api.services.v2.user_accounts.database_session import ( + AccountDatabaseSession, +) +from policyengine_api.services.v2.user_accounts.services import V2AccountService +from policyengine_api.services.v2.user_policies.database_session import ( + UserPolicyDatabaseSession, +) +from policyengine_api.services.v2.user_policies.services import ( + V2UserPolicyService, + mirror_legacy_user_policy_in_session, +) +from policyengine_api.services.v2.user_policies.types import ( + LegacyUserPolicySnapshot, + UserPolicyCreateFields, + UserPolicyUpdateInput, +) +from policyengine_api.services.v2.user_reports.database_session import ( + UserReportDatabaseSession, +) +from policyengine_api.services.v2.user_reports.services import V2UserReportService +from policyengine_api.services.v2.user_reports.types import UserReportCreateFields +from tests.unit.v2.user_service_support import default_legacy_references + +if TYPE_CHECKING: + from collections.abc import Iterator + + from sqlalchemy import Engine + +TABLE_BEFORE_THE_RENAME = "user_policies" +TABLE_AFTER_THE_RENAME = "user_policy_associations" +# The renamed table where a statement names it as a table, not inside a label +# such as ``user_policy_associations_id``. +RENAMED_TABLE_REFERENCE = re.compile(rf"\b{TABLE_AFTER_THE_RENAME}\b") +MIRRORED_LEGACY_USER_ID = "auth0|saved-policy-view" + + +@pytest.fixture +def cleanup_engine(disposable_v2_database_url) -> Iterator[Engine]: + engine = create_engine(disposable_v2_database_url) + try: + yield engine + finally: + engine.dispose() + + +@pytest.fixture +def previous_release_statements() -> list[str]: + """Every statement on saved policies, as sent with the earlier table name.""" + return [] + + +@pytest.fixture +def sessions( + managed_v2_roles, v2_engine_as_role, cleanup_engine, previous_release_statements +) -> Iterator[sessionmaker[Session]]: + """Open sessions as the runtime role that name saved policies as before. + + Each statement that names ``user_policy_associations`` is sent naming + ``user_policies`` instead. Results are matched to columns by position, so + the rewritten labels do not matter. + """ + runtime_engine = v2_engine_as_role(managed_v2_roles.runtime) + + @event.listens_for(runtime_engine, "before_cursor_execute", retval=True) + def name_saved_policies_as_before_the_rename( + _connection, _cursor, statement, parameters, _context, _executemany + ): + if RENAMED_TABLE_REFERENCE.search(statement): + statement = RENAMED_TABLE_REFERENCE.sub(TABLE_BEFORE_THE_RENAME, statement) + previous_release_statements.append(statement) + return statement, parameters + + factory = sessionmaker(runtime_engine, class_=Session, expire_on_commit=False) + issuer = f"https://saved-policy-view-{uuid4().hex[:12]}.example.test/" + factory.kw["info"] = {"issuer": issuer, "browser_user_ids": []} + try: + yield factory + finally: + with cleanup_engine.begin() as connection: + owned = select(UserIdentity.user_id).where(UserIdentity.issuer == issuer) + user_ids = [ + *connection.execute(owned).scalars(), + *factory.kw["info"]["browser_user_ids"], + ] + if user_ids: + connection.execute(delete(User).where(User.id.in_(user_ids))) + runtime_engine.dispose() + + +def _browser(sessions) -> UnauthenticatedPrincipal: + user_id = uuid4() + sessions.kw["info"]["browser_user_ids"].append(user_id) + return UnauthenticatedPrincipal(user_id=user_id) + + +def _saved_policy_row(cleanup_engine, association_id: UUID) -> dict | None: + with cleanup_engine.connect() as connection: + row = ( + connection.execute( + text(f"SELECT * FROM {TABLE_AFTER_THE_RENAME} WHERE id = :id"), + {"id": association_id}, + ) + .mappings() + .one_or_none() + ) + return None if row is None else dict(row) + + +def _statement_kinds(statements: list[str]) -> set[str]: + """Return the kind of each statement: its first word.""" + return {statement.split(maxsplit=1)[0].upper() for statement in statements} + + +def test_runtime_role_holds_its_saved_policy_privileges_on_the_view( + sessions, managed_v2_roles +) -> None: + with sessions() as session: + held = { + privilege: session.execute( + text("SELECT has_table_privilege(:role, :view, :privilege)"), + { + "role": managed_v2_roles.runtime, + "view": f"public.{TABLE_BEFORE_THE_RENAME}", + "privilege": privilege, + }, + ).scalar_one() + for privilege in ("SELECT", "INSERT", "UPDATE", "DELETE") + } + + assert held == dict.fromkeys(("SELECT", "INSERT", "UPDATE", "DELETE"), True) + + +def test_previous_release_saves_reads_renames_and_deletes_through_the_view( + sessions, cleanup_engine, previous_release_statements +) -> None: + browser = _browser(sessions) + policies = V2UserPolicyService( + UserPolicyDatabaseSession(sessions), default_legacy_references() + ) + + created = policies.create_user_policy( + principal=browser, + association_input=UserPolicyCreateFields( + country_id="us", legacy_policy_id=1, name="Saved" + ), + ) + stored_after_create = _saved_policy_row(cleanup_engine, created.id) + read = policies.get_user_policy( + principal=browser, country_id="us", association_id=created.id + ) + page = policies.list_user_policies(principal=browser, country_id="us") + renamed = policies.patch_user_policy( + principal=browser, + country_id="us", + association_id=created.id, + association_input=UserPolicyUpdateInput(name="Renamed"), + ) + stored_after_rename = _saved_policy_row(cleanup_engine, created.id) + policies.delete_user_policy( + principal=browser, country_id="us", association_id=created.id + ) + + assert stored_after_create is not None + assert stored_after_create["user_id"] == browser.user_id + assert stored_after_create["created_at"] == created.created_at + assert read.id == created.id + assert [association.id for association in page.items] == [created.id] + assert renamed.name == "Renamed" + assert stored_after_rename is not None + assert stored_after_rename["name"] == "Renamed" + assert _saved_policy_row(cleanup_engine, created.id) is None + assert _statement_kinds(previous_release_statements) == { + "INSERT", + "SELECT", + "UPDATE", + "DELETE", + } + + +def test_previous_release_merges_saved_policies_and_checks_reports_through_the_view( + sessions, cleanup_engine, previous_release_statements +) -> None: + """Sign-in moves a browser's saved policies, and reports name them.""" + browser = _browser(sessions) + signed_in = UserPrincipal( + issuer=sessions.kw["info"]["issuer"], subject="google-oauth2|view-merge" + ) + references = default_legacy_references() + policies = V2UserPolicyService(UserPolicyDatabaseSession(sessions), references) + reports = V2UserReportService(UserReportDatabaseSession(sessions), references) + accounts = V2AccountService(AccountDatabaseSession(sessions)) + + account_id = accounts.get_account(principal=signed_in, country_id="us").id + saved = policies.create_user_policy( + principal=browser, + association_input=UserPolicyCreateFields(country_id="us", legacy_policy_id=1), + ) + report = reports.create_user_report( + principal=browser, + association_input=UserReportCreateFields( + country_id="us", + legacy_report_id=615, + user_policy_id=saved.id, + baseline_user_policy_id=saved.id, + ), + ) + accounts.upgrade_account( + principal=signed_in, + unauthenticated_user_id=browser.user_id, + country_id="us", + ) + exported = accounts.export_account(principal=signed_in, country_id="us") + + stored = _saved_policy_row(cleanup_engine, saved.id) + assert stored is not None + assert stored["user_id"] == account_id + assert report.user_policy_id == saved.id + assert [policy.id for policy in exported.policies] == [saved.id] + # The sign-in merge moves every saved policy of the browser's user. + assert any( + re.match( + rf"UPDATE {TABLE_BEFORE_THE_RENAME} SET .*\buser_id=.* WHERE " + rf"{TABLE_BEFORE_THE_RENAME}\.user_id = ", + statement, + ) + for statement in previous_release_statements + ) + + +def _seed_catalog(cleanup_engine) -> tuple[UUID, str]: + parameter_name = f"gov.saved_policy_view.rate_{uuid4().hex[:8]}" + with Session(cleanup_engine) as session, session.begin(): + # The policy copy selects the catalog by this model name. + model = TaxBenefitModel(name="policyengine-us") + version = TaxBenefitModelVersion( + model=model, + version=POLICYENGINE_VERSION, + current_law_id=1, + metadata_time_periods=[2026], + ) + session.add(Parameter(name=parameter_name, tax_benefit_model_version=version)) + session.flush() + return model.id, parameter_name + + +def _delete_mirrored_rows(cleanup_engine, model_id: UUID) -> None: + with cleanup_engine.begin() as connection: + user_ids = select(LegacyUserMapping.user_id).where( + LegacyUserMapping.legacy_user_id == MIRRORED_LEGACY_USER_ID + ) + mirrored_user_ids = connection.execute(user_ids).scalars().all() + connection.execute( + delete(LegacyUserMapping).where( + LegacyUserMapping.legacy_user_id == MIRRORED_LEGACY_USER_ID + ) + ) + # Deleting the user deletes its saved policies and their v1 mappings. + connection.execute(delete(User).where(User.id.in_(mirrored_user_ids))) + policy_ids = select(Policy.id).where(Policy.tax_benefit_model_id == model_id) + for model in (LegacyPolicyMapping, ParameterValue): + connection.execute(delete(model).where(model.policy_id.in_(policy_ids))) + connection.execute( + delete(Policy).where(Policy.tax_benefit_model_id == model_id) + ) + version_ids = select(TaxBenefitModelVersion.id).where( + TaxBenefitModelVersion.model_id == model_id + ) + connection.execute( + delete(Parameter).where( + Parameter.tax_benefit_model_version_id.in_(version_ids) + ) + ) + connection.execute( + delete(TaxBenefitModelVersion).where( + TaxBenefitModelVersion.model_id == model_id + ) + ) + connection.execute( + delete(TaxBenefitModel).where(TaxBenefitModel.id == model_id) + ) + + +def _legacy_saved_policy(reform_label: str) -> LegacyUserPolicySnapshot: + return LegacyUserPolicySnapshot.model_validate( + { + "country_id": "us", + "legacy_user_policy_id": 901, + "reform_id": 801, + "reform_label": reform_label, + "baseline_id": 1, + "baseline_label": "Current law", + "user_id": MIRRORED_LEGACY_USER_ID, + "year": "2026", + "geography": "us", + "dataset": "enhanced_cps_2024", + "number_of_provisions": 1, + "api_version": "1.0.0", + "added_date": 1, + "updated_date": 2, + "budgetary_impact": None, + "type": None, + } + ) + + +def test_previous_release_copies_v1_saved_policies_through_the_view( + sessions, cleanup_engine, previous_release_statements +) -> None: + """The Stage 10 saved-policy copy inserts and then updates through the view.""" + model_id, parameter_name = _seed_catalog(cleanup_engine) + reform = LegacyPolicySnapshot( + country_id="us", + legacy_policy_id=801, + label="Ignored legacy label", + api_version=COUNTRY_PACKAGE_VERSIONS["us"], + policy_json={parameter_name: {"2026": 0.2}}, + source_policy_hash=f"view-{uuid4().hex}", + ) + try: + with sessions.begin() as session: + first = mirror_legacy_user_policy_in_session( + session, _legacy_saved_policy("Copied"), reform, source_revision=1 + ) + with sessions.begin() as session: + second = mirror_legacy_user_policy_in_session( + session, + _legacy_saved_policy("Copied again"), + reform, + source_revision=2, + changed_fields=frozenset({"reform_label"}), + ) + stored = _saved_policy_row(cleanup_engine, first.association_id) + finally: + _delete_mirrored_rows(cleanup_engine, model_id) + + assert first.association_created is True + assert second.association_id == first.association_id + assert second.association_created is False + assert stored is not None + assert stored["name"] == "Copied again" + assert {"INSERT", "SELECT", "UPDATE"} <= _statement_kinds( + previous_release_statements + ) diff --git a/tests/unit/v2/test_alembic_v2.py b/tests/unit/v2/test_alembic_v2.py index 1be9bc669..94d79f577 100644 --- a/tests/unit/v2/test_alembic_v2.py +++ b/tests/unit/v2/test_alembic_v2.py @@ -57,6 +57,15 @@ # The saved-policy rename and its parent, the Stage 12A revision. SAVED_POLICY_RENAME_REVISION = "fa7fa1035444" SAVED_POLICY_RENAME_PARENT_REVISION = "fbb7488ebe8a" +# The only hand-written statements in any v2 revision: the approved temporary +# view under the saved-policy table's earlier name, and its removal on +# downgrade. +SAVED_POLICY_VIEW_CREATION = ( + "CREATE VIEW user_policies AS SELECT id, created_at, updated_at, user_id, " + "policy_id, country_id, name, description, legacy_policy_id, baseline, " + "provenance FROM user_policy_associations" +) +SAVED_POLICY_VIEW_REMOVAL = "DROP VIEW user_policies" DISPOSABLE_URL = ( "postgresql+psycopg://postgres:test-password@127.0.0.1:5432/" f"{DISPOSABLE_DATABASE_NAME}" @@ -744,11 +753,21 @@ def test_v2_environment_commits_after_persistent_target_qualification() -> None: assert "validate_v2_head_schema(connection, target_metadata)" in env_source +def _executed_statements(body: str) -> list[str]: + """Return the SQL each ``op.execute`` call in a revision body runs.""" + return [ + "".join(re.findall(r'"([^"]*)"', call)) + for call in re.findall(r"op\.execute\((.*?)\)\n", body, flags=re.DOTALL) + ] + + def test_saved_policy_rename_revision_renames_the_table_in_place() -> None: """The rename keeps every saved reform: no table is created or dropped. Autogeneration replaces a renamed table with a new empty one; the revision - records its corrections, and every other operation is generated. + records its corrections, and every other operation is generated except the + approved temporary view under the old name, created after the rename and + dropped before the downgrade renames the table back. """ script = ScriptDirectory.from_config(Config(str(REPO / "alembic-v2.ini"))) rename = script.get_revision(SAVED_POLICY_RENAME_REVISION) @@ -757,18 +776,36 @@ def test_saved_policy_rename_revision_renames_the_table_in_place() -> None: revision.index("def upgrade()") : revision.index("def downgrade()") ] downgrade = revision[revision.index("def downgrade()") :] + rename_up = 'op.rename_table("user_policies", "user_policy_associations")' + rename_down = 'op.rename_table("user_policy_associations", "user_policies")' assert rename.down_revision == SAVED_POLICY_RENAME_PARENT_REVISION assert ( "Generation: uv run alembic -c alembic-v2.ini revision --autogenerate" in revision ) - assert 'op.rename_table("user_policies", "user_policy_associations")' in upgrade - assert 'op.rename_table("user_policy_associations", "user_policies")' in downgrade + assert rename_up in upgrade + assert rename_down in downgrade for body in (upgrade, downgrade): assert "op.create_table(" not in body assert "op.drop_table(" not in body - assert "op.execute(" not in body assert "op.bulk_insert(" not in body + assert body.count("Approved hand-written exception:") == 1 + assert _executed_statements(upgrade) == [SAVED_POLICY_VIEW_CREATION] + assert _executed_statements(downgrade) == [SAVED_POLICY_VIEW_REMOVAL] + assert upgrade.index(rename_up) < upgrade.index("op.execute(") + assert downgrade.index("op.execute(") < downgrade.index(rename_down) assert upgrade.count("Post-generation correction:") == 1 assert downgrade.count("Post-generation correction:") == 1 + + +def test_no_other_v2_revision_runs_hand_written_sql() -> None: + """Only the saved-policy rename's approved view uses ``op.execute``.""" + script = ScriptDirectory.from_config(Config(str(REPO / "alembic-v2.ini"))) + revisions_with_sql = { + revision.revision + for revision in script.walk_revisions() + if "op.execute(" in Path(revision.path).read_text(encoding="utf-8") + } + + assert revisions_with_sql == {SAVED_POLICY_RENAME_REVISION} diff --git a/tests/unit/v2/test_privilege_manifest.py b/tests/unit/v2/test_privilege_manifest.py index 1168d4888..380d9a81e 100644 --- a/tests/unit/v2/test_privilege_manifest.py +++ b/tests/unit/v2/test_privilege_manifest.py @@ -22,6 +22,7 @@ READ_WRITE, RUNTIME_ROLE, TABLE_ACCESS, + TEMPORARY_VIEW_TABLES, InvalidDeploymentEnvironmentError, TablePrivilege, declared_privileges, @@ -72,7 +73,28 @@ def test_every_v2_table_has_an_entry_for_every_managed_role() -> None: assert set(TABLE_ACCESS) == metadata_tables | {"alembic_version"} for role, privileges_by_table in declared_privileges(TEST_ENVIRONMENT).items(): - assert set(privileges_by_table) == set(TABLE_ACCESS), role + assert set(privileges_by_table) == set(TABLE_ACCESS) | set( + TEMPORARY_VIEW_TABLES + ), role + + +def test_each_temporary_view_grants_every_role_what_its_table_grants() -> None: + """A view under a renamed table's earlier name serves the earlier release. + + That release reads and writes the table through the view, so each role + needs on the view exactly what it holds on the table. + """ + metadata_tables = {table.name for table in V2_METADATA.tables.values()} + + assert TEMPORARY_VIEW_TABLES == {"user_policies": "user_policy_associations"} + for view, table in TEMPORARY_VIEW_TABLES.items(): + assert view not in metadata_tables + assert table in metadata_tables + for role, privileges_by_table in declared_privileges(TEST_ENVIRONMENT).items(): + assert privileges_by_table[view] == privileges_by_table[table], role + assert declared_privileges(TEST_ENVIRONMENT)[RUNTIME_ROLE]["user_policies"] == ( + READ_WRITE + ) def test_v2_schema_has_no_sequences_the_manifest_would_need_to_cover() -> None: From eb4c29c1db174f650bc49ec733ed5e9b374e39aa Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:56:01 +0400 Subject: [PATCH 7/8] Run the saved-policy view tests in the cross-database integration workflow The workflow runs an explicit list of integration modules, and test_v2_saved_policy_view.py was not in it. It now runs in the step for signed-in identity provisioning and user associations, and rule api.v2.runtime-privileges-tested-as-runtime-role names it, since it runs the saved-policy services as policyengine_v2_runtime. A unit test fails when an integration module that uses the managed v2 roles is missing from the workflow, or one that runs services as a managed role is missing from the rule's scope. No environment variable is added. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/v2-integration-check.yml | 2 +- docs/engineering/skills/alembic-migrations.md | 1 + rules/registry.toml | 2 +- tests/unit/test_alembic_workflows.py | 36 +++++++++++++++++++ 4 files changed, 39 insertions(+), 2 deletions(-) diff --git a/.github/workflows/v2-integration-check.yml b/.github/workflows/v2-integration-check.yml index 3f402ae80..7959a83c2 100644 --- a/.github/workflows/v2-integration-check.yml +++ b/.github/workflows/v2-integration-check.yml @@ -82,7 +82,7 @@ jobs: - name: Test v2 policy persistence and immediate v1 mirroring run: uv run coverage run -a --branch -m pytest -q tests/integration/test_v2_policy_persistence.py tests/integration/test_v1_policy_dual_write.py tests/integration/test_mysql_policy_dual_write.py tests/integration/test_v2_user_policy_mirroring.py tests/integration/test_v1_user_policy_dual_write.py - name: Test signed-in identity provisioning and user associations - run: uv run coverage run -a --branch -m pytest -q tests/integration/test_v2_user_identity_persistence.py + run: uv run coverage run -a --branch -m pytest -q tests/integration/test_v2_user_identity_persistence.py tests/integration/test_v2_saved_policy_view.py - name: Test comparison-run persistence and privilege reconciliation as the managed roles run: uv run coverage run -a --branch -m pytest -q tests/integration/test_v2_stage12_comparison_persistence.py tests/integration/test_v2_privilege_reconciliation.py - name: Test v2 household persistence and immediate v1 copying diff --git a/docs/engineering/skills/alembic-migrations.md b/docs/engineering/skills/alembic-migrations.md index a9f6ce77b..f656486ab 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -179,6 +179,7 @@ When a pull request adds or changes a v2 table, or a connector's use of one: uv run python scripts/reconcile_v2_privileges.py apply --create-missing-roles uv run python scripts/reconcile_v2_privileges.py verify uv run pytest -q tests/integration/test_v2_user_identity_persistence.py \ + tests/integration/test_v2_saved_policy_view.py \ tests/integration/test_v2_stage12_comparison_persistence.py \ tests/integration/test_v2_privilege_reconciliation.py ``` diff --git a/rules/registry.toml b/rules/registry.toml index 66b86bb8d..24faf8334 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -671,7 +671,7 @@ id = "api.v2.runtime-privileges-tested-as-runtime-role" category = "migration" statement = "The cross-database integration tests run the v2 user services as policyengine_v2_runtime and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI." rationale = "docs/engineering/skills/alembic-migrations.md#table-privileges" -scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_stage12_comparison_persistence.py"] +scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_saved_policy_view.py", "tests/integration/test_v2_stage12_comparison_persistence.py"] tier = "mechanical" status = "enforced" autofix = false diff --git a/tests/unit/test_alembic_workflows.py b/tests/unit/test_alembic_workflows.py index 797c82848..a0e85c6c5 100644 --- a/tests/unit/test_alembic_workflows.py +++ b/tests/unit/test_alembic_workflows.py @@ -3,6 +3,7 @@ import os import re import subprocess +import tomllib from pathlib import Path import pytest @@ -257,6 +258,41 @@ def test_v2_integration_check_grants_and_verifies_privileges_before_tests(): assert schema_upgrade < grant < verify < first_test +def _integration_modules_using(pattern: str) -> list[str]: + return sorted( + path.relative_to(REPO).as_posix() + for path in (REPO / "tests" / "integration").glob("test_*.py") + if re.search(pattern, path.read_text(encoding="utf-8")) + ) + + +def test_every_managed_role_integration_module_runs_in_ci_and_is_in_its_rule(): + """Integration modules that use the managed v2 roles run in the workflow. + + Those that run services as a managed role (``v2_engine_as_role``) are also + in the scope of rule ``api.v2.runtime-privileges-tested-as-runtime-role``. + """ + workflow = _workflow("v2-integration-check.yml") + rules = tomllib.loads((REPO / "rules" / "registry.toml").read_text()) + (runtime_role_rule,) = ( + rule + for rule in rules["rule"] + if rule["id"] == "api.v2.runtime-privileges-tested-as-runtime-role" + ) + managed_role_modules = _integration_modules_using( + r"\b(managed_v2_roles|v2_engine_as_role)\b" + ) + role_session_modules = _integration_modules_using(r"\bv2_engine_as_role\b") + + assert "tests/integration/test_v2_saved_policy_view.py" in role_session_modules + assert [module for module in managed_role_modules if module not in workflow] == [] + assert [ + module + for module in role_session_modules + if module not in runtime_role_rule["scope"] + ] == [] + + def test_release_migration_fails_closed_before_tests_and_cloud_run_deploy(): workflow = _workflow("push.yml") orchestration_script = ( From c0794eb2399d847a1fc640d06ac2fc8b9cdb8ad3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:00:05 +0400 Subject: [PATCH 8/8] Require each declared relation to exist with its declared kind The reconciliation read tables and views as one set of names, so a view satisfied a table entry of the same name and a table satisfied a temporary-view entry: before the rename, the user_policies table satisfied the user_policies view's entry. It now reads tables and views separately. A declared table must exist as a table and a TEMPORARY_VIEW_TABLES entry as a view; anything else is reported under "Declared tables missing" or "Declared views missing", nothing is granted, and verify reports every privilege declared on it as missing. Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/alembic-migrations.md | 6 +- .../data/v2/privilege_reconciliation.py | 98 ++++++++++++++----- tests/integration/conftest.py | 4 +- .../test_v2_privilege_reconciliation.py | 40 +++++++- .../unit/v2/test_privilege_reconciliation.py | 80 ++++++++++++++- 5 files changed, 192 insertions(+), 36 deletions(-) diff --git a/docs/engineering/skills/alembic-migrations.md b/docs/engineering/skills/alembic-migrations.md index f656486ab..093358e22 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -155,8 +155,10 @@ check the declarations: `apply` grants each declared privilege a role lacks. It never revokes: it reports privileges the file doesn't declare, and the projects' default -privileges, in the job summary. It grants nothing when a declared role or -table is missing. The roles themselves are provisioned outside the +privileges, in the job summary. It grants nothing when a declared role, +table or view is missing; a declared table must exist as a table and a +temporary view (`TEMPORARY_VIEW_TABLES`) as a view, so a relation of the other +kind counts as missing. The roles themselves are provisioned outside the repository; `--create-missing-roles` is accepted only for the disposable test database. diff --git a/policyengine_api/data/v2/privilege_reconciliation.py b/policyengine_api/data/v2/privilege_reconciliation.py index bb5d673ab..33af3896b 100644 --- a/policyengine_api/data/v2/privilege_reconciliation.py +++ b/policyengine_api/data/v2/privilege_reconciliation.py @@ -6,8 +6,10 @@ - ``apply`` reads the managed roles' current grants, grants every declared privilege a role lacks, and reports privileges the manifest doesn't declare. - It never revokes, and it changes nothing when a declared role or table is - missing. Running it again grants nothing more. + It never revokes, and it changes nothing when a declared role, table or view + is missing. A declared table must exist as a table and a declared temporary + view as a view; a relation of the other kind counts as missing. Running it + again grants nothing more. - ``verify`` checks every declared privilege with ``has_table_privilege`` (rule ``api.v2.deployed-privileges-match``) and fails on any that is missing. @@ -42,6 +44,7 @@ ) from policyengine_api.data.v2.privileges import ( PERSISTENT_ENVIRONMENTS, + TEMPORARY_VIEW_TABLES, TablePrivilege, declared_privileges, ) @@ -56,6 +59,8 @@ GITHUB_STEP_SUMMARY = "GITHUB_STEP_SUMMARY" SCHEMA = "public" SAFE_IDENTIFIER = re.compile(r"^[a-z_][a-z0-9_]{0,62}$") +# The manifest's temporary views; every other declared name is a table. +DECLARED_VIEWS: frozenset[str] = frozenset(TEMPORARY_VIEW_TABLES) class PrivilegeReconciliationError(RuntimeError): @@ -94,11 +99,20 @@ class ReconciliationPlan: undeclared: tuple[PrivilegeEntry, ...] missing_roles: tuple[str, ...] missing_tables: tuple[str, ...] + missing_views: tuple[str, ...] = () @property def can_apply(self) -> bool: - """Whether every declared role and table exists, so grants may run.""" - return not self.missing_roles and not self.missing_tables + """Whether every declared role, table and view exists, so grants may run.""" + return not (self.missing_roles or self.missing_tables or self.missing_views) + + +@dataclass(frozen=True) +class PublicRelations: + """The tables and views in the public schema, by name.""" + + tables: frozenset[str] + views: frozenset[str] def plan_reconciliation( @@ -106,21 +120,30 @@ def plan_reconciliation( granted: Mapping[str, Mapping[str, Collection[str]]], *, existing_roles: Collection[str], - existing_tables: Collection[str], + existing_relations: PublicRelations, + declared_views: Collection[str] = (), ) -> ReconciliationPlan: """Compare declared privileges with what the database grants. ``granted`` holds the managed roles' direct grants, by role then table. + Names in ``declared_views`` must exist as views, and every other declared + name as a table. """ missing_roles = tuple(sorted(set(declared) - set(existing_roles))) - declared_tables = {table for tables in declared.values() for table in tables} - missing_tables = tuple(sorted(declared_tables - set(existing_tables))) + declared_names = {table for tables in declared.values() for table in tables} + missing_views = tuple( + sorted((declared_names & set(declared_views)) - existing_relations.views) + ) + missing_tables = tuple( + sorted(declared_names - set(declared_views) - existing_relations.tables) + ) + missing_relations = {*missing_tables, *missing_views} grants = sorted( PrivilegeEntry(role, table, privilege.value) for role, tables in declared.items() if role not in missing_roles for table, privileges in tables.items() - if table not in missing_tables + if table not in missing_relations for privilege in privileges if privilege.value not in granted.get(role, {}).get(table, ()) ) @@ -140,6 +163,7 @@ def plan_reconciliation( undeclared=tuple(undeclared), missing_roles=missing_roles, missing_tables=missing_tables, + missing_views=missing_views, ) @@ -214,6 +238,12 @@ def render_summary( + ", ".join(f"`{table}`" for table in plan.missing_tables), "", ] + if plan.missing_views: + lines += [ + "Declared views missing from the database (nothing was granted): " + + ", ".join(f"`{view}`" for view in plan.missing_views), + "", + ] grant_heading = "Granted" if applied else "Would grant" lines += [f"### {grant_heading}", ""] if plan.grants: @@ -270,16 +300,17 @@ def render_summary( "SELECT rolname FROM pg_roles WHERE rolname IN :roles" ).bindparams(bindparam("roles", expanding=True)) -_PUBLIC_TABLES = text( +_PUBLIC_RELATIONS = text( """ - SELECT table_class.relname - FROM pg_class AS table_class - JOIN pg_namespace AS table_namespace - ON table_namespace.oid = table_class.relnamespace - WHERE table_namespace.nspname = :schema - AND table_class.relkind IN ('r', 'p', 'v') + SELECT relation_class.relname, relation_class.relkind + FROM pg_class AS relation_class + JOIN pg_namespace AS relation_namespace + ON relation_namespace.oid = relation_class.relnamespace + WHERE relation_namespace.nspname = :schema + AND relation_class.relkind IN ('r', 'p', 'v') """ ) +_TABLE_KINDS = frozenset({"r", "p"}) _DEFAULT_PRIVILEGES = text( """ @@ -312,13 +343,17 @@ def read_existing_roles(connection: Connection, roles: Collection[str]) -> set[s return set(connection.execute(_EXISTING_ROLES, {"roles": list(roles)}).scalars()) -def read_public_tables(connection: Connection) -> set[str]: - """Return every table and view in the public schema. +def read_public_relations(connection: Connection) -> PublicRelations: + """Return the tables and the views in the public schema. - Views count because the manifest declares privileges on temporary views as - it does on tables, and ``GRANT ... ON TABLE`` grants on a view too. + The manifest declares privileges on temporary views as it does on tables, + and ``GRANT ... ON TABLE`` grants on a view too. """ - return set(connection.execute(_PUBLIC_TABLES, {"schema": SCHEMA}).scalars()) + rows = connection.execute(_PUBLIC_RELATIONS, {"schema": SCHEMA}).all() + return PublicRelations( + tables=frozenset(name for name, kind in rows if kind in _TABLE_KINDS), + views=frozenset(name for name, kind in rows if kind == "v"), + ) def read_granted_privileges( @@ -368,17 +403,22 @@ def create_missing_roles(connection: Connection, roles: Iterable[str]) -> list[s def apply_declared_privileges( connection: Connection, declared: Mapping[str, Mapping[str, frozenset[TablePrivilege]]], + *, + declared_views: Collection[str] = DECLARED_VIEWS, ) -> tuple[ReconciliationPlan, list[DefaultPrivilege]]: """Grant every declared privilege a role lacks; never revoke. - Grants nothing when a declared role or table is missing. + Grants nothing when a declared role, table or view is missing, including a + declared view that exists as a table or a declared table that exists as a + view. """ roles = list(declared) plan = plan_reconciliation( declared, read_granted_privileges(connection, roles), existing_roles=read_existing_roles(connection, roles), - existing_tables=read_public_tables(connection), + existing_relations=read_public_relations(connection), + declared_views=declared_views, ) if plan.can_apply: for statement in grant_statements( @@ -391,16 +431,24 @@ def apply_declared_privileges( def check_declared_privileges( connection: Connection, declared: Mapping[str, Mapping[str, frozenset[TablePrivilege]]], + *, + declared_views: Collection[str] = DECLARED_VIEWS, ) -> list[PrivilegeEntry]: """Return every declared privilege a role doesn't effectively hold. - A missing role or table counts as missing every privilege declared for it. + A missing role, table or view counts as missing every privilege declared + for it; so does a declared view that exists as a table, or the reverse. """ existing_roles = read_existing_roles(connection, list(declared)) - existing_tables = read_public_tables(connection) + relations = read_public_relations(connection) + + def exists_as_declared(name: str) -> bool: + if name in declared_views: + return name in relations.views + return name in relations.tables def has_privilege(role: str, table: str, privilege: str) -> bool: - if role not in existing_roles or table not in existing_tables: + if role not in existing_roles or not exists_as_declared(table): return False return bool( connection.execute( diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index b90efebd2..eb855ebb2 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -105,8 +105,8 @@ def managed_v2_roles(disposable_v2_database_url: str) -> ManagedV2Roles: engine.dispose() if not plan.can_apply: pytest.fail( - "the disposable v2 database lacks declared roles or tables: " - f"{plan.missing_roles} {plan.missing_tables}" + "the disposable v2 database lacks declared roles, tables or views: " + f"{plan.missing_roles} {plan.missing_tables} {plan.missing_views}" ) return ManagedV2Roles( deployment_environment=deployment_environment, diff --git a/tests/integration/test_v2_privilege_reconciliation.py b/tests/integration/test_v2_privilege_reconciliation.py index 38db72d94..9596a5966 100644 --- a/tests/integration/test_v2_privilege_reconciliation.py +++ b/tests/integration/test_v2_privilege_reconciliation.py @@ -161,13 +161,47 @@ def test_apply_grants_and_verify_checks_privileges_on_a_declared_view( role = temporary_role() declared = {role: {temporary_view: READ_WRITE}} with superuser_engine.begin() as connection: - plan, _defaults = apply_declared_privileges(connection, declared) - missing = check_declared_privileges(connection, declared) + plan, _defaults = apply_declared_privileges( + connection, declared, declared_views={temporary_view} + ) + missing = check_declared_privileges( + connection, declared, declared_views={temporary_view} + ) - assert plan.missing_tables == () + assert plan.missing_tables == plan.missing_views == () assert {entry.privilege for entry in plan.grants} == { privilege.value for privilege in READ_WRITE } assert missing == [] for privilege in READ_WRITE: assert _has_privilege(superuser_engine, role, temporary_view, privilege.value) + + +def test_a_view_declared_as_a_table_or_a_table_declared_as_a_view_blocks_grants( + superuser_engine, temporary_role, temporary_view +) -> None: + """Each declared name must exist with its declared kind. + + ``regions`` is a table, declared here as a view, and the temporary view is + declared as a table: neither is granted, and both fail verification. + """ + role = temporary_role() + declared = {role: {"regions": READ, temporary_view: READ}} + with superuser_engine.begin() as connection: + plan, _defaults = apply_declared_privileges( + connection, declared, declared_views={"regions"} + ) + missing = check_declared_privileges( + connection, declared, declared_views={"regions"} + ) + + assert plan.missing_views == ("regions",) + assert plan.missing_tables == (temporary_view,) + assert not plan.can_apply + assert not _has_privilege(superuser_engine, role, "regions", "SELECT") + assert missing == sorted( + [ + PrivilegeEntry(role, "regions", "SELECT"), + PrivilegeEntry(role, temporary_view, "SELECT"), + ] + ) diff --git a/tests/unit/v2/test_privilege_reconciliation.py b/tests/unit/v2/test_privilege_reconciliation.py index 73ca30e19..a6d9590dc 100644 --- a/tests/unit/v2/test_privilege_reconciliation.py +++ b/tests/unit/v2/test_privilege_reconciliation.py @@ -11,6 +11,7 @@ from policyengine_api.data.v2.privilege_reconciliation import ( DefaultPrivilege, PrivilegeEntry, + PublicRelations, ReconciliationPlan, grant_statements, plan_reconciliation, @@ -48,12 +49,16 @@ def _declared() -> dict[str, dict[str, frozenset[TablePrivilege]]]: } +def _tables(names: set[str]) -> PublicRelations: + return PublicRelations(tables=frozenset(names), views=frozenset()) + + def test_missing_declared_privileges_become_grants() -> None: plan = plan_reconciliation( _declared(), granted={RUNTIME: {"users": {"SELECT", "INSERT"}}}, existing_roles={RUNTIME, STAGE12}, - existing_tables={"users", "legacy_policy_mappings", "reports"}, + existing_relations=_tables({"users", "legacy_policy_mappings", "reports"}), ) assert plan.grants == ( @@ -78,7 +83,7 @@ def test_privileges_beyond_the_manifest_are_reported_not_granted_or_revoked() -> STAGE12: {"users": {"SELECT"}}, }, existing_roles={RUNTIME, STAGE12}, - existing_tables={"users", "legacy_policy_mappings", "reports"}, + existing_relations=_tables({"users", "legacy_policy_mappings", "reports"}), ) assert plan.grants == () @@ -96,7 +101,9 @@ def test_privileges_on_tables_outside_the_manifest_are_reported() -> None: _declared(), granted={RUNTIME: {"scratch_table": {"SELECT"}}}, existing_roles={RUNTIME, STAGE12}, - existing_tables={"users", "legacy_policy_mappings", "reports", "scratch_table"}, + existing_relations=_tables( + {"users", "legacy_policy_mappings", "reports", "scratch_table"} + ), ) assert PrivilegeEntry(RUNTIME, "scratch_table", "SELECT") in plan.undeclared @@ -107,7 +114,7 @@ def test_missing_roles_and_tables_are_recorded_and_block_the_plan() -> None: _declared(), granted={}, existing_roles={RUNTIME}, - existing_tables={"users", "reports"}, + existing_relations=_tables({"users", "reports"}), ) assert plan.missing_roles == (STAGE12,) @@ -120,6 +127,71 @@ def test_missing_roles_and_tables_are_recorded_and_block_the_plan() -> None: ) +def test_a_relation_of_the_other_kind_does_not_satisfy_a_declaration() -> None: + """A declared table must be a table, and a declared view must be a view. + + Before the rename, ``user_policies`` is a table, which must not satisfy the + temporary view of that name; a view named like a declared table must not + satisfy the table either. + """ + declared = { + RUNTIME: {"users": READ_WRITE, "user_policies": READ_WRITE, "reports": READ} + } + + plan = plan_reconciliation( + declared, + granted={}, + existing_roles={RUNTIME}, + existing_relations=PublicRelations( + tables=frozenset({"user_policies", "reports"}), views=frozenset({"users"}) + ), + declared_views={"user_policies"}, + ) + + assert plan.missing_tables == ("users",) + assert plan.missing_views == ("user_policies",) + assert not plan.can_apply + + +def test_declared_views_that_exist_as_views_are_granted() -> None: + declared = {RUNTIME: {"users": READ, "user_policies": READ_WRITE}} + + plan = plan_reconciliation( + declared, + granted={}, + existing_roles={RUNTIME}, + existing_relations=PublicRelations( + tables=frozenset({"users"}), views=frozenset({"user_policies"}) + ), + declared_views={"user_policies"}, + ) + + assert plan.can_apply + assert plan.missing_tables == plan.missing_views == () + assert {entry.table for entry in plan.grants} == {"users", "user_policies"} + + +def test_summary_names_missing_views_separately_from_missing_tables() -> None: + plan = ReconciliationPlan( + grants=(), + undeclared=(), + missing_roles=(), + missing_tables=("users",), + missing_views=("user_policies",), + ) + + summary = render_summary(plan, (), deployment_environment="staging", applied=False) + + assert ( + "Declared tables missing from the database (nothing was granted): `users`" + in summary + ) + assert ( + "Declared views missing from the database (nothing was granted): " + "`user_policies`" in summary + ) + + def test_grant_statements_group_privileges_per_role_and_table() -> None: plan = ReconciliationPlan( grants=(