Warn on unknown keys beside values: in parameter YAML

changelog.d/reject-unknown-keys-beside-values.fixed.md

@@ -0,0 +1 @@
+Warn when a parameter YAML node carries an unknown key beside `values:` (for example an `uprating:` block misplaced outside `metadata:`), which was previously dropped silently.

policyengine_core/parameters/config.py

@@ -22,6 +22,11 @@
 
 ALLOWED_PARAM_TYPES = (float, int, bool, type(None), typing.List)
 COMMON_KEYS = {"description", "metadata", "documentation"}
+# Keys tolerated beside ``values:``/``value:`` for backward compatibility with
+# older OpenFisca-heritage parameter files. They are recognized (not flagged by
+# the unknown-key check, issue #505) even though the current convention nests
+# them under ``metadata:``.
+LEGACY_COMPAT_KEYS = {"reference", "unit"}
 FILE_EXTENSIONS = {".yaml", ".yml"}
 
 

policyengine_core/parameters/helpers.py

@@ -1,11 +1,13 @@
 import os
 import traceback
+import warnings
 
 import numpy
 
 from policyengine_core import parameters, periods
 from policyengine_core.errors import ParameterParsingError
 from policyengine_core.parameters import config
+from policyengine_core.warnings import ParameterKeyWarning
 
 
 def contains_nan(vector):
@@ -87,3 +89,47 @@ def _validate_parameter(parameter, data, data_type=None, allowed_keys=None):
             "'{}' must be of type {}.".format(parameter.name, type_map[data_type]),
             parameter.file_path,
         )
+
+    if allowed_keys is not None and isinstance(data, dict):
+        _warn_on_unknown_keys(parameter, data, allowed_keys)
+
+
+def _warn_on_unknown_keys(parameter, data, allowed_keys):
+    """Warn about keys placed alongside the recognized parameter keys.
+
+    Unknown siblings are silently discarded by the loader. The most damaging
+    case is an ``uprating:`` block written next to ``values:`` instead of under
+    ``metadata:``, which freezes an indexed parameter at its last explicit value
+    (see issue #505). Emit a ``ParameterKeyWarning`` naming the file, the
+    offending key, and the fix, rather than dropping it silently.
+
+    This is a warning (not an error) so country packages can sweep their trees
+    before it becomes a hard error in a future major release.
+    """
+    unknown_keys = [
+        key
+        for key in data.keys()
+        if key not in allowed_keys and key not in config.LEGACY_COMPAT_KEYS
+    ]
+    for key in unknown_keys:
+        location = (
+            "'{}'".format(parameter.file_path)
+            if parameter.file_path is not None
+            else "parameter '{}'".format(parameter.name)
+        )
+        hint = ""
+        if key == "uprating":
+            hint = " Move `uprating` under `metadata:`."
+        warnings.warn(
+            "Unknown key '{key}' in {location} (parameter '{name}'). "
+            "It sits outside the recognized keys and will be ignored.{hint} "
+            "Allowed keys are: {allowed}.".format(
+                key=key,
+                location=location,
+                name=parameter.name,
+                hint=hint,
+                allowed=", ".join(sorted(str(k) for k in allowed_keys)),
+            ),
+            ParameterKeyWarning,
+            stacklevel=3,
+        )

policyengine_core/parameters/parameter_scale_bracket.py

@@ -1,25 +1,33 @@
 from typing import Iterable
 from policyengine_core.parameters import Parameter, ParameterNode
+from policyengine_core.parameters.config import COMMON_KEYS
 
 
 class ParameterScaleBracket(ParameterNode):
     """
     A parameter scale bracket.
     """
 
-    _allowed_keys = ("amount", "threshold", "rate", "average_rate", "base")
+    # The tax-scale components a bracket can hold as children.
+    _component_keys = ("amount", "threshold", "rate", "average_rate", "base")
+
+    # Keys accepted on a bracket node. Beyond the components, brackets may carry
+    # the common ``description``/``metadata``/``documentation`` keys (metadata is
+    # used to route uprating to the components; see issue #390). Any other key is
+    # flagged by the loader (issue #505).
+    _allowed_keys = frozenset(_component_keys).union(COMMON_KEYS)
 
     @staticmethod
     def allowed_unit_keys():
-        return [key + "_unit" for key in ParameterScaleBracket._allowed_keys]
+        return [key + "_unit" for key in ParameterScaleBracket._component_keys]
 
     def get_descendants(self) -> Iterable[Parameter]:
-        for key in self._allowed_keys:
+        for key in self._component_keys:
             if key in self.children:
                 yield self.children[key]
 
     def propagate_uprating(self, uprating: str, threshold: bool = False) -> None:
-        for key in self._allowed_keys:
+        for key in self._component_keys:
             if key in self.children:
                 if key == "threshold" and not threshold:
                     continue

policyengine_core/warnings/__init__.py

@@ -1,3 +1,4 @@
 from .libyaml_warning import LibYAMLWarning
 from .memory_config_warning import MemoryConfigWarning
+from .parameter_key_warning import ParameterKeyWarning
 from .tempfile_warning import TempfileWarning

policyengine_core/warnings/parameter_key_warning.py

@@ -0,0 +1,16 @@
+class ParameterKeyWarning(UserWarning):
+    """
+    Warning raised when a parameter YAML node carries an unknown key beside
+    ``values:`` (or beside a scale bracket's components).
+
+    The most damaging case is an ``uprating:`` block written as a sibling of
+    ``values:`` instead of nested under ``metadata:``: the file looks like it
+    uprates, loads cleanly, and passes CI, while the parameter silently freezes
+    at its last explicit value. See issue #505.
+
+    This is emitted as a deprecation-style warning in minor releases; it is
+    intended to become a hard error in a future major release once country
+    packages have swept their parameter trees.
+    """
+
+    pass

tests/core/parameter_validation/test_unknown_keys.py

@@ -0,0 +1,155 @@
+# -*- coding: utf-8 -*-
+"""Tests for warning on unknown keys placed beside ``values:`` in parameter YAML.
+
+Regression tests for issue #505: an ``uprating:`` block written as a sibling of
+``values:`` (instead of nested under ``metadata:``) was silently dropped, freezing
+indexed parameters. The loader now emits a ``ParameterKeyWarning`` naming the file,
+the offending key, and the fix.
+"""
+
+import os
+import warnings
+
+import pytest
+
+from policyengine_core.parameters import ParameterNode, load_parameter_file
+from policyengine_core.warnings import ParameterKeyWarning
+
+BASE_DIR = os.path.dirname(os.path.abspath(__file__))
+
+
+def test_unknown_key_beside_values_warns_from_file():
+    path = os.path.join(BASE_DIR, "unknown_key_beside_values.yaml")
+    with pytest.warns(ParameterKeyWarning) as record:
+        load_parameter_file(path, "unknown_key_beside_values")
+
+    assert len(record) == 1
+    message = str(record[0].message)
+    # Names the offending key, the file, and points to the fix.
+    assert "uprating" in message
+    assert "unknown_key_beside_values.yaml" in message
+    assert "metadata" in message
+
+
+def test_uprating_under_metadata_does_not_warn():
+    path = os.path.join(BASE_DIR, "uprating_under_metadata.yaml")
+    with warnings.catch_warnings():
+        warnings.simplefilter("error", ParameterKeyWarning)
+        # Should not raise: the placement is correct.
+        param = load_parameter_file(path, "uprating_under_metadata")
+    assert param.metadata["uprating"] == "some.index"
+
+
+def test_unknown_key_beside_values_in_dict_warns():
+    with pytest.warns(ParameterKeyWarning, match="uprating"):
+        ParameterNode(
+            data={
+                "my_param": {
+                    "values": {"2020-01-01": 100},
+                    "uprating": {"parameter": "some.index"},
+                },
+            }
+        )
+
+
+def test_metadata_beside_values_in_dict_does_not_warn():
+    with warnings.catch_warnings():
+        warnings.simplefilter("error", ParameterKeyWarning)
+        node = ParameterNode(
+            data={
+                "my_param": {
+                    "values": {"2020-01-01": 100},
+                    "metadata": {"uprating": "some.index"},
+                },
+            }
+        )
+    assert node.my_param.metadata["uprating"] == "some.index"
+
+
+def test_unknown_key_on_scale_warns():
+    with pytest.warns(ParameterKeyWarning, match="bogus"):
+        ParameterNode(
+            data={
+                "scale": {
+                    "bogus": "oops",
+                    "brackets": [
+                        {
+                            "threshold": {"values": {"2020-01-01": 0}},
+                            "rate": {"values": {"2020-01-01": 0.1}},
+                        },
+                    ],
+                },
+            }
+        )
+
+
+def test_bracket_metadata_does_not_warn():
+    # Bracket-level metadata is a legitimate carrier (see issue #390).
+    with warnings.catch_warnings():
+        warnings.simplefilter("error", ParameterKeyWarning)
+        ParameterNode(
+            data={
+                "scale": {
+                    "brackets": [
+                        {
+                            "threshold": {"values": {"2020-01-01": 0}},
+                            "rate": {"values": {"2020-01-01": 0.1}},
+                            "metadata": {"uprating": "some.index"},
+                        },
+                    ],
+                },
+            }
+        )
+
+
+def test_unknown_key_on_bracket_component_leaf_warns():
+    with pytest.warns(ParameterKeyWarning, match="uprating"):
+        ParameterNode(
+            data={
+                "scale": {
+                    "brackets": [
+                        {
+                            "threshold": {
+                                "values": {"2020-01-01": 0},
+                                "uprating": "some.index",  # wrong: sibling of values
+                            },
+                            "rate": {"values": {"2020-01-01": 0.1}},
+                        },
+                    ],
+                },
+            }
+        )
+
+
+def test_unknown_key_on_bracket_warns():
+    # A dict-valued unknown key on a bracket would otherwise be silently turned
+    # into a phantom child node; the loader warns instead.
+    with pytest.warns(ParameterKeyWarning, match="bogus"):
+        ParameterNode(
+            data={
+                "scale": {
+                    "brackets": [
+                        {
+                            "threshold": {"values": {"2020-01-01": 0}},
+                            "rate": {"values": {"2020-01-01": 0.1}},
+                            "bogus": {"values": {"2020-01-01": 1}},
+                        },
+                    ],
+                },
+            }
+        )
+
+
+def test_valid_parameter_does_not_warn():
+    with warnings.catch_warnings():
+        warnings.simplefilter("error", ParameterKeyWarning)
+        ParameterNode(
+            data={
+                "my_param": {
+                    "description": "A parameter",
+                    "documentation": "Docs here",
+                    "values": {"2020-01-01": 100},
+                    "metadata": {"unit": "currency-USD"},
+                },
+            }
+        )

tests/core/parameter_validation/unknown_key_beside_values.yaml

@@ -0,0 +1,6 @@
+description: A parameter with an uprating block misplaced beside values.
+values:
+  2020-01-01: 100
+  2021-01-01: 110
+uprating:
+  parameter: some.index

tests/core/parameter_validation/uprating_under_metadata.yaml

@@ -0,0 +1,6 @@
+description: A parameter with an uprating block correctly nested under metadata.
+values:
+  2020-01-01: 100
+  2021-01-01: 110
+metadata:
+  uprating: some.index