Refuse new policies whose data names paths that are not parameters

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.

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:

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:

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,

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.
 

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}

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)

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"},
             },
         }