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..003c56bf8 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=error.error_entries, + ) 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..e9fc7bdd3 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 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 + ] + + @dataclass(frozen=True) class PolicySetResult: """Existing v1 return values and an optional detached mirror snapshot.""" @@ -62,9 +111,19 @@ 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, 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 @property def _sessions(self) -> sessionmaker[Session]: @@ -176,8 +235,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 +297,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 +309,17 @@ 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) or frozenset() + 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/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/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) 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_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_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 new file mode 100644 index 000000000..45129b576 --- /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, sorted(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..48e8eae39 --- /dev/null +++ b/tests/unit/services/test_policy_service_parameter_paths.py @@ -0,0 +1,117 @@ +"""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, + model_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["path"] for entry in raised.value.error_entries] == [ + "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 == [] + + +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") == [] diff --git a/tests/unit/utils/test_input_validation.py b/tests/unit/utils/test_input_validation.py index 922e22ac1..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"}, }, } @@ -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`." + ), + } + ]