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..72d44052b --- /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. `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/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 e464e9949..1d85250fe 100644 --- a/docs/migration/stage-10-v2-policies.md +++ b/docs/migration/stage-10-v2-policies.md @@ -174,6 +174,28 @@ 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 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 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/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/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 0ae6b4887..771aaa12d 100644 --- a/policyengine_api/fastapi_routes/v2/policies/response_models.py +++ b/policyengine_api/fastapi_routes/v2/policies/response_models.py @@ -3,15 +3,18 @@ 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 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): @@ -52,9 +55,22 @@ 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: + def from_read(cls, item: CheckedPolicyRead) -> PolicyItem: """Build the response item by validating the service read record's fields.""" return cls.model_validate(item) @@ -78,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/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_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/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/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/policyengine_api/services/v2/policies/services.py b/policyengine_api/services/v2/policies/services.py index eb8250fd1..a77cea0be 100644 --- a/policyengine_api/services/v2/policies/services.py +++ b/policyengine_api/services/v2/policies/services.py @@ -5,6 +5,9 @@ from typing import TYPE_CHECKING from policyengine_api.constants import POLICYENGINE_VERSION +from policyengine_api.data.v2.catalog.catalog_selection import ( + MetadataCatalogNotPublishedError, +) from policyengine_api.services.v2.policies.database_connectors.creates import ( create_legacy_policy_mapping, create_parameter_values, @@ -25,6 +28,8 @@ 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, policy_read, @@ -33,6 +38,8 @@ ) from policyengine_api.services.v2.policies.types import ( CanonicalPolicyContent, + CheckedPolicyPage, + CheckedPolicyRead, LegacyPolicyPersistenceResult, LegacyPolicySnapshot, NativePolicyCreation, @@ -185,6 +192,50 @@ 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[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 + 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 MetadataCatalogNotPublishedError: + 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 + 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( + checked_policy_read( + policy, + 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 +366,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.""" + 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. + """ 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, @@ -331,16 +398,29 @@ def list_policies( tax_benefit_model_id: UUID | None = None, offset: int = 0, limit: int = 100, - ) -> PolicyPage: - """Return a page of policies, optionally for one tax-benefit model.""" + ) -> CheckedPolicyPage: + """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 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 8b20a6c74..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, @@ -31,7 +33,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 +185,38 @@ 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 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 3783d5e82..d5ef827d1 100644 --- a/policyengine_api/services/v2/policies/types.py +++ b/policyengine_api/services/v2/policies/types.py @@ -177,6 +177,18 @@ class PolicyRead: parameter_values: tuple[PolicyParameterValueRead, ...] +@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) class PolicyPage: """One page of policies in creation order; ``has_more`` says if more follow.""" @@ -187,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/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/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/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/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_mirroring.py b/tests/unit/services/test_policy_mirroring.py index 8459e0a0f..9394f3b88 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,6 +141,8 @@ def test_failure_is_secret_safe_and_records_cloud_sql_as_only_completed_write( def _catalog_query_failure() -> MetadataCatalogUnavailableError: + # 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 @@ -167,7 +170,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/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"] 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 new file mode 100644 index 000000000..83b896cc2 --- /dev/null +++ b/tests/unit/v2/test_policy_missing_parameters.py @@ -0,0 +1,256 @@ +"""Report the parameters a stored v2 policy sets that the running catalog lacks.""" + +from __future__ import annotations + +from collections import deque +from datetime import UTC, datetime +from uuid import uuid4 + +import pytest +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, + ParameterValue, + Policy, + 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, +) +from policyengine_api.services.v2.policies.database_session import ( + PolicyDatabaseSession, +) +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 + +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 + + +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) + + +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,) + + +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 1267a14d8..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, @@ -43,8 +43,13 @@ CREATED_AT = datetime(2026, 1, 1, tzinfo=UTC) -def _policy_read(*, policy_id: UUID = POLICY_ID, country_id: str = "us") -> PolicyRead: - return PolicyRead( +def _policy_read( + *, + policy_id: UUID = POLICY_ID, + country_id: str = "us", + missing_parameters: tuple[str, ...] | None = (), +) -> CheckedPolicyRead: + return CheckedPolicyRead( id=policy_id, country_id=country_id, tax_benefit_model_id=MODEL_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: @@ -79,16 +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"]) + 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, @@ -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,41 @@ 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_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) @@ -406,7 +451,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"]