From 28febfb634eebc0c88845533d8a5c30fc2533326 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:25:26 +0400 Subject: [PATCH 1/5] Report a v2 policy's missing parameters on the v2 policy routes Every policy returned by GET /v2/policies/{policy_id}, GET /v2/policies and POST /v2/policies now carries missing_parameters: the parameter paths it sets that the catalog of the running PolicyEngine.py version no longer has, sorted. It is empty for a policy bound to the running catalog version, and null when that catalog is not published, since nothing can then be checked. The field is required in the response model, so it is always present. V2PolicyService checks the policies after reading them (with_missing_parameters): one catalog selection and one read of the running version's parameters by name, only for policies bound to an older version. The comparison is parameters_missing_from_model, the check the dual-write copy uses. Co-Authored-By: Claude Opus 5.5 --- ...-1k-143-report-missing-parameters.added.md | 1 + .../v2/policies/response_models.py | 17 +- .../services/v2/policies/services.py | 90 +++++++++- .../services/v2/policies/transformations.py | 21 ++- .../services/v2/policies/types.py | 3 + .../services/v2/policies/validators.py | 3 +- .../unit/v2/test_policy_missing_parameters.py | 157 ++++++++++++++++++ tests/unit/v2/test_policy_routes.py | 39 ++++- 8 files changed, 320 insertions(+), 11 deletions(-) create mode 100644 changelog.d/v2plan-1k-143-report-missing-parameters.added.md create mode 100644 tests/unit/v2/test_policy_missing_parameters.py diff --git a/changelog.d/v2plan-1k-143-report-missing-parameters.added.md b/changelog.d/v2plan-1k-143-report-missing-parameters.added.md new file mode 100644 index 000000000..48ed63e7c --- /dev/null +++ b/changelog.d/v2plan-1k-143-report-missing-parameters.added.md @@ -0,0 +1 @@ +The v2 policy routes (`GET /v2/policies/{policy_id}`, `GET /v2/policies` and `POST /v2/policies`) now return `missing_parameters` with each policy: the parameter paths it sets that the catalog of the running PolicyEngine.py version no longer has, or null when that catalog is not published. diff --git a/policyengine_api/fastapi_routes/v2/policies/response_models.py b/policyengine_api/fastapi_routes/v2/policies/response_models.py index 0ae6b4887..7340e190e 100644 --- a/policyengine_api/fastapi_routes/v2/policies/response_models.py +++ b/policyengine_api/fastapi_routes/v2/policies/response_models.py @@ -3,10 +3,10 @@ from __future__ import annotations from datetime import datetime -from typing import TYPE_CHECKING, Any, Generic, Literal, TypeVar +from typing import TYPE_CHECKING, Annotated, Any, Generic, Literal, TypeVar from uuid import UUID -from pydantic import BaseModel, ConfigDict, JsonValue +from pydantic import BaseModel, ConfigDict, Field, JsonValue from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse @@ -52,6 +52,19 @@ class PolicyItem(StrictPolicyAPIModel): created_at: datetime updated_at: datetime parameter_values: list[PolicyParameterValueItem] + missing_parameters: Annotated[ + list[str] | None, + Field( + description=( + "The parameter paths this policy sets that the current " + "tax-benefit model (the catalog of the running PolicyEngine.py " + "version) no longer has, sorted; empty when it has them all. " + "Null when that catalog is not published, so nothing could be " + "checked. A policy with missing parameters cannot be run on " + "the current model." + ) + ), + ] @classmethod def from_read(cls, item: PolicyRead) -> PolicyItem: diff --git a/policyengine_api/services/v2/policies/services.py b/policyengine_api/services/v2/policies/services.py index eb8250fd1..6249bb050 100644 --- a/policyengine_api/services/v2/policies/services.py +++ b/policyengine_api/services/v2/policies/services.py @@ -2,9 +2,15 @@ from __future__ import annotations +from dataclasses import replace from typing import TYPE_CHECKING +from sqlalchemy.exc import SQLAlchemyError + from policyengine_api.constants import POLICYENGINE_VERSION +from policyengine_api.data.v2.catalog.catalog_selection import ( + MetadataCatalogUnavailableError, +) from policyengine_api.services.v2.policies.database_connectors.creates import ( create_legacy_policy_mapping, create_parameter_values, @@ -25,6 +31,7 @@ from policyengine_api.services.v2.policies.transformations import ( canonical_policy_document, canonicalize_policy, + policy_missing_parameters, policy_page, policy_parameter_values_by_policy, policy_read, @@ -185,6 +192,52 @@ def read_policy_page( return policy_page(rows, values, offset=offset, limit=limit) +def with_missing_parameters( + session: Session, + policies: tuple[PolicyRead, ...], + *, + country_id: str, + running_policyengine_version: str = POLICYENGINE_VERSION, +) -> tuple[PolicyRead, ...]: + """Return the policies with the parameters each sets that the running catalog lacks. + + Each policy's ``missing_parameters`` is None when the running PolicyEngine.py + version has no published catalog for the country, since nothing can be + checked. A catalog that cannot be queried raises, as any other read would. + """ + try: + selected = read_policy_catalog( + session, + country_id, + running_policyengine_version=running_policyengine_version, + ) + except MetadataCatalogUnavailableError as error: + if isinstance(error.__cause__, SQLAlchemyError): + raise + return tuple(replace(policy, missing_parameters=None) for policy in policies) + running_model_version_id = selected.model_version.id + names_to_check = { + value.parameter_name + for policy in policies + if policy.tax_benefit_model_version_id != running_model_version_id + for value in policy.parameter_values + } + running_parameters = read_parameters_by_name( + session, model_version_id=running_model_version_id, names=names_to_check + ) + return tuple( + replace( + policy, + missing_parameters=policy_missing_parameters( + policy, + running_model_version_id=running_model_version_id, + running_parameter_names=running_parameters, + ), + ) + for policy in policies + ) + + def mirror_legacy_policy_in_session( session: Session, snapshot: LegacyPolicySnapshot, @@ -315,14 +368,30 @@ def create_policy( country_id=policy_input.country_id, policy_id=persisted.policy_id, ) - return NativePolicyCreation(item=item, created=persisted.created) + (checked,) = with_missing_parameters( + session, + (item,), + country_id=policy_input.country_id, + running_policyengine_version=self._running_policyengine_version, + ) + return NativePolicyCreation(item=checked, created=persisted.created) def get_policy(self, *, country_id: str, policy_id: UUID) -> PolicyRead: - """Return one policy of this country, or raise PolicyNotFoundError.""" + """Return one policy of this country, or raise PolicyNotFoundError. + + Its ``missing_parameters`` are checked against the running catalog. + """ with self._database_session.read() as session: - return read_complete_policy( + item = read_complete_policy( session, country_id=country_id, policy_id=policy_id ) + (checked,) = with_missing_parameters( + session, + (item,), + country_id=country_id, + running_policyengine_version=self._running_policyengine_version, + ) + return checked def list_policies( self, @@ -332,15 +401,26 @@ def list_policies( offset: int = 0, limit: int = 100, ) -> PolicyPage: - """Return a page of policies, optionally for one tax-benefit model.""" + """Return a page of policies, optionally for one tax-benefit model. + + Each policy's ``missing_parameters`` are checked against the running + catalog. + """ with self._database_session.read() as session: - return read_policy_page( + page = read_policy_page( session, country_id=country_id, tax_benefit_model_id=tax_benefit_model_id, offset=offset, limit=limit, ) + items = with_missing_parameters( + session, + page.items, + country_id=country_id, + running_policyengine_version=self._running_policyengine_version, + ) + return replace(page, items=items) def mirror_legacy_policy( self, snapshot: LegacyPolicySnapshot diff --git a/policyengine_api/services/v2/policies/transformations.py b/policyengine_api/services/v2/policies/transformations.py index 8b20a6c74..f40266461 100644 --- a/policyengine_api/services/v2/policies/transformations.py +++ b/policyengine_api/services/v2/policies/transformations.py @@ -31,7 +31,7 @@ ) if TYPE_CHECKING: - from collections.abc import Mapping + from collections.abc import Container, Mapping from uuid import UUID from policyengine_api.data.v2.catalog.catalog_selection import SelectedCatalog @@ -183,6 +183,25 @@ def policy_parameter_values_by_policy( return {policy_id: tuple(values) for policy_id, values in grouped.items()} +def policy_missing_parameters( + policy: PolicyRead, + *, + running_model_version_id: UUID, + running_parameter_names: Container[str], +) -> tuple[str, ...]: + """Return the parameters a stored policy sets that the running catalog lacks. + + A policy bound to the running catalog version lacks none: its values can + only name that version's parameters. + """ + if policy.tax_benefit_model_version_id == running_model_version_id: + return () + return parameters_missing_from_model( + (value.parameter_name for value in policy.parameter_values), + running_parameter_names, + ) + + def policy_read( policy: Policy, values: dict[UUID, tuple[PolicyParameterValueRead, ...]], diff --git a/policyengine_api/services/v2/policies/types.py b/policyengine_api/services/v2/policies/types.py index 3783d5e82..49d07422a 100644 --- a/policyengine_api/services/v2/policies/types.py +++ b/policyengine_api/services/v2/policies/types.py @@ -175,6 +175,9 @@ class PolicyRead: created_at: datetime updated_at: datetime parameter_values: tuple[PolicyParameterValueRead, ...] + # The parameters the policy sets that the running catalog lacks; None + # when there is no catalog for the running PolicyEngine.py version. + missing_parameters: tuple[str, ...] | None = None @dataclass(frozen=True) diff --git a/policyengine_api/services/v2/policies/validators.py b/policyengine_api/services/v2/policies/validators.py index ed7a1bbe2..67b03e6ab 100644 --- a/policyengine_api/services/v2/policies/validators.py +++ b/policyengine_api/services/v2/policies/validators.py @@ -63,7 +63,8 @@ def parameters_missing_from_model( """Return the parameter paths a policy sets that the model does not have. Each absent path appears once, in sorted order; an empty tuple means every - path exists. The v2 copy of a v1 policy is refused when it is not empty. + path exists. The v2 copy of a v1 policy is refused when it is not empty, and + the policy routes report it as a policy's ``missing_parameters``. """ return tuple( sorted( diff --git a/tests/unit/v2/test_policy_missing_parameters.py b/tests/unit/v2/test_policy_missing_parameters.py new file mode 100644 index 000000000..30430a793 --- /dev/null +++ b/tests/unit/v2/test_policy_missing_parameters.py @@ -0,0 +1,157 @@ +"""Report the parameters a stored v2 policy sets that the running catalog lacks.""" + +from __future__ import annotations + +from datetime import UTC, datetime + +import pytest +from sqlalchemy.orm import sessionmaker +from sqlmodel import Session, create_engine + +from policyengine_api.data.v2.models import ( + V2_METADATA, + Parameter, + ParameterValue, + Policy, + TaxBenefitModel, + TaxBenefitModelVersion, +) +from policyengine_api.services.v2.policies.database_session import ( + PolicyDatabaseSession, +) +from policyengine_api.services.v2.policies.services import V2PolicyService + +RUNNING_VERSION = "5.2.0" +EARLIER_VERSION = "5.1.0" +KEPT_PARAMETER = "gov.example.rate" +REMOVED_PARAMETER = "gov.example.removed_rate" + + +@pytest.fixture +def engine(): + database_engine = create_engine("sqlite://") + V2_METADATA.create_all(database_engine) + yield database_engine + database_engine.dispose() + + +@pytest.fixture +def session(engine): + with Session(engine) as database_session: + yield database_session + + +def _service(engine, running_policyengine_version: str = RUNNING_VERSION): + sessions = sessionmaker(engine, class_=Session, expire_on_commit=False) + return V2PolicyService( + PolicyDatabaseSession(sessions), + running_policyengine_version=running_policyengine_version, + ) + + +def _version( + session: Session, model: TaxBenefitModel, version: str, names: list[str] +) -> tuple[TaxBenefitModelVersion, dict[str, Parameter]]: + model_version = TaxBenefitModelVersion( + model=model, + version=version, + current_law_id=1, + metadata_time_periods=[2026], + ) + parameters = { + name: Parameter(name=name, tax_benefit_model_version=model_version) + for name in names + } + session.add(model_version) + session.add_all(parameters.values()) + session.commit() + return model_version, parameters + + +def _policy( + session: Session, + model_version: TaxBenefitModelVersion, + parameters: dict[str, Parameter], + *, + content_hash: str, +) -> Policy: + policy = Policy( + country_id="us", + tax_benefit_model_id=model_version.model_id, + tax_benefit_model_version_id=model_version.id, + canonicalization_version=1, + content_hash=content_hash * 64, + created_at=datetime(2026, 1, 1, tzinfo=UTC), + updated_at=datetime(2026, 1, 1, tzinfo=UTC), + ) + session.add(policy) + session.flush() + session.add_all( + ParameterValue( + parameter_id=parameter.id, + value_json=0.2, + start_date=datetime(2026, 1, 1, tzinfo=UTC), + policy_id=policy.id, + ) + for parameter in parameters.values() + ) + session.commit() + return policy + + +@pytest.fixture +def stored_policies(session: Session) -> dict[str, Policy]: + """Store a policy on an earlier catalog version and one on the running one.""" + model = TaxBenefitModel(name="policyengine-us") + earlier, earlier_parameters = _version( + session, model, EARLIER_VERSION, [KEPT_PARAMETER, REMOVED_PARAMETER] + ) + running, running_parameters = _version( + session, model, RUNNING_VERSION, [KEPT_PARAMETER] + ) + return { + "earlier": _policy(session, earlier, earlier_parameters, content_hash="a"), + "running": _policy(session, running, running_parameters, content_hash="b"), + } + + +def test_policy_on_an_earlier_catalog_names_the_parameters_the_running_one_lacks( + engine, stored_policies: dict[str, Policy] +) -> None: + policy = _service(engine).get_policy( + country_id="us", policy_id=stored_policies["earlier"].id + ) + + assert policy.missing_parameters == (REMOVED_PARAMETER,) + + +def test_policy_on_the_running_catalog_lacks_nothing( + engine, stored_policies: dict[str, Policy] +) -> None: + policy = _service(engine).get_policy( + country_id="us", policy_id=stored_policies["running"].id + ) + + assert policy.missing_parameters == () + + +def test_page_reports_each_policy_against_the_running_catalog( + engine, stored_policies: dict[str, Policy] +) -> None: + page = _service(engine).list_policies(country_id="us", offset=0, limit=10) + + assert {item.id: item.missing_parameters for item in page.items} == { + stored_policies["earlier"].id: (REMOVED_PARAMETER,), + stored_policies["running"].id: (), + } + + +def test_without_a_running_catalog_nothing_is_reported( + engine, stored_policies: dict[str, Policy] +) -> None: + # The running PolicyEngine.py release has no published catalog yet. + policy = _service(engine, running_policyengine_version="9.9.9").get_policy( + country_id="us", policy_id=stored_policies["earlier"].id + ) + + assert policy.missing_parameters is None diff --git a/tests/unit/v2/test_policy_routes.py b/tests/unit/v2/test_policy_routes.py index 1267a14d8..487c57b12 100644 --- a/tests/unit/v2/test_policy_routes.py +++ b/tests/unit/v2/test_policy_routes.py @@ -43,7 +43,12 @@ CREATED_AT = datetime(2026, 1, 1, tzinfo=UTC) -def _policy_read(*, policy_id: UUID = POLICY_ID, country_id: str = "us") -> PolicyRead: +def _policy_read( + *, + policy_id: UUID = POLICY_ID, + country_id: str = "us", + missing_parameters: tuple[str, ...] | None = (), +) -> PolicyRead: return PolicyRead( id=policy_id, country_id=country_id, @@ -61,6 +66,7 @@ def _policy_read(*, policy_id: UUID = POLICY_ID, country_id: str = "us") -> Poli end_date=None, ), ), + missing_parameters=missing_parameters, ) @@ -68,6 +74,7 @@ class FakePolicyService: def __init__(self) -> None: self.created = True self.error: Exception | None = None + self.missing_parameters: tuple[str, ...] | None = () self.calls: list[tuple[str, object]] = [] def _raise(self) -> None: @@ -82,7 +89,9 @@ def create_policy(self, command) -> NativePolicyCreation: def get_policy(self, **filters) -> PolicyRead: self.calls.append(("get_policy", filters)) self._raise() - return _policy_read(policy_id=filters["policy_id"]) + return _policy_read( + policy_id=filters["policy_id"], missing_parameters=self.missing_parameters + ) def list_policies(self, **filters) -> PolicyPage: self.calls.append(("list_policies", filters)) @@ -181,6 +190,7 @@ def test_create_returns_201_for_new_and_200_for_deduplicated_content() -> None: "end_date": None, } ], + "missing_parameters": [], } }, } @@ -274,6 +284,28 @@ def test_detail_is_country_scoped_and_returns_typed_not_found() -> None: assert flask_calls["count"] == 0 +@pytest.mark.parametrize( + ("missing_parameters", "reported"), + [ + (("gov.example.removed_rate",), ["gov.example.removed_rate"]), + ((), []), + (None, None), + ], +) +def test_detail_reports_the_parameters_the_running_model_lacks( + missing_parameters: tuple[str, ...] | None, + reported: list[str] | None, +) -> None: + service = FakePolicyService() + service.missing_parameters = missing_parameters + client, _flask_calls = _client(service) + + response = client.get(f"/v2/policies/{POLICY_ID}?country_id=us") + + assert response.status_code == 200 + assert response.json()["result"]["item"]["missing_parameters"] == reported + + def test_list_passes_exact_model_filter_and_canonical_pagination() -> None: service = FakePolicyService() client, _flask_calls = _client(service) @@ -406,7 +438,10 @@ def test_openapi_publishes_request_query_response_and_error_contracts() -> None: "created_at", "updated_at", "parameter_values", + "missing_parameters", } + missing_schema = item_schema["properties"]["missing_parameters"] + assert {"type": "null"} in missing_schema["anyOf"] assert "name" not in item_schema["properties"] assert "description" not in item_schema["properties"] From 4eaac4a2cf1caaa88ec2f428107a37acf0dd35de Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:30:43 +0400 Subject: [PATCH 2/5] Report a v1 policy's missing parameters on GET /{country}/policy/{id} v3 reads a saved reform's changes through GET /{country}/policy/{id}, so that response now also carries missing_parameters: the parameter paths the policy sets that the country model the Public API runs no longer has, sorted; empty when it has them all; null when the stored policy is not a parameter-path object or the country has no loaded model. Existing fields are unchanged. PolicyParameterService compares the paths with the loaded model's leaf parameters (PolicyEngineCountry.parameter_names, computed once per process) using parameters_missing_from_model, the comparison the v2 copy uses. It reads nothing from Supabase, so v1 reads stay on Cloud SQL and a Supabase outage cannot slow them. The v2 catalog of a release holds the same leaf parameters as that release's model, so a policy reported here is one the copy refuses as missing_parameters. The OpenAPI spec, the changelog fragment and the Stage 10 runbook describe the field on both the v1 and the v2 routes. Co-Authored-By: Claude Opus 5.5 --- ...-1k-143-report-missing-parameters.added.md | 2 +- docs/migration/stage-10-v2-policies.md | 19 +++++ policyengine_api/country.py | 14 ++++ policyengine_api/openapi_spec.yaml | 11 +++ policyengine_api/routes/policy_routes.py | 19 +++-- .../services/policy_parameter_service.py | 66 +++++++++++++++++ .../routes/test_policy_dual_write_routes.py | 43 ++++++++++++ .../services/test_policy_parameter_service.py | 70 +++++++++++++++++++ tests/unit/test_country.py | 24 +++++++ 9 files changed, 262 insertions(+), 6 deletions(-) create mode 100644 policyengine_api/services/policy_parameter_service.py create mode 100644 tests/unit/services/test_policy_parameter_service.py diff --git a/changelog.d/v2plan-1k-143-report-missing-parameters.added.md b/changelog.d/v2plan-1k-143-report-missing-parameters.added.md index 48ed63e7c..72d44052b 100644 --- a/changelog.d/v2plan-1k-143-report-missing-parameters.added.md +++ b/changelog.d/v2plan-1k-143-report-missing-parameters.added.md @@ -1 +1 @@ -The v2 policy routes (`GET /v2/policies/{policy_id}`, `GET /v2/policies` and `POST /v2/policies`) now return `missing_parameters` with each policy: the parameter paths it sets that the catalog of the running PolicyEngine.py version no longer has, or null when that catalog is not published. +The v2 policy routes (`GET /v2/policies/{policy_id}`, `GET /v2/policies` and `POST /v2/policies`) now return `missing_parameters` with each policy: the parameter paths it sets that the catalog of the running PolicyEngine.py version no longer has, or null when that catalog is not published. `GET /{country}/policy/{policy_id}` returns the same field, checked against the country model the Public API runs; it reads nothing from Supabase. diff --git a/docs/migration/stage-10-v2-policies.md b/docs/migration/stage-10-v2-policies.md index e464e9949..3704998cc 100644 --- a/docs/migration/stage-10-v2-policies.md +++ b/docs/migration/stage-10-v2-policies.md @@ -174,6 +174,25 @@ request headers used by v1 and v2, exposes `X-PolicyEngine-Request-Id`, and applies CORS headers to typed errors and service-unavailable responses. Route functions do not implement separate preflight behavior. +## Missing parameters + +A policy can set parameters that a later country package release renamed or +removed. Policy reads report them as `missing_parameters`, so a client can +tell its user and not run the policy: + +- `GET /{country}/policy/{policy_id}` checks the v1 policy's parameter paths + against the leaf parameters of the country model the Public API runs. It + reads nothing from Supabase, so v1 reads stay on Cloud SQL. The field is + null for a stored policy that is not a parameter-path object. +- `GET /v2/policies/{policy_id}`, `GET /v2/policies` and `POST /v2/policies` + check each policy against the catalog of the running PolicyEngine.py + version. The field is null when that catalog is not published. + +Both use the comparison the v2 copy uses (`parameters_missing_from_model`), +and the v2 catalog of a release holds the same leaf parameters as that +release's country model, so a policy reported with missing parameters is +one the copy refuses as `missing_parameters`. + ## Monitoring Monitor structured events with metric names `v1_policy_mirror_operations` and diff --git a/policyengine_api/country.py b/policyengine_api/country.py index 8c04362b4..a650eaf9b 100644 --- a/policyengine_api/country.py +++ b/policyengine_api/country.py @@ -7,6 +7,7 @@ import logging from dataclasses import dataclass from datetime import date, datetime +from functools import cached_property from importlib.metadata import version as get_package_version from pathlib import Path from typing import Any, NamedTuple @@ -149,6 +150,19 @@ def __init__(self, country_package_name: str, country_id: str) -> None: ) self.build_metadata() + @cached_property + def parameter_names(self) -> frozenset[str]: + """Return the path of every leaf parameter, the parameters a policy can set. + + Nodes (which hold parameters) and the scales and brackets above leaf + parameters are left out, as the v2 catalog leaves them out. + """ + return frozenset( + parameter.name + for parameter in self.tax_benefit_system.parameters.get_descendants() + if isinstance(parameter, Parameter) + ) + def build_metadata(self) -> None: """Build the JSON-safe metadata document served by the metadata endpoints.""" self.metadata = self._json_safe( diff --git a/policyengine_api/openapi_spec.yaml b/policyengine_api/openapi_spec.yaml index 6c2f5672d..f14775307 100644 --- a/policyengine_api/openapi_spec.yaml +++ b/policyengine_api/openapi_spec.yaml @@ -180,6 +180,17 @@ paths: type: string api_version: type: string + missing_parameters: + type: array + nullable: true + items: + type: string + description: >- + The parameter paths this policy sets that the + country model the API runs no longer has, sorted; + empty when it has them all. Null when they cannot + be checked. A policy with missing parameters cannot + be run on the current model. 404: description: Policy not found. content: diff --git a/policyengine_api/routes/policy_routes.py b/policyengine_api/routes/policy_routes.py index 51694175c..9226f3abf 100644 --- a/policyengine_api/routes/policy_routes.py +++ b/policyengine_api/routes/policy_routes.py @@ -28,6 +28,7 @@ PolicyMirrorUnavailableError, mirror_policy_after_commit, ) +from policyengine_api.services.policy_parameter_service import PolicyParameterService from policyengine_api.services.policy_service import PolicyService from policyengine_api.services.user_policy_mirroring import ( UserPolicyMirrorCollaborators, @@ -48,6 +49,7 @@ policy_bp = Blueprint("policy", __name__) policy_service = PolicyService() +policy_parameter_service = PolicyParameterService() user_policy_service = UserPolicyService() # v1 policy and saved-policy IDs are MySQL signed INT values. @@ -254,8 +256,11 @@ def get_policy(country_id: str, policy_id: int | str) -> Response: policy_id (int | str): ID of the stored policy; cast to int. Returns: - Response: The policy as JSON under ``result``. An unknown ID gives 404, - and an invalid policy read-source setting gives 503. + Response: The policy as JSON under ``result``, with + ``missing_parameters``: the parameter paths it sets that the country + model the Public API runs no longer has (sorted, empty when it has + them all), or null when they cannot be checked. An unknown ID gives + 404, and an invalid policy read-source setting gives 503. """ try: @@ -267,11 +272,15 @@ def get_policy(country_id: str, policy_id: int | str) -> Response: policy_id = int(policy_id) policy = policy_service.get_policy(country_id, policy_id) - result = None if policy is None else _serialize_policy(policy) - - if result is None: + if policy is None: msg = f"Policy #{policy_id} not found." raise NotFound(msg) + result = { + **_serialize_policy(policy), + "missing_parameters": policy_parameter_service.missing_parameters( + country_id, policy.policy_json + ), + } return Response( json.dumps({"status": "ok", "message": None, "result": result}), diff --git a/policyengine_api/services/policy_parameter_service.py b/policyengine_api/services/policy_parameter_service.py new file mode 100644 index 000000000..b9cc7597c --- /dev/null +++ b/policyengine_api/services/policy_parameter_service.py @@ -0,0 +1,66 @@ +"""Check a v1 policy's parameter paths against the country model the API runs.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from policyengine_api.services.v2.policies.validators import ( + parameters_missing_from_model, +) + +if TYPE_CHECKING: + from collections.abc import Callable, Container + + +def loaded_model_parameter_names(country_id: str) -> Container[str] | None: + """Return the leaf parameter paths of the country model the API runs. + + None for a country without a loaded model. The models load at startup + (see policyengine_api.warmup); a process that has not loaded them loads + them here. + """ + from policyengine_api.country import ( # noqa: PLC0415 # importing country loads every country model; only a check that needs one does + COUNTRIES, + ) + + country = COUNTRIES.get(country_id) + return None if country is None else country.parameter_names + + +class PolicyParameterService: + """Report which parameters a v1 policy sets that the current model lacks. + + The current model is the country package the Public API runs, whose + catalog the v2 copy of the same release holds. The comparison is the one + the v2 copy uses, parameters_missing_from_model. Nothing is read from + Supabase, so v1 policy reads stay on Cloud SQL. + """ + + def __init__( + self, + model_parameter_names: Callable[[str], Container[str] | None] | None = None, + ) -> None: + """Read model parameter paths through ``model_parameter_names``. + + Without it, the loaded country models are read. + """ + self._model_parameter_names = ( + model_parameter_names or loaded_model_parameter_names + ) + + def missing_parameters( + self, country_id: str, policy_json: object + ) -> list[str] | None: + """Return the parameter paths the policy sets that the model lacks, sorted. + + Return None when nothing can be checked: the stored policy is not a + parameter-path object, or the country has no loaded model. + """ + if not isinstance(policy_json, dict): + return None + if not policy_json: + return [] + model_parameter_names = self._model_parameter_names(country_id) + if model_parameter_names is None: + return None + return list(parameters_missing_from_model(policy_json, model_parameter_names)) diff --git a/tests/unit/routes/test_policy_dual_write_routes.py b/tests/unit/routes/test_policy_dual_write_routes.py index 87f5a3d52..03ef27ece 100644 --- a/tests/unit/routes/test_policy_dual_write_routes.py +++ b/tests/unit/routes/test_policy_dual_write_routes.py @@ -344,6 +344,49 @@ def test_v1_policy_reads_require_cloud_sql_and_never_invoke_v2( invalid_get.assert_not_called() +@pytest.mark.parametrize( + "missing_parameters", + [["gov.irs.income.bracket.thresholds.1.WIDOW"], [], None], + ids=["missing", "none-missing", "not-checked"], +) +def test_v1_policy_read_reports_the_parameters_the_model_lacks( + monkeypatch, missing_parameters: list[str] | None +) -> None: + policy = Policy( + id=96567, + country_id="us", + label=None, + api_version="1.633.2", + policy_json={"gov.dol.minimum_wage": {"2026-01-01.2100-12-31": 9}}, + policy_hash="legacy/base64+hash=", + ) + monkeypatch.setenv("DB_READ_POLICY", "cloud_sql") + with ( + patch( + "policyengine_api.routes.policy_routes.policy_service.get_policy", + return_value=policy, + ), + patch( + "policyengine_api.routes.policy_routes.policy_parameter_service.missing_parameters", + return_value=missing_parameters, + ) as check, + ): + response = _client().get("/us/policy/96567") + + assert response.status_code == 200 + result = json.loads(response.text)["result"] + assert result == { + "id": 96567, + "country_id": "us", + "label": None, + "api_version": "1.633.2", + "policy_json": {"gov.dol.minimum_wage": {"2026-01-01.2100-12-31": 9}}, + "policy_hash": "legacy/base64+hash=", + "missing_parameters": missing_parameters, + } + check.assert_called_once_with("us", policy.policy_json) + + def test_policy_search_failure_does_not_expose_exception_details( monkeypatch, ) -> None: diff --git a/tests/unit/services/test_policy_parameter_service.py b/tests/unit/services/test_policy_parameter_service.py new file mode 100644 index 000000000..f20333659 --- /dev/null +++ b/tests/unit/services/test_policy_parameter_service.py @@ -0,0 +1,70 @@ +"""Check a v1 policy's parameter paths against the country model the API runs.""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from policyengine_api.services.policy_parameter_service import ( + PolicyParameterService, +) + +KEPT_PARAMETER = "gov.dol.minimum_wage" +REMOVED_PARAMETER = "gov.irs.income.bracket.thresholds.1.WIDOW" +OTHER_REMOVED_PARAMETER = "gov.abolitions.business_rates" +MODEL_PARAMETERS = frozenset( + {KEPT_PARAMETER, "gov.irs.credits.ctc.amount.base[0].amount"} +) + + +def _service(model_parameters: frozenset[str] | None = MODEL_PARAMETERS): + model_parameter_names = MagicMock(return_value=model_parameters) + return ( + PolicyParameterService(model_parameter_names=model_parameter_names), + model_parameter_names, + ) + + +def test_policy_names_each_parameter_the_model_lacks_once_in_order() -> None: + service, model_parameter_names = _service() + + missing = service.missing_parameters( + "us", + { + REMOVED_PARAMETER: {"2026-01-01.2100-12-31": 0}, + KEPT_PARAMETER: {"2026-01-01.2100-12-31": 9}, + OTHER_REMOVED_PARAMETER: {"2026": True}, + }, + ) + + assert missing == [OTHER_REMOVED_PARAMETER, REMOVED_PARAMETER] + model_parameter_names.assert_called_once_with("us") + + +def test_policy_whose_parameters_all_exist_lacks_nothing() -> None: + service, _model_parameter_names = _service() + + assert ( + service.missing_parameters("us", {KEPT_PARAMETER: {"2026-01-01.2100-12-31": 9}}) + == [] + ) + + +def test_empty_policy_lacks_nothing_without_reading_the_model() -> None: + service, model_parameter_names = _service() + + assert service.missing_parameters("us", {}) == [] + model_parameter_names.assert_not_called() + + +def test_country_without_a_loaded_model_is_not_checked() -> None: + service, _model_parameter_names = _service(model_parameters=None) + + assert service.missing_parameters("us", {KEPT_PARAMETER: {"2026": 9}}) is None + + +def test_policy_json_that_is_not_an_object_is_not_checked() -> None: + service, model_parameter_names = _service() + + assert service.missing_parameters("us", None) is None + assert service.missing_parameters("us", ["not", "an", "object"]) is None + model_parameter_names.assert_not_called() diff --git a/tests/unit/test_country.py b/tests/unit/test_country.py index 48af0f3eb..b84c090ea 100644 --- a/tests/unit/test_country.py +++ b/tests/unit/test_country.py @@ -16,6 +16,7 @@ _serialize_axis_result, get_requested_computations, ) +from policyengine_api.services.policy_parameter_service import PolicyParameterService class ExampleEnum(Enum): @@ -463,3 +464,26 @@ def __init__(self, *, situation, reform=None): assert simulation.tax_benefit_system == "modern-system" assert simulation.reform is None assert simulation.situation == {"households": {"household": {}}} + + +def test_loaded_model_parameter_names_are_its_leaf_parameters() -> None: + names = COUNTRIES["us"].parameter_names + + assert "gov.dol.minimum_wage" in names + assert "gov.irs.credits.ctc.amount.base[0].amount" in names + # A node holds parameters but is not one a policy can set. + assert "gov.irs.credits.ctc.amount" not in names + # Renamed to SURVIVING_SPOUSE in a later policyengine-us release. + assert "gov.irs.income.bracket.thresholds.1.WIDOW" not in names + + +def test_policy_parameter_service_checks_against_the_loaded_model() -> None: + missing = PolicyParameterService().missing_parameters( + "us", + { + "gov.dol.minimum_wage": {"2026-01-01.2100-12-31": 9}, + "gov.irs.income.bracket.thresholds.1.WIDOW": {"2026": 0}, + }, + ) + + assert missing == ["gov.irs.income.bracket.thresholds.1.WIDOW"] From b8cc4c0c22b41eb32c3d1bac1a09d37caf9a8230 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:47:58 +0400 Subject: [PATCH 3/5] Read parameter names in batches, so a large policy page cannot exceed the bind limit Review found that GET /v2/policies could now fail with 503: the missing parameter check reads every distinct parameter name of a page's policies on older catalog versions in one IN filter, and a page of up to 500 policies with up to 1,000 values each can bind more than the 65,535 values a PostgreSQL statement allows. read_parameters_by_name now reads names in batches of 1,000. The copy, which reads at most 1,000 names, runs one statement as before. The recording session used in unit tests gains all(), which this read calls. Co-Authored-By: Claude Opus 5.5 --- .../v2/policies/database_connectors/reads.py | 31 +++++++++++++------ tests/fixtures/recording_session.py | 3 ++ .../unit/v2/test_policy_missing_parameters.py | 30 ++++++++++++++++++ 3 files changed, 54 insertions(+), 10 deletions(-) diff --git a/policyengine_api/services/v2/policies/database_connectors/reads.py b/policyengine_api/services/v2/policies/database_connectors/reads.py index 6c649b384..bd1685d01 100644 --- a/policyengine_api/services/v2/policies/database_connectors/reads.py +++ b/policyengine_api/services/v2/policies/database_connectors/reads.py @@ -22,6 +22,9 @@ if TYPE_CHECKING: from uuid import UUID +# Parameter names bound in one statement; far below PostgreSQL's 65,535 limit. +PARAMETER_NAME_BATCH_SIZE = 1_000 + def read_policy_catalog( session: Session, @@ -61,16 +64,24 @@ def read_version_parameter_ids( def read_parameters_by_name( session: Session, *, model_version_id: UUID, names: set[str] ) -> dict[str, Parameter]: - """Return this model version's parameters with the given names, keyed by name.""" - if not names: - return {} - parameters = session.exec( - select(Parameter).where( - Parameter.tax_benefit_model_version_id == model_version_id, - col(Parameter.name).in_(names), - ) - ).all() - return {parameter.name: parameter for parameter in parameters} + """Return this model version's parameters with the given names, keyed by name. + + The names are read in batches of PARAMETER_NAME_BATCH_SIZE, because a + PostgreSQL statement takes at most 65,535 bound values and a page of + policies can name more parameters than that. + """ + ordered_names = sorted(names) + found: dict[str, Parameter] = {} + for start in range(0, len(ordered_names), PARAMETER_NAME_BATCH_SIZE): + batch = ordered_names[start : start + PARAMETER_NAME_BATCH_SIZE] + parameters = session.exec( + select(Parameter).where( + Parameter.tax_benefit_model_version_id == model_version_id, + col(Parameter.name).in_(batch), + ) + ).all() + found.update((parameter.name, parameter) for parameter in parameters) + return found def read_policy_by_content_identity( diff --git a/tests/fixtures/recording_session.py b/tests/fixtures/recording_session.py index 544ce20f2..9a9b5fa50 100644 --- a/tests/fixtures/recording_session.py +++ b/tests/fixtures/recording_session.py @@ -29,6 +29,9 @@ def one_or_none(self) -> object: def scalar_one_or_none(self) -> object: return self.value + def all(self) -> object: + return self.value + @dataclass class RecordingSession: diff --git a/tests/unit/v2/test_policy_missing_parameters.py b/tests/unit/v2/test_policy_missing_parameters.py index 30430a793..c6c7fffea 100644 --- a/tests/unit/v2/test_policy_missing_parameters.py +++ b/tests/unit/v2/test_policy_missing_parameters.py @@ -2,7 +2,9 @@ from __future__ import annotations +from collections import deque from datetime import UTC, datetime +from uuid import uuid4 import pytest from sqlalchemy.orm import sessionmaker @@ -16,10 +18,15 @@ TaxBenefitModel, TaxBenefitModelVersion, ) +from policyengine_api.services.v2.policies.database_connectors.reads import ( + PARAMETER_NAME_BATCH_SIZE, + read_parameters_by_name, +) from policyengine_api.services.v2.policies.database_session import ( PolicyDatabaseSession, ) from policyengine_api.services.v2.policies.services import V2PolicyService +from tests.fixtures.recording_session import RecordingSession RUNNING_VERSION = "5.2.0" EARLIER_VERSION = "5.1.0" @@ -155,3 +162,26 @@ def test_without_a_running_catalog_nothing_is_reported( ) assert policy.missing_parameters is None + + +def test_parameter_names_are_read_in_batches_of_bounded_size() -> None: + # A page of policies on an earlier catalog can name tens of thousands of + # parameters; PostgreSQL drivers refuse more than 65,535 bound values. + names = {f"gov.example.parameter_{index}" for index in range(2_500)} + session = RecordingSession(results=deque([[], [], []])) + + found = read_parameters_by_name(session, model_version_id=uuid4(), names=names) + + statements = [ + operation for operation in session.operations if operation[0] == "exec" + ] + name_batches = [ + value + for _method, _sql, parameters in statements + for value in parameters.values() + if isinstance(value, list) + ] + assert found == {} + assert len(statements) == 3 + assert all(len(batch) <= PARAMETER_NAME_BATCH_SIZE for batch in name_batches) + assert sorted(name for batch in name_batches for name in batch) == sorted(names) From 7610500538a571e7c3f7f603e271171e80440917 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:39:20 +0400 Subject: [PATCH 4/5] Tell a catalog that is not published from one that cannot be queried, by type with_missing_parameters returned null for an absent catalog and re-raised a failed catalog query by checking whether the exception's cause was a SQLAlchemyError, which depended on how select_catalog chained it, and no test covered the failure branch. select_catalog now raises MetadataCatalogNotPublishedError, a MetadataCatalogUnavailableError, for an absent catalog: the v2 routes return null for that type only, and the copy's catalog_absent and catalog_query_failed reasons come from the type too. Existing handlers of MetadataCatalogUnavailableError are unchanged. Also from review: - the migration contract registry pins result.item.missing_parameters on the v2 policy create and detail routes, and the generated migration contracts are refreshed; - a test covers create_policy filling in the field; - the Stage 10 runbook no longer says every policy reported as missing is refused by the copy: a copy made before a release removed the parameter is kept. Co-Authored-By: Claude Opus 5.5 --- docs/engineering/migration-contracts.md | 4 +- docs/generated/migration_contracts.json | 6 ++- docs/migration/stage-10-v2-policies.md | 7 ++- .../data/v2/catalog/catalog_selection.py | 10 +++- policyengine_api/services/policy_mirroring.py | 7 +-- .../services/v2/policies/services.py | 8 +-- tests/contract/registry.py | 2 + tests/unit/services/test_policy_mirroring.py | 8 +-- .../unit/v2/test_policy_legacy_translation.py | 7 ++- .../unit/v2/test_policy_missing_parameters.py | 54 +++++++++++++++++++ 10 files changed, 92 insertions(+), 21 deletions(-) diff --git a/docs/engineering/migration-contracts.md b/docs/engineering/migration-contracts.md index f39a5e369..45f16079f 100644 --- a/docs/engineering/migration-contracts.md +++ b/docs/engineering/migration-contracts.md @@ -46,8 +46,8 @@ Generated from `policyengine_api/migration_registry.py` and `tests/contract/regi | Method | Path | Status | Route group | Required headers | Stable response fields | | --- | --- | ---: | --- | --- | --- | -| `POST` | `/v2/policies?country_id=us` | 201 | `policy` | none | `status`, `message`, `result.item.id`, `result.item.country_id`, `result.item.tax_benefit_model_id`, `result.item.parameter_values`, `result.item.created_at`, `result.item.updated_at` | -| `GET` | `/v2/policies/{policy_id}?country_id=us` | 200 | `policy` | none | `status`, `message`, `result.item.id`, `result.item.country_id`, `result.item.tax_benefit_model_id`, `result.item.parameter_values`, `result.item.created_at`, `result.item.updated_at` | +| `POST` | `/v2/policies?country_id=us` | 201 | `policy` | none | `status`, `message`, `result.item.id`, `result.item.country_id`, `result.item.tax_benefit_model_id`, `result.item.parameter_values`, `result.item.created_at`, `result.item.updated_at`, `result.item.missing_parameters` | +| `GET` | `/v2/policies/{policy_id}?country_id=us` | 200 | `policy` | none | `status`, `message`, `result.item.id`, `result.item.country_id`, `result.item.tax_benefit_model_id`, `result.item.parameter_values`, `result.item.created_at`, `result.item.updated_at`, `result.item.missing_parameters` | | `GET` | `/v2/policies?country_id=us` | 200 | `policy` | none | `status`, `message`, `result.items`, `result.offset`, `result.limit`, `result.has_more` | ### `saved_policy_v1_compatibility` diff --git a/docs/generated/migration_contracts.json b/docs/generated/migration_contracts.json index 7a0f1b67d..2d4c6b16e 100644 --- a/docs/generated/migration_contracts.json +++ b/docs/generated/migration_contracts.json @@ -166,7 +166,8 @@ "result.item.tax_benefit_model_id", "result.item.parameter_values", "result.item.created_at", - "result.item.updated_at" + "result.item.updated_at", + "result.item.missing_parameters" ] }, { @@ -183,7 +184,8 @@ "result.item.tax_benefit_model_id", "result.item.parameter_values", "result.item.created_at", - "result.item.updated_at" + "result.item.updated_at", + "result.item.missing_parameters" ] }, { diff --git a/docs/migration/stage-10-v2-policies.md b/docs/migration/stage-10-v2-policies.md index 3704998cc..1d85250fe 100644 --- a/docs/migration/stage-10-v2-policies.md +++ b/docs/migration/stage-10-v2-policies.md @@ -190,8 +190,11 @@ tell its user and not run the policy: Both use the comparison the v2 copy uses (`parameters_missing_from_model`), and the v2 catalog of a release holds the same leaf parameters as that -release's country model, so a policy reported with missing parameters is -one the copy refuses as `missing_parameters`. +release's country model. So a v1 policy reported with missing parameters +is one a first copy under the running release refuses as +`missing_parameters`. A policy copied before a release removed its +parameter keeps that copy, since an existing copy is not translated again; +it is still reported as missing on both routes. ## Monitoring diff --git a/policyengine_api/data/v2/catalog/catalog_selection.py b/policyengine_api/data/v2/catalog/catalog_selection.py index d9b50fc76..10b689151 100644 --- a/policyengine_api/data/v2/catalog/catalog_selection.py +++ b/policyengine_api/data/v2/catalog/catalog_selection.py @@ -21,6 +21,14 @@ class MetadataCatalogUnavailableError(RuntimeError): """Raised when an initialized catalog cannot be read.""" +class MetadataCatalogNotPublishedError(MetadataCatalogUnavailableError): + """Raised when the running PolicyEngine.py version has no catalog for a country. + + The catalog was queried and holds no such version, as opposed to a query + that failed. + """ + + class UnsupportedPreviewCountryError(ValueError): """Raised when a country has no Stage 9 resource catalog.""" @@ -107,7 +115,7 @@ def select_catalog( f"the running PolicyEngine.py {selected_version} catalog " f"is absent for {country_id}" ) - raise MetadataCatalogUnavailableError(msg) + raise MetadataCatalogNotPublishedError(msg) model, model_version = row return SelectedCatalog( country_id=country_id, diff --git a/policyengine_api/services/policy_mirroring.py b/policyengine_api/services/policy_mirroring.py index b71dc0d35..30363a30b 100644 --- a/policyengine_api/services/policy_mirroring.py +++ b/policyengine_api/services/policy_mirroring.py @@ -11,6 +11,7 @@ from sqlalchemy.exc import SQLAlchemyError from policyengine_api.data.v2.catalog.catalog_selection import ( + MetadataCatalogNotPublishedError, MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, ) @@ -186,9 +187,9 @@ def classify_policy_copy_failure(error: Exception) -> PolicyCopyFailure: ) if isinstance(error, MetadataCatalogUnavailableError): reason = ( - PolicyCopyFailureReason.CATALOG_QUERY_FAILED - if isinstance(error.__cause__, SQLAlchemyError) - else PolicyCopyFailureReason.CATALOG_ABSENT + PolicyCopyFailureReason.CATALOG_ABSENT + if isinstance(error, MetadataCatalogNotPublishedError) + else PolicyCopyFailureReason.CATALOG_QUERY_FAILED ) return PolicyCopyFailure( PolicyCopyFailureCategory.CATALOG_OR_TRANSLATION, reason diff --git a/policyengine_api/services/v2/policies/services.py b/policyengine_api/services/v2/policies/services.py index 6249bb050..10d0e7848 100644 --- a/policyengine_api/services/v2/policies/services.py +++ b/policyengine_api/services/v2/policies/services.py @@ -5,11 +5,9 @@ from dataclasses import replace from typing import TYPE_CHECKING -from sqlalchemy.exc import SQLAlchemyError - from policyengine_api.constants import POLICYENGINE_VERSION from policyengine_api.data.v2.catalog.catalog_selection import ( - MetadataCatalogUnavailableError, + MetadataCatalogNotPublishedError, ) from policyengine_api.services.v2.policies.database_connectors.creates import ( create_legacy_policy_mapping, @@ -211,9 +209,7 @@ def with_missing_parameters( country_id, running_policyengine_version=running_policyengine_version, ) - except MetadataCatalogUnavailableError as error: - if isinstance(error.__cause__, SQLAlchemyError): - raise + except MetadataCatalogNotPublishedError: return tuple(replace(policy, missing_parameters=None) for policy in policies) running_model_version_id = selected.model_version.id names_to_check = { diff --git a/tests/contract/registry.py b/tests/contract/registry.py index 778c3da26..7e8312654 100644 --- a/tests/contract/registry.py +++ b/tests/contract/registry.py @@ -181,6 +181,7 @@ class WorkflowContract: "result.item.parameter_values", "result.item.created_at", "result.item.updated_at", + "result.item.missing_parameters", ), route_group="policy", ), @@ -197,6 +198,7 @@ class WorkflowContract: "result.item.parameter_values", "result.item.created_at", "result.item.updated_at", + "result.item.missing_parameters", ), route_group="policy", ), diff --git a/tests/unit/services/test_policy_mirroring.py b/tests/unit/services/test_policy_mirroring.py index 8459e0a0f..034b29951 100644 --- a/tests/unit/services/test_policy_mirroring.py +++ b/tests/unit/services/test_policy_mirroring.py @@ -10,6 +10,7 @@ from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.data.v2.catalog.catalog_selection import ( + MetadataCatalogNotPublishedError, MetadataCatalogUnavailableError, MetadataCatalogVersionNotFoundError, ) @@ -140,9 +141,8 @@ def test_failure_is_secret_safe_and_records_cloud_sql_as_only_completed_write( def _catalog_query_failure() -> MetadataCatalogUnavailableError: - error = MetadataCatalogUnavailableError("the v2 metadata catalog cannot be queried") - error.__cause__ = OperationalError("select secret", {}, Exception("credential")) - return error + # Raised without a chained database error: the type alone decides. + return MetadataCatalogUnavailableError("the v2 metadata catalog cannot be queried") def _translation_failure( @@ -167,7 +167,7 @@ def _translation_failure( id="catalog-query-failed", ), pytest.param( - MetadataCatalogUnavailableError("catalog absent secret"), + MetadataCatalogNotPublishedError("catalog absent secret"), "catalog_or_translation", "catalog_absent", id="catalog-absent", diff --git a/tests/unit/v2/test_policy_legacy_translation.py b/tests/unit/v2/test_policy_legacy_translation.py index 07f69f3ef..6f780fb79 100644 --- a/tests/unit/v2/test_policy_legacy_translation.py +++ b/tests/unit/v2/test_policy_legacy_translation.py @@ -10,6 +10,9 @@ from sqlalchemy.orm import sessionmaker from sqlmodel import Session, create_engine +from policyengine_api.data.v2.catalog.catalog_selection import ( + MetadataCatalogNotPublishedError, +) from policyengine_api.data.v2.models import ( V2_METADATA, Parameter, @@ -376,7 +379,9 @@ def test_reverse_legacy_range_is_rejected() -> None: def test_unknown_policyengine_version_never_falls_back() -> None: engine, session, _model, _version, _first, _second = _session_and_catalog() try: - with pytest.raises(Exception, match=r"running PolicyEngine\.py"): + with pytest.raises( + MetadataCatalogNotPublishedError, match=r"running PolicyEngine\.py" + ): _translate( session, _snapshot(), diff --git a/tests/unit/v2/test_policy_missing_parameters.py b/tests/unit/v2/test_policy_missing_parameters.py index c6c7fffea..8163b7268 100644 --- a/tests/unit/v2/test_policy_missing_parameters.py +++ b/tests/unit/v2/test_policy_missing_parameters.py @@ -10,6 +10,9 @@ from sqlalchemy.orm import sessionmaker from sqlmodel import Session, create_engine +from policyengine_api.data.v2.catalog.catalog_selection import ( + MetadataCatalogUnavailableError, +) from policyengine_api.data.v2.models import ( V2_METADATA, Parameter, @@ -18,6 +21,7 @@ TaxBenefitModel, TaxBenefitModelVersion, ) +from policyengine_api.services.v2.policies import services as policy_services from policyengine_api.services.v2.policies.database_connectors.reads import ( PARAMETER_NAME_BATCH_SIZE, read_parameters_by_name, @@ -26,6 +30,10 @@ PolicyDatabaseSession, ) from policyengine_api.services.v2.policies.services import V2PolicyService +from policyengine_api.services.v2.policies.types import ( + NativePolicyCreationInput, + PolicyCreationResult, +) from tests.fixtures.recording_session import RecordingSession RUNNING_VERSION = "5.2.0" @@ -185,3 +193,49 @@ def test_parameter_names_are_read_in_batches_of_bounded_size() -> None: assert len(statements) == 3 assert all(len(batch) <= PARAMETER_NAME_BATCH_SIZE for batch in name_batches) assert sorted(name for batch in name_batches for name in batch) == sorted(names) + + +def test_catalog_that_cannot_be_queried_fails_the_read( + engine, stored_policies: dict[str, Policy], monkeypatch +) -> None: + def unreadable_catalog(*_args, **_kwargs): + msg = "the v2 metadata catalog cannot be queried" + raise MetadataCatalogUnavailableError(msg) + + monkeypatch.setattr(policy_services, "read_policy_catalog", unreadable_catalog) + + with pytest.raises(MetadataCatalogUnavailableError): + _service(engine).get_policy( + country_id="us", policy_id=stored_policies["earlier"].id + ) + + +def test_created_policy_on_an_earlier_catalog_names_what_the_running_one_lacks( + engine, stored_policies: dict[str, Policy], monkeypatch +) -> None: + # Creation is replaced by the stored policy (its inserts need PostgreSQL); + # the read and check after it are the service's own. + stored = stored_policies["earlier"] + monkeypatch.setattr( + policy_services, + "resolve_policy_creation_input", + lambda _session, _content, **_selection: None, + ) + monkeypatch.setattr( + policy_services, + "create_resolved_policy", + lambda _session, _resolved: PolicyCreationResult( + policy_id=stored.id, created=False + ), + ) + + creation = _service(engine).create_policy( + NativePolicyCreationInput( + country_id="us", + tax_benefit_model_id=stored.tax_benefit_model_id, + parameter_values=[], + policyengine_version=EARLIER_VERSION, + ) + ) + + assert creation.item.missing_parameters == (REMOVED_PARAMETER,) From e29bd6e4c71637137e7e5b8f77fd01b838f618dc Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:55:46 +0400 Subject: [PATCH 5/5] Return a checked policy type from the v2 policy service PolicyRead.missing_parameters defaulted to None, which is also what the response documents for an unpublished catalog, so a future read that skipped the check would have told clients the catalog is unpublished. The field moves to CheckedPolicyRead (and pages of them to CheckedPolicyPage), which only with_missing_parameters builds; the v2 policy service, its route protocol and the response models now take those types, so mypy rejects a route result that was never checked. Tests also cover the field in the GET /v2/policies response, and the catalog-query-failure case again chains the database error, as select_catalog raises it, so the test still shows that neither message reaches the copy log. Co-Authored-By: Claude Opus 5.5 --- .../fastapi_routes/dependencies.py | 8 ++--- .../v2/policies/response_models.py | 9 ++++-- .../services/v2/policies/services.py | 20 ++++++++----- .../services/v2/policies/transformations.py | 15 ++++++++++ .../services/v2/policies/types.py | 27 ++++++++++++++--- tests/unit/services/test_policy_mirroring.py | 7 +++-- .../unit/v2/test_policy_missing_parameters.py | 15 ++++++++++ tests/unit/v2/test_policy_routes.py | 29 ++++++++++++++----- 8 files changed, 101 insertions(+), 29 deletions(-) diff --git a/policyengine_api/fastapi_routes/dependencies.py b/policyengine_api/fastapi_routes/dependencies.py index 5d5a1a38d..dfd288f7c 100644 --- a/policyengine_api/fastapi_routes/dependencies.py +++ b/policyengine_api/fastapi_routes/dependencies.py @@ -48,10 +48,10 @@ ParameterValueFilter, ) from policyengine_api.services.v2.policies.types import ( + CheckedPolicyPage, + CheckedPolicyRead, NativePolicyCreation, NativePolicyCreationInput, - PolicyPage, - PolicyRead, ) from policyengine_api.services.v2.user_accounts.types import ( AccountDeletion, @@ -326,7 +326,7 @@ def create_policy( """ ... - def get_policy(self, *, country_id: str, policy_id: UUID) -> PolicyRead: + def get_policy(self, *, country_id: str, policy_id: UUID) -> CheckedPolicyRead: """Return one policy of the country with its parameter values. Raise ``PolicyNotFoundError`` when no such policy exists in that country. @@ -340,7 +340,7 @@ def list_policies( tax_benefit_model_id: UUID | None = None, offset: int = 0, limit: int = 100, - ) -> PolicyPage: + ) -> CheckedPolicyPage: """Return a page of the country's policies, optionally for one model.""" ... diff --git a/policyengine_api/fastapi_routes/v2/policies/response_models.py b/policyengine_api/fastapi_routes/v2/policies/response_models.py index 7340e190e..771aaa12d 100644 --- a/policyengine_api/fastapi_routes/v2/policies/response_models.py +++ b/policyengine_api/fastapi_routes/v2/policies/response_models.py @@ -11,7 +11,10 @@ from policyengine_api.fastapi_routes.v2.errors import V2ErrorResponse if TYPE_CHECKING: - from policyengine_api.services.v2.policies.types import PolicyPage, PolicyRead + from policyengine_api.services.v2.policies.types import ( + CheckedPolicyPage, + CheckedPolicyRead, + ) class StrictPolicyAPIModel(BaseModel): @@ -67,7 +70,7 @@ class PolicyItem(StrictPolicyAPIModel): ] @classmethod - def from_read(cls, item: PolicyRead) -> PolicyItem: + def from_read(cls, item: CheckedPolicyRead) -> PolicyItem: """Build the response item by validating the service read record's fields.""" return cls.model_validate(item) @@ -91,7 +94,7 @@ class PolicyPageResult(StrictPolicyAPIModel): has_more: bool @classmethod - def from_page(cls, page: PolicyPage) -> PolicyPageResult: + def from_page(cls, page: CheckedPolicyPage) -> PolicyPageResult: """Build the response page from a service page, converting each item.""" return cls( items=[PolicyItem.from_read(item) for item in page.items], diff --git a/policyengine_api/services/v2/policies/services.py b/policyengine_api/services/v2/policies/services.py index 10d0e7848..a77cea0be 100644 --- a/policyengine_api/services/v2/policies/services.py +++ b/policyengine_api/services/v2/policies/services.py @@ -2,7 +2,6 @@ from __future__ import annotations -from dataclasses import replace from typing import TYPE_CHECKING from policyengine_api.constants import POLICYENGINE_VERSION @@ -29,6 +28,7 @@ from policyengine_api.services.v2.policies.transformations import ( canonical_policy_document, canonicalize_policy, + checked_policy_read, policy_missing_parameters, policy_page, policy_parameter_values_by_policy, @@ -38,6 +38,8 @@ ) from policyengine_api.services.v2.policies.types import ( CanonicalPolicyContent, + CheckedPolicyPage, + CheckedPolicyRead, LegacyPolicyPersistenceResult, LegacyPolicySnapshot, NativePolicyCreation, @@ -196,7 +198,7 @@ def with_missing_parameters( *, country_id: str, running_policyengine_version: str = POLICYENGINE_VERSION, -) -> tuple[PolicyRead, ...]: +) -> tuple[CheckedPolicyRead, ...]: """Return the policies with the parameters each sets that the running catalog lacks. Each policy's ``missing_parameters`` is None when the running PolicyEngine.py @@ -210,7 +212,7 @@ def with_missing_parameters( running_policyengine_version=running_policyengine_version, ) except MetadataCatalogNotPublishedError: - return tuple(replace(policy, missing_parameters=None) for policy in policies) + return tuple(checked_policy_read(policy, None) for policy in policies) running_model_version_id = selected.model_version.id names_to_check = { value.parameter_name @@ -222,9 +224,9 @@ def with_missing_parameters( session, model_version_id=running_model_version_id, names=names_to_check ) return tuple( - replace( + checked_policy_read( policy, - missing_parameters=policy_missing_parameters( + policy_missing_parameters( policy, running_model_version_id=running_model_version_id, running_parameter_names=running_parameters, @@ -372,7 +374,7 @@ def create_policy( ) return NativePolicyCreation(item=checked, created=persisted.created) - def get_policy(self, *, country_id: str, policy_id: UUID) -> PolicyRead: + def get_policy(self, *, country_id: str, policy_id: UUID) -> CheckedPolicyRead: """Return one policy of this country, or raise PolicyNotFoundError. Its ``missing_parameters`` are checked against the running catalog. @@ -396,7 +398,7 @@ def list_policies( tax_benefit_model_id: UUID | None = None, offset: int = 0, limit: int = 100, - ) -> PolicyPage: + ) -> CheckedPolicyPage: """Return a page of policies, optionally for one tax-benefit model. Each policy's ``missing_parameters`` are checked against the running @@ -416,7 +418,9 @@ def list_policies( country_id=country_id, running_policyengine_version=self._running_policyengine_version, ) - return replace(page, items=items) + return CheckedPolicyPage( + items=items, offset=page.offset, limit=page.limit, has_more=page.has_more + ) def mirror_legacy_policy( self, snapshot: LegacyPolicySnapshot diff --git a/policyengine_api/services/v2/policies/transformations.py b/policyengine_api/services/v2/policies/transformations.py index f40266461..f41569862 100644 --- a/policyengine_api/services/v2/policies/transformations.py +++ b/policyengine_api/services/v2/policies/transformations.py @@ -4,6 +4,7 @@ import hashlib import json +from dataclasses import fields as dataclass_fields from datetime import UTC, date, datetime, time from decimal import Decimal from typing import TYPE_CHECKING, Any @@ -14,6 +15,7 @@ from policyengine_api.services.v2.policies.types import ( CanonicalPolicyContent, + CheckedPolicyRead, LegacyPolicySnapshot, LegacyPolicyTranslationReason, PolicyCreationInput, @@ -202,6 +204,19 @@ def policy_missing_parameters( ) +def checked_policy_read( + policy: PolicyRead, missing_parameters: tuple[str, ...] | None +) -> CheckedPolicyRead: + """Return the policy read with the result of its missing-parameter check.""" + return CheckedPolicyRead( + **{ + field.name: getattr(policy, field.name) + for field in dataclass_fields(PolicyRead) + }, + missing_parameters=missing_parameters, + ) + + def policy_read( policy: Policy, values: dict[UUID, tuple[PolicyParameterValueRead, ...]], diff --git a/policyengine_api/services/v2/policies/types.py b/policyengine_api/services/v2/policies/types.py index 49d07422a..d5ef827d1 100644 --- a/policyengine_api/services/v2/policies/types.py +++ b/policyengine_api/services/v2/policies/types.py @@ -175,9 +175,18 @@ class PolicyRead: created_at: datetime updated_at: datetime parameter_values: tuple[PolicyParameterValueRead, ...] - # The parameters the policy sets that the running catalog lacks; None - # when there is no catalog for the running PolicyEngine.py version. - missing_parameters: tuple[str, ...] | None = None + + +@dataclass(frozen=True) +class CheckedPolicyRead(PolicyRead): + """A stored policy checked against the running catalog, as routes return it. + + ``missing_parameters`` lists the parameter names the policy sets that the + running catalog lacks, sorted; it is None only when the running + PolicyEngine.py version has no published catalog for the country. + """ + + missing_parameters: tuple[str, ...] | None @dataclass(frozen=True) @@ -190,9 +199,19 @@ class PolicyPage: has_more: bool +@dataclass(frozen=True) +class CheckedPolicyPage: + """A page of policies, each checked against the running catalog.""" + + items: tuple[CheckedPolicyRead, ...] + offset: int + limit: int + has_more: bool + + @dataclass(frozen=True) class NativePolicyCreation: """Complete policy read plus whether this request inserted it.""" - item: PolicyRead + item: CheckedPolicyRead created: bool diff --git a/tests/unit/services/test_policy_mirroring.py b/tests/unit/services/test_policy_mirroring.py index 034b29951..9394f3b88 100644 --- a/tests/unit/services/test_policy_mirroring.py +++ b/tests/unit/services/test_policy_mirroring.py @@ -141,8 +141,11 @@ def test_failure_is_secret_safe_and_records_cloud_sql_as_only_completed_write( def _catalog_query_failure() -> MetadataCatalogUnavailableError: - # Raised without a chained database error: the type alone decides. - return MetadataCatalogUnavailableError("the v2 metadata catalog cannot be queried") + # As select_catalog raises it, with the database error chained; the type + # alone decides the reason, and neither message may reach the log. + error = MetadataCatalogUnavailableError("the v2 metadata catalog cannot be queried") + error.__cause__ = OperationalError("select secret", {}, Exception("credential")) + return error def _translation_failure( diff --git a/tests/unit/v2/test_policy_missing_parameters.py b/tests/unit/v2/test_policy_missing_parameters.py index 8163b7268..83b896cc2 100644 --- a/tests/unit/v2/test_policy_missing_parameters.py +++ b/tests/unit/v2/test_policy_missing_parameters.py @@ -31,8 +31,10 @@ ) from policyengine_api.services.v2.policies.services import V2PolicyService from policyengine_api.services.v2.policies.types import ( + CheckedPolicyRead, NativePolicyCreationInput, PolicyCreationResult, + PolicyRead, ) from tests.fixtures.recording_session import RecordingSession @@ -239,3 +241,16 @@ def test_created_policy_on_an_earlier_catalog_names_what_the_running_one_lacks( ) assert creation.item.missing_parameters == (REMOVED_PARAMETER,) + + +def test_only_a_checked_read_carries_missing_parameters( + engine, stored_policies: dict[str, Policy] +) -> None: + # A read that skipped the check cannot be mistaken for "catalog not + # published": only the checked type has the field. + policy = _service(engine).get_policy( + country_id="us", policy_id=stored_policies["running"].id + ) + + assert isinstance(policy, CheckedPolicyRead) + assert "missing_parameters" not in PolicyRead.__dataclass_fields__ diff --git a/tests/unit/v2/test_policy_routes.py b/tests/unit/v2/test_policy_routes.py index 487c57b12..c78a17a24 100644 --- a/tests/unit/v2/test_policy_routes.py +++ b/tests/unit/v2/test_policy_routes.py @@ -23,10 +23,10 @@ RouteImplementationSettings, ) from policyengine_api.services.v2.policies.types import ( + CheckedPolicyPage, + CheckedPolicyRead, NativePolicyCreation, - PolicyPage, PolicyParameterValueRead, - PolicyRead, ) from policyengine_api.services.v2.policies.validators import ( PolicyCatalogValidationError, @@ -48,8 +48,8 @@ def _policy_read( policy_id: UUID = POLICY_ID, country_id: str = "us", missing_parameters: tuple[str, ...] | None = (), -) -> PolicyRead: - return PolicyRead( +) -> CheckedPolicyRead: + return CheckedPolicyRead( id=policy_id, country_id=country_id, tax_benefit_model_id=MODEL_ID, @@ -86,18 +86,18 @@ def create_policy(self, command) -> NativePolicyCreation: self._raise() return NativePolicyCreation(item=_policy_read(), created=self.created) - def get_policy(self, **filters) -> PolicyRead: + def get_policy(self, **filters) -> CheckedPolicyRead: self.calls.append(("get_policy", filters)) self._raise() return _policy_read( policy_id=filters["policy_id"], missing_parameters=self.missing_parameters ) - def list_policies(self, **filters) -> PolicyPage: + def list_policies(self, **filters) -> CheckedPolicyPage: self.calls.append(("list_policies", filters)) self._raise() - return PolicyPage( - items=(_policy_read(),), + return CheckedPolicyPage( + items=(_policy_read(missing_parameters=self.missing_parameters),), offset=filters["offset"], limit=filters["limit"], has_more=False, @@ -306,6 +306,19 @@ def test_detail_reports_the_parameters_the_running_model_lacks( assert response.json()["result"]["item"]["missing_parameters"] == reported +def test_list_reports_each_policy_s_missing_parameters() -> None: + service = FakePolicyService() + service.missing_parameters = ("gov.example.removed_rate",) + client, _flask_calls = _client(service) + + response = client.get("/v2/policies?country_id=us") + + assert response.status_code == 200 + assert [ + item["missing_parameters"] for item in response.json()["result"]["items"] + ] == [["gov.example.removed_rate"]] + + def test_list_passes_exact_model_filter_and_canonical_pagination() -> None: service = FakePolicyService() client, _flask_calls = _client(service)