diff --git a/changelog.d/reject-unknown-keys-beside-values.fixed.md b/changelog.d/reject-unknown-keys-beside-values.fixed.md new file mode 100644 index 000000000..5316edefb --- /dev/null +++ b/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. diff --git a/policyengine_core/parameters/config.py b/policyengine_core/parameters/config.py index 7adec9d25..f4ab79d03 100644 --- a/policyengine_core/parameters/config.py +++ b/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"} diff --git a/policyengine_core/parameters/helpers.py b/policyengine_core/parameters/helpers.py index b11a8aa87..8435bf46b 100644 --- a/policyengine_core/parameters/helpers.py +++ b/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, + ) diff --git a/policyengine_core/parameters/parameter_scale_bracket.py b/policyengine_core/parameters/parameter_scale_bracket.py index 8d80e1148..e4b31bccc 100644 --- a/policyengine_core/parameters/parameter_scale_bracket.py +++ b/policyengine_core/parameters/parameter_scale_bracket.py @@ -1,5 +1,6 @@ from typing import Iterable from policyengine_core.parameters import Parameter, ParameterNode +from policyengine_core.parameters.config import COMMON_KEYS class ParameterScaleBracket(ParameterNode): @@ -7,19 +8,26 @@ 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 diff --git a/policyengine_core/warnings/__init__.py b/policyengine_core/warnings/__init__.py index e1e6ff12d..139a190d9 100644 --- a/policyengine_core/warnings/__init__.py +++ b/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 diff --git a/policyengine_core/warnings/parameter_key_warning.py b/policyengine_core/warnings/parameter_key_warning.py new file mode 100644 index 000000000..1f0546474 --- /dev/null +++ b/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 diff --git a/tests/core/parameter_validation/test_unknown_keys.py b/tests/core/parameter_validation/test_unknown_keys.py new file mode 100644 index 000000000..962e89cd3 --- /dev/null +++ b/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"}, + }, + } + ) diff --git a/tests/core/parameter_validation/unknown_key_beside_values.yaml b/tests/core/parameter_validation/unknown_key_beside_values.yaml new file mode 100644 index 000000000..db5dcf47e --- /dev/null +++ b/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 diff --git a/tests/core/parameter_validation/uprating_under_metadata.yaml b/tests/core/parameter_validation/uprating_under_metadata.yaml new file mode 100644 index 000000000..90f195593 --- /dev/null +++ b/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