From b10522972f42882afc883c7506fd7600d8179aa2 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:32:43 +0400 Subject: [PATCH 1/4] Add failing tests for policy parameter path checks A new v1 policy may name only parameters that the running country model can set a value on. These tests cover unknown paths, parameter groups, scales, scale brackets and missing contributed parameters for the US and the UK, plus valid scale bracket and contributed parameters, and check that re-saving an identical stored policy still returns it. They also cover the household calculation check, which currently accepts parameter groups because the metadata lists them as parameterNode entries. Co-Authored-By: Claude Opus 5.5 --- tests/fixtures/policy_parameter_paths.py | 51 ++++++ .../test_calculate_policy_parameters.py | 112 +++++++++++++ .../routes/test_policy_parameter_paths.py | 154 ++++++++++++++++++ .../test_policy_service_parameter_paths.py | 98 +++++++++++ tests/unit/utils/test_input_validation.py | 28 ++++ 5 files changed, 443 insertions(+) create mode 100644 tests/fixtures/policy_parameter_paths.py create mode 100644 tests/unit/routes/test_calculate_policy_parameters.py create mode 100644 tests/unit/routes/test_policy_parameter_paths.py create mode 100644 tests/unit/services/test_policy_service_parameter_paths.py diff --git a/tests/fixtures/policy_parameter_paths.py b/tests/fixtures/policy_parameter_paths.py new file mode 100644 index 000000000..dc9f70408 --- /dev/null +++ b/tests/fixtures/policy_parameter_paths.py @@ -0,0 +1,51 @@ +"""Policy paths in the installed US and UK models, for parameter-path checks.""" + +import pytest + +POLICY_PERIOD = "2026-01-01.2026-12-31" + +# Paths that do not name a parameter a policy can set a value on, by country. +NON_PARAMETER_PATHS = { + "us": { + "unknown path": "gov.x", + "parameter group": "gov.irs.credits.ctc.amount", + "parameter scale": "gov.irs.credits.ctc.amount.base", + "scale bracket": "gov.irs.credits.ctc.amount.base[0]", + "missing contributed parameter": "gov.contrib.states.mt.hb1.in_effect", + }, + "uk": { + "unknown path": "gov.x", + "parameter group": "gov.hmrc.income_tax.rates", + "parameter scale": "gov.hmrc.income_tax.rates.uk", + "scale bracket": "gov.hmrc.income_tax.rates.uk[0]", + "missing contributed parameter": ( + "gov.contrib.ubi_center.basic_income.in_effect" + ), + }, +} + +# Parameters a policy can set: a scale bracket's value, a contributed parameter +# and a contributed scale bracket's value. +SETTABLE_PARAMETER_PATHS = { + "us": ( + "gov.irs.credits.ctc.amount.base[0].amount", + "gov.contrib.states.mt.ctc.in_effect", + "gov.contrib.states.mt.ctc.amount[0].amount", + ), + "uk": ( + "gov.hmrc.income_tax.rates.uk[0].rate", + "gov.contrib.ubi_center.basic_income.amount.flat", + "gov.contrib.ubi_center.wealth_tax[0].rate", + ), +} + +NON_PARAMETER_CASES = [ + pytest.param(country_id, path, id=f"{country_id}-{description}") + for country_id, paths_by_description in NON_PARAMETER_PATHS.items() + for description, path in paths_by_description.items() +] + + +def policy_data(*parameter_paths: str) -> dict[str, dict[str, int]]: + """Return a policy that sets each path to 1 for the test period.""" + return {parameter_path: {POLICY_PERIOD: 1} for parameter_path in parameter_paths} diff --git a/tests/unit/routes/test_calculate_policy_parameters.py b/tests/unit/routes/test_calculate_policy_parameters.py new file mode 100644 index 000000000..d5debbe52 --- /dev/null +++ b/tests/unit/routes/test_calculate_policy_parameters.py @@ -0,0 +1,112 @@ +"""A household calculation's policy may set only parameters, not parameter groups. + +The checks use the metadata of the loaded US and UK models. The metadata lists +groups, scales and scale brackets too, with the type ``parameterNode``. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest +from flask import Flask + +from policyengine_api.country import COUNTRIES +from policyengine_api.extensions import cache +from policyengine_api.routes import household_routes +from policyengine_api.services.household_calculation_service import ( + HouseholdCalculationService, +) +from tests.fixtures.policy_parameter_paths import ( + NON_PARAMETER_CASES, + POLICY_PERIOD, + SETTABLE_PARAMETER_PATHS, +) + +HOUSEHOLD = {"people": {"you": {"age": {"2026": 40}}}} + + +class MetadataOnlyCountry: + """A loaded model's metadata, with a calculation that must never be reached.""" + + def __init__(self, country_id: str) -> None: + self.metadata = COUNTRIES[country_id].metadata + + def calculate(self, *_args, **_kwargs): + pytest.fail("a refused calculation request must not be calculated") + + +def _calculation_service(country_id: str) -> HouseholdCalculationService: + country = MetadataOnlyCountry(country_id) + return HouseholdCalculationService(country_provider=lambda: {country_id: country}) + + +def _calculate_client(monkeypatch, country_id: str): + monkeypatch.setattr( + household_routes, + "household_calculation_service", + _calculation_service(country_id), + ) + app = Flask(__name__) + app.config.update(TESTING=True, CACHE_TYPE="NullCache") + cache.init_app(app) + app.register_blueprint(household_routes.household_bp) + return app.test_client() + + +@pytest.mark.parametrize(("country_id", "non_parameter_path"), NON_PARAMETER_CASES) +def test_calculate_refuses_a_policy_path_that_is_not_a_parameter( + monkeypatch, + country_id, + non_parameter_path, +): + client = _calculate_client(monkeypatch, country_id) + + with patch.object( + household_routes, + "start_observability_id", + ) as start_observability_id: + response = client.post( + f"/{country_id}/calculate", + json={ + "household": HOUSEHOLD, + "policy": {non_parameter_path: {POLICY_PERIOD: 1}}, + }, + ) + + assert response.status_code == 400 + body = response.get_json() + assert body["status"] == "error" + assert body["result"] is None + assert body["message"] == ( + "Unrecognized calculate input(s): Unrecognized policy parameter " + f"`{non_parameter_path}` at `policy.{non_parameter_path}`." + ) + assert body["errors"] == [ + { + "type": "policy_parameter", + "name": non_parameter_path, + "path": f"policy.{non_parameter_path}", + "message": ( + f"Unrecognized policy parameter `{non_parameter_path}` at " + f"`policy.{non_parameter_path}`." + ), + } + ] + start_observability_id.assert_not_called() + + +@pytest.mark.parametrize("country_id", ["us", "uk"]) +def test_calculation_accepts_bracket_and_contributed_parameters(country_id): + policy = { + parameter_path: {POLICY_PERIOD: 1} + for parameter_path in SETTABLE_PARAMETER_PATHS[country_id] + } + + prepared = _calculation_service(country_id).prepare_household_calculation( + country_id, + HOUSEHOLD, + policy, + ) + + assert prepared.policy_json == policy diff --git a/tests/unit/routes/test_policy_parameter_paths.py b/tests/unit/routes/test_policy_parameter_paths.py new file mode 100644 index 000000000..6403e04ee --- /dev/null +++ b/tests/unit/routes/test_policy_parameter_paths.py @@ -0,0 +1,154 @@ +"""A new v1 policy may set only parameters that the running country model defines. + +Re-saving a policy identical to a stored one returns the stored policy, whatever +its parameter paths, so policies that use since-removed parameters still resolve. +""" + +from __future__ import annotations + +import pytest +from flask import Flask +from sqlalchemy import select + +from policyengine_api.data.v1_models import Policy +from policyengine_api.routes.policy_routes import policy_bp +from policyengine_api.utils import hash_object +from tests.fixtures.policy_parameter_paths import ( + NON_PARAMETER_CASES, + NON_PARAMETER_PATHS, + SETTABLE_PARAMETER_PATHS, + policy_data, +) + + +@pytest.fixture(autouse=True) +def cloud_sql_only_writes(monkeypatch): + monkeypatch.setenv("DB_WRITE_POLICY", "cloud_sql") + + +def _policy_client(): + app = Flask(__name__) + app.config["TESTING"] = True + app.register_blueprint(policy_bp) + return app.test_client() + + +def _stored_policies(orm_session_factory) -> list[Policy]: + with orm_session_factory() as session: + return list(session.scalars(select(Policy))) + + +def _assert_rejected_paths(response, country_id: str, rejected_paths: list[str]): + assert response.status_code == 400 + assert response.mimetype == "application/json" + body = response.get_json() + assert body["status"] == "error" + assert body["result"] is None + for rejected_path in rejected_paths: + assert rejected_path in body["message"] + assert body["errors"] == [ + { + "type": "policy_parameter", + "name": rejected_path, + "path": f"data.{rejected_path}", + "message": ( + f"Unrecognized policy parameter `{rejected_path}` at " + f"`data.{rejected_path}`." + ), + } + for rejected_path in rejected_paths + ] + assert f"policyengine-{country_id}" in body["message"] + + +@pytest.mark.parametrize(("country_id", "non_parameter_path"), NON_PARAMETER_CASES) +def test_new_policy_naming_a_path_that_is_not_a_parameter_returns_400( + orm_session_factory, + country_id, + non_parameter_path, +): + response = _policy_client().post( + f"/{country_id}/policy", + json={"label": "Rejected", "data": policy_data(non_parameter_path)}, + ) + + _assert_rejected_paths(response, country_id, [non_parameter_path]) + assert _stored_policies(orm_session_factory) == [] + + +@pytest.mark.parametrize("country_id", ["us", "uk"]) +def test_new_policy_400_lists_every_path_that_is_not_a_parameter( + orm_session_factory, + country_id, +): + non_parameter_paths = list(NON_PARAMETER_PATHS[country_id].values()) + settable_path = SETTABLE_PARAMETER_PATHS[country_id][0] + requested_policy = policy_data( + non_parameter_paths[0], + settable_path, + *non_parameter_paths[1:], + ) + + response = _policy_client().post( + f"/{country_id}/policy", + json={"label": "Several rejected paths", "data": requested_policy}, + ) + + _assert_rejected_paths(response, country_id, non_parameter_paths) + assert settable_path not in response.get_json()["message"] + assert _stored_policies(orm_session_factory) == [] + + +@pytest.mark.parametrize("country_id", ["us", "uk"]) +def test_new_policy_setting_bracket_and_contributed_parameters_is_created( + orm_session_factory, + country_id, +): + requested_policy = policy_data(*SETTABLE_PARAMETER_PATHS[country_id]) + + response = _policy_client().post( + f"/{country_id}/policy", + json={"label": "Settable parameters", "data": requested_policy}, + ) + + assert response.status_code == 201 + stored_policies = _stored_policies(orm_session_factory) + assert [policy.id for policy in stored_policies] == [ + response.get_json()["result"]["policy_id"] + ] + assert stored_policies[0].policy_json == requested_policy + + +@pytest.mark.parametrize("label", ["Montana HB1", None]) +def test_resaving_a_stored_policy_with_removed_parameters_returns_it( + orm_session_factory, + label, +): + requested_policy = policy_data( + NON_PARAMETER_PATHS["us"]["missing contributed parameter"], + NON_PARAMETER_PATHS["us"]["parameter group"], + ) + with orm_session_factory.begin() as session: + session.add( + Policy( + id=98768, + country_id="us", + label=label, + api_version="2.0.0", + policy_json=requested_policy, + policy_hash=hash_object(requested_policy), + ) + ) + + response = _policy_client().post( + "/us/policy", + json={"label": label, "data": requested_policy}, + ) + + assert response.status_code == 200 + assert response.get_json() == { + "status": "ok", + "message": "Policy already exists", + "result": {"policy_id": 98768}, + } + assert [policy.id for policy in _stored_policies(orm_session_factory)] == [98768] diff --git a/tests/unit/services/test_policy_service_parameter_paths.py b/tests/unit/services/test_policy_service_parameter_paths.py new file mode 100644 index 000000000..48e0f75c5 --- /dev/null +++ b/tests/unit/services/test_policy_service_parameter_paths.py @@ -0,0 +1,98 @@ +"""PolicyService checks a new policy's parameter paths against a parameter catalog.""" + +import pytest + +from policyengine_api.data.v1_models import Policy +from policyengine_api.services.policy_service import ( + PolicyService, + UnsettableParameterPathsError, +) +from policyengine_api.utils import hash_object + +SETTABLE_NAMES = frozenset({"gov.tax.rate", "gov.tax.brackets[0].rate"}) + + +class RecordingParameterCatalog: + """Settable parameter names for every country, recording each lookup.""" + + def __init__(self) -> None: + self.looked_up_country_ids: list[str] = [] + + def __call__(self, country_id: str) -> frozenset[str]: + self.looked_up_country_ids.append(country_id) + return SETTABLE_NAMES + + +@pytest.fixture +def parameter_catalog(): + return RecordingParameterCatalog() + + +@pytest.fixture +def service(orm_session_factory, parameter_catalog): + return PolicyService( + orm_session_factory, + settable_parameter_names=parameter_catalog, + ) + + +def test_new_policy_with_paths_outside_the_catalog_is_refused_and_not_stored( + service, + parameter_catalog, +): + with pytest.raises(UnsettableParameterPathsError) as raised: + service.set_policy( + "US", + "Refused", + { + "gov.tax": {"2026-01-01.2026-12-31": 1}, + "gov.tax.rate": {"2026-01-01.2026-12-31": 0.2}, + "gov.x": {"2026-01-01.2026-12-31": 1}, + }, + ) + + assert raised.value.country_id == "us" + assert raised.value.parameter_paths == ("gov.tax", "gov.x") + assert [entry.to_dict()["path"] for entry in raised.value.invalid_inputs] == [ + "data.gov.tax", + "data.gov.x", + ] + assert parameter_catalog.looked_up_country_ids == ["us"] + assert service.search_policies("us", "Refused") == [] + + +def test_new_policy_with_catalog_paths_is_stored(service): + policy_json = { + "gov.tax.rate": {"2026-01-01.2026-12-31": 0.2}, + "gov.tax.brackets[0].rate": {"2026-01-01.2026-12-31": 0.1}, + } + + result = service.set_policy("us", "Accepted", policy_json) + + assert result.is_existing_policy is False + assert service.get_policy_json("us", result.policy_id) == policy_json + + +def test_existing_identical_policy_is_returned_without_checking_its_paths( + service, + orm_session_factory, + parameter_catalog, +): + removed_parameter_policy = {"gov.removed": {"2026-01-01.2026-12-31": 1}} + with orm_session_factory.begin() as session: + session.add( + Policy( + id=98768, + country_id="us", + label="Stored", + api_version="2.0.0", + policy_json=removed_parameter_policy, + policy_hash=hash_object(removed_parameter_policy), + ) + ) + + result = service.set_policy("us", "Stored", removed_parameter_policy) + + assert result.is_existing_policy is True + assert result.policy_id == 98768 + assert parameter_catalog.looked_up_country_ids == [] diff --git a/tests/unit/utils/test_input_validation.py b/tests/unit/utils/test_input_validation.py index 922e22ac1..4625e1d98 100644 --- a/tests/unit/utils/test_input_validation.py +++ b/tests/unit/utils/test_input_validation.py @@ -66,3 +66,31 @@ def test__find_unrecognized_inputs__reports_unknown_entity_variable_axis_and_pol "household.axes[0][0].name", "policy.gov.irs.income.exemption.amunt", ] + + +def test__find_unrecognized_inputs__reports_policy_keys_that_are_parameter_groups(): + metadata = { + **METADATA, + "parameters": { + "gov.irs.income.exemption": {"type": "parameterNode"}, + "gov.irs.income.exemption.amount": {"type": "parameter"}, + }, + } + policy = { + "gov.irs.income.exemption": {"2025-01-01.2025-12-31": 100}, + "gov.irs.income.exemption.amount": {"2025-01-01.2025-12-31": 100}, + } + + errors = find_unrecognized_inputs({}, policy, metadata) + + assert [error.to_dict() for error in errors] == [ + { + "type": "policy_parameter", + "name": "gov.irs.income.exemption", + "path": "policy.gov.irs.income.exemption", + "message": ( + "Unrecognized policy parameter `gov.irs.income.exemption` at " + "`policy.gov.irs.income.exemption`." + ), + } + ] From 49074079e12482ab608f38fae7cf78415bd7c2f1 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:42:46 +0400 Subject: [PATCH 2/4] Refuse new policies whose data names paths that are not parameters POST //policy now checks every key in data when the request would store a new policy. Each key must name a Parameter in the country's loaded tax-benefit system, the one the Public API uses for its metadata and household calculations, so scale bracket values and contributed parameters the model defines are accepted. Unknown paths, parameter groups, scales and scale brackets get a 400 whose message and errors list every refused path. Re-saving a policy identical to a stored one still returns it with a 200, without checking its paths. The household calculation check tested only whether a policy key appears in the metadata's parameters, which also lists groups, scales and brackets as parameterNode entries. It now accepts only entries of type parameter, so a group gets the existing 400 instead of reaching the model and failing with a 500. The metadata is unchanged. Fixes #4122 Co-Authored-By: Claude Opus 5.5 --- changelog.d/4122.fixed.md | 1 + policyengine_api/openapi_spec.yaml | 35 ++++++++ policyengine_api/routes/policy_routes.py | 38 +++++--- policyengine_api/services/policy_service.py | 87 +++++++++++++++++-- policyengine_api/utils/input_validation.py | 17 +++- .../test_calculate_deprecated_inputs.py | 2 +- tests/unit/routes/test_canonical_spm.py | 2 +- ...st_household_and_user_policy_orm_routes.py | 2 +- .../routes/test_policy_dual_write_routes.py | 12 ++- .../routes/test_policy_parameter_paths.py | 2 +- .../test_policy_service_parameter_paths.py | 2 +- tests/unit/utils/test_input_validation.py | 2 +- 12 files changed, 176 insertions(+), 26 deletions(-) create mode 100644 changelog.d/4122.fixed.md diff --git a/changelog.d/4122.fixed.md b/changelog.d/4122.fixed.md new file mode 100644 index 000000000..03617703a --- /dev/null +++ b/changelog.d/4122.fixed.md @@ -0,0 +1 @@ +Creating a new policy now returns a 400 that lists every key in `data` that is not a parameter in the country model, such as an unknown path or a parameter group; re-saving a policy identical to a stored one still returns it. Household calculations now refuse parameter groups with their existing 400 instead of failing with a 500. diff --git a/policyengine_api/openapi_spec.yaml b/policyengine_api/openapi_spec.yaml index 49eeaa24a..576301a33 100644 --- a/policyengine_api/openapi_spec.yaml +++ b/policyengine_api/openapi_spec.yaml @@ -123,6 +123,41 @@ paths: properties: policy_id: type: integer + "400": + description: >- + Invalid request body, or a new policy whose data names paths that + are not parameters in the country model, such as unknown paths and + parameter groups. For those paths, the message and the errors list + name every one. Re-saving a policy identical to a stored one returns + the stored policy without checking its paths. + content: + application/json: + schema: + type: object + properties: + status: + type: string + message: + type: string + result: + type: object + nullable: true + errors: + type: array + description: Present when data names paths that are not parameters. + items: + type: object + required: [type, name, path, message] + properties: + type: + type: string + enum: [policy_parameter] + name: + type: string + path: + type: string + message: + type: string 404: description: Invalid country ID. content: diff --git a/policyengine_api/routes/policy_routes.py b/policyengine_api/routes/policy_routes.py index 9226f3abf..90c63e70d 100644 --- a/policyengine_api/routes/policy_routes.py +++ b/policyengine_api/routes/policy_routes.py @@ -28,8 +28,14 @@ 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.policy_parameter_service import ( + PolicyParameterService, + loaded_model_parameter_names, +) +from policyengine_api.services.policy_service import ( + PolicyService, + UnsettableParameterPathsError, +) from policyengine_api.services.user_policy_mirroring import ( UserPolicyMirrorCollaborators, UserPolicyMirrorUnavailableError, @@ -48,7 +54,7 @@ ) policy_bp = Blueprint("policy", __name__) -policy_service = PolicyService() +policy_service = PolicyService(model_parameter_names=loaded_model_parameter_names) policy_parameter_service = PolicyParameterService() user_policy_service = UserPolicyService() @@ -295,8 +301,10 @@ def set_policy(country_id: str) -> Response: A policy is identical when its country, content hash and label all match; that case returns 200, a new policy returns 201. An invalid payload gives - 400, and an unavailable write configuration gives 503. Under dual-write, a - failed v2 copy is logged and does not change the response. + 400, as does a new policy whose ``data`` names paths that are not parameters + in the country model; that 400 lists every such path. An unavailable write + configuration gives 503. Under dual-write, a failed v2 copy is logged and + does not change the response. Args: country_id (str): Country to store the policy under. @@ -318,12 +326,20 @@ def set_policy(country_id: str) -> Response: policy_json = payload.pop("data", None) mirror_to_v2 = _should_mirror_to_v2(country_id, write_source) - creation = policy_service.set_policy( - country_id, - label, - policy_json, - prepare_for_mirroring=mirror_to_v2, - ) + try: + creation = policy_service.set_policy( + country_id, + label, + policy_json, + prepare_for_mirroring=mirror_to_v2, + ) + except UnsettableParameterPathsError as error: + return _make_error_response( + error, + 400, + result=None, + errors=[invalid_input.to_dict() for invalid_input in error.invalid_inputs], + ) policy_id, message, is_existing_policy = creation if mirror_to_v2: diff --git a/policyengine_api/services/policy_service.py b/policyengine_api/services/policy_service.py index 01c3851e8..ad719a9ac 100644 --- a/policyengine_api/services/policy_service.py +++ b/policyengine_api/services/policy_service.py @@ -1,4 +1,8 @@ -"""v1 policy storage: lookup, label search and deduplicated creation.""" +"""v1 policy storage: lookup, label search and deduplicated creation. + +A new policy may set only parameters that the running country model defines; +re-saving a policy identical to a stored one returns the stored policy. +""" from __future__ import annotations @@ -8,14 +12,22 @@ from sqlalchemy import select -from policyengine_api.constants import COUNTRY_PACKAGE_VERSIONS +from policyengine_api.constants import ( + COUNTRIES, + COUNTRY_PACKAGE_NAMES, + COUNTRY_PACKAGE_VERSIONS, +) from policyengine_api.data.orm import get_v1_session_factory from policyengine_api.data.v1_models import Policy from policyengine_api.services.v2.policies.types import LegacyPolicySnapshot +from policyengine_api.services.v2.policies.validators import ( + parameters_missing_from_model, +) from policyengine_api.utils import hash_object +from policyengine_api.utils.input_validation import UnrecognizedInput if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Callable, Container, Iterator from sqlalchemy.orm import Session, sessionmaker @@ -40,6 +52,43 @@ def __init__(self, *policy_ids: int) -> None: super().__init__(*policy_ids) +_COUNTRY_PACKAGE_NAME_BY_COUNTRY_ID = dict( + zip(COUNTRIES, COUNTRY_PACKAGE_NAMES, strict=True) +) + + +class UnsettableParameterPathsError(ValueError): + """Raised when a new policy names paths that are not settable parameters. + + A path is refused when the running country model does not define it, or + defines it as a parameter group, scale or scale bracket rather than a + parameter that holds values. + """ + + def __init__(self, country_id: str, parameter_paths: tuple[str, ...]) -> None: + """Keep the refused paths, sorted, for the 400 response.""" + self.country_id = country_id + self.parameter_paths = tuple(sorted(parameter_paths)) + package_name = _COUNTRY_PACKAGE_NAME_BY_COUNTRY_ID[country_id].replace("_", "-") + super().__init__( + "Policy data contains keys that are not parameters in " + f"{package_name} {COUNTRY_PACKAGE_VERSIONS[country_id]}: " + + ", ".join(self.parameter_paths) + ) + + @property + def invalid_inputs(self) -> list[UnrecognizedInput]: + """Describe each refused path as an entry for the response's ``errors``.""" + return [ + UnrecognizedInput( + input_type="policy_parameter", + name=parameter_path, + path=f"data.{parameter_path}", + ) + for parameter_path in self.parameter_paths + ] + + @dataclass(frozen=True) class PolicySetResult: """Existing v1 return values and an optional detached mirror snapshot.""" @@ -62,9 +111,18 @@ class PolicyService: def __init__( self, session_factory: sessionmaker[Session] | None = None, + model_parameter_names: Callable[[str], Container[str] | None] | None = None, ) -> None: - """Use this session factory, or the application's v1 one if omitted.""" + """Use these collaborators, or the defaults for those omitted. + + ``session_factory`` defaults to the application's v1 one. + ``model_parameter_names`` returns, for a country ID, the leaf parameter + paths of its model, the parameters a new policy may set, or None when + the country has no loaded model. Without it, ``set_policy`` stores any + parameter path; the policy route always supplies it. + """ self._injected_session_factory = session_factory + self._model_parameter_names = model_parameter_names @property def _sessions(self) -> sessionmaker[Session]: @@ -176,8 +234,11 @@ def set_policy( ) -> PolicySetResult: """Store a policy, or return the existing one with the same content and label. - Raise ValueError for an unknown country. With ``prepare_for_mirroring`` the - result carries a snapshot of the stored row, for copying the policy into v2. + Raise ValueError for an unknown country, and UnsettableParameterPathsError + when a new policy names paths that are not settable parameters. An + existing identical policy is returned without checking its paths. With + ``prepare_for_mirroring`` the result carries a snapshot of the stored row, + for copying the policy into v2. """ country_id = country_id.lower() if country_id not in COUNTRY_PACKAGE_VERSIONS: @@ -235,6 +296,7 @@ def _set_policy( if existing is not None: return existing, "Policy already exists", True + self._check_parameter_paths(country_id, policy_json) policy = Policy( country_id=country_id, label=label, @@ -246,6 +308,19 @@ def _set_policy( session.flush() return policy, "Policy created", False + def _check_parameter_paths(self, country_id: str, policy_json: dict) -> None: + """Refuse paths the model lacks, compared as the v2 copy compares them.""" + if self._model_parameter_names is None or not policy_json: + return + model_parameter_names = self._model_parameter_names(country_id) + if model_parameter_names is None: + return + unsettable_paths = parameters_missing_from_model( + policy_json, model_parameter_names + ) + if unsettable_paths: + raise UnsettableParameterPathsError(country_id, unsettable_paths) + def _get_unique_policy_with_label( self, session: Session, diff --git a/policyengine_api/utils/input_validation.py b/policyengine_api/utils/input_validation.py index 724cbfee1..499988768 100644 --- a/policyengine_api/utils/input_validation.py +++ b/policyengine_api/utils/input_validation.py @@ -205,7 +205,12 @@ def find_unrecognized_policy_inputs( policy: dict, metadata: dict, ) -> list[UnrecognizedInput]: - """Find policy keys that are not parameters in the country metadata.""" + """Find policy keys that are not parameters in the country metadata. + + The metadata's ``parameters`` also lists parameter groups, scales and scale + brackets, with the type ``parameterNode``. They hold no values of their own, + so a key naming one is reported like a name the model does not define. + """ if not isinstance(policy, dict): return [] @@ -217,10 +222,18 @@ def find_unrecognized_policy_inputs( path=f"policy.{parameter_name}", ) for parameter_name in policy - if parameter_name not in parameters + if not _is_settable_parameter(parameters.get(parameter_name)) ] +def _is_settable_parameter(parameter_metadata: object) -> bool: + """Say whether a metadata entry describes a parameter that holds values.""" + return ( + isinstance(parameter_metadata, dict) + and parameter_metadata.get("type") == "parameter" + ) + + def format_invalid_inputs_message(errors: Sequence[InvalidInput]) -> str: """Join the problems' messages into one error message for the response. diff --git a/tests/unit/routes/test_calculate_deprecated_inputs.py b/tests/unit/routes/test_calculate_deprecated_inputs.py index 1576c53b2..5ba3d356b 100644 --- a/tests/unit/routes/test_calculate_deprecated_inputs.py +++ b/tests/unit/routes/test_calculate_deprecated_inputs.py @@ -37,7 +37,7 @@ def __init__(self): }, }, "parameters": { - "gov.irs.income.exemption.amount": {}, + "gov.irs.income.exemption.amount": {"type": "parameter"}, }, } diff --git a/tests/unit/routes/test_canonical_spm.py b/tests/unit/routes/test_canonical_spm.py index c10ebf22a..02ba000fc 100644 --- a/tests/unit/routes/test_canonical_spm.py +++ b/tests/unit/routes/test_canonical_spm.py @@ -46,7 +46,7 @@ class Country: "age": {"entity": "person", "definitionPeriod": "year", "name": "age"} }, "entities": {"person": {"plural": "people", "roles": {}}}, - "parameters": {"gov.test.amount": {}}, + "parameters": {"gov.test.amount": {"type": "parameter"}}, } def __init__(self): diff --git a/tests/unit/routes/test_household_and_user_policy_orm_routes.py b/tests/unit/routes/test_household_and_user_policy_orm_routes.py index 46efdcd1b..bdccbe327 100644 --- a/tests/unit/routes/test_household_and_user_policy_orm_routes.py +++ b/tests/unit/routes/test_household_and_user_policy_orm_routes.py @@ -123,7 +123,7 @@ def test_household_under_policy_calculates_and_caches_json_as_an_object( metadata={ "variables": {}, "entities": {"person": {"plural": "people", "roles": {}}}, - "parameters": {"gov.example.parameter": {}}, + "parameters": {"gov.example.parameter": {"type": "parameter"}}, }, ) service = HouseholdCalculationService( diff --git a/tests/unit/routes/test_policy_dual_write_routes.py b/tests/unit/routes/test_policy_dual_write_routes.py index 03ef27ece..0f5be68e3 100644 --- a/tests/unit/routes/test_policy_dual_write_routes.py +++ b/tests/unit/routes/test_policy_dual_write_routes.py @@ -225,6 +225,16 @@ def test_destination_failure_returns_the_stored_policy_and_logs_an_error( ) -> None: """Run the real policy and copy services, with only the v2 write failing.""" monkeypatch.setenv("DB_WRITE_POLICY", "dual_write") + # The real service creates only policies that set parameters the US model + # defines. + body = { + "label": "Legacy label", + "data": { + "gov.irs.credits.ctc.amount.base[0].amount": { + "2026-01-01.2026-12-31": 2500, + }, + }, + } failing_destination = MagicMock() failing_destination.mirror_legacy_policy.side_effect = LegacyPolicyTranslationError( "private parameter value", @@ -237,7 +247,7 @@ def test_destination_failure_returns_the_stored_policy_and_logs_an_error( ), patch("policyengine_api.services.policy_mirroring.logger") as copy_logger, ): - response = _client().post("/us/policy", json=_body()) + response = _client().post("/us/policy", json=body) assert response.status_code == 201 assert response.json["message"] == "Policy created" diff --git a/tests/unit/routes/test_policy_parameter_paths.py b/tests/unit/routes/test_policy_parameter_paths.py index 6403e04ee..45129b576 100644 --- a/tests/unit/routes/test_policy_parameter_paths.py +++ b/tests/unit/routes/test_policy_parameter_paths.py @@ -94,7 +94,7 @@ def test_new_policy_400_lists_every_path_that_is_not_a_parameter( json={"label": "Several rejected paths", "data": requested_policy}, ) - _assert_rejected_paths(response, country_id, non_parameter_paths) + _assert_rejected_paths(response, country_id, sorted(non_parameter_paths)) assert settable_path not in response.get_json()["message"] assert _stored_policies(orm_session_factory) == [] diff --git a/tests/unit/services/test_policy_service_parameter_paths.py b/tests/unit/services/test_policy_service_parameter_paths.py index 48e0f75c5..778c75301 100644 --- a/tests/unit/services/test_policy_service_parameter_paths.py +++ b/tests/unit/services/test_policy_service_parameter_paths.py @@ -32,7 +32,7 @@ def parameter_catalog(): def service(orm_session_factory, parameter_catalog): return PolicyService( orm_session_factory, - settable_parameter_names=parameter_catalog, + model_parameter_names=parameter_catalog, ) diff --git a/tests/unit/utils/test_input_validation.py b/tests/unit/utils/test_input_validation.py index 4625e1d98..e2bbc264a 100644 --- a/tests/unit/utils/test_input_validation.py +++ b/tests/unit/utils/test_input_validation.py @@ -21,7 +21,7 @@ }, }, "parameters": { - "gov.irs.income.exemption.amount": {}, + "gov.irs.income.exemption.amount": {"type": "parameter"}, }, } From 953f6bb9aae364820e0a27af7eb6a052f9518cf4 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 22:48:19 +0400 Subject: [PATCH 3/4] Use a real US parameter in the legacy policy creation test The test created a policy that sets sample_parameter, which the US model does not define, so policy creation now refuses it with a 400. It sets gov.contrib.states.mt.ctc.in_effect instead. Co-Authored-By: Claude Opus 5.5 --- tests/to_refactor/python/test_policy.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/to_refactor/python/test_policy.py b/tests/to_refactor/python/test_policy.py index 9e38c1727..6011fc5a7 100644 --- a/tests/to_refactor/python/test_policy.py +++ b/tests/to_refactor/python/test_policy.py @@ -11,7 +11,10 @@ class TestPolicyCreation: # Define the policy to test against country_id = "us" - policy_json: ClassVar[dict] = {"sample_parameter": {"2024-01-01.2025-12-31": True}} + # A new policy may set only parameters the US model defines. + policy_json: ClassVar[dict] = { + "gov.contrib.states.mt.ctc.in_effect": {"2024-01-01.2025-12-31": True} + } label = "dworkin" test_policy: ClassVar[dict] = {"data": policy_json, "label": label} policy_hash = hash_object(policy_json) From af1eba9959cd7be4f3a9b01819fa5681330e0eb0 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:57:21 +0400 Subject: [PATCH 4/4] Build refused-path errors once and refuse policies with no loaded model UnsettableParameterPathsError now returns the 400 response's errors entries itself, so the policy route passes them through instead of converting each one. A model parameter lookup that returns None, for a country without a loaded model, now leaves a new policy no parameters to set, so every path is refused. Every country the policy service accepts has a loaded model, so this replaces a branch that could not be reached in production, and a service test covers it. Co-Authored-By: Claude Opus 5.5 --- policyengine_api/routes/policy_routes.py | 2 +- policyengine_api/services/policy_service.py | 15 +++++++------ .../test_policy_service_parameter_paths.py | 21 ++++++++++++++++++- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/policyengine_api/routes/policy_routes.py b/policyengine_api/routes/policy_routes.py index 90c63e70d..003c56bf8 100644 --- a/policyengine_api/routes/policy_routes.py +++ b/policyengine_api/routes/policy_routes.py @@ -338,7 +338,7 @@ def set_policy(country_id: str) -> Response: error, 400, result=None, - errors=[invalid_input.to_dict() for invalid_input in error.invalid_inputs], + errors=error.error_entries, ) policy_id, message, is_existing_policy = creation diff --git a/policyengine_api/services/policy_service.py b/policyengine_api/services/policy_service.py index ad719a9ac..e9fc7bdd3 100644 --- a/policyengine_api/services/policy_service.py +++ b/policyengine_api/services/policy_service.py @@ -77,14 +77,14 @@ def __init__(self, country_id: str, parameter_paths: tuple[str, ...]) -> None: ) @property - def invalid_inputs(self) -> list[UnrecognizedInput]: - """Describe each refused path as an entry for the response's ``errors``.""" + def error_entries(self) -> list[dict]: + """Return the 400 response's ``errors``: one entry for each refused path.""" return [ UnrecognizedInput( input_type="policy_parameter", name=parameter_path, path=f"data.{parameter_path}", - ) + ).to_dict() for parameter_path in self.parameter_paths ] @@ -118,8 +118,9 @@ def __init__( ``session_factory`` defaults to the application's v1 one. ``model_parameter_names`` returns, for a country ID, the leaf parameter paths of its model, the parameters a new policy may set, or None when - the country has no loaded model. Without it, ``set_policy`` stores any - parameter path; the policy route always supplies it. + the country has no loaded model, which leaves a new policy no + parameters to set. Without it, ``set_policy`` stores any parameter + path; the policy route always supplies it. """ self._injected_session_factory = session_factory self._model_parameter_names = model_parameter_names @@ -312,9 +313,7 @@ def _check_parameter_paths(self, country_id: str, policy_json: dict) -> None: """Refuse paths the model lacks, compared as the v2 copy compares them.""" if self._model_parameter_names is None or not policy_json: return - model_parameter_names = self._model_parameter_names(country_id) - if model_parameter_names is None: - return + model_parameter_names = self._model_parameter_names(country_id) or frozenset() unsettable_paths = parameters_missing_from_model( policy_json, model_parameter_names ) diff --git a/tests/unit/services/test_policy_service_parameter_paths.py b/tests/unit/services/test_policy_service_parameter_paths.py index 778c75301..48e8eae39 100644 --- a/tests/unit/services/test_policy_service_parameter_paths.py +++ b/tests/unit/services/test_policy_service_parameter_paths.py @@ -53,7 +53,7 @@ def test_new_policy_with_paths_outside_the_catalog_is_refused_and_not_stored( assert raised.value.country_id == "us" assert raised.value.parameter_paths == ("gov.tax", "gov.x") - assert [entry.to_dict()["path"] for entry in raised.value.invalid_inputs] == [ + assert [entry["path"] for entry in raised.value.error_entries] == [ "data.gov.tax", "data.gov.x", ] @@ -96,3 +96,22 @@ def test_existing_identical_policy_is_returned_without_checking_its_paths( assert result.is_existing_policy is True assert result.policy_id == 98768 assert parameter_catalog.looked_up_country_ids == [] + + +def test_new_policy_for_a_country_without_a_loaded_model_is_refused( + orm_session_factory, +): + service_without_model = PolicyService( + orm_session_factory, + model_parameter_names=lambda _country_id: None, + ) + + with pytest.raises(UnsettableParameterPathsError) as raised: + service_without_model.set_policy( + "us", + "No loaded model", + {"gov.tax.rate": {"2026-01-01.2026-12-31": 0.2}}, + ) + + assert raised.value.parameter_paths == ("gov.tax.rate",) + assert service_without_model.search_policies("us", "No loaded model") == []