From b8c8d751e66471b29b911e0662a907cedea5ac22 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 22 Oct 2026 22:51:17 +0400 Subject: [PATCH 01/23] Derive each catalog version's parameter tree as rows of direct children A version's tree comes from its stored parameters and parameter nodes alone. A "[" starts a child as a "." does, so a bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on; a path without a stored label gets one made from the last part of its name. Each row also records whether a reform can change it, with the same rules as the tree v3 builds from v1 metadata, and its position among all siblings and among those a reform can change. Co-Authored-By: Claude Opus 5.5 --- .../data/v2/catalog/parameter_tree.py | 250 ++++++++++++++ tests/unit/v2/test_parameter_tree.py | 305 ++++++++++++++++++ 2 files changed, 555 insertions(+) create mode 100644 policyengine_api/data/v2/catalog/parameter_tree.py create mode 100644 tests/unit/v2/test_parameter_tree.py diff --git a/policyengine_api/data/v2/catalog/parameter_tree.py b/policyengine_api/data/v2/catalog/parameter_tree.py new file mode 100644 index 000000000..66e76e947 --- /dev/null +++ b/policyengine_api/data/v2/catalog/parameter_tree.py @@ -0,0 +1,250 @@ +"""Each catalog version's parameter tree, as rows of direct children. + +The tree is derived from a version's stored parameters and parameter nodes +alone, so it can be written when a version is published and for any version +stored before the tree existed. Every node and parameter of the version is one +row, under the path of its parent (the empty string for top-level paths). + +A ``[`` starts a child as a ``.`` does: a bracket schedule such as +``gov.hmrc.income_tax.rates.uk`` is one node whose children are its brackets +``uk[0]``, ``uk[1]`` and so on, labelled "Bracket 1", "Bracket 2"; a bracket's +own children (``rate``, ``threshold``) are unchanged. A path with no stored +label gets one generated from the last part of its name. + +Each row also records whether a reform can change it, which mirrors the tree +the app's newer interface builds from v1 metadata +(``app/src/libs/buildParameterTree.ts``): a parameter under ``gov`` whose +economy or household flag is set and whose path names neither ``taxsim``, +``gov.abolitions`` nor ``pycache``, and every node with such a parameter +below it. A version published before catalog format 2 has no flags, so only +the path rules apply to it. +""" + +from __future__ import annotations + +import re +from collections import defaultdict +from dataclasses import dataclass +from typing import TYPE_CHECKING, Literal + +if TYPE_CHECKING: + from collections.abc import Iterable + from uuid import UUID + +REFORM_TREE_PREFIX = "gov." +REFORM_EXCLUDED_PATH_PARTS = ("taxsim", "gov.abolitions", "pycache") + +_BRACKET_INDEX = re.compile(r"\[(\d+)\]$") +_NAME_PARTS = re.compile(r"[.\[]") + +ChildType = Literal["node", "parameter"] + + +@dataclass(frozen=True, slots=True) +class TreeNodeSource: + """A stored parameter node: its name and stored label.""" + + name: str + label: str | None + + +@dataclass(frozen=True, slots=True) +class TreeParameterSource: + """A stored parameter: its ID, name, stored label and flags.""" + + id: UUID + name: str + label: str | None + economy: bool | None + household: bool | None + + +@dataclass(frozen=True, slots=True) +class ParameterTreeChildRow: + """One direct child in a version's parameter tree. + + ``position`` orders the children of one parent; ``reform_position`` orders + only those a reform can change and is None for the others. Node rows carry + their direct child counts (all children, and those a reform can change); + parameter rows carry the parameter's ID and flags instead. + """ + + parent_path: str + path: str + position: int + label: str + child_type: ChildType + child_count: int | None + reform_changeable: bool + reform_position: int | None + reform_child_count: int | None + parameter_id: UUID | None + economy: bool | None + household: bool | None + + +def parent_path(path: str) -> str: + """Return the path of a node's or parameter's parent, or "" at the top.""" + delimiter_index = max(path.rfind("."), path.rfind("[")) + return path[:delimiter_index] if delimiter_index > 0 else "" + + +def bracket_index(path: str) -> int | None: + """Return the index of a bracket path such as ``rates.uk[0]``, else None.""" + match = _BRACKET_INDEX.search(path) + return int(match.group(1)) if match else None + + +def generated_label(path: str) -> str: + """Return a label made from the last part of a name, as v3 makes one.""" + words = _NAME_PARTS.split(path)[-1].replace("_", " ") + return words[:1].upper() + words[1:] + + +def _display_label(label: str) -> str: + """Return a label as v3 shows it: underscores as spaces, first letter upper.""" + words = label.replace("_", " ") + return words[:1].upper() + words[1:] + + +def _child_label(path: str, stored_label: str | None) -> str: + index = bracket_index(path) + if index is not None: + return f"Bracket {index + 1}" + if stored_label and stored_label.strip(): + return stored_label + return generated_label(path) + + +def is_reform_changeable_parameter(parameter: TreeParameterSource) -> bool: + """Return whether a reform can change this parameter, as v3's tree decides.""" + if not parameter.name.startswith(REFORM_TREE_PREFIX): + return False + if any(part in parameter.name for part in REFORM_EXCLUDED_PATH_PARTS): + return False + if parameter.economy is None and parameter.household is None: + return True + return bool(parameter.economy) or bool(parameter.household) + + +def _ancestors(path: str) -> list[str]: + ancestors = [] + current = parent_path(path) + while current: + ancestors.append(current) + current = parent_path(current) + return ancestors + + +def _position_key(path: str) -> tuple[int, int, str]: + index = bracket_index(path) + if index is not None: + return (0, index, "") + return (1, 0, path) + + +def _reform_position_key(path: str, label: str) -> tuple[int, int, str, str]: + index = bracket_index(path) + if index is not None: + return (0, index, "", "") + return (1, 0, _display_label(label).casefold(), path) + + +@dataclass(frozen=True, slots=True) +class _TreeIndex: + """The maps a version's tree rows are built from.""" + + children: dict[str, list[str]] + labels: dict[str, str] + changeable: frozenset[str] + parameters_by_path: dict[str, TreeParameterSource] + + def row( + self, path: str, *, parent: str, position: int, reform_position: int | None + ) -> ParameterTreeChildRow: + direct_children = self.children.get(path, []) + parameter = self.parameters_by_path.get(path) + leaf = parameter if parameter is not None and not direct_children else None + return ParameterTreeChildRow( + parent_path=parent, + path=path, + position=position, + label=self.labels[path], + child_type="node" if leaf is None else "parameter", + child_count=len(direct_children) if leaf is None else None, + reform_changeable=path in self.changeable, + reform_position=reform_position, + reform_child_count=( + sum(child in self.changeable for child in direct_children) + if leaf is None + else None + ), + parameter_id=None if leaf is None else leaf.id, + economy=None if leaf is None else leaf.economy, + household=None if leaf is None else leaf.household, + ) + + +def _stored_label( + path: str, + stored_labels: dict[str, str | None], + parameters_by_path: dict[str, TreeParameterSource], +) -> str | None: + if path in stored_labels: + return stored_labels[path] + parameter = parameters_by_path.get(path) + return None if parameter is None else parameter.label + + +def build_parameter_tree( + *, + nodes: Iterable[TreeNodeSource], + parameters: Iterable[TreeParameterSource], +) -> tuple[ParameterTreeChildRow, ...]: + """Return one row per node and parameter of a version, parents first.""" + parameters_by_path = {parameter.name: parameter for parameter in parameters} + stored_labels = {node.name: node.label for node in nodes} + paths = set(stored_labels) | set(parameters_by_path) + for path in list(paths): + paths.update(_ancestors(path)) + paths.discard("") + + changeable: set[str] = set() + for parameter in parameters_by_path.values(): + if is_reform_changeable_parameter(parameter): + changeable.add(parameter.name) + changeable.update(_ancestors(parameter.name)) + + children: defaultdict[str, list[str]] = defaultdict(list) + for path in paths: + children[parent_path(path)].append(path) + + index = _TreeIndex( + children=dict(children), + labels={ + path: _child_label( + path, _stored_label(path, stored_labels, parameters_by_path) + ) + for path in paths + }, + changeable=frozenset(changeable), + parameters_by_path=parameters_by_path, + ) + rows: list[ParameterTreeChildRow] = [] + for parent in sorted(index.children, key=lambda path: (path.count("."), path)): + siblings = sorted(index.children[parent], key=_position_key) + reform_siblings = sorted( + (path for path in siblings if path in index.changeable), + key=lambda path: _reform_position_key(path, index.labels[path]), + ) + reform_positions = {path: order for order, path in enumerate(reform_siblings)} + rows.extend( + index.row( + path, + parent=parent, + position=position, + reform_position=reform_positions.get(path), + ) + for position, path in enumerate(siblings) + ) + return tuple(rows) diff --git a/tests/unit/v2/test_parameter_tree.py b/tests/unit/v2/test_parameter_tree.py new file mode 100644 index 000000000..ef3234023 --- /dev/null +++ b/tests/unit/v2/test_parameter_tree.py @@ -0,0 +1,305 @@ +"""Each catalog version's parameter tree, derived as rows of direct children.""" + +from __future__ import annotations + +from uuid import uuid4 + +from policyengine_api.data.v2.catalog.parameter_tree import ( + ParameterTreeChildRow, + TreeNodeSource, + TreeParameterSource, + build_parameter_tree, + generated_label, + parent_path, +) + + +def _parameter( + name: str, + *, + label: str | None = None, + economy: bool | None = True, + household: bool | None = True, +) -> TreeParameterSource: + return TreeParameterSource( + id=uuid4(), + name=name, + label=label, + economy=economy, + household=household, + ) + + +def _rows_by_path( + rows: tuple[ParameterTreeChildRow, ...], +) -> dict[str, ParameterTreeChildRow]: + paths = [row.path for row in rows] + assert len(paths) == len(set(paths)), "each path appears once" + return {row.path: row for row in rows} + + +def _children( + rows: tuple[ParameterTreeChildRow, ...], parent: str +) -> list[ParameterTreeChildRow]: + return sorted( + (row for row in rows if row.parent_path == parent), + key=lambda row: row.position, + ) + + +def _reform_children( + rows: tuple[ParameterTreeChildRow, ...], parent: str +) -> list[ParameterTreeChildRow]: + visible = [ + row for row in rows if row.parent_path == parent and row.reform_changeable + ] + positions = [row.reform_position for row in visible] + assert None not in positions + assert sorted(positions) == list(range(len(visible))) + return sorted(visible, key=lambda row: row.reform_position or 0) + + +UK_RATES = "gov.hmrc.income_tax.rates" + + +def _uk_income_tax_tree() -> tuple[ParameterTreeChildRow, ...]: + """UK income tax: a schedule with a stored node row and one without.""" + nodes = [ + TreeNodeSource(name="gov", label=None), + TreeNodeSource(name="gov.hmrc", label="HMRC"), + TreeNodeSource(name="gov.hmrc.income_tax", label="Income Tax"), + TreeNodeSource(name=UK_RATES, label="Income Tax rates"), + TreeNodeSource(name=f"{UK_RATES}.uk", label="Income Tax main rates"), + TreeNodeSource(name=f"{UK_RATES}.property", label=None), + ] + parameters = [ + _parameter(f"{UK_RATES}.uk[0].rate", label="Basic rate"), + _parameter(f"{UK_RATES}.uk[0].threshold", label="Basic rate threshold"), + _parameter(f"{UK_RATES}.uk[1].rate", label="Higher rate"), + _parameter(f"{UK_RATES}.uk[1].threshold", label="Higher rate threshold"), + _parameter(f"{UK_RATES}.dividends[0].rate"), + _parameter(f"{UK_RATES}.dividends[0].threshold"), + _parameter(f"{UK_RATES}.property.allowance", label="Property allowance"), + ] + return build_parameter_tree(nodes=nodes, parameters=parameters) + + +def test_parent_path_treats_a_bracket_as_a_child_of_its_schedule() -> None: + assert parent_path("gov") == "" + assert parent_path("gov.hmrc") == "gov" + assert parent_path(f"{UK_RATES}.uk") == UK_RATES + assert parent_path(f"{UK_RATES}.uk[0]") == f"{UK_RATES}.uk" + assert parent_path(f"{UK_RATES}.uk[0].rate") == f"{UK_RATES}.uk[0]" + + +def test_generated_labels_follow_the_last_part_of_the_name() -> None: + assert generated_label(f"{UK_RATES}.dividends") == "Dividends" + assert generated_label("gov.example.non_primary_residence") == ( + "Non primary residence" + ) + assert generated_label(f"{UK_RATES}.uk[0].rate") == "Rate" + + +def test_uk_schedules_nest_their_brackets_under_one_node() -> None: + rows = _uk_income_tax_tree() + by_path = _rows_by_path(rows) + + rates_children = _children(rows, UK_RATES) + assert [row.path for row in rates_children] == [ + f"{UK_RATES}.dividends", + f"{UK_RATES}.property", + f"{UK_RATES}.uk", + ] + assert all(row.child_type == "node" for row in rates_children) + + stored_schedule = by_path[f"{UK_RATES}.uk"] + assert stored_schedule.label == "Income Tax main rates" + assert stored_schedule.child_count == 2 + assert [row.path for row in _children(rows, f"{UK_RATES}.uk")] == [ + f"{UK_RATES}.uk[0]", + f"{UK_RATES}.uk[1]", + ] + assert [row.label for row in _children(rows, f"{UK_RATES}.uk")] == [ + "Bracket 1", + "Bracket 2", + ] + + unstored_schedule = by_path[f"{UK_RATES}.dividends"] + assert unstored_schedule.label == "Dividends" + assert unstored_schedule.child_type == "node" + assert unstored_schedule.child_count == 1 + + first_bracket = by_path[f"{UK_RATES}.uk[0]"] + assert first_bracket.child_type == "node" + assert first_bracket.child_count == 2 + assert [ + (row.path, row.label, row.child_type) + for row in _children(rows, f"{UK_RATES}.uk[0]") + ] == [ + (f"{UK_RATES}.uk[0].rate", "Basic rate", "parameter"), + (f"{UK_RATES}.uk[0].threshold", "Basic rate threshold", "parameter"), + ] + + +def test_us_schedule_brackets_are_ordered_by_number_not_by_text() -> None: + schedule = "gov.aca.age_curves.al" + parameters = [ + _parameter(f"{schedule}[{index}].{part}") + for index in (0, 1, 2, 10, 11) + for part in ("amount", "threshold") + ] + nodes = [TreeNodeSource(name=schedule, label="ACA premium age curve factors - AL")] + + rows = build_parameter_tree(nodes=nodes, parameters=parameters) + brackets = _children(rows, schedule) + + assert [row.path for row in brackets] == [ + f"{schedule}[0]", + f"{schedule}[1]", + f"{schedule}[2]", + f"{schedule}[10]", + f"{schedule}[11]", + ] + assert [row.label for row in brackets] == [ + "Bracket 1", + "Bracket 2", + "Bracket 3", + "Bracket 11", + "Bracket 12", + ] + assert [row.path for row in _reform_children(rows, schedule)] == [ + row.path for row in brackets + ] + assert _rows_by_path(rows)[schedule].label == "ACA premium age curve factors - AL" + assert [row.path for row in _children(rows, "")] == ["gov"] + assert [row.path for row in _children(rows, "gov")] == ["gov.aca"] + + +def test_a_bracket_keeps_its_bracket_label_over_a_stored_one() -> None: + schedule = f"{UK_RATES}.uk" + rows = build_parameter_tree( + nodes=[TreeNodeSource(name=f"{schedule}[0]", label="First band")], + parameters=[_parameter(f"{schedule}[0].rate")], + ) + + assert _rows_by_path(rows)[f"{schedule}[0]"].label == "Bracket 1" + + +def test_parameters_carry_their_ids_and_flags_and_nodes_their_counts() -> None: + rate = _parameter("gov.example.rate", label="Example rate", household=False) + rows = build_parameter_tree( + nodes=[TreeNodeSource(name="gov.example", label="Example policy")], + parameters=[rate], + ) + by_path = _rows_by_path(rows) + + leaf = by_path["gov.example.rate"] + assert leaf.child_type == "parameter" + assert leaf.parameter_id == rate.id + assert (leaf.economy, leaf.household) == (True, False) + assert leaf.child_count is None + assert leaf.reform_child_count is None + node = by_path["gov.example"] + assert node.parameter_id is None + assert (node.economy, node.household) == (None, None) + assert (node.child_count, node.reform_child_count) == (1, 1) + + +def test_a_stored_node_without_children_is_an_empty_node() -> None: + rows = build_parameter_tree( + nodes=[TreeNodeSource(name="gov.empty", label="Nothing here")], + parameters=[_parameter("gov.example.rate")], + ) + empty = _rows_by_path(rows)["gov.empty"] + + assert empty.child_type == "node" + assert empty.child_count == 0 + assert empty.reform_child_count == 0 + assert empty.reform_changeable is False + assert empty.reform_position is None + + +def test_the_reform_view_keeps_only_what_a_reform_can_change() -> None: + parameters = [ + _parameter("gov.tax.rate", label="Tax rate", economy=True, household=False), + _parameter("gov.tax.credit", label="Credit", economy=False, household=True), + _parameter("gov.tax.hidden", label="Hidden", economy=False, household=False), + _parameter("gov.unchangeable.only", economy=False, household=False), + _parameter("gov.abolitions.income_tax", label="Abolish income tax"), + _parameter("gov.contrib.taxsim.rate", label="TAXSIM rate"), + _parameter("calibration.gov.target", label="Calibration target"), + ] + rows = build_parameter_tree(nodes=[], parameters=parameters) + by_path = _rows_by_path(rows) + + assert [row.path for row in _reform_children(rows, "")] == ["gov"] + assert [row.path for row in _reform_children(rows, "gov")] == ["gov.tax"] + assert [row.path for row in _reform_children(rows, "gov.tax")] == [ + "gov.tax.credit", + "gov.tax.rate", + ] + assert by_path["gov.tax"].child_count == 3 + assert by_path["gov.tax"].reform_child_count == 2 + assert by_path["gov"].child_count == 4 + assert by_path["gov"].reform_child_count == 1 + for path in ( + "gov.tax.hidden", + "gov.unchangeable", + "gov.abolitions", + "gov.contrib", + "calibration", + ): + assert by_path[path].reform_changeable is False + assert by_path[path].reform_position is None + + +def test_the_reform_view_is_sorted_by_label_as_v3_shows_it() -> None: + parameters = [ + _parameter("gov.b_first", label="zebra crossing"), + _parameter("gov.a_second", label="Apple orchard"), + _parameter("gov.c_third"), + _parameter("gov.d_fourth", label="banana_split"), + ] + rows = build_parameter_tree(nodes=[], parameters=parameters) + + assert [row.path for row in _children(rows, "gov")] == [ + "gov.a_second", + "gov.b_first", + "gov.c_third", + "gov.d_fourth", + ] + assert [row.label for row in _reform_children(rows, "gov")] == [ + "Apple orchard", + "banana_split", + "C third", + "zebra crossing", + ] + + +def test_parameters_without_flags_count_as_changeable_in_the_reform_view() -> None: + rows = build_parameter_tree( + nodes=[], + parameters=[_parameter("gov.older.rate", economy=None, household=None)], + ) + + assert _rows_by_path(rows)["gov.older.rate"].reform_changeable is True + + +def test_every_row_has_its_parent_and_counts_match_the_rows() -> None: + rows = _uk_income_tax_tree() + by_path = _rows_by_path(rows) + + for row in rows: + if row.parent_path: + assert row.parent_path in by_path + children = [child for child in rows if child.parent_path == row.path] + if row.child_type == "node": + assert row.child_count == len(children) + assert row.reform_child_count == sum( + child.reform_changeable for child in children + ) + else: + assert children == [] + for parent in {row.parent_path for row in rows}: + positions = [row.position for row in rows if row.parent_path == parent] + assert sorted(positions) == list(range(len(positions))) From 8732183658f0183017842eb9402c0e902e204569 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 15:49:46 +0400 Subject: [PATCH 02/23] Store each catalog version's parameter tree in its own table The new table parameter_tree_children holds one row per node and parameter of a version, under its parent's path, with its position among all siblings and among those a reform can change. Two unique indexes, (version, parent path, position) and (version, parent path, reform position), serve one level of the tree in one read. Rows are deleted with their version or parameter. The revision is autogenerated with no corrections and writes no rows. The Public API's runtime role may read the table and the catalog writer may read and insert rows. Co-Authored-By: Claude Opus 5.5 --- ...store_each_catalog_version_s_parameter_.py | 115 ++++++++++++++++++ policyengine_api/data/v2/migration_target.py | 2 +- policyengine_api/data/v2/models/__init__.py | 2 + policyengine_api/data/v2/models/metadata.py | 66 ++++++++++ policyengine_api/data/v2/privileges.py | 2 + .../integration/test_alembic_v2_lifecycle.py | 113 +++++++++++++++++ tests/unit/v2/test_alembic_v2.py | 46 +++++++ tests/unit/v2/test_import_side_effects.py | 2 +- tests/unit/v2/test_models.py | 78 ++++++++++++ 9 files changed, 424 insertions(+), 2 deletions(-) create mode 100644 migrations/v2/versions/3869eb814a90_store_each_catalog_version_s_parameter_.py diff --git a/migrations/v2/versions/3869eb814a90_store_each_catalog_version_s_parameter_.py b/migrations/v2/versions/3869eb814a90_store_each_catalog_version_s_parameter_.py new file mode 100644 index 000000000..7ac26f0e6 --- /dev/null +++ b/migrations/v2/versions/3869eb814a90_store_each_catalog_version_s_parameter_.py @@ -0,0 +1,115 @@ +"""store each catalog version's parameter tree + +Revision ID: 3869eb814a90 +Revises: f5c1d9b4685b +Create Date: 2026-10-22 22:51:53.959795 +Generation: uv run alembic -c alembic-v2.ini revision --autogenerate +""" + +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +import sqlmodel + + +revision: str = "3869eb814a90" +down_revision: Union[str, None] = "f5c1d9b4685b" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.create_table( + "parameter_tree_children", + sa.Column("id", sa.Uuid(), nullable=False), + sa.Column( + "created_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + sa.Column("tax_benefit_model_version_id", sa.Uuid(), nullable=False), + sa.Column( + "parent_path", sqlmodel.sql.sqltypes.AutoString(length=512), nullable=False + ), + sa.Column("path", sqlmodel.sql.sqltypes.AutoString(length=512), nullable=False), + sa.Column("position", sa.Integer(), nullable=False), + sa.Column( + "label", sqlmodel.sql.sqltypes.AutoString(length=512), nullable=False + ), + sa.Column( + "child_type", sqlmodel.sql.sqltypes.AutoString(length=16), nullable=False + ), + sa.Column("child_count", sa.Integer(), nullable=True), + sa.Column("reform_position", sa.Integer(), nullable=True), + sa.Column("reform_child_count", sa.Integer(), nullable=True), + sa.Column("parameter_id", sa.Uuid(), nullable=True), + sa.Column("economy", sa.Boolean(), nullable=True), + sa.Column("household", sa.Boolean(), nullable=True), + sa.CheckConstraint( + "(child_type = 'parameter') = (parameter_id IS NOT NULL)", + name=op.f("ck_parameter_tree_children_parameter_reference"), + ), + sa.CheckConstraint( + "child_type IN ('node', 'parameter')", + name=op.f("ck_parameter_tree_children_child_type"), + ), + sa.ForeignKeyConstraint( + ["parameter_id"], + ["parameters.id"], + name=op.f("fk_parameter_tree_children_parameter_id_parameters"), + ondelete="CASCADE", + ), + sa.ForeignKeyConstraint( + ["tax_benefit_model_version_id"], + ["tax_benefit_model_versions.id"], + name=op.f( + "fk_parameter_tree_children_tax_benefit_model_version_id_tax_benefit_model_versions" + ), + ondelete="CASCADE", + ), + sa.PrimaryKeyConstraint("id", name=op.f("pk_parameter_tree_children")), + sa.UniqueConstraint( + "tax_benefit_model_version_id", + "path", + name="uq_parameter_tree_children_model_version_path", + ), + ) + op.create_index( + "ix_parameter_tree_children_listing", + "parameter_tree_children", + ["tax_benefit_model_version_id", "parent_path", "position"], + unique=True, + ) + op.create_index( + op.f("ix_parameter_tree_children_parameter_id"), + "parameter_tree_children", + ["parameter_id"], + unique=False, + ) + op.create_index( + "ix_parameter_tree_children_reform_listing", + "parameter_tree_children", + ["tax_benefit_model_version_id", "parent_path", "reform_position"], + unique=True, + ) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_index( + "ix_parameter_tree_children_reform_listing", + table_name="parameter_tree_children", + ) + op.drop_index( + op.f("ix_parameter_tree_children_parameter_id"), + table_name="parameter_tree_children", + ) + op.drop_index( + "ix_parameter_tree_children_listing", table_name="parameter_tree_children" + ) + op.drop_table("parameter_tree_children") + # ### end Alembic commands ### diff --git a/policyengine_api/data/v2/migration_target.py b/policyengine_api/data/v2/migration_target.py index d463fad16..c22f670bc 100644 --- a/policyengine_api/data/v2/migration_target.py +++ b/policyengine_api/data/v2/migration_target.py @@ -24,7 +24,7 @@ from collections.abc import Mapping V2_ALEMBIC_DISPOSABLE_TEST = "V2_ALEMBIC_DISPOSABLE_TEST" -V2_ALEMBIC_HEAD_REVISION = "f5c1d9b4685b" +V2_ALEMBIC_HEAD_REVISION = "3869eb814a90" DISPOSABLE_DATABASE_NAME = "policyengine_v2_alembic_test" DISPOSABLE_HOSTS = frozenset({"localhost", "127.0.0.1", "::1", "postgres"}) MIGRATION_ROLE = "policyengine_v2_migrator" diff --git a/policyengine_api/data/v2/models/__init__.py b/policyengine_api/data/v2/models/__init__.py index 1b1a73baf..7cc03c7b4 100644 --- a/policyengine_api/data/v2/models/__init__.py +++ b/policyengine_api/data/v2/models/__init__.py @@ -46,6 +46,7 @@ DatasetVersion, Parameter, ParameterNode, + ParameterTreeChild, ParameterValue, Region, RegionType, @@ -124,6 +125,7 @@ "OutputStatus", "Parameter", "ParameterNode", + "ParameterTreeChild", "ParameterValue", "Policy", "Poverty", diff --git a/policyengine_api/data/v2/models/metadata.py b/policyengine_api/data/v2/models/metadata.py index 5431a6abc..a0fda410e 100644 --- a/policyengine_api/data/v2/models/metadata.py +++ b/policyengine_api/data/v2/models/metadata.py @@ -464,3 +464,69 @@ class ParameterValue(IdentifiedModel, table=True): parameter: Parameter = Relationship(back_populates="values") policy: Optional["Policy"] = Relationship(back_populates="parameter_values") dynamic: Optional["Dynamic"] = Relationship(back_populates="parameter_values") + + +class ParameterTreeChild(IdentifiedModel, table=True): + """One direct child in a catalog version's parameter tree. + + Derived from the version's parameters and parameter nodes when the version + is published, or by the next publication for a version stored before the + tree existed. Every node and parameter of the version is one row under its + parent's path ("" for top-level paths), so listing one level reads one + index. ``position`` orders all children of a parent; ``reform_position`` + orders only those a reform can change and is null for the others. Node rows + count their direct children; parameter rows reference their parameter and + copy its flags. + """ + + __tablename__ = "parameter_tree_children" + __table_args__ = ( + sa.UniqueConstraint( + "tax_benefit_model_version_id", + "path", + name="uq_parameter_tree_children_model_version_path", + ), + sa.Index( + "ix_parameter_tree_children_listing", + "tax_benefit_model_version_id", + "parent_path", + "position", + unique=True, + ), + sa.Index( + "ix_parameter_tree_children_reform_listing", + "tax_benefit_model_version_id", + "parent_path", + "reform_position", + unique=True, + ), + sa.CheckConstraint( + "child_type IN ('node', 'parameter')", + name="ck_parameter_tree_children_child_type", + ), + sa.CheckConstraint( + "(child_type = 'parameter') = (parameter_id IS NOT NULL)", + name="ck_parameter_tree_children_parameter_reference", + ), + ) + + tax_benefit_model_version_id: UUID = Field( + foreign_key="tax_benefit_model_versions.id", + ondelete="CASCADE", + ) + parent_path: str = Field(max_length=512) + path: str = Field(max_length=512) + position: int + label: str = Field(max_length=512) + child_type: str = Field(max_length=16) + child_count: int | None = None + reform_position: int | None = None + reform_child_count: int | None = None + parameter_id: UUID | None = Field( + default=None, + foreign_key="parameters.id", + ondelete="CASCADE", + index=True, + ) + economy: bool | None = None + household: bool | None = None diff --git a/policyengine_api/data/v2/privileges.py b/policyengine_api/data/v2/privileges.py index 5014230f0..49a35c070 100644 --- a/policyengine_api/data/v2/privileges.py +++ b/policyengine_api/data/v2/privileges.py @@ -116,6 +116,8 @@ def _access( "dataset_versions": _access(catalog_writer=READ), "datasets": _access(runtime=READ, catalog_writer=READ_INSERT_UPDATE), "parameter_nodes": _access(runtime=READ, catalog_writer=READ_INSERT_UPDATE), + # Written with each catalog version, and for versions stored before it. + "parameter_tree_children": _access(runtime=READ, catalog_writer=READ_INSERT), # The Public API also inserts a v2 policy's own parameter values. "parameter_values": _access(runtime=READ_INSERT, catalog_writer=READ_INSERT_UPDATE), "parameters": _access(runtime=READ, catalog_writer=READ_INSERT_UPDATE), diff --git a/tests/integration/test_alembic_v2_lifecycle.py b/tests/integration/test_alembic_v2_lifecycle.py index 89837594a..c71100c40 100644 --- a/tests/integration/test_alembic_v2_lifecycle.py +++ b/tests/integration/test_alembic_v2_lifecycle.py @@ -1547,3 +1547,116 @@ def test_catalog_format_2_columns_leave_earlier_versions_empty() -> None: ) command.upgrade(config, "head") engine.dispose() + + +PARAMETER_TREE_PARENT_REVISION = "f5c1d9b4685b" + + +def test_parameter_tree_table_is_created_empty_and_dropped_without_data_loss() -> None: + """The parameter tree revision creates an empty table and downgrades cleanly. + + The revision writes no rows: catalog publication writes each version's + tree. A tree row is deleted with its version or its parameter, and + downgrading drops the table while every catalog row stays. + """ + config = _config() + engine = create_engine(_disposable_url()) + try: + command.downgrade(config, PARAMETER_TREE_PARENT_REVISION) + assert "parameter_tree_children" not in inspect(engine).get_table_names() + with engine.begin() as connection: + version_id = _catalog_version_published_before_format_2(connection) + + command.upgrade(config, "head") + + indexes = { + index["name"]: (index["column_names"], index["unique"]) + for index in inspect(engine).get_indexes("parameter_tree_children") + } + assert indexes["ix_parameter_tree_children_listing"] == ( + ["tax_benefit_model_version_id", "parent_path", "position"], + True, + ) + assert indexes["ix_parameter_tree_children_reform_listing"] == ( + ["tax_benefit_model_version_id", "parent_path", "reform_position"], + True, + ) + with engine.begin() as connection: + assert ( + connection.execute( + text("SELECT count(*) FROM parameter_tree_children") + ).scalar_one() + == 0 + ) + parameter_id = connection.execute( + text( + "SELECT id FROM parameters " + "WHERE tax_benefit_model_version_id = :version_id" + ), + {"version_id": version_id}, + ).scalar_one() + for path, parent, position, child_type, child_count, reference in ( + ("gov", "", 0, "node", 1, None), + ("gov.probe", "gov", 0, "node", 1, None), + ("gov.probe.rate", "gov.probe", 0, "parameter", None, parameter_id), + ): + connection.execute( + text( + "INSERT INTO parameter_tree_children (id, " + "tax_benefit_model_version_id, parent_path, path, position, " + "label, child_type, child_count, parameter_id) VALUES " + "(:id, :version_id, :parent, :path, :position, :path, " + ":child_type, :child_count, :parameter_id)" + ), + { + "id": uuid4(), + "version_id": version_id, + "parent": parent, + "path": path, + "position": position, + "child_type": child_type, + "child_count": child_count, + "parameter_id": reference, + }, + ) + with pytest.raises(sa.exc.IntegrityError), connection.begin_nested(): + connection.execute( + text( + "INSERT INTO parameter_tree_children (id, " + "tax_benefit_model_version_id, parent_path, path, position, " + "label, child_type) VALUES (:id, :version_id, 'gov', " + "'gov.other', 0, 'Other', 'node')" + ), + {"id": uuid4(), "version_id": version_id}, + ) + connection.execute( + text("DELETE FROM parameters WHERE id = :parameter_id"), + {"parameter_id": parameter_id}, + ) + assert connection.execute( + text("SELECT path FROM parameter_tree_children ORDER BY path") + ).scalars().all() == ["gov", "gov.probe"] + + command.downgrade(config, PARAMETER_TREE_PARENT_REVISION) + + assert "parameter_tree_children" not in inspect(engine).get_table_names() + with engine.connect() as connection: + assert ( + connection.execute( + text( + "SELECT count(*) FROM variables " + "WHERE tax_benefit_model_version_id = :version_id" + ), + {"version_id": version_id}, + ).scalar_one() + == 1 + ) + finally: + command.upgrade(config, "head") + with engine.begin() as connection: + connection.execute( + text( + "DELETE FROM tax_benefit_models WHERE name = 'catalog-format-probe'" + ) + ) + engine.dispose() diff --git a/tests/unit/v2/test_alembic_v2.py b/tests/unit/v2/test_alembic_v2.py index d6eb45ae0..4bdc47718 100644 --- a/tests/unit/v2/test_alembic_v2.py +++ b/tests/unit/v2/test_alembic_v2.py @@ -44,6 +44,7 @@ "legacy_policy_mappings", "legacy_user_mappings", "legacy_user_policy_mappings", + "parameter_tree_children", "stage12_evaluation_reports", "stage12_evaluation_simulations", "user_identities", @@ -66,6 +67,7 @@ "provenance FROM user_policy_associations" ) SAVED_POLICY_VIEW_REMOVAL = "DROP VIEW user_policies" +CATALOG_FORMAT_2_REVISION = "f5c1d9b4685b" DISPOSABLE_URL = ( "postgresql+psycopg://postgres:test-password@127.0.0.1:5432/" f"{DISPOSABLE_DATABASE_NAME}" @@ -247,6 +249,7 @@ def test_v2_revision_chain_has_generated_policy_and_user_identity_changes() -> N assert script.get_heads() == [V2_ALEMBIC_HEAD_REVISION] assert [revision.revision for revision in script.walk_revisions()] == [ V2_ALEMBIC_HEAD_REVISION, + CATALOG_FORMAT_2_REVISION, SAVED_POLICY_RENAME_REVISION, SAVED_POLICY_RENAME_PARENT_REVISION, "91af4fc8d8c1", @@ -861,3 +864,46 @@ def test_catalog_format_2_revision_adds_only_nullable_columns() -> None: assert "op.execute(" not in body assert "op.bulk_insert(" not in body assert "server_default" not in body + + +def test_parameter_tree_revision_creates_only_the_children_table() -> None: + """The revision after catalog format 2 creates the parameter tree's table. + + It is generated, creates the table with its two listing indexes and its + parameter index, drops them on downgrade, and writes no data: rows are + written by catalog publication. + """ + script = ScriptDirectory.from_config(Config(str(REPO / "alembic-v2.ini"))) + children = [ + revision + for revision in script.walk_revisions() + if revision.down_revision == CATALOG_FORMAT_2_REVISION + ] + assert len(children) == 1 + assert children[0].revision == V2_ALEMBIC_HEAD_REVISION + revision = Path(children[0].path).read_text(encoding="utf-8") + upgrade = revision[ + revision.index("def upgrade()") : revision.index("def downgrade()") + ] + downgrade = revision[revision.index("def downgrade()") :] + + assert ( + "Generation: uv run alembic -c alembic-v2.ini revision --autogenerate" + in revision + ) + assert upgrade.count("op.create_table(") == 1 + assert '"parameter_tree_children"' in upgrade + assert upgrade.count("op.create_index(") == 3 + assert downgrade.count("op.drop_index(") == 3 + assert downgrade.count("op.drop_table(") == 1 + for index_name in ( + "ix_parameter_tree_children_listing", + "ix_parameter_tree_children_reform_listing", + "ix_parameter_tree_children_parameter_id", + ): + assert index_name in upgrade + assert index_name in downgrade + for body in (upgrade, downgrade): + assert "op.execute(" not in body + assert "op.bulk_insert(" not in body + assert "op.add_column(" not in body diff --git a/tests/unit/v2/test_import_side_effects.py b/tests/unit/v2/test_import_side_effects.py index 7b753004d..af6913897 100644 --- a/tests/unit/v2/test_import_side_effects.py +++ b/tests/unit/v2/test_import_side_effects.py @@ -69,7 +69,7 @@ def reject_ddl(*args, **kwargs): import sys after = set(pathlib.Path.cwd().iterdir()) assert before == after -assert len(V2_METADATA.tables) == 39 +assert len(V2_METADATA.tables) == 40 assert "policyengine_api.data.v2.catalog.initialization" not in sys.modules assert "policyengine_api.data.v2.policy_migration_qualification" not in sys.modules assert "policyengine_api.data.v2.household_migration_qualification" not in sys.modules diff --git a/tests/unit/v2/test_models.py b/tests/unit/v2/test_models.py index c93d214b0..551201478 100644 --- a/tests/unit/v2/test_models.py +++ b/tests/unit/v2/test_models.py @@ -752,3 +752,81 @@ def test_catalog_format_2_json_columns_have_typed_shapes() -> None: (CatalogEnumValue, EnumValueRecord), ): assert get_type_hints(typed_shape) == get_type_hints(record) + + +def test_parameter_tree_children_have_one_row_per_path_in_listing_order() -> None: + """Each version's tree rows are unique by path and listed per parent by index. + + The full listing and the listing of what a reform can change each read one + unique index (version, parent path, position); a parameter row references + its parameter, and both references are removed with their rows. + """ + table = V2_METADATA.tables["parameter_tree_children"] + + assert {column.name for column in table.columns} == { + "id", + "created_at", + "tax_benefit_model_version_id", + "parent_path", + "path", + "position", + "label", + "child_type", + "child_count", + "reform_position", + "reform_child_count", + "parameter_id", + "economy", + "household", + } + for column_name in ("parent_path", "path", "label"): + assert table.c[column_name].type.length == 512 + assert not table.c[column_name].nullable + for column_name in ( + "child_count", + "reform_position", + "reform_child_count", + "parameter_id", + "economy", + "household", + ): + assert table.c[column_name].nullable + unique_constraints = { + tuple(column.name for column in constraint.columns) + for constraint in table.constraints + if isinstance(constraint, sa.UniqueConstraint) + } + assert ("tax_benefit_model_version_id", "path") in unique_constraints + indexes = { + index.name: (tuple(column.name for column in index.columns), index.unique) + for index in table.indexes + } + assert indexes["ix_parameter_tree_children_listing"] == ( + ("tax_benefit_model_version_id", "parent_path", "position"), + True, + ) + assert indexes["ix_parameter_tree_children_reform_listing"] == ( + ("tax_benefit_model_version_id", "parent_path", "reform_position"), + True, + ) + assert indexes["ix_parameter_tree_children_parameter_id"] == ( + ("parameter_id",), + False, + ) + foreign_keys = { + foreign_key.parent.name: (foreign_key.column.table.name, foreign_key.ondelete) + for foreign_key in table.foreign_keys + } + assert foreign_keys == { + "tax_benefit_model_version_id": ("tax_benefit_model_versions", "CASCADE"), + "parameter_id": ("parameters", "CASCADE"), + } + checks = { + constraint.name + for constraint in table.constraints + if isinstance(constraint, sa.CheckConstraint) + } + assert checks == { + "ck_parameter_tree_children_child_type", + "ck_parameter_tree_children_parameter_reference", + } From b35c854dcc192699514e0044792eb6bd3d3efa8d Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 15:52:36 +0400 Subject: [PATCH 03/23] Write each stored catalog version's parameter tree during publication Publication now writes, in its transaction and under its lock, the parameter tree of every stored catalog version that has parameters but no tree: the versions it publishes and versions stored before the tree existed. The rows are derived from catalog rows already stored, so writing them is a row-only, idempotent insert that changes no other table; a version that has its tree keeps it. Publication fails if a version is still without a tree, and the evidence lists each version whose tree was written. Co-Authored-By: Claude Opus 5.5 --- docs/migration/stage-9-v2-metadata.md | 29 +++ .../data/v2/catalog/publication.py | 19 +- .../v2/catalog/publication_parameter_tree.py | 165 ++++++++++++++++++ .../data/v2/catalog/publication_types.py | 7 + .../test_v2_catalog_publication.py | 132 ++++++++++++++ tests/unit/v2/test_catalog_publication.py | 8 + 6 files changed, 358 insertions(+), 2 deletions(-) create mode 100644 policyengine_api/data/v2/catalog/publication_parameter_tree.py diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index 8e9ea0123..41c62b695 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -305,6 +305,33 @@ since they describe staged regions that were not published. Every other table must still match exactly. Region reference changes therefore take effect in the next catalog version that is published. +### Parameter tree + +Publication also writes each catalog version's parameter tree into +`parameter_tree_children`, which `/v2/parameters/children` reads one level at +a time. The rows are derived from the version's stored parameters and +parameter nodes alone: every node and parameter is one row under its parent's +path, a `[` starts a child as a `.` does (so a bracket schedule is one node +whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on), +and a path without a stored label gets one made from the last part of its +name. Each row also records its position among all its siblings and, if a +reform can change it, among the siblings a reform can change: a parameter +under `gov` whose economy or household flag is set and whose path names +neither `taxsim`, `gov.abolitions` nor `pycache`, and every node above such a +parameter. These are the rules of the tree the app's newer interface builds +from v1 metadata. A version stored before catalog format 2 has no flags, so +only the path rules apply to it. + +In the same transaction, and on every run, the command writes the tree of +every stored catalog version that has parameters but no tree: the versions it +publishes and versions stored before the tree existed. This is a row-only, +idempotent insert of rows derived from catalog rows already stored; it changes +no other table, and a version that has its tree keeps it. The evidence's +`parameter_trees_written` lists each version written, with its model, version +and row count, and is empty once every stored version has its tree. +Publication fails if a stored version with parameters is still without a tree +when it finishes. + ## Validation evidence and retry Successful JSON evidence contains: @@ -315,6 +342,8 @@ Successful JSON evidence contains: - fallback counts and omitted-region counts by country and region type, and the stored and staged region counts of any stored version whose regions were kept; +- the model, version and row count of each catalog version whose parameter + tree was written; - elapsed publication time; and - a success outcome. diff --git a/policyengine_api/data/v2/catalog/publication.py b/policyengine_api/data/v2/catalog/publication.py index a8951d6b5..47b287cd5 100644 --- a/policyengine_api/data/v2/catalog/publication.py +++ b/policyengine_api/data/v2/catalog/publication.py @@ -4,11 +4,16 @@ import logging import time +from dataclasses import replace from typing import TYPE_CHECKING import sqlalchemy as sa from sqlalchemy import Connection, Engine +from policyengine_api.data.v2.catalog.publication_parameter_tree import ( + assert_every_parameter_tree_written, + write_missing_parameter_trees, +) from policyengine_api.data.v2.catalog.publication_reconciliation import ( assert_canonical_value_uniqueness, assert_country_matches, @@ -118,7 +123,12 @@ def publish_catalog( *, checkpoint: Callable[[str, Connection], None] | None = None, ) -> PublicationEvidence: - """Publish one complete catalog atomically and return non-secret evidence.""" + """Publish one complete catalog atomically and return non-secret evidence. + + In the same transaction it writes the parameter tree of every stored + catalog version that has none: the versions published now, and versions + stored before the tree existed. + """ started = time.monotonic() with engine.begin() as connection: _verify_expected_revision(connection) @@ -134,6 +144,7 @@ def publish_catalog( for country in catalog.countries: if country.country_id not in existing: publish_new_country(connection, country) + parameter_trees_written = write_missing_parameter_trees(connection) if checkpoint is not None: checkpoint("after_reconciliation", connection) @@ -144,13 +155,17 @@ def publish_catalog( include_regions=country.country_id not in existing, ) assert_canonical_value_uniqueness(connection) + assert_every_parameter_tree_written(connection) if _protected_row_counts(connection) != before: msg = "publication changed simulation, report, or dataset-version rows" raise CatalogPublicationError(msg) if checkpoint is not None: checkpoint("after_validation", connection) - return _evidence(catalog, kept_region_summaries, started=started) + return replace( + _evidence(catalog, kept_region_summaries, started=started), + parameter_trees_written=parameter_trees_written, + ) def _check_stored_versions( diff --git a/policyengine_api/data/v2/catalog/publication_parameter_tree.py b/policyengine_api/data/v2/catalog/publication_parameter_tree.py new file mode 100644 index 000000000..4c59d0096 --- /dev/null +++ b/policyengine_api/data/v2/catalog/publication_parameter_tree.py @@ -0,0 +1,165 @@ +"""Write the parameter tree of each stored catalog version that has none. + +A version's tree rows are derived from its stored parameters and parameter +nodes alone (``parameter_tree``), so a version published in the current +transaction and a version stored before the tree existed are written the same +way. Writing only inserts: a version that already has rows keeps them, and no +other table changes. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from typing import TYPE_CHECKING +from uuid import uuid4 + +import sqlalchemy as sa + +from policyengine_api.data.v2.catalog.parameter_tree import ( + ParameterTreeChildRow, + TreeNodeSource, + TreeParameterSource, + build_parameter_tree, +) +from policyengine_api.data.v2.catalog.publication_staging import copy_rows +from policyengine_api.data.v2.catalog.publication_types import ( + CatalogPublicationError, +) +from policyengine_api.data.v2.models import ( + Parameter, + ParameterNode, + ParameterTreeChild, + TaxBenefitModel, + TaxBenefitModelVersion, +) + +if TYPE_CHECKING: + from collections.abc import Iterator + from uuid import UUID + + from sqlalchemy import Connection + +MODELS = TaxBenefitModel.__table__ +MODEL_VERSIONS = TaxBenefitModelVersion.__table__ +PARAMETERS = Parameter.__table__ +PARAMETER_NODES = ParameterNode.__table__ +PARAMETER_TREE_CHILDREN = ParameterTreeChild.__table__ + + +def _versions_without_tree() -> sa.Select: + has_tree = sa.exists().where( + PARAMETER_TREE_CHILDREN.c.tax_benefit_model_version_id == MODEL_VERSIONS.c.id + ) + has_parameters_or_nodes = sa.or_( + sa.exists().where( + PARAMETERS.c.tax_benefit_model_version_id == MODEL_VERSIONS.c.id + ), + sa.exists().where( + PARAMETER_NODES.c.tax_benefit_model_version_id == MODEL_VERSIONS.c.id + ), + ) + return ( + sa.select(MODEL_VERSIONS.c.id, MODELS.c.name, MODEL_VERSIONS.c.version) + .select_from( + MODEL_VERSIONS.join(MODELS, MODELS.c.id == MODEL_VERSIONS.c.model_id) + ) + .where(~has_tree, has_parameters_or_nodes) + .order_by(MODELS.c.name, MODEL_VERSIONS.c.version) + ) + + +def _stored_nodes(connection: Connection, version_id: UUID) -> list[TreeNodeSource]: + return [ + TreeNodeSource(name=row.name, label=row.label) + for row in connection.execute( + sa.select(PARAMETER_NODES.c.name, PARAMETER_NODES.c.label).where( + PARAMETER_NODES.c.tax_benefit_model_version_id == version_id + ) + ) + ] + + +def _stored_parameters( + connection: Connection, version_id: UUID +) -> list[TreeParameterSource]: + return [ + TreeParameterSource( + id=row.id, + name=row.name, + label=row.label, + economy=row.economy, + household=row.household, + ) + for row in connection.execute( + sa.select( + PARAMETERS.c.id, + PARAMETERS.c.name, + PARAMETERS.c.label, + PARAMETERS.c.economy, + PARAMETERS.c.household, + ).where(PARAMETERS.c.tax_benefit_model_version_id == version_id) + ) + ] + + +def _table_rows( + version_id: UUID, + rows: tuple[ParameterTreeChildRow, ...], +) -> Iterator[tuple[object, ...]]: + created_at = datetime.now(UTC) + column_names = [column.name for column in PARAMETER_TREE_CHILDREN.columns] + for row in rows: + values: dict[str, object] = { + "id": uuid4(), + "created_at": created_at, + "tax_benefit_model_version_id": version_id, + "parent_path": row.parent_path, + "path": row.path, + "position": row.position, + "label": row.label, + "child_type": row.child_type, + "child_count": row.child_count, + "reform_position": row.reform_position, + "reform_child_count": row.reform_child_count, + "parameter_id": row.parameter_id, + "economy": row.economy, + "household": row.household, + } + yield tuple(values[name] for name in column_names) + + +def write_missing_parameter_trees( + connection: Connection, +) -> tuple[tuple[str, str, int], ...]: + """Write the tree of every stored version that has parameters but no tree. + + Returns the model name, version and number of rows written for each + version, in model and version order. Runs inside publication's + transaction and advisory lock. + """ + written = [] + for version_id, model_name, version in connection.execute( + _versions_without_tree() + ).all(): + rows = build_parameter_tree( + nodes=_stored_nodes(connection, version_id), + parameters=_stored_parameters(connection, version_id), + ) + count = copy_rows( + connection, + table=PARAMETER_TREE_CHILDREN, + rows=_table_rows(version_id, rows), + ) + written.append((model_name, version, count)) + return tuple(written) + + +def assert_every_parameter_tree_written(connection: Connection) -> None: + """Raise CatalogPublicationError if a version with parameters has no tree.""" + missing = connection.execute(_versions_without_tree()).first() + if missing is not None: + msg = ( + f"the stored catalog version {missing.version} of {missing.name} " + "has no parameter tree" + ) + raise CatalogPublicationError(msg) diff --git a/policyengine_api/data/v2/catalog/publication_types.py b/policyengine_api/data/v2/catalog/publication_types.py index 515232560..203699d5a 100644 --- a/policyengine_api/data/v2/catalog/publication_types.py +++ b/policyengine_api/data/v2/catalog/publication_types.py @@ -22,6 +22,9 @@ class PublicationEvidence: # Stored catalog versions whose region rows were kept although the staged # regions differ: country, version, stored and staged region counts. kept_region_summaries: tuple[tuple[str, str, int, int], ...] = () + # Stored catalog versions whose parameter tree publication wrote: model, + # version and rows written. Empty once every stored version has its tree. + parameter_trees_written: tuple[tuple[str, str, int], ...] = () def as_dict(self) -> dict[str, object]: """Return the evidence as the JSON object printed after a publication.""" @@ -55,5 +58,9 @@ def as_dict(self) -> dict[str, object]: } for country_id, version, stored, staged in self.kept_region_summaries ], + "parameter_trees_written": [ + {"model": model_name, "version": version, "rows": rows} + for model_name, version, rows in self.parameter_trees_written + ], "elapsed_seconds": round(self.elapsed_seconds, 3), } diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index a5ba64d1e..586a6059f 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -798,3 +798,135 @@ def test_republication_detects_a_changed_enum_label_parameter_path_unit_or_perio ) assert _stored_us_variables(publication_engine) == stored + + +def _stored_tree(engine: Engine, model_name: str, version: str) -> list[tuple]: + with engine.connect() as connection: + return [ + tuple(row) + for row in connection.execute( + text( + "SELECT tree.parent_path, tree.path, tree.position, tree.label, " + "tree.child_type, tree.child_count, tree.reform_position, " + "tree.reform_child_count, parameter.name, tree.economy, " + "tree.household " + "FROM parameter_tree_children AS tree " + "JOIN tax_benefit_model_versions AS version " + "ON version.id = tree.tax_benefit_model_version_id " + "JOIN tax_benefit_models AS model ON model.id = version.model_id " + "LEFT JOIN parameters AS parameter " + "ON parameter.id = tree.parameter_id " + "WHERE model.name = :model_name AND version.version = :version " + "ORDER BY tree.parent_path, tree.position" + ), + {"model_name": model_name, "version": version}, + ) + ] + + +def _delete_tree(engine: Engine, version: str) -> None: + """Make the stored versions of one .py version look stored before the tree.""" + with engine.begin() as connection: + connection.execute( + text( + "DELETE FROM parameter_tree_children AS tree " + "USING tax_benefit_model_versions AS version " + "WHERE version.id = tree.tax_benefit_model_version_id " + "AND version.version = :version" + ), + {"version": version}, + ) + + +FIXTURE_TREE = [ + ("", "gov", 0, "Gov", "node", 1, 0, 1, None, None, None), + ("gov", "gov.example", 0, "Example policy", "node", 1, 0, 1, None, None, None), + ( + "gov.example", + "gov.example.rate", + 0, + "Example rate", + "parameter", + None, + 0, + None, + "gov.example.rate", + True, + False, + ), +] + + +def test_publication_writes_each_new_version_parameter_tree( + publication_engine: Engine, +) -> None: + catalog = normalized_catalog() + + evidence = publish_catalog(publication_engine, catalog) + + for country in catalog.countries: + assert ( + _stored_tree( + publication_engine, + country.model.name, + country.model_version.version, + ) + == FIXTURE_TREE + ) + assert evidence.parameter_trees_written == tuple( + (country.model.name, country.model_version.version, len(FIXTURE_TREE)) + for country in sorted(catalog.countries, key=lambda item: item.model.name) + ) + + +def test_publication_writes_the_tree_of_stored_versions_that_have_none( + publication_engine: Engine, +) -> None: + older_version = "1.0.0" + older = normalized_catalog(policyengine_version=older_version) + catalog = normalized_catalog() + older_country_version = older.country("us").model_version.version + publish_catalog(publication_engine, older) + publish_catalog(publication_engine, catalog) + current_country_version = catalog.country("us").model_version.version + _delete_tree(publication_engine, older_country_version) + _delete_tree(publication_engine, current_country_version) + before_ids = _identifiers(publication_engine) + before_counts = _counts(publication_engine) + + evidence = publish_catalog(publication_engine, catalog) + again = publish_catalog(publication_engine, catalog) + + assert { + (model_name, version) + for model_name, version, _rows in evidence.parameter_trees_written + } == { + (country.model.name, version) + for country in catalog.countries + for version in {older_country_version, current_country_version} + } + assert again.parameter_trees_written == () + for country in catalog.countries: + for version in (older_country_version, current_country_version): + assert ( + _stored_tree(publication_engine, country.model.name, version) + == FIXTURE_TREE + ) + assert _identifiers(publication_engine) == before_ids + assert _counts(publication_engine) == before_counts + + +def test_publication_fails_when_a_stored_version_is_left_without_a_tree( + publication_engine: Engine, + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr( + publication_module, + "write_missing_parameter_trees", + lambda _connection: (), + ) + + with pytest.raises(CatalogPublicationError, match="has no parameter tree"): + publish_catalog(publication_engine, normalized_catalog()) + + assert _counts(publication_engine)["tax_benefit_model_versions"] == 0 diff --git a/tests/unit/v2/test_catalog_publication.py b/tests/unit/v2/test_catalog_publication.py index 47cb262ba..5e2766926 100644 --- a/tests/unit/v2/test_catalog_publication.py +++ b/tests/unit/v2/test_catalog_publication.py @@ -284,6 +284,7 @@ def test_completion_evidence_contains_only_reviewed_non_secret_fields() -> None: elapsed_seconds=2.3456, omitted_region_summaries=(("us", "place", 333),), kept_region_summaries=(("uk", "6.7.5", 5, 1_015),), + parameter_trees_written=(("policyengine-uk", "6.7.5", 2_430),), ).as_dict() assert evidence == { @@ -305,6 +306,13 @@ def test_completion_evidence_contains_only_reviewed_non_secret_fields() -> None: "staged_regions": 1_015, } ], + "parameter_trees_written": [ + { + "model": "policyengine-uk", + "version": "6.7.5", + "rows": 2_430, + } + ], "elapsed_seconds": 2.346, } assert ( From 8bd67742f70af9e2e9dffa94ea578e718c99bfc5 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:16:08 +0400 Subject: [PATCH 04/23] Serve /v2/parameters/children from the stored parameter tree The route now reads one level of the version's stored tree with one indexed read, so a bracket schedule is one node whose children are its brackets, and every child has its stored or generated label. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those. The route reads its query through the shared strict models, so unknown or repeated parameters and an unsupported country return 422, and a version whose tree has not been written returns 503. Co-Authored-By: Claude Opus 5.5 --- .../v2plan-1f-117-parameter-children.added.md | 1 + ...2plan-1f-117-parameter-children.changed.md | 1 + docs/migration/stage-9-v2-metadata.md | 9 + .../fastapi_routes/dependencies.py | 8 +- .../v2/metadata/parameter_routes.py | 8 +- policyengine_api/query_parameters.py | 15 +- .../reads_parameter_tree.py | 171 ++++--------- .../services/v2/metadata/services.py | 28 +- .../services/v2/metadata/transformations.py | 7 +- .../services/v2/metadata/types.py | 13 + .../services/v2/metadata/validators.py | 8 + rules/registry.toml | 2 +- tests/integration/test_v2_metadata_routes.py | 157 +++++++++++- tests/unit/v2/test_metadata_routes.py | 123 +++++++++ tests/unit/v2/test_metadata_service.py | 241 +++++++++++++++++- 15 files changed, 651 insertions(+), 141 deletions(-) create mode 100644 changelog.d/v2plan-1f-117-parameter-children.added.md create mode 100644 changelog.d/v2plan-1f-117-parameter-children.changed.md diff --git a/changelog.d/v2plan-1f-117-parameter-children.added.md b/changelog.d/v2plan-1f-117-parameter-children.added.md new file mode 100644 index 000000000..ee9da1020 --- /dev/null +++ b/changelog.d/v2plan-1f-117-parameter-children.added.md @@ -0,0 +1 @@ +Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and a path without a stored label gets one made from the last part of its name. On every run, publication also writes the tree of each stored version that has none, so versions published earlier get it from the next deploy; the evidence lists each version written. diff --git a/changelog.d/v2plan-1f-117-parameter-children.changed.md b/changelog.d/v2plan-1f-117-parameter-children.changed.md new file mode 100644 index 000000000..6b1782870 --- /dev/null +++ b/changelog.d/v2plan-1f-117-parameter-children.changed.md @@ -0,0 +1 @@ +/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its stored or generated label. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters and an unsupported country return 422, and a catalog version whose tree has not been written returns 503. diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index 41c62b695..ee0aae891 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -332,6 +332,15 @@ and row count, and is empty once every stored version has its tree. Publication fails if a stored version with parameters is still without a tree when it finishes. +`/v2/parameters/children` lists one level with one indexed read: +`parent_path` names the node ("" for the top level), and +`reform_changeable=true` keeps only the children a reform can change, sorted +by label, with child counts that count only those. Without it, children are +in path order, with a schedule's brackets in bracket order. A node carries +its child count, a parameter its summary. An unknown parent path has no +children. A catalog version whose tree has not been written returns 503, +which means the publication step has not run since the version was stored. + ## Validation evidence and retry Successful JSON evidence contains: diff --git a/policyengine_api/fastapi_routes/dependencies.py b/policyengine_api/fastapi_routes/dependencies.py index 0be11f8fc..f3a343827 100644 --- a/policyengine_api/fastapi_routes/dependencies.py +++ b/policyengine_api/fastapi_routes/dependencies.py @@ -55,6 +55,7 @@ MetadataRegion, MetadataVariable, ParameterFilter, + ParameterTreeLevel, ParameterValueFilter, ) from policyengine_api.services.v2.policies.types import ( @@ -223,13 +224,14 @@ def list_parameter_children( country_id: str, policyengine_version: str | None = None, *, - parent_path: str = "", + level: ParameterTreeLevel | None = None, offset: int = 0, limit: int = 100, ) -> MetadataPageResult[MetadataParameterChild]: - """Return the nodes and parameters one level below ``parent_path``. + """Return the nodes and parameters one level below the level's parent path. - An empty ``parent_path`` lists the top level of the parameter tree. + An empty parent path lists the top level of the parameter tree; the + level's ``reform_changeable`` keeps only what a reform can change. """ ... diff --git a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py index 51383ccc0..b66f2087f 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py @@ -29,6 +29,7 @@ ) from policyengine_api.services.v2.metadata.types import ( ParameterFilter, + ParameterTreeLevel, ParameterValueFilter, ) @@ -76,7 +77,7 @@ def list_parameters( @router.get( "/parameters/children", response_model=MetadataParameterChildPageResponse, - summary="List direct children of one parameter path", + summary="List one level of a catalog version's parameter tree", ) def list_parameter_children( query: ParameterChildrenQueryDependency, @@ -87,7 +88,10 @@ def list_parameter_children( lambda reader: reader.list_parameter_children( query.country_id, query.policyengine_version, - parent_path=query.parent_path, + level=ParameterTreeLevel( + parent_path=query.parent_path, + reform_changeable=query.reform_changeable, + ), offset=query.offset, limit=query.limit, ), diff --git a/policyengine_api/query_parameters.py b/policyengine_api/query_parameters.py index 0d94c8484..8085bd650 100644 --- a/policyengine_api/query_parameters.py +++ b/policyengine_api/query_parameters.py @@ -261,11 +261,22 @@ class RegionCollectionQuery(CatalogPageQuery): class ParameterChildrenQuery(CatalogPageQuery): - """Query contract for a page of one parameter path's direct children.""" + """Query contract for one level of a catalog version's parameter tree.""" parent_path: str = Field( default="", - description="Dotted path whose direct children are listed; empty for the root", + max_length=512, + description=( + "Path of the node whose direct children are listed; empty for the " + "top level. A bracket schedule's brackets are its children." + ), + ) + reform_changeable: bool = Field( + default=False, + description=( + "List only the children a reform can change, sorted by label, with " + "child counts that count only those" + ), ) diff --git a/policyengine_api/services/v2/metadata/database_connectors/reads_parameter_tree.py b/policyengine_api/services/v2/metadata/database_connectors/reads_parameter_tree.py index 49f232a8e..6a7eb34d1 100644 --- a/policyengine_api/services/v2/metadata/database_connectors/reads_parameter_tree.py +++ b/policyengine_api/services/v2/metadata/database_connectors/reads_parameter_tree.py @@ -1,4 +1,4 @@ -"""Database selections for direct parameter-tree children.""" +"""Database selections for one level of a catalog version's parameter tree.""" from __future__ import annotations @@ -7,158 +7,91 @@ import sqlalchemy as sa from sqlmodel import Session, col -from policyengine_api.data.v2.models import Parameter, ParameterNode +from policyengine_api.data.v2.models import Parameter, ParameterTreeChild from policyengine_api.services.v2.metadata.database_connectors.reads import read_rows if TYPE_CHECKING: from uuid import UUID - from sqlalchemy.orm import Mapped - - -def _escaped_like(value: str) -> str: - return value.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") - - -def _path_segment( - remainder: sa.ColumnElement[str], dialect: str -) -> sa.ColumnElement[str]: - dot_position = ( - sa.func.instr(remainder, ".") - if dialect == "sqlite" - else sa.func.strpos(remainder, ".") - ) - return sa.case( - (dot_position > 0, sa.func.substr(remainder, 1, dot_position - 1)), - else_=remainder, - ) - - -def _child_path( - column: Mapped[str], prefix: str, dialect: str -) -> sa.ColumnElement[str]: - remainder = sa.func.substr(column, len(prefix) + 1) - return sa.literal(prefix) + _path_segment(remainder, dialect) - - -def _direct_child_path( - column: Mapped[str], parent_path: sa.ColumnElement[str], dialect: str -) -> sa.ColumnElement[str]: - remainder = sa.func.substr(column, sa.func.length(parent_path) + 2) - return parent_path + "." + _path_segment(remainder, dialect) - - -def _has_path_prefix( - column: Mapped[str], parent_path: sa.ColumnElement[str] -) -> sa.ColumnElement[bool]: - return ( - sa.func.substr(column, 1, sa.func.length(parent_path) + 1) == parent_path + "." - ) + from policyengine_api.services.v2.metadata.types import ParameterTreeLevel def _parameter_children_statement( *, model_version_id: UUID, - parent_path: str, - dialect: str, + level: ParameterTreeLevel, offset: int, limit: int, ) -> sa.Select[Any]: - prefix = f"{parent_path}." if parent_path else "" - escaped_prefix = _escaped_like(prefix) - node_name = col(ParameterNode.name) - node_model_version_id = col(ParameterNode.tax_benefit_model_version_id) - parameter_name = col(Parameter.name) - parameter_model_version_id = col(Parameter.tax_benefit_model_version_id) - node_child_path = _child_path(node_name, prefix, dialect) - parameter_child_path = _child_path(parameter_name, prefix, dialect) - paths = sa.union( - sa.select(node_child_path.label("path")).where( - node_model_version_id == model_version_id, - node_name.like(f"{escaped_prefix}%", escape="\\"), - ), - sa.select(parameter_child_path.label("path")).where( - parameter_model_version_id == model_version_id, - parameter_name.like(f"{escaped_prefix}%", escape="\\"), - ), - ).subquery() - direct_child_paths = sa.union( - sa.select(_direct_child_path(node_name, paths.c.path, dialect).label("path")) - .where( - node_model_version_id == model_version_id, - _has_path_prefix(node_name, paths.c.path), - ) - .correlate(paths), - sa.select( - _direct_child_path(parameter_name, paths.c.path, dialect).label("path") - ) - .where( - parameter_model_version_id == model_version_id, - _has_path_prefix(parameter_name, paths.c.path), - ) - .correlate(paths), - ).subquery() - direct_child_count = ( - sa.select(sa.func.count()) - .select_from(direct_child_paths) - .correlate(paths) - .scalar_subquery() + position = ( + col(ParameterTreeChild.reform_position) + if level.reform_changeable + else col(ParameterTreeChild.position) + ) + child_count = ( + col(ParameterTreeChild.reform_child_count) + if level.reform_changeable + else col(ParameterTreeChild.child_count) ) - parameter_id = col(Parameter.id) - is_node = sa.or_(direct_child_count > 0, parameter_id.is_(None)) - return ( + statement: sa.Select[Any] = ( sa.select( - paths.c.path, - sa.func.coalesce(col(ParameterNode.label), col(Parameter.label)).label( - "label" - ), - sa.case((is_node, "node"), else_="parameter").label("type"), - sa.case((is_node, direct_child_count), else_=None).label("child_count"), + col(ParameterTreeChild.path).label("path"), + col(ParameterTreeChild.label).label("label"), + col(ParameterTreeChild.child_type).label("type"), + child_count.label("child_count"), Parameter, ) - .select_from( - paths.outerjoin( - ParameterNode, - sa.and_( - node_name == paths.c.path, - node_model_version_id == model_version_id, - ), - ).outerjoin( - Parameter, - sa.and_( - parameter_name == paths.c.path, - parameter_model_version_id == model_version_id, - ), - ) + .select_from(ParameterTreeChild) + .outerjoin( + Parameter, + col(Parameter.id) == col(ParameterTreeChild.parameter_id), + ) + .where( + col(ParameterTreeChild.tax_benefit_model_version_id) == model_version_id, + col(ParameterTreeChild.parent_path) == level.parent_path, ) - .order_by(paths.c.path) - .offset(offset) - .limit(limit + 1) ) + if level.reform_changeable: + statement = statement.where(position.is_not(None)) + return statement.order_by(position).offset(offset).limit(limit + 1) def read_parameter_children( session: Session, *, model_version_id: UUID, - parent_path: str, + level: ParameterTreeLevel, offset: int, limit: int, ) -> list[Any]: - """Return a page of the direct children of parent_path in a model version's tree. - - Each row has the child's path, label, type (node or parameter), child count - and the stored Parameter (None for a node). The session's dialect - selects SQLite or PostgreSQL string functions; limit + 1 rows are fetched so - the caller can tell whether another page exists. + """Return a page of the level's direct children from the stored tree. + + One read of a listing index: every child in tree order, or for a + reform-changeable level only the children a reform can change, in label order, + with child counts that count only those. Each row has the child's path, + label, type (node or parameter), child count and the stored Parameter + (None for a node); limit + 1 rows are fetched so the caller can tell + whether another page exists. """ return read_rows( session, _parameter_children_statement( model_version_id=model_version_id, - parent_path=parent_path, - dialect=session.get_bind().dialect.name, + level=level, offset=offset, limit=limit, ), ) + + +def read_parameter_tree_written(session: Session, *, model_version_id: UUID) -> bool: + """Return whether the version's parameter tree has any stored row.""" + rows = read_rows( + session, + sa.select( + sa.exists().where( + col(ParameterTreeChild.tax_benefit_model_version_id) == model_version_id + ) + ), + ) + return bool(rows[0][0]) diff --git a/policyengine_api/services/v2/metadata/services.py b/policyengine_api/services/v2/metadata/services.py index 4496e3b30..4153ba0fb 100644 --- a/policyengine_api/services/v2/metadata/services.py +++ b/policyengine_api/services/v2/metadata/services.py @@ -21,6 +21,7 @@ ) from policyengine_api.services.v2.metadata.database_connectors.reads_parameter_tree import ( # noqa: E501 # a dotted module path cannot be wrapped read_parameter_children, + read_parameter_tree_written, ) from policyengine_api.services.v2.metadata.database_connectors.reads_parameters import ( read_current_law_values_by_parameter_name, @@ -78,9 +79,11 @@ MetadataRegion, MetadataVariable, ParameterFilter, + ParameterTreeLevel, ParameterValueFilter, ) from policyengine_api.services.v2.metadata.validators import ( + ParameterTreeUnavailableError, require_metadata_resource, validate_economy_options, validate_metadata_page, @@ -345,25 +348,38 @@ def list_parameter_children( country_id: str, policyengine_version: str | None = None, *, - parent_path: str = "", + level: ParameterTreeLevel | None = None, offset: int = 0, limit: int = 100, ) -> MetadataPageResult[MetadataParameterChild]: - """Return a page of the direct children of parent_path in the parameter tree. + """Return a page of one level of the version's stored parameter tree. - An empty parent_path lists the top-level children. A child is a parameter - when it is a leaf with a parameter row, and a node otherwise. + The level's parent path "" lists the top-level children. A bracket + schedule is one node whose children are its brackets. With the level's + ``reform_changeable``, only the children a reform can change are listed, + sorted by label. Raise ParameterTreeUnavailableError when the version's + tree has not been written. """ + selected_level = level if level is not None else ParameterTreeLevel() selected = self._select_paginated_catalog( country_id, policyengine_version, offset=offset, limit=limit ) + session = self._database_session.session rows = read_parameter_children( - self._database_session.session, + session, model_version_id=selected.model_version.id, - parent_path=parent_path, + level=selected_level, offset=offset, limit=limit, ) + if not rows and not read_parameter_tree_written( + session, model_version_id=selected.model_version.id + ): + msg = ( + f"the parameter tree of catalog version " + f"{selected.policyengine_version} has not been written" + ) + raise ParameterTreeUnavailableError(msg) return page_result( selected, metadata_parameter_children(rows), diff --git a/policyengine_api/services/v2/metadata/transformations.py b/policyengine_api/services/v2/metadata/transformations.py index 00c197cac..3b1ad02c3 100644 --- a/policyengine_api/services/v2/metadata/transformations.py +++ b/policyengine_api/services/v2/metadata/transformations.py @@ -325,13 +325,14 @@ def metadata_region(region: Region) -> MetadataRegion: def metadata_parameter_children(rows: list[Any]) -> list[MetadataParameterChild]: """Build parameter-tree children from read_parameter_children rows. - Parameter leaves carry the same summary as the parameter routes, and a child - with no label is labelled with the last segment of its path. + Parameter leaves carry the same summary as the parameter routes. Every + stored child has a label: its stored label, "Bracket N" for a bracket, or + one generated from the last part of its name. """ return [ MetadataParameterChild( path=row.path, - label=row.label or row.path.rsplit(".", 1)[-1], + label=row.label, type=row.type, child_count=row.child_count, parameter=( diff --git a/policyengine_api/services/v2/metadata/types.py b/policyengine_api/services/v2/metadata/types.py index 63a77944e..ce362d58f 100644 --- a/policyengine_api/services/v2/metadata/types.py +++ b/policyengine_api/services/v2/metadata/types.py @@ -66,6 +66,19 @@ class ParameterNodeLabelRow(NamedTuple): label: str | None +@dataclass(frozen=True) +class ParameterTreeLevel: + """Which level of a catalog version's parameter tree a listing reads. + + ``parent_path`` names the node whose direct children are listed ("" for + the top level); ``reform_changeable`` keeps only the children a reform can + change, sorted by label, with child counts that count only those. + """ + + parent_path: str = "" + reform_changeable: bool = False + + class StrictResponseModel(BaseModel): """Base for metadata results that rejects unknown fields and non-finite numbers.""" diff --git a/policyengine_api/services/v2/metadata/validators.py b/policyengine_api/services/v2/metadata/validators.py index bdc59c8d2..92ad20402 100644 --- a/policyengine_api/services/v2/metadata/validators.py +++ b/policyengine_api/services/v2/metadata/validators.py @@ -24,6 +24,14 @@ class InvalidMetadataPageError(ValueError): """Raised when collection pagination is outside the documented bounds.""" +class ParameterTreeUnavailableError(MetadataCatalogUnavailableError): + """Raised when a selected catalog version's parameter tree is not written. + + Catalog publication writes every stored version's tree, so this means the + publication step has not run since the version was stored. + """ + + def validate_metadata_page(offset: int, limit: int) -> tuple[int, int]: """Return offset and limit unchanged, or raise InvalidMetadataPageError. diff --git a/rules/registry.toml b/rules/registry.toml index 24faf8334..2613a20a9 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -671,7 +671,7 @@ id = "api.v2.runtime-privileges-tested-as-runtime-role" category = "migration" statement = "The cross-database integration tests run the v2 user services as policyengine_v2_runtime and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI." rationale = "docs/engineering/skills/alembic-migrations.md#table-privileges" -scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_saved_policy_view.py", "tests/integration/test_v2_stage12_comparison_persistence.py"] +scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_saved_policy_view.py", "tests/integration/test_v2_stage12_comparison_persistence.py", "tests/integration/test_v2_metadata_routes.py"] tier = "mechanical" status = "enforced" autofix = false diff --git a/tests/integration/test_v2_metadata_routes.py b/tests/integration/test_v2_metadata_routes.py index 7f479d183..e0119b71d 100644 --- a/tests/integration/test_v2_metadata_routes.py +++ b/tests/integration/test_v2_metadata_routes.py @@ -5,6 +5,7 @@ import os from datetime import UTC, datetime from pathlib import Path +from types import SimpleNamespace from uuid import uuid4 import pytest @@ -18,6 +19,7 @@ from sqlmodel import Session, select from policyengine_api.asgi_factory import create_asgi_app +from policyengine_api.data.v2.catalog.extraction import extract_catalog from policyengine_api.data.v2.catalog.publication import publish_catalog from policyengine_api.data.v2.models import ( Dataset, @@ -37,7 +39,15 @@ MetadataDatabaseSession, ) from policyengine_api.services.v2.metadata.services import V2MetadataService -from tests.fixtures.v2_catalog import POLICYENGINE_VERSION, normalized_catalog +from policyengine_api.services.v2.metadata.types import ParameterTreeLevel +from tests.fixtures.v2_catalog import ( + POLICYENGINE_VERSION, + bundle, + installed_version, + normalized_catalog, + region_references, + source_models, +) REPO = Path(__file__).parents[2] DISPOSABLE_DATABASE = "policyengine_v2_alembic_test" @@ -723,3 +733,148 @@ def test_postgres_current_values_and_their_lifetime_use_one_instant( expected_values ) assert response.headers["cache-control"] == expected_cache_control + + +SCHEDULE = "gov.hmrc.income_tax.rates.uk" + + +def _catalog_with_a_bracket_schedule(): + """Return the fixture catalog with a UK-style income tax schedule added. + + The schedule has a stored node row; a second schedule has none. + """ + models = source_models() + uk = models["uk"] + nodes = [ + SimpleNamespace(name=SCHEDULE, label="Income Tax main rates", description=None) + ] + parameters = [ + SimpleNamespace( + name=name, + label=label, + description=None, + data_type=float, + unit="/1", + economy=economy, + household=household, + parameter_values=[ + SimpleNamespace( + value=0.2, + start_date=datetime(2025, 1, 1, tzinfo=UTC), + end_date=None, + ) + ], + ) + for name, label, economy, household in ( + (f"{SCHEDULE}[0].rate", "Basic rate", True, True), + (f"{SCHEDULE}[0].threshold", "Basic rate threshold", True, True), + (f"{SCHEDULE}[1].rate", "Higher rate", True, True), + (f"{SCHEDULE}[1].threshold", "Higher rate threshold", False, False), + ("gov.hmrc.income_tax.rates.dividends[0].rate", None, True, False), + ) + ] + uk.parameter_nodes.extend(nodes) + uk.parameters.extend(parameters) + uk.parameter_nodes_by_name.update({node.name: node for node in nodes}) + uk.parameters_by_name.update( + {parameter.name: parameter for parameter in parameters} + ) + return extract_catalog( + bundle=bundle(), + policyengine_version=POLICYENGINE_VERSION, + models=models, + installed_version=installed_version, + region_references=region_references(), + ) + + +def _items(response) -> list[tuple]: + assert response.status_code == 200, response.text + return [ + (item["path"], item["label"], item["type"], item["child_count"]) + for item in response.json()["result"]["items"] + ] + + +def test_postgres_parameter_tree_nests_schedules_and_filters_reform_changes( + published_engine: Engine, +) -> None: + with published_engine.begin() as connection: + connection.execute(text("TRUNCATE tax_benefit_models CASCADE")) + publish_catalog(published_engine, _catalog_with_a_bracket_schedule()) + client = _client(published_engine) + query = {"country_id": "uk"} + + rates = client.get( + "/v2/parameters/children", + params={**query, "parent_path": "gov.hmrc.income_tax.rates"}, + ) + schedule = client.get( + "/v2/parameters/children", params={**query, "parent_path": SCHEDULE} + ) + bracket = client.get( + "/v2/parameters/children", + params={**query, "parent_path": f"{SCHEDULE}[1]"}, + ) + reform_bracket = client.get( + "/v2/parameters/children", + params={ + **query, + "parent_path": f"{SCHEDULE}[1]", + "reform_changeable": "true", + }, + ) + reform_rates = client.get( + "/v2/parameters/children", + params={ + **query, + "parent_path": "gov.hmrc.income_tax.rates", + "reform_changeable": "true", + }, + ) + + assert _items(rates) == [ + ("gov.hmrc.income_tax.rates.dividends", "Dividends", "node", 1), + (SCHEDULE, "Income Tax main rates", "node", 2), + ] + assert _items(schedule) == [ + (f"{SCHEDULE}[0]", "Bracket 1", "node", 2), + (f"{SCHEDULE}[1]", "Bracket 2", "node", 2), + ] + assert _items(bracket) == [ + (f"{SCHEDULE}[1].rate", "Higher rate", "parameter", None), + (f"{SCHEDULE}[1].threshold", "Higher rate threshold", "parameter", None), + ] + assert _items(reform_bracket) == [ + (f"{SCHEDULE}[1].rate", "Higher rate", "parameter", None), + ] + assert _items(reform_rates) == [ + ("gov.hmrc.income_tax.rates.dividends", "Dividends", "node", 1), + (SCHEDULE, "Income Tax main rates", "node", 2), + ] + leaf = bracket.json()["result"]["items"][0]["parameter"] + assert leaf["name"] == f"{SCHEDULE}[1].rate" + + +def test_postgres_parameter_tree_reads_as_the_runtime_role( + published_engine: Engine, + managed_v2_roles, + v2_engine_as_role, +) -> None: + """The Public API's runtime role can read the tree with its declared privileges.""" + runtime_engine = v2_engine_as_role(managed_v2_roles.runtime, poolclass=NullPool) + try: + with Session(runtime_engine) as session: + result = V2MetadataService( + MetadataDatabaseSession(session), + running_policyengine_version=POLICYENGINE_VERSION, + ).list_parameter_children( + "us", + level=ParameterTreeLevel(parent_path="gov", reform_changeable=True), + ) + finally: + runtime_engine.dispose() + + assert [(item.path, item.child_count) for item in result.items] == [ + ("gov.example", 1) + ] diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 037b2952f..480156625 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -54,10 +54,12 @@ MetadataTimePeriodOption, MetadataVariable, ParameterFilter, + ParameterTreeLevel, ) from policyengine_api.services.v2.metadata.validators import ( InvalidMetadataPageError, MetadataResourceNotFoundError, + ParameterTreeUnavailableError, ) @@ -1473,3 +1475,124 @@ def test_openapi_requires_every_catalog_format_2_field_as_nullable() -> None: assert set(schema["properties"]) <= set(schema["required"]), schema_name for field in fields: assert {"type": "null"} in schema["properties"][field]["anyOf"], field + + +def test_parameter_children_route_forwards_its_level_and_page() -> None: + results, _ids = _resource_results() + reader = ResourceReader(results) + + response = _client(lambda: reader).get( + "/v2/parameters/children", + params={ + "country_id": "UK", + "policyengine_version": "4.19.0", + "parent_path": "gov.hmrc.income_tax.rates.uk", + "reform_changeable": "true", + "offset": 10, + "limit": 20, + }, + ) + + assert response.status_code == 200 + assert reader.calls == [ + ( + "list_parameter_children", + ("uk", "4.19.0"), + { + "level": ParameterTreeLevel( + parent_path="gov.hmrc.income_tax.rates.uk", + reform_changeable=True, + ), + "offset": 10, + "limit": 20, + }, + ) + ] + + +def test_parameter_children_route_defaults_to_the_full_top_level() -> None: + results, _ids = _resource_results() + reader = ResourceReader(results) + + response = _client(lambda: reader).get( + "/v2/parameters/children", params={"country_id": "us"} + ) + + assert response.status_code == 200 + assert reader.calls == [ + ( + "list_parameter_children", + ("us", None), + {"level": ParameterTreeLevel(), "offset": 0, "limit": 100}, + ) + ] + + +@pytest.mark.parametrize( + "query", + [ + "country_id=us&parentpath=gov", + "country_id=us&parent_path=gov&parent_path=gov.irs", + "country_id=us&reform_changeable=yes-please", + "country_id=us&limit=501", + "country_id=ca", + "parent_path=gov", + "country_id=us&parent_path=" + "x" * 513, + ], +) +def test_parameter_children_route_rejects_invalid_queries(query: str) -> None: + calls = [] + + def factory(): + calls.append("called") + results, _ids = _resource_results() + return ResourceReader(results) + + response = _client(factory).get(f"/v2/parameters/children?{query}") + + assert response.status_code == 422 + assert response.json() == { + "status": "error", + "message": "Invalid API v2 request", + } + assert calls == [] + + +def test_parameter_children_route_reports_an_unwritten_tree_as_unavailable() -> None: + results, _ids = _resource_results() + reader = ResourceReader( + results, + error=ParameterTreeUnavailableError("tree not written"), + ) + + response = _client(lambda: reader).get( + "/v2/parameters/children", params={"country_id": "us"} + ) + + assert response.status_code == 503 + assert response.json()["status"] == "error" + + +def test_parameter_children_openapi_declares_the_query_contract() -> None: + results, _ids = _resource_results() + schema = _client(lambda: ResourceReader(results)).get("/v2/openapi.json").json() + + parameters = { + parameter["name"]: parameter + for parameter in schema["paths"]["/v2/parameters/children"]["get"]["parameters"] + } + + assert set(parameters) == { + "country_id", + "policyengine_version", + "offset", + "limit", + "parent_path", + "reform_changeable", + } + assert parameters["country_id"]["required"] is True + assert parameters["parent_path"]["schema"]["default"] == "" + assert parameters["parent_path"]["schema"]["maxLength"] == 512 + assert parameters["reform_changeable"]["schema"]["type"] == "boolean" + assert parameters["reform_changeable"]["schema"]["default"] is False + assert parameters["limit"]["schema"]["maximum"] == 500 diff --git a/tests/unit/v2/test_metadata_service.py b/tests/unit/v2/test_metadata_service.py index b2f35d7be..fd6dac31c 100644 --- a/tests/unit/v2/test_metadata_service.py +++ b/tests/unit/v2/test_metadata_service.py @@ -18,11 +18,17 @@ MetadataCatalogVersionNotFoundError, UnsupportedPreviewCountryError, ) +from policyengine_api.data.v2.catalog.parameter_tree import ( + TreeNodeSource, + TreeParameterSource, + build_parameter_tree, +) from policyengine_api.data.v2.models import ( V2_METADATA, Dataset, Parameter, ParameterNode, + ParameterTreeChild, ParameterValue, Region, RegionType, @@ -45,11 +51,13 @@ MetadataEnumValue, ParameterFilter, ParameterNodeLabelRow, + ParameterTreeLevel, ParameterValueFilter, ) from policyengine_api.services.v2.metadata.validators import ( InvalidMetadataPageError, MetadataResourceNotFoundError, + ParameterTreeUnavailableError, ) from tests.fixtures.v2_catalog import POLICYENGINE_VERSION, normalized_catalog @@ -248,6 +256,57 @@ def _model_version( ).one() +def _write_parameter_tree(session: Session, model_version_id) -> None: + """Write a version's tree from its stored rows, as catalog publication does.""" + nodes = session.exec( + select(ParameterNode).where( + ParameterNode.tax_benefit_model_version_id == model_version_id + ) + ).all() + parameters = session.exec( + select(Parameter).where( + Parameter.tax_benefit_model_version_id == model_version_id + ) + ).all() + rows = build_parameter_tree( + nodes=[TreeNodeSource(name=node.name, label=node.label) for node in nodes], + parameters=[ + TreeParameterSource( + id=parameter.id, + name=parameter.name, + label=parameter.label, + economy=parameter.economy, + household=parameter.household, + ) + for parameter in parameters + ], + ) + session.add_all( + ParameterTreeChild( + tax_benefit_model_version_id=model_version_id, + parent_path=row.parent_path, + path=row.path, + position=row.position, + label=row.label, + child_type=row.child_type, + child_count=row.child_count, + reform_position=row.reform_position, + reform_child_count=row.reform_child_count, + parameter_id=row.parameter_id, + economy=row.economy, + household=row.household, + ) + for row in rows + ) + session.commit() + + +def _level(parent_path: str = "", *, reform_changeable: bool = False): + return ParameterTreeLevel( + parent_path=parent_path, reform_changeable=reform_changeable + ) + + def _service( session: Session, *, selected_catalogs: SelectedCatalogStore | None = None ) -> V2MetadataService: @@ -433,14 +492,15 @@ def test_parameter_children_are_loaded_one_level_at_a_time( ) catalog_session.add_all([nested_node, nested_parameter]) catalog_session.commit() + _write_parameter_tree(catalog_session, model_version.id) service = _service(catalog_session) root = service.list_parameter_children("us") - government = service.list_parameter_children("us", parent_path="gov") - example = service.list_parameter_children("us", parent_path="gov.example") + government = service.list_parameter_children("us", level=_level("gov")) + example = service.list_parameter_children("us", level=_level("gov.example")) nested = service.list_parameter_children( "us", - parent_path="gov.example.nested", + level=_level("gov.example.nested"), ) assert [(item.path, item.type) for item in root.items] == [("gov", "node")] @@ -460,6 +520,176 @@ def test_parameter_children_are_loaded_one_level_at_a_time( assert example.items[1].parameter is not None +def _add_parameters(session: Session, model_version, parameters) -> None: + session.add_all( + Parameter( + id=uuid4(), + tax_benefit_model_version_id=model_version.id, + name=name, + label=label, + description=None, + data_type="float", + unit="/1", + economy=economy, + household=household, + ) + for name, label, economy, household in parameters + ) + + +RATES = "gov.example.rates" + + +def test_bracket_schedules_are_one_node_whose_children_are_their_brackets( + catalog_session: Session, +) -> None: + model_version = _model_version(catalog_session, "policyengine-us") + catalog_session.add_all( + [ + ParameterNode( + id=uuid4(), + tax_benefit_model_version_id=model_version.id, + name=RATES, + label="Example rates", + description=None, + ), + ParameterNode( + id=uuid4(), + tax_benefit_model_version_id=model_version.id, + name=f"{RATES}.main", + label="Main rates", + description=None, + ), + ] + ) + _add_parameters( + catalog_session, + model_version, + [ + (f"{RATES}.main[0].rate", "Basic rate", True, True), + (f"{RATES}.main[0].threshold", "Basic threshold", True, True), + (f"{RATES}.main[1].rate", "Higher rate", True, True), + (f"{RATES}.main[1].threshold", "Higher threshold", True, True), + (f"{RATES}.dividends[0].rate", None, True, True), + ], + ) + catalog_session.commit() + _write_parameter_tree(catalog_session, model_version.id) + service = _service(catalog_session) + + rates = service.list_parameter_children("us", level=_level(RATES)) + schedule = service.list_parameter_children("us", level=_level(f"{RATES}.main")) + bracket = service.list_parameter_children("us", level=_level(f"{RATES}.main[0]")) + + assert [ + (item.path, item.label, item.type, item.child_count) for item in rates.items + ] == [ + (f"{RATES}.dividends", "Dividends", "node", 1), + (f"{RATES}.main", "Main rates", "node", 2), + ] + assert [ + (item.path, item.label, item.type, item.child_count) for item in schedule.items + ] == [ + (f"{RATES}.main[0]", "Bracket 1", "node", 2), + (f"{RATES}.main[1]", "Bracket 2", "node", 2), + ] + assert [(item.path, item.label, item.type) for item in bracket.items] == [ + (f"{RATES}.main[0].rate", "Basic rate", "parameter"), + (f"{RATES}.main[0].threshold", "Basic threshold", "parameter"), + ] + assert bracket.items[0].parameter is not None + assert bracket.items[0].parameter.name == f"{RATES}.main[0].rate" + assert bracket.items[0].child_count is None + + +def test_the_reform_changeable_level_lists_only_what_a_reform_can_change( + catalog_session: Session, +) -> None: + model_version = _model_version(catalog_session, "policyengine-us") + _add_parameters( + catalog_session, + model_version, + [ + ("gov.tax.zero_rate", "Zero rate", True, False), + ("gov.tax.allowance", "Allowance", False, True), + ("gov.tax.internal", "Internal", False, False), + ("gov.abolitions.tax", "Abolish tax", True, True), + ], + ) + catalog_session.commit() + _write_parameter_tree(catalog_session, model_version.id) + service = _service(catalog_session) + + full = service.list_parameter_children("us", level=_level("gov.tax")) + changeable = service.list_parameter_children( + "us", level=_level("gov.tax", reform_changeable=True) + ) + government = service.list_parameter_children( + "us", level=_level("gov", reform_changeable=True) + ) + + assert [item.path for item in full.items] == [ + "gov.tax.allowance", + "gov.tax.internal", + "gov.tax.zero_rate", + ] + assert [item.label for item in changeable.items] == ["Allowance", "Zero rate"] + assert {item.path: item.child_count for item in government.items} == { + "gov.example": 1, + "gov.tax": 2, + } + + +def test_parameter_tree_levels_are_paged(catalog_session: Session) -> None: + model_version = _model_version(catalog_session, "policyengine-us") + _add_parameters( + catalog_session, + model_version, + [(f"gov.paged.item_{index}", None, True, True) for index in range(3)], + ) + catalog_session.commit() + _write_parameter_tree(catalog_session, model_version.id) + service = _service(catalog_session) + + first = service.list_parameter_children( + "us", level=_level("gov.paged"), offset=0, limit=2 + ) + last = service.list_parameter_children( + "us", level=_level("gov.paged"), offset=2, limit=2 + ) + + assert [item.path for item in first.items] == [ + "gov.paged.item_0", + "gov.paged.item_1", + ] + assert first.has_more is True + assert [item.path for item in last.items] == ["gov.paged.item_2"] + assert last.has_more is False + + +def test_an_unwritten_parameter_tree_is_reported_as_unavailable( + catalog_session: Session, +) -> None: + service = _service(catalog_session) + + with pytest.raises(ParameterTreeUnavailableError, match="has not been written"): + service.list_parameter_children("us") + + +def test_an_unknown_parent_in_a_written_tree_has_no_children( + catalog_session: Session, +) -> None: + model_version = _model_version(catalog_session, "policyengine-us") + _write_parameter_tree(catalog_session, model_version.id) + + result = _service(catalog_session).list_parameter_children( + "us", level=_level("gov.nowhere") + ) + + assert result.items == [] + assert result.has_more is False + + def test_resource_filters_and_details_remain_inside_selected_catalog( catalog_session: Session, ) -> None: @@ -1122,12 +1352,15 @@ def test_value_and_node_label_reads_return_typed_rows( def test_parameter_tree_leaves_carry_the_same_summary_as_parameter_pages( catalog_session: Session, ) -> None: + _write_parameter_tree( + catalog_session, _model_version(catalog_session, "policyengine-us").id + ) service = _service(catalog_session) leaf = next( child for child in service.list_parameter_children( - "us", parent_path="gov.example" + "us", level=_level("gov.example") ).items if child.type == "parameter" ) From 472a6286a8e1b693c4b30f41942c3d9be82e1983 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:00:06 +0400 Subject: [PATCH 05/23] Return parameter tree labels in the form v3 shows them v3's tree shows every label with its underscores as spaces and its first letter upper-cased, but stored labels were returned unchanged while generated labels took that form, so one page could mix both and differ from v3 (for example "assumed uncapped single bus fare"). Stored labels now take the same form in both views, and the reform view sorts by it. Unlabelled schedules get a label from their name, such as "Uk" and "Al". Co-Authored-By: Claude Opus 5.5 --- .../v2plan-1f-117-parameter-children.added.md | 2 +- ...2plan-1f-117-parameter-children.changed.md | 2 +- docs/migration/stage-9-v2-metadata.md | 6 ++- .../data/v2/catalog/parameter_tree.py | 29 ++++++++------- tests/unit/v2/test_parameter_tree.py | 37 ++++++++++++++++++- 5 files changed, 56 insertions(+), 20 deletions(-) diff --git a/changelog.d/v2plan-1f-117-parameter-children.added.md b/changelog.d/v2plan-1f-117-parameter-children.added.md index ee9da1020..0fb55b528 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.added.md +++ b/changelog.d/v2plan-1f-117-parameter-children.added.md @@ -1 +1 @@ -Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and a path without a stored label gets one made from the last part of its name. On every run, publication also writes the tree of each stored version that has none, so versions published earlier get it from the next deploy; the evidence lists each version written. +Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and labels take the form the app's v3 workspace shows, with a path without a stored label getting one made from the last part of its name. On every run, publication also writes the tree of each stored version that has none, so versions published earlier get it from the next deploy; the evidence lists each version written. diff --git a/changelog.d/v2plan-1f-117-parameter-children.changed.md b/changelog.d/v2plan-1f-117-parameter-children.changed.md index 6b1782870..325b52553 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.changed.md +++ b/changelog.d/v2plan-1f-117-parameter-children.changed.md @@ -1 +1 @@ -/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its stored or generated label. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters and an unsupported country return 422, and a catalog version whose tree has not been written returns 503. +/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its label in the form the app's v3 workspace shows: the stored label with underscores as spaces and its first letter upper-cased, or one made from the last part of its name. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters and an unsupported country return 422, and a catalog version whose tree has not been written returns 503. diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index ee0aae891..48eef5523 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -313,8 +313,10 @@ a time. The rows are derived from the version's stored parameters and parameter nodes alone: every node and parameter is one row under its parent's path, a `[` starts a child as a `.` does (so a bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on), -and a path without a stored label gets one made from the last part of its -name. Each row also records its position among all its siblings and, if a +and labels take the form the app's newer interface shows in every view: the +stored label with underscores as spaces and its first letter upper-cased, or, +without one, the last part of the name in that form (so an unlabelled schedule +`...rates.uk` is "Uk"). Each row also records its position among all its siblings and, if a reform can change it, among the siblings a reform can change: a parameter under `gov` whose economy or household flag is set and whose path names neither `taxsim`, `gov.abolitions` nor `pycache`, and every node above such a diff --git a/policyengine_api/data/v2/catalog/parameter_tree.py b/policyengine_api/data/v2/catalog/parameter_tree.py index 66e76e947..81a611f22 100644 --- a/policyengine_api/data/v2/catalog/parameter_tree.py +++ b/policyengine_api/data/v2/catalog/parameter_tree.py @@ -1,15 +1,17 @@ """Each catalog version's parameter tree, as rows of direct children. The tree is derived from a version's stored parameters and parameter nodes -alone, so it can be written when a version is published and for any version -stored before the tree existed. Every node and parameter of the version is one -row, under the path of its parent (the empty string for top-level paths). +alone, and is written when a version is published. Every node and parameter +of the version is one row, under the path of its parent (the empty string for +top-level paths). A ``[`` starts a child as a ``.`` does: a bracket schedule such as ``gov.hmrc.income_tax.rates.uk`` is one node whose children are its brackets ``uk[0]``, ``uk[1]`` and so on, labelled "Bracket 1", "Bracket 2"; a bracket's -own children (``rate``, ``threshold``) are unchanged. A path with no stored -label gets one generated from the last part of its name. +own children (``rate``, ``threshold``) are unchanged. Labels take the form +the app's newer interface (v3) shows, in every view: a stored label with its +underscores as spaces and its first letter upper-cased, or, for a path with no +stored label, the last part of its name in the same form. Each row also records whether a reform can change it, which mirrors the tree the app's newer interface builds from v1 metadata @@ -95,24 +97,23 @@ def bracket_index(path: str) -> int | None: return int(match.group(1)) if match else None -def generated_label(path: str) -> str: - """Return a label made from the last part of a name, as v3 makes one.""" - words = _NAME_PARTS.split(path)[-1].replace("_", " ") - return words[:1].upper() + words[1:] - - -def _display_label(label: str) -> str: +def display_label(label: str) -> str: """Return a label as v3 shows it: underscores as spaces, first letter upper.""" words = label.replace("_", " ") return words[:1].upper() + words[1:] +def generated_label(path: str) -> str: + """Return a label made from the last part of a name, as v3 makes one.""" + return display_label(_NAME_PARTS.split(path)[-1]) + + def _child_label(path: str, stored_label: str | None) -> str: index = bracket_index(path) if index is not None: return f"Bracket {index + 1}" if stored_label and stored_label.strip(): - return stored_label + return display_label(stored_label) return generated_label(path) @@ -147,7 +148,7 @@ def _reform_position_key(path: str, label: str) -> tuple[int, int, str, str]: index = bracket_index(path) if index is not None: return (0, index, "", "") - return (1, 0, _display_label(label).casefold(), path) + return (1, 0, label.casefold(), path) @dataclass(frozen=True, slots=True) diff --git a/tests/unit/v2/test_parameter_tree.py b/tests/unit/v2/test_parameter_tree.py index ef3234023..ecd1da4f5 100644 --- a/tests/unit/v2/test_parameter_tree.py +++ b/tests/unit/v2/test_parameter_tree.py @@ -270,9 +270,9 @@ def test_the_reform_view_is_sorted_by_label_as_v3_shows_it() -> None: ] assert [row.label for row in _reform_children(rows, "gov")] == [ "Apple orchard", - "banana_split", + "Banana split", "C third", - "zebra crossing", + "Zebra crossing", ] @@ -303,3 +303,36 @@ def test_every_row_has_its_parent_and_counts_match_the_rows() -> None: for parent in {row.parent_path for row in rows}: positions = [row.position for row in rows if row.parent_path == parent] assert sorted(positions) == list(range(len(positions))) + + +def test_stored_labels_take_the_form_v3_shows_in_both_views() -> None: + rows = build_parameter_tree( + nodes=[ + TreeNodeSource( + name="gov.transport", label="assumed uncapped single bus fare" + ) + ], + parameters=[ + _parameter("gov.transport.fare", label="ACA rating area NY (Child_only)"), + ], + ) + by_path = _rows_by_path(rows) + + assert by_path["gov.transport"].label == "Assumed uncapped single bus fare" + assert by_path["gov.transport.fare"].label == "ACA rating area NY (Child only)" + assert by_path["gov.transport"].reform_changeable is True + + +def test_schedules_without_a_stored_label_get_one_from_their_name() -> None: + rows = build_parameter_tree( + nodes=[], + parameters=[ + _parameter(f"{UK_RATES}.uk[0].rate"), + _parameter("gov.aca.age_curves.al[0].amount"), + ], + ) + by_path = _rows_by_path(rows) + + assert by_path[f"{UK_RATES}.uk"].label == "Uk" + assert by_path["gov.aca.age_curves.al"].label == "Al" + assert by_path[f"{UK_RATES}.uk[0]"].label == "Bracket 1" From a38f6c018e1b6e9d509a33653f8dc57f81cc115e Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:03:59 +0400 Subject: [PATCH 06/23] Write the parameter tree only for the catalog versions a publication publishes Writing trees for versions stored before the tree existed derives rows from what earlier releases stored, which the migration rules leave to a human migration decision, so publication now writes the tree only of the versions it publishes, and fails if one of those is still without a tree. A version without a tree returns 503 with a message that names it and says it was published before the tree existed. Co-Authored-By: Claude Opus 5.5 --- .../v2plan-1f-117-parameter-children.added.md | 2 +- ...2plan-1f-117-parameter-children.changed.md | 2 +- docs/migration/stage-9-v2-metadata.md | 30 ++++---- .../data/v2/catalog/publication.py | 18 +++-- .../v2/catalog/publication_parameter_tree.py | 72 ++++++++++--------- .../data/v2/catalog/publication_types.py | 4 +- policyengine_api/data/v2/models/metadata.py | 6 +- policyengine_api/data/v2/privileges.py | 2 +- .../fastapi_routes/v2/metadata/common.py | 11 ++- .../services/v2/metadata/services.py | 10 +-- .../services/v2/metadata/validators.py | 7 +- .../test_v2_catalog_publication.py | 49 ++++++------- tests/unit/v2/test_metadata_routes.py | 9 +-- tests/unit/v2/test_metadata_service.py | 8 ++- 14 files changed, 128 insertions(+), 102 deletions(-) diff --git a/changelog.d/v2plan-1f-117-parameter-children.added.md b/changelog.d/v2plan-1f-117-parameter-children.added.md index 0fb55b528..0bbb886db 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.added.md +++ b/changelog.d/v2plan-1f-117-parameter-children.added.md @@ -1 +1 @@ -Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and labels take the form the app's v3 workspace shows, with a path without a stored label getting one made from the last part of its name. On every run, publication also writes the tree of each stored version that has none, so versions published earlier get it from the next deploy; the evidence lists each version written. +Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and labels take the form the app's v3 workspace shows, with a path without a stored label getting one made from the last part of its name. Publication writes the tree of each version it publishes, and its evidence lists them; versions stored before this change keep having no tree. diff --git a/changelog.d/v2plan-1f-117-parameter-children.changed.md b/changelog.d/v2plan-1f-117-parameter-children.changed.md index 325b52553..c1e7778e9 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.changed.md +++ b/changelog.d/v2plan-1f-117-parameter-children.changed.md @@ -1 +1 @@ -/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its label in the form the app's v3 workspace shows: the stored label with underscores as spaces and its first letter upper-cased, or one made from the last part of its name. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters and an unsupported country return 422, and a catalog version whose tree has not been written returns 503. +/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its label in the form the app's v3 workspace shows: the stored label with underscores as spaces and its first letter upper-cased, or one made from the last part of its name. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters and an unsupported country return 422, and a catalog version published before the tree existed returns 503 with a message naming the version. diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index 48eef5523..0f5796f87 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -316,23 +316,27 @@ whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on), and labels take the form the app's newer interface shows in every view: the stored label with underscores as spaces and its first letter upper-cased, or, without one, the last part of the name in that form (so an unlabelled schedule -`...rates.uk` is "Uk"). Each row also records its position among all its siblings and, if a -reform can change it, among the siblings a reform can change: a parameter +`...rates.uk` is "Uk"). Each row also records its position among all its +siblings and, if a reform can change it, among the siblings a reform can +change: a parameter under `gov` whose economy or household flag is set and whose path names neither `taxsim`, `gov.abolitions` nor `pycache`, and every node above such a parameter. These are the rules of the tree the app's newer interface builds from v1 metadata. A version stored before catalog format 2 has no flags, so only the path rules apply to it. -In the same transaction, and on every run, the command writes the tree of -every stored catalog version that has parameters but no tree: the versions it -publishes and versions stored before the tree existed. This is a row-only, -idempotent insert of rows derived from catalog rows already stored; it changes -no other table, and a version that has its tree keeps it. The evidence's -`parameter_trees_written` lists each version written, with its model, version -and row count, and is empty once every stored version has its tree. -Publication fails if a stored version with parameters is still without a tree -when it finishes. +In the same transaction, the command writes the tree of each catalog version +it publishes, from the parameters and parameter nodes it has just stored. The +evidence's `parameter_trees_written` lists each, with its model, version and +row count, and publication fails if one is still without a tree when it +finishes. A version that is already stored is left as it is. + +Versions stored before the tree existed therefore have no tree. Writing one +for them would derive rows from what earlier releases stored, a data +migration that the Alembic skill's rules leave to a human migration decision, +so it is not done. Such a version gets a tree only if a person decides on +that migration; a new PolicyEngine.py version published after this change +gets its tree when it is published. `/v2/parameters/children` lists one level with one indexed read: `parent_path` names the node ("" for the top level), and @@ -340,8 +344,8 @@ when it finishes. by label, with child counts that count only those. Without it, children are in path order, with a schedule's brackets in bracket order. A node carries its child count, a parameter its summary. An unknown parent path has no -children. A catalog version whose tree has not been written returns 503, -which means the publication step has not run since the version was stored. +children. A catalog version without a tree returns 503 with a message that +names the version and says it was published before the tree existed. ## Validation evidence and retry diff --git a/policyengine_api/data/v2/catalog/publication.py b/policyengine_api/data/v2/catalog/publication.py index 47b287cd5..4b39dbf77 100644 --- a/policyengine_api/data/v2/catalog/publication.py +++ b/policyengine_api/data/v2/catalog/publication.py @@ -11,8 +11,8 @@ from sqlalchemy import Connection, Engine from policyengine_api.data.v2.catalog.publication_parameter_tree import ( - assert_every_parameter_tree_written, - write_missing_parameter_trees, + assert_parameter_trees_written, + write_parameter_trees, ) from policyengine_api.data.v2.catalog.publication_reconciliation import ( assert_canonical_value_uniqueness, @@ -125,9 +125,9 @@ def publish_catalog( ) -> PublicationEvidence: """Publish one complete catalog atomically and return non-secret evidence. - In the same transaction it writes the parameter tree of every stored - catalog version that has none: the versions published now, and versions - stored before the tree existed. + In the same transaction it writes the parameter tree of each catalog + version it publishes. A version stored before the tree existed keeps + having none. """ started = time.monotonic() with engine.begin() as connection: @@ -141,10 +141,14 @@ def publish_catalog( existing, kept_region_summaries = _check_stored_versions(connection, catalog) + published_version_ids = [] for country in catalog.countries: if country.country_id not in existing: publish_new_country(connection, country) - parameter_trees_written = write_missing_parameter_trees(connection) + published_version_ids.append(country.model_version.id) + parameter_trees_written = write_parameter_trees( + connection, published_version_ids + ) if checkpoint is not None: checkpoint("after_reconciliation", connection) @@ -155,7 +159,7 @@ def publish_catalog( include_regions=country.country_id not in existing, ) assert_canonical_value_uniqueness(connection) - assert_every_parameter_tree_written(connection) + assert_parameter_trees_written(connection, published_version_ids) if _protected_row_counts(connection) != before: msg = "publication changed simulation, report, or dataset-version rows" raise CatalogPublicationError(msg) diff --git a/policyengine_api/data/v2/catalog/publication_parameter_tree.py b/policyengine_api/data/v2/catalog/publication_parameter_tree.py index 4c59d0096..88179a288 100644 --- a/policyengine_api/data/v2/catalog/publication_parameter_tree.py +++ b/policyengine_api/data/v2/catalog/publication_parameter_tree.py @@ -1,10 +1,13 @@ -"""Write the parameter tree of each stored catalog version that has none. +"""Write the parameter tree of each catalog version a publication publishes. -A version's tree rows are derived from its stored parameters and parameter -nodes alone (``parameter_tree``), so a version published in the current -transaction and a version stored before the tree existed are written the same -way. Writing only inserts: a version that already has rows keeps them, and no -other table changes. +A version's tree rows are derived from the parameters and parameter nodes the +publication has just stored for it (``parameter_tree``), in the same +transaction. Writing only inserts rows for those versions; no other table +changes. + +Versions stored before the tree existed get none here: deriving their rows +from what earlier releases stored would be a data migration, which the +migration rules leave to a human decision. """ from __future__ import annotations @@ -34,7 +37,7 @@ ) if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Iterator, Sequence from uuid import UUID from sqlalchemy import Connection @@ -46,24 +49,13 @@ PARAMETER_TREE_CHILDREN = ParameterTreeChild.__table__ -def _versions_without_tree() -> sa.Select: - has_tree = sa.exists().where( - PARAMETER_TREE_CHILDREN.c.tax_benefit_model_version_id == MODEL_VERSIONS.c.id - ) - has_parameters_or_nodes = sa.or_( - sa.exists().where( - PARAMETERS.c.tax_benefit_model_version_id == MODEL_VERSIONS.c.id - ), - sa.exists().where( - PARAMETER_NODES.c.tax_benefit_model_version_id == MODEL_VERSIONS.c.id - ), - ) +def _versions(version_ids: Sequence[UUID]) -> sa.Select: return ( sa.select(MODEL_VERSIONS.c.id, MODELS.c.name, MODEL_VERSIONS.c.version) .select_from( MODEL_VERSIONS.join(MODELS, MODELS.c.id == MODEL_VERSIONS.c.model_id) ) - .where(~has_tree, has_parameters_or_nodes) + .where(MODEL_VERSIONS.c.id.in_(version_ids)) .order_by(MODELS.c.name, MODEL_VERSIONS.c.version) ) @@ -128,18 +120,20 @@ def _table_rows( yield tuple(values[name] for name in column_names) -def write_missing_parameter_trees( +def write_parameter_trees( connection: Connection, + version_ids: Sequence[UUID], ) -> tuple[tuple[str, str, int], ...]: - """Write the tree of every stored version that has parameters but no tree. + """Write the tree of each catalog version just published. Returns the model name, version and number of rows written for each version, in model and version order. Runs inside publication's - transaction and advisory lock. + transaction and advisory lock, after the versions' parameters and nodes + are stored. """ written = [] for version_id, model_name, version in connection.execute( - _versions_without_tree() + _versions(version_ids) ).all(): rows = build_parameter_tree( nodes=_stored_nodes(connection, version_id), @@ -154,12 +148,24 @@ def write_missing_parameter_trees( return tuple(written) -def assert_every_parameter_tree_written(connection: Connection) -> None: - """Raise CatalogPublicationError if a version with parameters has no tree.""" - missing = connection.execute(_versions_without_tree()).first() - if missing is not None: - msg = ( - f"the stored catalog version {missing.version} of {missing.name} " - "has no parameter tree" - ) - raise CatalogPublicationError(msg) +def assert_parameter_trees_written( + connection: Connection, + version_ids: Sequence[UUID], +) -> None: + """Raise CatalogPublicationError if a version just published has no tree.""" + for version_id, model_name, version in connection.execute( + _versions(version_ids) + ).all(): + has_tree = connection.execute( + sa.select( + sa.exists().where( + PARAMETER_TREE_CHILDREN.c.tax_benefit_model_version_id == version_id + ) + ) + ).scalar_one() + if not has_tree: + msg = ( + f"the published catalog version {version} of {model_name} " + "has no parameter tree" + ) + raise CatalogPublicationError(msg) diff --git a/policyengine_api/data/v2/catalog/publication_types.py b/policyengine_api/data/v2/catalog/publication_types.py index 203699d5a..a8133d840 100644 --- a/policyengine_api/data/v2/catalog/publication_types.py +++ b/policyengine_api/data/v2/catalog/publication_types.py @@ -22,8 +22,8 @@ class PublicationEvidence: # Stored catalog versions whose region rows were kept although the staged # regions differ: country, version, stored and staged region counts. kept_region_summaries: tuple[tuple[str, str, int, int], ...] = () - # Stored catalog versions whose parameter tree publication wrote: model, - # version and rows written. Empty once every stored version has its tree. + # Catalog versions published now, with their parameter tree: model, + # version and tree rows written. Empty when no version is published. parameter_trees_written: tuple[tuple[str, str, int], ...] = () def as_dict(self) -> dict[str, object]: diff --git a/policyengine_api/data/v2/models/metadata.py b/policyengine_api/data/v2/models/metadata.py index a0fda410e..feb22d42e 100644 --- a/policyengine_api/data/v2/models/metadata.py +++ b/policyengine_api/data/v2/models/metadata.py @@ -470,9 +470,9 @@ class ParameterTreeChild(IdentifiedModel, table=True): """One direct child in a catalog version's parameter tree. Derived from the version's parameters and parameter nodes when the version - is published, or by the next publication for a version stored before the - tree existed. Every node and parameter of the version is one row under its - parent's path ("" for top-level paths), so listing one level reads one + is published; versions stored before the tree existed have no rows. Every + node and parameter of the version is one row under its parent's path ("" + for top-level paths), so listing one level reads one index. ``position`` orders all children of a parent; ``reform_position`` orders only those a reform can change and is null for the others. Node rows count their direct children; parameter rows reference their parameter and diff --git a/policyengine_api/data/v2/privileges.py b/policyengine_api/data/v2/privileges.py index 49a35c070..02671e912 100644 --- a/policyengine_api/data/v2/privileges.py +++ b/policyengine_api/data/v2/privileges.py @@ -116,7 +116,7 @@ def _access( "dataset_versions": _access(catalog_writer=READ), "datasets": _access(runtime=READ, catalog_writer=READ_INSERT_UPDATE), "parameter_nodes": _access(runtime=READ, catalog_writer=READ_INSERT_UPDATE), - # Written with each catalog version, and for versions stored before it. + # Written with each catalog version that publication publishes. "parameter_tree_children": _access(runtime=READ, catalog_writer=READ_INSERT), # The Public API also inserts a v2 policy's own parameter values. "parameter_values": _access(runtime=READ_INSERT, catalog_writer=READ_INSERT_UPDATE), diff --git a/policyengine_api/fastapi_routes/v2/metadata/common.py b/policyengine_api/fastapi_routes/v2/metadata/common.py index a2dab29ee..0f5593d34 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/common.py +++ b/policyengine_api/fastapi_routes/v2/metadata/common.py @@ -33,6 +33,7 @@ from policyengine_api.services.v2.metadata.validators import ( InvalidMetadataPageError, MetadataResourceNotFoundError, + ParameterTreeUnavailableError, ) from policyengine_api.vary_headers import unique_vary_values @@ -348,8 +349,14 @@ def run_metadata_read( MetadataResourceNotFoundError, ) as error: return v2_error_response(404, str(error)) - except (V2ConfigurationError, MetadataCatalogUnavailableError): - return v2_error_response(503, "V2 metadata catalog is unavailable") + except (V2ConfigurationError, MetadataCatalogUnavailableError) as error: + # Only a missing parameter tree has a message written for callers. + message = ( + str(error) + if isinstance(error, ParameterTreeUnavailableError) + else "V2 metadata catalog is unavailable" + ) + return v2_error_response(503, message) except Exception: # noqa: BLE001 # preview must return typed errors return v2_error_response(500, "V2 metadata query failed") finally: diff --git a/policyengine_api/services/v2/metadata/services.py b/policyengine_api/services/v2/metadata/services.py index 4153ba0fb..96cc8a507 100644 --- a/policyengine_api/services/v2/metadata/services.py +++ b/policyengine_api/services/v2/metadata/services.py @@ -357,8 +357,9 @@ def list_parameter_children( The level's parent path "" lists the top-level children. A bracket schedule is one node whose children are its brackets. With the level's ``reform_changeable``, only the children a reform can change are listed, - sorted by label. Raise ParameterTreeUnavailableError when the version's - tree has not been written. + sorted by label. Raise ParameterTreeUnavailableError when the version has + no tree, which is the case for versions published before the tree + existed. """ selected_level = level if level is not None else ParameterTreeLevel() selected = self._select_paginated_catalog( @@ -376,8 +377,9 @@ def list_parameter_children( session, model_version_id=selected.model_version.id ): msg = ( - f"the parameter tree of catalog version " - f"{selected.policyengine_version} has not been written" + f"catalog version {selected.policyengine_version} has no parameter " + "tree: a tree is written when a catalog version is published, and " + "this version was published before" ) raise ParameterTreeUnavailableError(msg) return page_result( diff --git a/policyengine_api/services/v2/metadata/validators.py b/policyengine_api/services/v2/metadata/validators.py index 92ad20402..92c8774a4 100644 --- a/policyengine_api/services/v2/metadata/validators.py +++ b/policyengine_api/services/v2/metadata/validators.py @@ -25,10 +25,11 @@ class InvalidMetadataPageError(ValueError): class ParameterTreeUnavailableError(MetadataCatalogUnavailableError): - """Raised when a selected catalog version's parameter tree is not written. + """Raised when a selected catalog version has no parameter tree. - Catalog publication writes every stored version's tree, so this means the - publication step has not run since the version was stored. + Catalog publication writes a version's tree when it publishes the version, + so versions published before the tree existed have none. The message names + the version and is safe to return to the caller. """ diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index 586a6059f..cb54f3b1f 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -879,51 +879,46 @@ def test_publication_writes_each_new_version_parameter_tree( ) -def test_publication_writes_the_tree_of_stored_versions_that_have_none( +def test_publication_writes_no_tree_for_a_version_stored_without_one( publication_engine: Engine, ) -> None: - older_version = "1.0.0" - older = normalized_catalog(policyengine_version=older_version) + """A version stored before the tree existed keeps having none. + + Writing it would derive rows from what earlier releases stored, which the + migration rules leave to a human migration decision; only the versions a + publication publishes get a tree. + """ + older = normalized_catalog(policyengine_version="1.0.0") catalog = normalized_catalog() - older_country_version = older.country("us").model_version.version + older_version = older.country("us").model_version.version + current_version = catalog.country("us").model_version.version publish_catalog(publication_engine, older) - publish_catalog(publication_engine, catalog) - current_country_version = catalog.country("us").model_version.version - _delete_tree(publication_engine, older_country_version) - _delete_tree(publication_engine, current_country_version) - before_ids = _identifiers(publication_engine) - before_counts = _counts(publication_engine) + _delete_tree(publication_engine, older_version) evidence = publish_catalog(publication_engine, catalog) + _delete_tree(publication_engine, current_version) + before_ids = _identifiers(publication_engine) again = publish_catalog(publication_engine, catalog) - assert { - (model_name, version) - for model_name, version, _rows in evidence.parameter_trees_written - } == { - (country.model.name, version) - for country in catalog.countries - for version in {older_country_version, current_country_version} - } + assert evidence.parameter_trees_written == tuple( + (country.model.name, current_version, len(FIXTURE_TREE)) + for country in sorted(catalog.countries, key=lambda item: item.model.name) + ) assert again.parameter_trees_written == () for country in catalog.countries: - for version in (older_country_version, current_country_version): - assert ( - _stored_tree(publication_engine, country.model.name, version) - == FIXTURE_TREE - ) + for version in (older_version, current_version): + assert _stored_tree(publication_engine, country.model.name, version) == [] assert _identifiers(publication_engine) == before_ids - assert _counts(publication_engine) == before_counts -def test_publication_fails_when_a_stored_version_is_left_without_a_tree( +def test_publication_fails_when_a_published_version_is_left_without_a_tree( publication_engine: Engine, monkeypatch: pytest.MonkeyPatch, ) -> None: monkeypatch.setattr( publication_module, - "write_missing_parameter_trees", - lambda _connection: (), + "write_parameter_trees", + lambda _connection, _version_ids: (), ) with pytest.raises(CatalogPublicationError, match="has no parameter tree"): diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 480156625..b3d38d1cb 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -1560,17 +1560,18 @@ def factory(): def test_parameter_children_route_reports_an_unwritten_tree_as_unavailable() -> None: results, _ids = _resource_results() - reader = ResourceReader( - results, - error=ParameterTreeUnavailableError("tree not written"), + message = ( + "catalog version 6.7.5 has no parameter tree: a tree is written when a " + "catalog version is published, and this version was published before" ) + reader = ResourceReader(results, error=ParameterTreeUnavailableError(message)) response = _client(lambda: reader).get( "/v2/parameters/children", params={"country_id": "us"} ) assert response.status_code == 503 - assert response.json()["status"] == "error" + assert response.json() == {"status": "error", "message": message} def test_parameter_children_openapi_declares_the_query_contract() -> None: diff --git a/tests/unit/v2/test_metadata_service.py b/tests/unit/v2/test_metadata_service.py index fd6dac31c..9a8206221 100644 --- a/tests/unit/v2/test_metadata_service.py +++ b/tests/unit/v2/test_metadata_service.py @@ -672,7 +672,13 @@ def test_an_unwritten_parameter_tree_is_reported_as_unavailable( ) -> None: service = _service(catalog_session) - with pytest.raises(ParameterTreeUnavailableError, match="has not been written"): + with pytest.raises( + ParameterTreeUnavailableError, + match=( + f"catalog version {POLICYENGINE_VERSION} has no parameter tree: " + "a tree is written when a catalog version is published" + ), + ): service.list_parameter_children("us") From c5bb6301dc24d3dc1e1e382fc09f0789a1630351 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:04:31 +0400 Subject: [PATCH 07/23] Publish the parameter tree as the catalog writer role in a test The catalog writer's INSERT grant on the tree table was never used as that role. The new test publishes the fixture catalog through a connection that acts as the catalog writer with only its declared privileges, and checks each published version's tree. Co-Authored-By: Claude Opus 5.5 --- rules/registry.toml | 2 +- .../test_v2_catalog_publication.py | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/rules/registry.toml b/rules/registry.toml index 2613a20a9..571f9aeb0 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -671,7 +671,7 @@ id = "api.v2.runtime-privileges-tested-as-runtime-role" category = "migration" statement = "The cross-database integration tests run the v2 user services as policyengine_v2_runtime and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI." rationale = "docs/engineering/skills/alembic-migrations.md#table-privileges" -scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_saved_policy_view.py", "tests/integration/test_v2_stage12_comparison_persistence.py", "tests/integration/test_v2_metadata_routes.py"] +scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_saved_policy_view.py", "tests/integration/test_v2_stage12_comparison_persistence.py", "tests/integration/test_v2_metadata_routes.py", "tests/integration/test_v2_catalog_publication.py"] tier = "mechanical" status = "enforced" autofix = false diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index cb54f3b1f..c62931dc3 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -33,6 +33,7 @@ TaxBenefitModel, TaxBenefitModelVersion, ) +from policyengine_api.data.v2.privileges import CATALOG_WRITER_ROLE from policyengine_api.data.v2.settings import V2_MIGRATION_DATABASE_URL from tests.fixtures.v2_catalog import ( POLICYENGINE_VERSION, @@ -925,3 +926,35 @@ def test_publication_fails_when_a_published_version_is_left_without_a_tree( publish_catalog(publication_engine, normalized_catalog()) assert _counts(publication_engine)["tax_benefit_model_versions"] == 0 + + +def test_the_catalog_writer_role_publishes_the_parameter_tree( + publication_engine: Engine, + managed_v2_roles, + v2_engine_as_role, +) -> None: + """Publication as the catalog writer, with only its declared privileges. + + It writes each published version's tree, which needs the role's INSERT + grant on the tree table. + """ + writer_engine = v2_engine_as_role(CATALOG_WRITER_ROLE, poolclass=NullPool) + catalog = normalized_catalog() + try: + evidence = publish_catalog(writer_engine, catalog) + finally: + writer_engine.dispose() + + assert evidence.parameter_trees_written == tuple( + (country.model.name, country.model_version.version, len(FIXTURE_TREE)) + for country in sorted(catalog.countries, key=lambda item: item.model.name) + ) + for country in catalog.countries: + assert ( + _stored_tree( + publication_engine, + country.model.name, + country.model_version.version, + ) + == FIXTURE_TREE + ) From 20d9a419de0ec7db4bdde6d688f5889df7b8cfc8 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:05:35 +0400 Subject: [PATCH 08/23] Document the children route's statuses and page the reform view in a test The route's OpenAPI description, its changelog fragment and the Stage 9 document now say that invalid query parameters, including a malformed policyengine_version and an unsupported country, return 422 (both returned 400 before), and that a version published before the tree existed returns 503. A service test pages through the reform view in label order. Co-Authored-By: Claude Opus 5.5 --- ...2plan-1f-117-parameter-children.changed.md | 2 +- docs/migration/stage-9-v2-metadata.md | 7 +++-- .../v2/metadata/parameter_routes.py | 9 ++++++ tests/unit/v2/test_metadata_routes.py | 4 +++ tests/unit/v2/test_metadata_service.py | 28 +++++++++++++++++++ 5 files changed, 47 insertions(+), 3 deletions(-) diff --git a/changelog.d/v2plan-1f-117-parameter-children.changed.md b/changelog.d/v2plan-1f-117-parameter-children.changed.md index c1e7778e9..fc7472871 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.changed.md +++ b/changelog.d/v2plan-1f-117-parameter-children.changed.md @@ -1 +1 @@ -/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its label in the form the app's v3 workspace shows: the stored label with underscores as spaces and its first letter upper-cased, or one made from the last part of its name. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters and an unsupported country return 422, and a catalog version published before the tree existed returns 503 with a message naming the version. +/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its label in the form the app's v3 workspace shows: the stored label with underscores as spaces and its first letter upper-cased, or one made from the last part of its name. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters, an unsupported country and a malformed policyengine_version return 422 (an unsupported country and a malformed version returned 400 before), and a catalog version published before the tree existed returns 503 with a message naming the version. diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index 0f5796f87..b0ad5a5dd 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -344,8 +344,11 @@ gets its tree when it is published. by label, with child counts that count only those. Without it, children are in path order, with a schedule's brackets in bracket order. A node carries its child count, a parameter its summary. An unknown parent path has no -children. A catalog version without a tree returns 503 with a message that -names the version and says it was published before the tree existed. +children. The route reads its query through the shared strict query models, +so unknown or repeated parameters, an unsupported country and a malformed +`policyengine_version` return 422 (the last two returned 400 before). A +catalog version without a tree returns 503 with a message that names the +version and says it was published before the tree existed. ## Validation evidence and retry diff --git a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py index b66f2087f..4d1e8d4d5 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py @@ -78,6 +78,15 @@ def list_parameters( "/parameters/children", response_model=MetadataParameterChildPageResponse, summary="List one level of a catalog version's parameter tree", + description=( + "Lists the direct children of `parent_path` (the top level when " + "empty). With `reform_changeable=true`, only the children a reform " + "can change, sorted by label, with child counts that count only " + "those. Invalid query parameters return 422, including a malformed " + "policyengine_version, an unsupported country, and an unknown or " + "repeated parameter. A catalog version published before the tree " + "existed has no tree and returns 503 with a message naming it." + ), ) def list_parameter_children( query: ParameterChildrenQueryDependency, diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index b3d38d1cb..2b7a270a0 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -1597,3 +1597,7 @@ def test_parameter_children_openapi_declares_the_query_contract() -> None: assert parameters["reform_changeable"]["schema"]["type"] == "boolean" assert parameters["reform_changeable"]["schema"]["default"] is False assert parameters["limit"]["schema"]["maximum"] == 500 + description = schema["paths"]["/v2/parameters/children"]["get"]["description"] + assert "422" in description + assert "malformed policyengine_version" in description + assert "503" in description diff --git a/tests/unit/v2/test_metadata_service.py b/tests/unit/v2/test_metadata_service.py index 9a8206221..ca5b81a05 100644 --- a/tests/unit/v2/test_metadata_service.py +++ b/tests/unit/v2/test_metadata_service.py @@ -667,6 +667,34 @@ def test_parameter_tree_levels_are_paged(catalog_session: Session) -> None: assert last.has_more is False +def test_the_reform_changeable_level_is_paged_in_label_order( + catalog_session: Session, +) -> None: + model_version = _model_version(catalog_session, "policyengine-us") + _add_parameters( + catalog_session, + model_version, + [ + ("gov.paged.c_item", "Cherry", True, False), + ("gov.paged.a_item", "Banana", False, True), + ("gov.paged.b_item", "Apple", True, True), + ("gov.paged.d_item", "Date", False, False), + ], + ) + catalog_session.commit() + _write_parameter_tree(catalog_session, model_version.id) + service = _service(catalog_session) + level = _level("gov.paged", reform_changeable=True) + + first = service.list_parameter_children("us", level=level, offset=0, limit=2) + last = service.list_parameter_children("us", level=level, offset=2, limit=2) + + assert [item.label for item in first.items] == ["Apple", "Banana"] + assert first.has_more is True + assert [item.label for item in last.items] == ["Cherry"] + assert last.has_more is False + + def test_an_unwritten_parameter_tree_is_reported_as_unavailable( catalog_session: Session, ) -> None: From cc44898630bf32dc193f29aad6251c5eecb87ca0 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:37:57 +0400 Subject: [PATCH 09/23] Test the parameter tree with records shaped like PolicyEngine.py's output The new unit and Postgres route tests build the tree from bracket parameters labelled the way PolicyEngine.py generates them ("... (tier 1 amount)"), a breakdown parameter labelled from its parent ("Standard deduction (Single)"), a schedule without a node row, as the locked PolicyEngine.py publishes, and one with a labelled node row, as the release that publishes schedules will. Co-Authored-By: Claude Opus 5.5 --- tests/integration/test_v2_metadata_routes.py | 125 +++++++++++++++++++ tests/unit/v2/test_parameter_tree.py | 64 ++++++++++ 2 files changed, 189 insertions(+) diff --git a/tests/integration/test_v2_metadata_routes.py b/tests/integration/test_v2_metadata_routes.py index e0119b71d..1e78a6508 100644 --- a/tests/integration/test_v2_metadata_routes.py +++ b/tests/integration/test_v2_metadata_routes.py @@ -878,3 +878,128 @@ def test_postgres_parameter_tree_reads_as_the_runtime_role( assert [(item.path, item.child_count) for item in result.items] == [ ("gov.example", 1) ] + + +def _catalog_shaped_like_policyengine_py_output(): + """Return the fixture catalog with US records shaped like PolicyEngine.py's. + + Bracket parameters carry the labels PolicyEngine.py generates, and a + breakdown parameter its parent's label with the key. One schedule has no + node row, as the locked PolicyEngine.py publishes; the other has one with + its label, as the release that publishes schedules will. + """ + models = source_models() + us = models["us"] + nodes = [ + SimpleNamespace( + name="gov.irs.deductions.standard.amount", + label="Standard deduction", + description=None, + ), + SimpleNamespace( + name="gov.aca.age_curves.ak", + label="ACA premium age curve factors - AK", + description=None, + ), + ] + labelled_parameters = [ + (f"gov.aca.age_curves.{state}[{index}].{field}", f"{label} ({tier} {field})") + for state, label in ( + ("al", "ACA premium age curve factors - AL"), + ("ak", "ACA premium age curve factors - AK"), + ) + for index, tier in ((0, "tier 1"), (1, "tier 2")) + for field in ("amount", "threshold") + ] + [ + ("gov.irs.deductions.standard.amount.SINGLE", "Standard deduction (Single)"), + ("gov.irs.deductions.standard.amount.JOINT", "Standard deduction (Joint)"), + ] + parameters = [ + SimpleNamespace( + name=name, + label=label, + description=None, + data_type=float, + unit="currency-USD", + economy=True, + household=True, + parameter_values=[ + SimpleNamespace( + value=1.0, + start_date=datetime(2025, 1, 1, tzinfo=UTC), + end_date=None, + ) + ], + ) + for name, label in labelled_parameters + ] + us.parameter_nodes.extend(nodes) + us.parameters.extend(parameters) + us.parameter_nodes_by_name.update({node.name: node for node in nodes}) + us.parameters_by_name.update( + {parameter.name: parameter for parameter in parameters} + ) + return extract_catalog( + bundle=bundle(), + policyengine_version=POLICYENGINE_VERSION, + models=models, + installed_version=installed_version, + region_references=region_references(), + ) + + +def test_postgres_parameter_tree_from_records_shaped_like_policyengine_py( + published_engine: Engine, +) -> None: + with published_engine.begin() as connection: + connection.execute(text("TRUNCATE tax_benefit_models CASCADE")) + publish_catalog(published_engine, _catalog_shaped_like_policyengine_py_output()) + client = _client(published_engine) + + def level(parent_path: str) -> list[tuple]: + return _items( + client.get( + "/v2/parameters/children", + params={"country_id": "us", "parent_path": parent_path}, + ) + ) + + assert level("gov.aca.age_curves") == [ + ("gov.aca.age_curves.ak", "ACA premium age curve factors - AK", "node", 2), + ("gov.aca.age_curves.al", "Al", "node", 2), + ] + assert level("gov.aca.age_curves.al") == [ + ("gov.aca.age_curves.al[0]", "Bracket 1", "node", 2), + ("gov.aca.age_curves.al[1]", "Bracket 2", "node", 2), + ] + assert level("gov.aca.age_curves.al[1]") == [ + ( + "gov.aca.age_curves.al[1].amount", + "ACA premium age curve factors - AL (tier 2 amount)", + "parameter", + None, + ), + ( + "gov.aca.age_curves.al[1].threshold", + "ACA premium age curve factors - AL (tier 2 threshold)", + "parameter", + None, + ), + ] + assert level("gov.irs.deductions.standard") == [ + ("gov.irs.deductions.standard.amount", "Standard deduction", "node", 2), + ] + assert level("gov.irs.deductions.standard.amount") == [ + ( + "gov.irs.deductions.standard.amount.JOINT", + "Standard deduction (Joint)", + "parameter", + None, + ), + ( + "gov.irs.deductions.standard.amount.SINGLE", + "Standard deduction (Single)", + "parameter", + None, + ), + ] diff --git a/tests/unit/v2/test_parameter_tree.py b/tests/unit/v2/test_parameter_tree.py index ecd1da4f5..3cdcbe7b0 100644 --- a/tests/unit/v2/test_parameter_tree.py +++ b/tests/unit/v2/test_parameter_tree.py @@ -336,3 +336,67 @@ def test_schedules_without_a_stored_label_get_one_from_their_name() -> None: assert by_path[f"{UK_RATES}.uk"].label == "Uk" assert by_path["gov.aca.age_curves.al"].label == "Al" assert by_path[f"{UK_RATES}.uk[0]"].label == "Bracket 1" + + +AGE_CURVE = "gov.aca.age_curves.al" +STANDARD_DEDUCTION = "gov.irs.deductions.standard.amount" +PY_SHAPED_PARAMETERS = ( + # Bracket parameters labelled as PolicyEngine.py generates them. + (f"{AGE_CURVE}[0].amount", "ACA premium age curve factors - AL (tier 1 amount)"), + ( + f"{AGE_CURVE}[0].threshold", + "ACA premium age curve factors - AL (tier 1 threshold)", + ), + (f"{AGE_CURVE}[1].amount", "ACA premium age curve factors - AL (tier 2 amount)"), + # Breakdown parameters labelled from their parent node's label. + (f"{STANDARD_DEDUCTION}.SINGLE", "Standard deduction (Single)"), + (f"{STANDARD_DEDUCTION}.JOINT", "Standard deduction (Joint)"), +) + + +def _py_shaped_tree(*, schedule_has_node_row: bool): + nodes = [TreeNodeSource(name=STANDARD_DEDUCTION, label="Standard deduction")] + if schedule_has_node_row: + nodes.append( + TreeNodeSource(name=AGE_CURVE, label="ACA premium age curve factors - AL") + ) + return build_parameter_tree( + nodes=nodes, + parameters=[ + _parameter(name, label=label) for name, label in PY_SHAPED_PARAMETERS + ], + ) + + +def test_a_tree_from_the_locked_policyengine_py_labels_its_schedule_by_name() -> None: + """The locked PolicyEngine.py stores no node row for a bracket schedule.""" + rows = _py_shaped_tree(schedule_has_node_row=False) + by_path = _rows_by_path(rows) + + assert by_path[AGE_CURVE].label == "Al" + assert [(row.label, row.child_count) for row in _children(rows, AGE_CURVE)] == [ + ("Bracket 1", 2), + ("Bracket 2", 1), + ] + assert [row.label for row in _children(rows, f"{AGE_CURVE}[0]")] == [ + "ACA premium age curve factors - AL (tier 1 amount)", + "ACA premium age curve factors - AL (tier 1 threshold)", + ] + assert by_path[STANDARD_DEDUCTION].label == "Standard deduction" + assert [row.label for row in _children(rows, STANDARD_DEDUCTION)] == [ + "Standard deduction (Joint)", + "Standard deduction (Single)", + ] + + +def test_a_tree_from_a_release_with_schedule_nodes_uses_the_schedule_label() -> None: + """A PolicyEngine.py release that publishes schedules stores their label.""" + rows = _py_shaped_tree(schedule_has_node_row=True) + + assert _rows_by_path(rows)[AGE_CURVE].label == ( + "ACA premium age curve factors - AL" + ) + assert [row.label for row in _children(rows, AGE_CURVE)] == [ + "Bracket 1", + "Bracket 2", + ] From 97904be68b26568fe76d2e7d4a90800b7d9f6e21 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:38:32 +0400 Subject: [PATCH 10/23] Type a parameter tree row's child type with its two values ParameterTreeChild.child_type is now typed Literal["node", "parameter"], the two values its check constraint allows, so reads keep the type. The column is unchanged, and alembic check finds no new operations. Co-Authored-By: Claude Opus 5.5 --- policyengine_api/data/v2/models/metadata.py | 7 +++++-- tests/unit/v2/test_models.py | 13 +++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/policyengine_api/data/v2/models/metadata.py b/policyengine_api/data/v2/models/metadata.py index feb22d42e..099822a76 100644 --- a/policyengine_api/data/v2/models/metadata.py +++ b/policyengine_api/data/v2/models/metadata.py @@ -2,11 +2,12 @@ from datetime import datetime from enum import StrEnum -from typing import TYPE_CHECKING, Any, Optional +from typing import TYPE_CHECKING, Any, Literal, Optional from uuid import UUID import sqlalchemy as sa from sqlmodel import Field, Relationship +from sqlmodel.sql.sqltypes import AutoString from typing_extensions import TypedDict from policyengine_api.data.v2.models.base import ( @@ -518,7 +519,9 @@ class ParameterTreeChild(IdentifiedModel, table=True): path: str = Field(max_length=512) position: int label: str = Field(max_length=512) - child_type: str = Field(max_length=16) + # The column stays a plain string, kept to these two values by + # ck_parameter_tree_children_child_type. + child_type: Literal["node", "parameter"] = Field(sa_type=AutoString(length=16)) child_count: int | None = None reform_position: int | None = None reform_child_count: int | None = None diff --git a/tests/unit/v2/test_models.py b/tests/unit/v2/test_models.py index 551201478..464a76d28 100644 --- a/tests/unit/v2/test_models.py +++ b/tests/unit/v2/test_models.py @@ -1,5 +1,7 @@ """Structural contract tests for the complete reviewed v2 SQLModel schema.""" +from typing import Literal + import sqlalchemy as sa from sqlalchemy.dialects import postgresql from sqlalchemy.orm import configure_mappers @@ -17,6 +19,7 @@ LegacyPolicyMapping, LegacyUserMapping, LegacyUserPolicyMapping, + ParameterTreeChild, ParameterValue, Policy, Simulation, @@ -830,3 +833,13 @@ def test_parameter_tree_children_have_one_row_per_path_in_listing_order() -> Non "ck_parameter_tree_children_child_type", "ck_parameter_tree_children_parameter_reference", } + + +def test_parameter_tree_child_type_is_typed_with_its_two_values() -> None: + """Reads of a tree row keep the child type's two values as a type.""" + annotation = ParameterTreeChild.model_fields["child_type"].annotation + + assert annotation == Literal["node", "parameter"] + column = V2_METADATA.tables["parameter_tree_children"].c["child_type"] + assert isinstance(column.type, AutoString) + assert column.type.length == 16 From 9983a3aed0a2f57fe314ecb53a165668930f7a82 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:39:26 +0400 Subject: [PATCH 11/23] Drop the reform rule for parameters without flags Trees are written only for versions published with catalog format 2, whose parameters always have both flags, so the rule that treated a parameter with neither flag as changeable could not apply. It is removed with its test and the documentation about it, and the service tests now store their fixture parameters' flags as a format 2 publication does. The children transformation's docstring now describes the displayed label form. Co-Authored-By: Claude Opus 5.5 --- docs/migration/stage-9-v2-metadata.md | 4 ++-- policyengine_api/data/v2/catalog/parameter_tree.py | 6 ++---- policyengine_api/services/v2/metadata/transformations.py | 5 +++-- tests/unit/v2/test_parameter_tree.py | 9 --------- 4 files changed, 7 insertions(+), 17 deletions(-) diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index b0ad5a5dd..665990b21 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -322,8 +322,8 @@ change: a parameter under `gov` whose economy or household flag is set and whose path names neither `taxsim`, `gov.abolitions` nor `pycache`, and every node above such a parameter. These are the rules of the tree the app's newer interface builds -from v1 metadata. A version stored before catalog format 2 has no flags, so -only the path rules apply to it. +from v1 metadata. Trees are written only for versions published with catalog +format 2, so every parameter in a tree has both flags. In the same transaction, the command writes the tree of each catalog version it publishes, from the parameters and parameter nodes it has just stored. The diff --git a/policyengine_api/data/v2/catalog/parameter_tree.py b/policyengine_api/data/v2/catalog/parameter_tree.py index 81a611f22..e653dcdb5 100644 --- a/policyengine_api/data/v2/catalog/parameter_tree.py +++ b/policyengine_api/data/v2/catalog/parameter_tree.py @@ -18,8 +18,8 @@ (``app/src/libs/buildParameterTree.ts``): a parameter under ``gov`` whose economy or household flag is set and whose path names neither ``taxsim``, ``gov.abolitions`` nor ``pycache``, and every node with such a parameter -below it. A version published before catalog format 2 has no flags, so only -the path rules apply to it. +below it. Trees are written only for versions published with catalog format +2, so every parameter has both flags. """ from __future__ import annotations @@ -123,8 +123,6 @@ def is_reform_changeable_parameter(parameter: TreeParameterSource) -> bool: return False if any(part in parameter.name for part in REFORM_EXCLUDED_PATH_PARTS): return False - if parameter.economy is None and parameter.household is None: - return True return bool(parameter.economy) or bool(parameter.household) diff --git a/policyengine_api/services/v2/metadata/transformations.py b/policyengine_api/services/v2/metadata/transformations.py index 3b1ad02c3..83bf941ed 100644 --- a/policyengine_api/services/v2/metadata/transformations.py +++ b/policyengine_api/services/v2/metadata/transformations.py @@ -326,8 +326,9 @@ def metadata_parameter_children(rows: list[Any]) -> list[MetadataParameterChild] """Build parameter-tree children from read_parameter_children rows. Parameter leaves carry the same summary as the parameter routes. Every - stored child has a label: its stored label, "Bracket N" for a bracket, or - one generated from the last part of its name. + stored child has a label in the form v3 shows it: its stored label with + underscores as spaces and its first letter upper-cased, "Bracket N" for a + bracket, or the last part of its name in that form. """ return [ MetadataParameterChild( diff --git a/tests/unit/v2/test_parameter_tree.py b/tests/unit/v2/test_parameter_tree.py index 3cdcbe7b0..e61c57bf3 100644 --- a/tests/unit/v2/test_parameter_tree.py +++ b/tests/unit/v2/test_parameter_tree.py @@ -276,15 +276,6 @@ def test_the_reform_view_is_sorted_by_label_as_v3_shows_it() -> None: ] -def test_parameters_without_flags_count_as_changeable_in_the_reform_view() -> None: - rows = build_parameter_tree( - nodes=[], - parameters=[_parameter("gov.older.rate", economy=None, household=None)], - ) - - assert _rows_by_path(rows)["gov.older.rate"].reform_changeable is True - - def test_every_row_has_its_parent_and_counts_match_the_rows() -> None: rows = _uk_income_tax_tree() by_path = _rows_by_path(rows) From bfe79da424a8d5b8b36ffc48d45fad26c8b83e01 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:41:17 +0400 Subject: [PATCH 12/23] Declare exactly the errors the parameter tree route returns The route's OpenAPI 503 now says that a catalog version published before the tree existed has no tree, and that the message then names the version; the other metadata routes keep the shared description. Like them, the route declares no 400, since every invalid query is a 422. Co-Authored-By: Claude Opus 5.5 --- .../fastapi_routes/v2/metadata/common.py | 12 ++++++++++++ .../fastapi_routes/v2/metadata/parameter_routes.py | 2 ++ tests/unit/v2/test_metadata_routes.py | 12 ++++++++++++ 3 files changed, 26 insertions(+) diff --git a/policyengine_api/fastapi_routes/v2/metadata/common.py b/policyengine_api/fastapi_routes/v2/metadata/common.py index 0f5593d34..81b8c8e71 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/common.py +++ b/policyengine_api/fastapi_routes/v2/metadata/common.py @@ -61,6 +61,18 @@ } """The extra response of the one metadata route that takes the country in its path.""" +PARAMETER_TREE_ERROR_RESPONSES: dict[int | str, dict[str, Any]] = { + 503: { + "model": V2ErrorResponse, + "description": ( + "The initialized v2 catalog is unavailable, or the catalog version " + "has no parameter tree because it was published before the tree " + "existed; the message then names the version." + ), + }, +} +"""The parameter tree route's 503, which also covers a version without a tree.""" + ERROR_RESPONSES: dict[int | str, dict[str, Any]] = { 404: { "model": V2ErrorResponse, diff --git a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py index 4d1e8d4d5..8980ab953 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py @@ -10,6 +10,7 @@ from policyengine_api.fastapi_routes.v2.metadata.common import ( METADATA_RESPONSES, + PARAMETER_TREE_ERROR_RESPONSES, CacheableMetadataRoute, read_resource, take_utc_instant_for_read, @@ -77,6 +78,7 @@ def list_parameters( @router.get( "/parameters/children", response_model=MetadataParameterChildPageResponse, + responses=PARAMETER_TREE_ERROR_RESPONSES, summary="List one level of a catalog version's parameter tree", description=( "Lists the direct children of `parent_path` (the top level when " diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 2b7a270a0..774eddf9f 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -1601,3 +1601,15 @@ def test_parameter_children_openapi_declares_the_query_contract() -> None: assert "422" in description assert "malformed policyengine_version" in description assert "503" in description + + +def test_parameter_children_openapi_declares_exactly_its_error_responses() -> None: + results, _ids = _resource_results() + schema = _client(lambda: ResourceReader(results)).get("/v2/openapi.json").json() + responses = schema["paths"]["/v2/parameters/children"]["get"]["responses"] + parameter_responses = schema["paths"]["/v2/parameters"]["get"]["responses"] + + assert set(responses) == {"200", "304", "404", "405", "422", "500", "503"} + assert "PolicyEngine.py version" in responses["422"]["description"] + assert "parameter tree" in responses["503"]["description"] + assert "parameter tree" not in parameter_responses["503"]["description"] From 6c3bbcfe9209b66811f7f8d6d4a24084ffab17c1 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:23:08 +0400 Subject: [PATCH 13/23] Give the installed catalog version its parameter tree when it has none When an earlier release stored the installed catalog version without a tree, publication now writes one in the same transaction, built from the installed packages' catalog records, the same tree a fresh publication writes. Only the stored parameters' IDs are read, by name, so the rows reference them; the stored rows were already checked against the catalog. Other stored versions still get none, and publication fails if a version in the catalog is left without a tree. Co-Authored-By: Claude Opus 5.5 --- .../v2plan-1f-117-parameter-children.added.md | 2 +- docs/migration/stage-9-v2-metadata.md | 28 +-- .../data/v2/catalog/publication.py | 15 +- .../v2/catalog/publication_parameter_tree.py | 160 ++++++++++-------- .../data/v2/catalog/publication_types.py | 4 +- .../test_v2_catalog_publication.py | 81 +++++++-- 6 files changed, 181 insertions(+), 109 deletions(-) diff --git a/changelog.d/v2plan-1f-117-parameter-children.added.md b/changelog.d/v2plan-1f-117-parameter-children.added.md index 0bbb886db..e68ef3c91 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.added.md +++ b/changelog.d/v2plan-1f-117-parameter-children.added.md @@ -1 +1 @@ -Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and labels take the form the app's v3 workspace shows, with a path without a stored label getting one made from the last part of its name. Publication writes the tree of each version it publishes, and its evidence lists them; versions stored before this change keep having no tree. +Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and labels take the form the app's v3 workspace shows, with a path without a stored label getting one made from the last part of its name. Publication writes the tree of each version it publishes, and of the installed version when an earlier release stored it without one, built from the installed packages' catalog; its evidence lists them. Other versions stored before this change keep having no tree. diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index 665990b21..259cf9e00 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -325,18 +325,22 @@ parameter. These are the rules of the tree the app's newer interface builds from v1 metadata. Trees are written only for versions published with catalog format 2, so every parameter in a tree has both flags. -In the same transaction, the command writes the tree of each catalog version -it publishes, from the parameters and parameter nodes it has just stored. The -evidence's `parameter_trees_written` lists each, with its model, version and -row count, and publication fails if one is still without a tree when it -finishes. A version that is already stored is left as it is. - -Versions stored before the tree existed therefore have no tree. Writing one -for them would derive rows from what earlier releases stored, a data -migration that the Alembic skill's rules leave to a human migration decision, -so it is not done. Such a version gets a tree only if a person decides on -that migration; a new PolicyEngine.py version published after this change -gets its tree when it is published. +In the same transaction, the command writes the tree of each version in the +catalog it publishes, which comes from the installed packages, when that +version has none: the versions it publishes now, and the installed version when +an earlier release stored it without a tree. The rows are built from the +catalog's own parameter and node records; only the stored parameters' IDs are +read, by name, so the rows reference them, after publication has checked that +the stored rows match the catalog. This is a row-only, idempotent insert of +package-derived rows, which the deployment-seeder passage of the Alembic skill +allows. The evidence's `parameter_trees_written` lists each version written, +with its model, version and row count, and publication fails if a version in +the catalog is still without a tree when it finishes. + +Other stored versions get no tree. Writing one for them would derive rows from +what earlier releases stored, a data migration that the Alembic skill's rules +leave to a human migration decision, so it is not done; such a version gets a +tree only if a person decides on that migration. `/v2/parameters/children` lists one level with one indexed read: `parent_path` names the node ("" for the top level), and diff --git a/policyengine_api/data/v2/catalog/publication.py b/policyengine_api/data/v2/catalog/publication.py index 4b39dbf77..b9a017904 100644 --- a/policyengine_api/data/v2/catalog/publication.py +++ b/policyengine_api/data/v2/catalog/publication.py @@ -125,9 +125,10 @@ def publish_catalog( ) -> PublicationEvidence: """Publish one complete catalog atomically and return non-secret evidence. - In the same transaction it writes the parameter tree of each catalog - version it publishes. A version stored before the tree existed keeps - having none. + In the same transaction it writes the parameter tree of each version in + the catalog that has none, built from the catalog: the versions it + publishes, and the installed version when an earlier release stored it + without a tree. Other stored versions keep having none. """ started = time.monotonic() with engine.begin() as connection: @@ -141,14 +142,10 @@ def publish_catalog( existing, kept_region_summaries = _check_stored_versions(connection, catalog) - published_version_ids = [] for country in catalog.countries: if country.country_id not in existing: publish_new_country(connection, country) - published_version_ids.append(country.model_version.id) - parameter_trees_written = write_parameter_trees( - connection, published_version_ids - ) + parameter_trees_written = write_parameter_trees(connection, catalog.countries) if checkpoint is not None: checkpoint("after_reconciliation", connection) @@ -159,7 +156,7 @@ def publish_catalog( include_regions=country.country_id not in existing, ) assert_canonical_value_uniqueness(connection) - assert_parameter_trees_written(connection, published_version_ids) + assert_parameter_trees_written(connection, catalog.countries) if _protected_row_counts(connection) != before: msg = "publication changed simulation, report, or dataset-version rows" raise CatalogPublicationError(msg) diff --git a/policyengine_api/data/v2/catalog/publication_parameter_tree.py b/policyengine_api/data/v2/catalog/publication_parameter_tree.py index 88179a288..213d9d2a8 100644 --- a/policyengine_api/data/v2/catalog/publication_parameter_tree.py +++ b/policyengine_api/data/v2/catalog/publication_parameter_tree.py @@ -1,13 +1,18 @@ -"""Write the parameter tree of each catalog version a publication publishes. - -A version's tree rows are derived from the parameters and parameter nodes the -publication has just stored for it (``parameter_tree``), in the same -transaction. Writing only inserts rows for those versions; no other table -changes. - -Versions stored before the tree existed get none here: deriving their rows -from what earlier releases stored would be a data migration, which the -migration rules leave to a human decision. +"""Write the parameter tree of the installed catalog's versions. + +Publication writes the tree of each country version in the catalog it +publishes, which is extracted from the installed packages: versions it +publishes now, and the installed version when an earlier release stored it +without a tree. The rows are built from the catalog's own parameter and node +records (``parameter_tree``), never from rows an earlier release stored. Only +the stored parameters' IDs are read, by name, so tree rows reference the stored +parameters; publication has already checked that those stored rows match the +catalog. Writing only inserts rows for a version that has no tree; no other +table changes. + +Other stored versions get no tree here: deriving their rows from what earlier +releases stored would be a data migration, which the migration rules leave to +a human decision. """ from __future__ import annotations @@ -30,7 +35,6 @@ ) from policyengine_api.data.v2.models import ( Parameter, - ParameterNode, ParameterTreeChild, TaxBenefitModel, TaxBenefitModelVersion, @@ -42,56 +46,80 @@ from sqlalchemy import Connection + from policyengine_api.data.v2.catalog.records import CountryCatalog + MODELS = TaxBenefitModel.__table__ MODEL_VERSIONS = TaxBenefitModelVersion.__table__ PARAMETERS = Parameter.__table__ -PARAMETER_NODES = ParameterNode.__table__ PARAMETER_TREE_CHILDREN = ParameterTreeChild.__table__ -def _versions(version_ids: Sequence[UUID]) -> sa.Select: - return ( - sa.select(MODEL_VERSIONS.c.id, MODELS.c.name, MODEL_VERSIONS.c.version) +def _stored_version_id(connection: Connection, country: CountryCatalog) -> UUID: + return connection.execute( + sa.select(MODEL_VERSIONS.c.id) .select_from( MODEL_VERSIONS.join(MODELS, MODELS.c.id == MODEL_VERSIONS.c.model_id) ) - .where(MODEL_VERSIONS.c.id.in_(version_ids)) - .order_by(MODELS.c.name, MODEL_VERSIONS.c.version) + .where( + MODELS.c.name == country.model.name, + MODEL_VERSIONS.c.version == country.model_version.version, + ) + ).scalar_one() + + +def _has_tree(connection: Connection, version_id: UUID) -> bool: + return bool( + connection.execute( + sa.select( + sa.exists().where( + PARAMETER_TREE_CHILDREN.c.tax_benefit_model_version_id == version_id + ) + ) + ).scalar_one() ) -def _stored_nodes(connection: Connection, version_id: UUID) -> list[TreeNodeSource]: - return [ - TreeNodeSource(name=row.name, label=row.label) +def _stored_parameter_ids(connection: Connection, version_id: UUID) -> dict[str, UUID]: + return { + row.name: row.id for row in connection.execute( - sa.select(PARAMETER_NODES.c.name, PARAMETER_NODES.c.label).where( - PARAMETER_NODES.c.tax_benefit_model_version_id == version_id + sa.select(PARAMETERS.c.name, PARAMETERS.c.id).where( + PARAMETERS.c.tax_benefit_model_version_id == version_id ) ) - ] + } -def _stored_parameters( - connection: Connection, version_id: UUID -) -> list[TreeParameterSource]: - return [ - TreeParameterSource( - id=row.id, - name=row.name, - label=row.label, - economy=row.economy, - household=row.household, - ) - for row in connection.execute( - sa.select( - PARAMETERS.c.id, - PARAMETERS.c.name, - PARAMETERS.c.label, - PARAMETERS.c.economy, - PARAMETERS.c.household, - ).where(PARAMETERS.c.tax_benefit_model_version_id == version_id) - ) +def _catalog_tree( + country: CountryCatalog, stored_parameter_ids: dict[str, UUID] +) -> tuple[ParameterTreeChildRow, ...]: + missing = [ + parameter.name + for parameter in country.parameters + if parameter.name not in stored_parameter_ids ] + if missing: + msg = ( + f"the stored {country.country_id} catalog version " + f"{country.model_version.version} lacks the parameter {missing[0]}" + ) + raise CatalogPublicationError(msg) + return build_parameter_tree( + nodes=[ + TreeNodeSource(name=node.name, label=node.label) + for node in country.parameter_nodes + ], + parameters=[ + TreeParameterSource( + id=stored_parameter_ids[parameter.name], + name=parameter.name, + label=parameter.label, + economy=parameter.economy, + household=parameter.household, + ) + for parameter in country.parameters + ], + ) def _table_rows( @@ -122,50 +150,38 @@ def _table_rows( def write_parameter_trees( connection: Connection, - version_ids: Sequence[UUID], + countries: Sequence[CountryCatalog], ) -> tuple[tuple[str, str, int], ...]: - """Write the tree of each catalog version just published. + """Write the tree of each catalog country's stored version that has none. Returns the model name, version and number of rows written for each - version, in model and version order. Runs inside publication's - transaction and advisory lock, after the versions' parameters and nodes - are stored. + version, in model name order. Runs inside publication's transaction and + advisory lock, after the countries' versions are stored and checked. """ written = [] - for version_id, model_name, version in connection.execute( - _versions(version_ids) - ).all(): - rows = build_parameter_tree( - nodes=_stored_nodes(connection, version_id), - parameters=_stored_parameters(connection, version_id), - ) + for country in countries: + version_id = _stored_version_id(connection, country) + if _has_tree(connection, version_id): + continue + rows = _catalog_tree(country, _stored_parameter_ids(connection, version_id)) count = copy_rows( connection, table=PARAMETER_TREE_CHILDREN, rows=_table_rows(version_id, rows), ) - written.append((model_name, version, count)) - return tuple(written) + written.append((country.model.name, country.model_version.version, count)) + return tuple(sorted(written)) def assert_parameter_trees_written( connection: Connection, - version_ids: Sequence[UUID], + countries: Sequence[CountryCatalog], ) -> None: - """Raise CatalogPublicationError if a version just published has no tree.""" - for version_id, model_name, version in connection.execute( - _versions(version_ids) - ).all(): - has_tree = connection.execute( - sa.select( - sa.exists().where( - PARAMETER_TREE_CHILDREN.c.tax_benefit_model_version_id == version_id - ) - ) - ).scalar_one() - if not has_tree: + """Raise CatalogPublicationError if a catalog country's version has no tree.""" + for country in countries: + if not _has_tree(connection, _stored_version_id(connection, country)): msg = ( - f"the published catalog version {version} of {model_name} " - "has no parameter tree" + f"the {country.country_id} catalog version " + f"{country.model_version.version} has no parameter tree" ) raise CatalogPublicationError(msg) diff --git a/policyengine_api/data/v2/catalog/publication_types.py b/policyengine_api/data/v2/catalog/publication_types.py index a8133d840..d0d62f19c 100644 --- a/policyengine_api/data/v2/catalog/publication_types.py +++ b/policyengine_api/data/v2/catalog/publication_types.py @@ -22,8 +22,8 @@ class PublicationEvidence: # Stored catalog versions whose region rows were kept although the staged # regions differ: country, version, stored and staged region counts. kept_region_summaries: tuple[tuple[str, str, int, int], ...] = () - # Catalog versions published now, with their parameter tree: model, - # version and tree rows written. Empty when no version is published. + # The catalog's versions whose parameter tree this publication wrote: + # model, version and tree rows. Empty once each has its tree. parameter_trees_written: tuple[tuple[str, str, int], ...] = () def as_dict(self) -> dict[str, object]: diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index c62931dc3..cabe8c055 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -880,25 +880,51 @@ def test_publication_writes_each_new_version_parameter_tree( ) -def test_publication_writes_no_tree_for_a_version_stored_without_one( +def _stored_tree_with_parameter_ids( + engine: Engine, model_name: str, version: str +) -> list[tuple]: + with engine.connect() as connection: + return [ + tuple(row) + for row in connection.execute( + text( + "SELECT tree.parent_path, tree.path, tree.position, tree.label, " + "tree.child_type, tree.child_count, tree.reform_position, " + "tree.reform_child_count, tree.parameter_id, tree.economy, " + "tree.household " + "FROM parameter_tree_children AS tree " + "JOIN tax_benefit_model_versions AS version " + "ON version.id = tree.tax_benefit_model_version_id " + "JOIN tax_benefit_models AS model ON model.id = version.model_id " + "WHERE model.name = :model_name AND version.version = :version " + "ORDER BY tree.parent_path, tree.position" + ), + {"model_name": model_name, "version": version}, + ) + ] + + +def test_publication_gives_the_installed_stored_version_its_tree( publication_engine: Engine, ) -> None: - """A version stored before the tree existed keeps having none. + """The installed version, stored by a release without the tree, gets one. - Writing it would derive rows from what earlier releases stored, which the - migration rules leave to a human migration decision; only the versions a - publication publishes get a tree. + It is built from the installed packages' catalog, the same as a fresh + publication's; writing it again changes nothing. """ - older = normalized_catalog(policyengine_version="1.0.0") catalog = normalized_catalog() - older_version = older.country("us").model_version.version current_version = catalog.country("us").model_version.version - publish_catalog(publication_engine, older) - _delete_tree(publication_engine, older_version) - - evidence = publish_catalog(publication_engine, catalog) + publish_catalog(publication_engine, catalog) + fresh = { + country.model.name: _stored_tree_with_parameter_ids( + publication_engine, country.model.name, current_version + ) + for country in catalog.countries + } _delete_tree(publication_engine, current_version) before_ids = _identifiers(publication_engine) + + evidence = publish_catalog(publication_engine, catalog) again = publish_catalog(publication_engine, catalog) assert evidence.parameter_trees_written == tuple( @@ -907,11 +933,40 @@ def test_publication_writes_no_tree_for_a_version_stored_without_one( ) assert again.parameter_trees_written == () for country in catalog.countries: - for version in (older_version, current_version): - assert _stored_tree(publication_engine, country.model.name, version) == [] + assert ( + _stored_tree_with_parameter_ids( + publication_engine, country.model.name, current_version + ) + == fresh[country.model.name] + ) assert _identifiers(publication_engine) == before_ids +def test_publication_writes_no_tree_for_another_version_stored_without_one( + publication_engine: Engine, +) -> None: + """A stored version that is not the installed one keeps having no tree. + + Writing it would derive rows from what an earlier release stored, which + the migration rules leave to a human migration decision. + """ + older = normalized_catalog(policyengine_version="1.0.0") + catalog = normalized_catalog() + older_version = older.country("us").model_version.version + publish_catalog(publication_engine, older) + _delete_tree(publication_engine, older_version) + + evidence = publish_catalog(publication_engine, catalog) + again = publish_catalog(publication_engine, catalog) + + assert {version for _model, version, _rows in evidence.parameter_trees_written} == { + catalog.country("us").model_version.version + } + assert again.parameter_trees_written == () + for country in catalog.countries: + assert _stored_tree(publication_engine, country.model.name, older_version) == [] + + def test_publication_fails_when_a_published_version_is_left_without_a_tree( publication_engine: Engine, monkeypatch: pytest.MonkeyPatch, From 89156a2cb7651a094305d71e4470d419069ab7ae Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:23:49 +0400 Subject: [PATCH 14/23] Test that parameter tree responses are cached like other metadata A version's tree never changes, so the route serves an ETag, a day-long Cache-Control for a named version and five minutes for the running one, a Vary on Origin, and a 304 for a matching If-None-Match, as the other metadata routes do; a version without a tree gets a 503 with none of them. Co-Authored-By: Claude Opus 5.5 --- tests/unit/v2/test_metadata_routes.py | 41 +++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 774eddf9f..8fe8af499 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -1613,3 +1613,44 @@ def test_parameter_children_openapi_declares_exactly_its_error_responses() -> No assert "PolicyEngine.py version" in responses["422"]["description"] assert "parameter tree" in responses["503"]["description"] assert "parameter tree" not in parameter_responses["503"]["description"] + + +def test_parameter_tree_responses_are_cached_like_other_metadata() -> None: + """A version's tree never changes, so it is cached as other metadata is.""" + results, _ids = _resource_results() + client = _client(lambda: ResourceReader(results)) + path = "/v2/parameters/children?country_id=us&parent_path=gov.example" + + running = client.get(path, headers={"Origin": "https://app.policyengine.org"}) + named = client.get(f"{path}&policyengine_version=4.20.3") + revalidated = client.get( + f"{path}&policyengine_version=4.20.3", + headers={"If-None-Match": named.headers["etag"]}, + ) + + assert running.status_code == named.status_code == 200 + assert running.headers["cache-control"] == "public, max-age=300" + assert named.headers["cache-control"] == "public, max-age=86400" + assert running.headers["etag"] + assert "origin" in _vary_values(running) + assert revalidated.status_code == 304 + assert revalidated.content == b"" + assert revalidated.headers["etag"] == named.headers["etag"] + + +def test_a_missing_parameter_tree_is_not_cached() -> None: + results, _ids = _resource_results() + reader = ResourceReader( + results, + error=ParameterTreeUnavailableError("catalog version 4.20.3 has no tree"), + ) + + response = _client(lambda: reader).get( + "/v2/parameters/children", + params={"country_id": "us", "policyengine_version": "4.20.3"}, + ) + + assert response.status_code == 503 + assert "etag" not in response.headers + assert "cache-control" not in response.headers + assert "origin" not in _vary_values(response) From 4d14c0dc3f9a881f19b370017ae84fedcdf57322 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:09:22 +0400 Subject: [PATCH 15/23] Write no parameter tree for a version stored before catalog format 2 Such a version's parameter rows store no economy or household flags, so a tree built from the installed catalog would carry flags, reform positions and reform counts its own rows do not have. Publication now writes a tree for a stored version only when its catalog format is 2, and the check that every catalog version has a tree leaves such a version out; it keeps returning the 503 that names it. Co-Authored-By: Claude Opus 5.5 --- .../v2/catalog/publication_parameter_tree.py | 39 +++++++++++++------ .../test_v2_catalog_publication.py | 25 ++++++++++++ 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/policyengine_api/data/v2/catalog/publication_parameter_tree.py b/policyengine_api/data/v2/catalog/publication_parameter_tree.py index 213d9d2a8..cbb4dcf62 100644 --- a/policyengine_api/data/v2/catalog/publication_parameter_tree.py +++ b/policyengine_api/data/v2/catalog/publication_parameter_tree.py @@ -54,9 +54,12 @@ PARAMETER_TREE_CHILDREN = ParameterTreeChild.__table__ -def _stored_version_id(connection: Connection, country: CountryCatalog) -> UUID: - return connection.execute( - sa.select(MODEL_VERSIONS.c.id) +def _stored_version( + connection: Connection, country: CountryCatalog +) -> tuple[UUID, int | None]: + """Return the stored version's ID and catalog format (None before format 2).""" + row = connection.execute( + sa.select(MODEL_VERSIONS.c.id, MODEL_VERSIONS.c.catalog_format) .select_from( MODEL_VERSIONS.join(MODELS, MODELS.c.id == MODEL_VERSIONS.c.model_id) ) @@ -64,7 +67,8 @@ def _stored_version_id(connection: Connection, country: CountryCatalog) -> UUID: MODELS.c.name == country.model.name, MODEL_VERSIONS.c.version == country.model_version.version, ) - ).scalar_one() + ).one() + return row.id, row.catalog_format def _has_tree(connection: Connection, version_id: UUID) -> bool: @@ -90,9 +94,14 @@ def _stored_parameter_ids(connection: Connection, version_id: UUID) -> dict[str, } -def _catalog_tree( +def catalog_parameter_tree( country: CountryCatalog, stored_parameter_ids: dict[str, UUID] ) -> tuple[ParameterTreeChildRow, ...]: + """Build a country version's tree from its catalog records. + + Each parameter row references the stored parameter of the same name; + raise CatalogPublicationError if a catalog parameter has no stored row. + """ missing = [ parameter.name for parameter in country.parameters @@ -154,16 +163,20 @@ def write_parameter_trees( ) -> tuple[tuple[str, str, int], ...]: """Write the tree of each catalog country's stored version that has none. - Returns the model name, version and number of rows written for each + A version stored before catalog format 2 gets none: its parameter rows + store no flags, so a tree built with the catalog's flags would not match + them. Returns the model name, version and number of rows written for each version, in model name order. Runs inside publication's transaction and advisory lock, after the countries' versions are stored and checked. """ written = [] for country in countries: - version_id = _stored_version_id(connection, country) - if _has_tree(connection, version_id): + version_id, catalog_format = _stored_version(connection, country) + if catalog_format is None or _has_tree(connection, version_id): continue - rows = _catalog_tree(country, _stored_parameter_ids(connection, version_id)) + rows = catalog_parameter_tree( + country, _stored_parameter_ids(connection, version_id) + ) count = copy_rows( connection, table=PARAMETER_TREE_CHILDREN, @@ -177,9 +190,13 @@ def assert_parameter_trees_written( connection: Connection, countries: Sequence[CountryCatalog], ) -> None: - """Raise CatalogPublicationError if a catalog country's version has no tree.""" + """Raise CatalogPublicationError if a catalog country's version has no tree. + + Versions stored before catalog format 2 are exempt: they get no tree. + """ for country in countries: - if not _has_tree(connection, _stored_version_id(connection, country)): + version_id, catalog_format = _stored_version(connection, country) + if catalog_format is not None and not _has_tree(connection, version_id): msg = ( f"the {country.country_id} catalog version " f"{country.model_version.version} has no parameter tree" diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index cabe8c055..663effc74 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -942,6 +942,31 @@ def test_publication_gives_the_installed_stored_version_its_tree( assert _identifiers(publication_engine) == before_ids +def test_an_installed_version_stored_before_format_2_gets_no_tree( + publication_engine: Engine, +) -> None: + """A version stored before catalog format 2 stores no flags, so no tree. + + Its tree would carry the installed catalog's flags, which its own + parameter rows do not store; it keeps returning the clear 503 instead, and + publication still succeeds. + """ + catalog = normalized_catalog() + current_version = catalog.country("us").model_version.version + publish_catalog(publication_engine, catalog) + _clear_format_2_fields(publication_engine) + _delete_tree(publication_engine, current_version) + + evidence = publish_catalog(publication_engine, catalog) + + assert evidence.as_dict()["outcome"] == "ok" + assert evidence.parameter_trees_written == () + for country in catalog.countries: + assert ( + _stored_tree(publication_engine, country.model.name, current_version) == [] + ) + + def test_publication_writes_no_tree_for_another_version_stored_without_one( publication_engine: Engine, ) -> None: From 03d0818645f5792e3c8390e8e912f8d31dc4bff4 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:10:09 +0400 Subject: [PATCH 16/23] Test the parameter tree's link to stored parameters by name A unit test builds a catalog version's tree with stored IDs for every parameter and with none, which fails naming the missing parameter. The Postgres tests share one tree query, and the fail-closed test names the argument it now receives (catalog countries). Co-Authored-By: Claude Opus 5.5 --- .../test_v2_catalog_publication.py | 35 +++++++------------ tests/unit/v2/test_catalog_publication.py | 28 +++++++++++++++ 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index 663effc74..a7ec27fe2 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -801,7 +801,14 @@ def test_republication_detects_a_changed_enum_label_parameter_path_unit_or_perio assert _stored_us_variables(publication_engine) == stored -def _stored_tree(engine: Engine, model_name: str, version: str) -> list[tuple]: +def _stored_tree( + engine: Engine, + model_name: str, + version: str, + *, + parameter_column: str = "parameter.name", +) -> list[tuple]: + """Return a stored version's tree rows; each parameter by name by default.""" with engine.connect() as connection: return [ tuple(row) @@ -809,7 +816,7 @@ def _stored_tree(engine: Engine, model_name: str, version: str) -> list[tuple]: text( "SELECT tree.parent_path, tree.path, tree.position, tree.label, " "tree.child_type, tree.child_count, tree.reform_position, " - "tree.reform_child_count, parameter.name, tree.economy, " + f"tree.reform_child_count, {parameter_column}, tree.economy, " "tree.household " "FROM parameter_tree_children AS tree " "JOIN tax_benefit_model_versions AS version " @@ -883,25 +890,9 @@ def test_publication_writes_each_new_version_parameter_tree( def _stored_tree_with_parameter_ids( engine: Engine, model_name: str, version: str ) -> list[tuple]: - with engine.connect() as connection: - return [ - tuple(row) - for row in connection.execute( - text( - "SELECT tree.parent_path, tree.path, tree.position, tree.label, " - "tree.child_type, tree.child_count, tree.reform_position, " - "tree.reform_child_count, tree.parameter_id, tree.economy, " - "tree.household " - "FROM parameter_tree_children AS tree " - "JOIN tax_benefit_model_versions AS version " - "ON version.id = tree.tax_benefit_model_version_id " - "JOIN tax_benefit_models AS model ON model.id = version.model_id " - "WHERE model.name = :model_name AND version.version = :version " - "ORDER BY tree.parent_path, tree.position" - ), - {"model_name": model_name, "version": version}, - ) - ] + return _stored_tree( + engine, model_name, version, parameter_column="tree.parameter_id" + ) def test_publication_gives_the_installed_stored_version_its_tree( @@ -999,7 +990,7 @@ def test_publication_fails_when_a_published_version_is_left_without_a_tree( monkeypatch.setattr( publication_module, "write_parameter_trees", - lambda _connection, _version_ids: (), + lambda _connection, _countries: (), ) with pytest.raises(CatalogPublicationError, match="has no parameter tree"): diff --git a/tests/unit/v2/test_catalog_publication.py b/tests/unit/v2/test_catalog_publication.py index 5e2766926..1d0c8042a 100644 --- a/tests/unit/v2/test_catalog_publication.py +++ b/tests/unit/v2/test_catalog_publication.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import re from io import StringIO from pathlib import Path from unittest.mock import patch @@ -20,6 +21,12 @@ publication_reconciliation, publication_staging, ) +from policyengine_api.data.v2.catalog.publication_parameter_tree import ( + catalog_parameter_tree, +) +from policyengine_api.data.v2.catalog.publication_types import ( + CatalogPublicationError, +) from policyengine_api.data.v2.migration_target import V2_ALEMBIC_HEAD_REVISION from policyengine_api.data.v2.models import ( Dataset, @@ -397,3 +404,24 @@ def test_matching_kept_regions_log_nothing( caplog: pytest.LogCaptureFixture, ) -> None: assert _kept_region_records(monkeypatch, caplog, (0, 0)) == [] + + +def test_a_catalog_tree_needs_a_stored_row_for_every_parameter() -> None: + """Each tree parameter references the stored parameter of the same name.""" + country = normalized_catalog().country("us") + stored_ids = {parameter.name: parameter.id for parameter in country.parameters} + + rows = catalog_parameter_tree(country, stored_ids) + leaves = [row for row in rows if row.child_type == "parameter"] + + assert [(row.path, row.parameter_id) for row in leaves] == [ + (parameter.name, parameter.id) for parameter in country.parameters + ] + with pytest.raises( + CatalogPublicationError, + match=re.escape( + f"the stored us catalog version {country.model_version.version} " + "lacks the parameter gov.example.rate" + ), + ): + catalog_parameter_tree(country, {}) From b7300b3c039b80d01f1fc6779683f4f3496cb2b0 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:11:23 +0400 Subject: [PATCH 17/23] State the parameter tree's real rule in its documents and messages The module and model docstrings, the runbook, the changelog fragments, the unavailable-tree error and service docstrings, the 503 message and the route's OpenAPI descriptions now say that publication builds trees from the installed packages' catalog, for the versions it publishes and for the installed version stored with catalog format 2 but without one, and that any other version has none and returns the 503. Co-Authored-By: Claude Opus 5.5 --- .../v2plan-1f-117-parameter-children.added.md | 2 +- ...2plan-1f-117-parameter-children.changed.md | 2 +- docs/migration/stage-9-v2-metadata.md | 24 +++++++++---------- .../data/v2/catalog/parameter_tree.py | 17 ++++++------- policyengine_api/data/v2/models/metadata.py | 9 +++---- .../fastapi_routes/v2/metadata/common.py | 5 ++-- .../v2/metadata/parameter_routes.py | 6 +++-- .../services/v2/metadata/services.py | 8 +++---- .../services/v2/metadata/validators.py | 7 +++--- tests/unit/v2/test_metadata_routes.py | 5 ++-- tests/unit/v2/test_metadata_service.py | 2 +- 11 files changed, 47 insertions(+), 40 deletions(-) diff --git a/changelog.d/v2plan-1f-117-parameter-children.added.md b/changelog.d/v2plan-1f-117-parameter-children.added.md index e68ef3c91..23473a174 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.added.md +++ b/changelog.d/v2plan-1f-117-parameter-children.added.md @@ -1 +1 @@ -Catalog publication writes each catalog version's parameter tree into a new table, parameter_tree_children, derived from the version's stored parameters and parameter nodes: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and labels take the form the app's v3 workspace shows, with a path without a stored label getting one made from the last part of its name. Publication writes the tree of each version it publishes, and of the installed version when an earlier release stored it without one, built from the installed packages' catalog; its evidence lists them. Other versions stored before this change keep having no tree. +Catalog publication writes parameter trees into a new table, parameter_tree_children, built from the installed packages' catalog records for each version: one row per node and parameter under its parent's path, with its label, its position among its siblings, its direct child counts, and whether a reform can change it. A bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on, and labels take the form the app's v3 workspace shows, with a path without a stored label getting one made from the last part of its name. Publication writes the tree of each version it publishes, and of the installed version when an earlier release stored it with catalog format 2 but without one; its evidence lists them. Other stored versions, and an installed version stored before catalog format 2, have no tree. diff --git a/changelog.d/v2plan-1f-117-parameter-children.changed.md b/changelog.d/v2plan-1f-117-parameter-children.changed.md index fc7472871..a21e91b00 100644 --- a/changelog.d/v2plan-1f-117-parameter-children.changed.md +++ b/changelog.d/v2plan-1f-117-parameter-children.changed.md @@ -1 +1 @@ -/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its label in the form the app's v3 workspace shows: the stored label with underscores as spaces and its first letter upper-cased, or one made from the last part of its name. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters, an unsupported country and a malformed policyengine_version return 422 (an unsupported country and a malformed version returned 400 before), and a catalog version published before the tree existed returns 503 with a message naming the version. +/v2/parameters/children reads one level of the stored parameter tree with one indexed read. A bracket schedule is now listed as one node whose children are its brackets, instead of its brackets appearing beside its parent's groups under their raw names, and every child carries its label in the form the app's v3 workspace shows: the stored label with underscores as spaces and its first letter upper-cased, or one made from the last part of its name. reform_changeable=true lists only the children a reform can change, sorted by label, with child counts that count only those, following the rules of the tree the app's v3 workspace builds. The route now reads its query through the shared strict models, so unknown or repeated parameters, an unsupported country and a malformed policyengine_version return 422 (an unsupported country and a malformed version returned 400 before), and a catalog version without a tree returns 503 with a message naming the version. diff --git a/docs/migration/stage-9-v2-metadata.md b/docs/migration/stage-9-v2-metadata.md index 259cf9e00..ae3ca794e 100644 --- a/docs/migration/stage-9-v2-metadata.md +++ b/docs/migration/stage-9-v2-metadata.md @@ -307,35 +307,35 @@ next catalog version that is published. ### Parameter tree -Publication also writes each catalog version's parameter tree into -`parameter_tree_children`, which `/v2/parameters/children` reads one level at -a time. The rows are derived from the version's stored parameters and -parameter nodes alone: every node and parameter is one row under its parent's -path, a `[` starts a child as a `.` does (so a bracket schedule is one node +Publication also writes parameter trees into `parameter_tree_children`, which +`/v2/parameters/children` reads one level at a time. A tree is built from a +catalog version's parameter and node records: every node and parameter is one +row under its parent's path, a `[` starts a child as a `.` does (so a bracket schedule is one node whose children are its brackets, labelled "Bracket 1", "Bracket 2" and so on), and labels take the form the app's newer interface shows in every view: the stored label with underscores as spaces and its first letter upper-cased, or, without one, the last part of the name in that form (so an unlabelled schedule `...rates.uk` is "Uk"). Each row also records its position among all its siblings and, if a reform can change it, among the siblings a reform can -change: a parameter -under `gov` whose economy or household flag is set and whose path names +change: a parameter under `gov` whose economy or household flag is set and whose path names neither `taxsim`, `gov.abolitions` nor `pycache`, and every node above such a parameter. These are the rules of the tree the app's newer interface builds -from v1 metadata. Trees are written only for versions published with catalog -format 2, so every parameter in a tree has both flags. +from v1 metadata. Trees are written only for versions with catalog format 2, +so every parameter in a tree has both flags. In the same transaction, the command writes the tree of each version in the catalog it publishes, which comes from the installed packages, when that version has none: the versions it publishes now, and the installed version when -an earlier release stored it without a tree. The rows are built from the +an earlier release stored it with catalog format 2 but without a tree. An +installed version stored before catalog format 2 gets none, since its parameter +rows store no flags. The rows are built from the catalog's own parameter and node records; only the stored parameters' IDs are read, by name, so the rows reference them, after publication has checked that the stored rows match the catalog. This is a row-only, idempotent insert of package-derived rows, which the deployment-seeder passage of the Alembic skill allows. The evidence's `parameter_trees_written` lists each version written, with its model, version and row count, and publication fails if a version in -the catalog is still without a tree when it finishes. +the catalog with catalog format 2 is still without a tree when it finishes. Other stored versions get no tree. Writing one for them would derive rows from what earlier releases stored, a data migration that the Alembic skill's rules @@ -352,7 +352,7 @@ children. The route reads its query through the shared strict query models, so unknown or repeated parameters, an unsupported country and a malformed `policyengine_version` return 422 (the last two returned 400 before). A catalog version without a tree returns 503 with a message that names the -version and says it was published before the tree existed. +version. ## Validation evidence and retry diff --git a/policyengine_api/data/v2/catalog/parameter_tree.py b/policyengine_api/data/v2/catalog/parameter_tree.py index e653dcdb5..7f83b579d 100644 --- a/policyengine_api/data/v2/catalog/parameter_tree.py +++ b/policyengine_api/data/v2/catalog/parameter_tree.py @@ -1,9 +1,10 @@ """Each catalog version's parameter tree, as rows of direct children. -The tree is derived from a version's stored parameters and parameter nodes -alone, and is written when a version is published. Every node and parameter -of the version is one row, under the path of its parent (the empty string for -top-level paths). +The tree is built from a catalog version's parameter and node records: catalog +publication builds it from the installed packages' catalog, for the versions it +publishes and for the installed version when it is stored with catalog format 2 +but without a tree. Every node and parameter of the version is one row, under +the path of its parent (the empty string for top-level paths). A ``[`` starts a child as a ``.`` does: a bracket schedule such as ``gov.hmrc.income_tax.rates.uk`` is one node whose children are its brackets @@ -18,8 +19,8 @@ (``app/src/libs/buildParameterTree.ts``): a parameter under ``gov`` whose economy or household flag is set and whose path names neither ``taxsim``, ``gov.abolitions`` nor ``pycache``, and every node with such a parameter -below it. Trees are written only for versions published with catalog format -2, so every parameter has both flags. +below it. Trees are written only for versions with catalog format 2, so every +parameter has both flags. """ from __future__ import annotations @@ -44,7 +45,7 @@ @dataclass(frozen=True, slots=True) class TreeNodeSource: - """A stored parameter node: its name and stored label.""" + """A parameter node of a catalog version: its name and stored label.""" name: str label: str | None @@ -52,7 +53,7 @@ class TreeNodeSource: @dataclass(frozen=True, slots=True) class TreeParameterSource: - """A stored parameter: its ID, name, stored label and flags.""" + """A parameter of a catalog version: its stored ID, name, label and flags.""" id: UUID name: str diff --git a/policyengine_api/data/v2/models/metadata.py b/policyengine_api/data/v2/models/metadata.py index 099822a76..2f841f0e2 100644 --- a/policyengine_api/data/v2/models/metadata.py +++ b/policyengine_api/data/v2/models/metadata.py @@ -470,10 +470,11 @@ class ParameterValue(IdentifiedModel, table=True): class ParameterTreeChild(IdentifiedModel, table=True): """One direct child in a catalog version's parameter tree. - Derived from the version's parameters and parameter nodes when the version - is published; versions stored before the tree existed have no rows. Every - node and parameter of the version is one row under its parent's path ("" - for top-level paths), so listing one level reads one + Catalog publication writes a version's rows from the installed packages' + catalog: for each version it publishes, and for the installed version when + it is stored with catalog format 2 but without a tree. Other versions have + no rows. Every node and parameter of the version is one row under its + parent's path ("" for top-level paths), so listing one level reads one index. ``position`` orders all children of a parent; ``reform_position`` orders only those a reform can change and is null for the others. Node rows count their direct children; parameter rows reference their parameter and diff --git a/policyengine_api/fastapi_routes/v2/metadata/common.py b/policyengine_api/fastapi_routes/v2/metadata/common.py index 81b8c8e71..5993303f8 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/common.py +++ b/policyengine_api/fastapi_routes/v2/metadata/common.py @@ -66,8 +66,9 @@ "model": V2ErrorResponse, "description": ( "The initialized v2 catalog is unavailable, or the catalog version " - "has no parameter tree because it was published before the tree " - "existed; the message then names the version." + "has no parameter tree (publication writes trees only for the " + "versions it publishes and for the installed version with catalog " + "format 2); the message then names the version." ), }, } diff --git a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py index 8980ab953..b28148a52 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py +++ b/policyengine_api/fastapi_routes/v2/metadata/parameter_routes.py @@ -86,8 +86,10 @@ def list_parameters( "can change, sorted by label, with child counts that count only " "those. Invalid query parameters return 422, including a malformed " "policyengine_version, an unsupported country, and an unknown or " - "repeated parameter. A catalog version published before the tree " - "existed has no tree and returns 503 with a message naming it." + "repeated parameter. A catalog version without a tree (publication " + "writes trees only for the versions it publishes and for the " + "installed version with catalog format 2) returns 503 with a " + "message naming it." ), ) def list_parameter_children( diff --git a/policyengine_api/services/v2/metadata/services.py b/policyengine_api/services/v2/metadata/services.py index 96cc8a507..7b48720fc 100644 --- a/policyengine_api/services/v2/metadata/services.py +++ b/policyengine_api/services/v2/metadata/services.py @@ -358,8 +358,8 @@ def list_parameter_children( schedule is one node whose children are its brackets. With the level's ``reform_changeable``, only the children a reform can change are listed, sorted by label. Raise ParameterTreeUnavailableError when the version has - no tree, which is the case for versions published before the tree - existed. + no tree: catalog publication writes trees only for the versions it + publishes and for the installed version stored with catalog format 2. """ selected_level = level if level is not None else ParameterTreeLevel() selected = self._select_paginated_catalog( @@ -378,8 +378,8 @@ def list_parameter_children( ): msg = ( f"catalog version {selected.policyengine_version} has no parameter " - "tree: a tree is written when a catalog version is published, and " - "this version was published before" + "tree: catalog publication writes trees only for the versions it " + "publishes and for the installed version with catalog format 2" ) raise ParameterTreeUnavailableError(msg) return page_result( diff --git a/policyengine_api/services/v2/metadata/validators.py b/policyengine_api/services/v2/metadata/validators.py index 92c8774a4..62e62a802 100644 --- a/policyengine_api/services/v2/metadata/validators.py +++ b/policyengine_api/services/v2/metadata/validators.py @@ -27,9 +27,10 @@ class InvalidMetadataPageError(ValueError): class ParameterTreeUnavailableError(MetadataCatalogUnavailableError): """Raised when a selected catalog version has no parameter tree. - Catalog publication writes a version's tree when it publishes the version, - so versions published before the tree existed have none. The message names - the version and is safe to return to the caller. + Catalog publication writes a tree, from the installed packages, for each + version it publishes and for the installed version when it is stored with + catalog format 2 but without one; any other version has none. The message + names the version and is safe to return to the caller. """ diff --git a/tests/unit/v2/test_metadata_routes.py b/tests/unit/v2/test_metadata_routes.py index 8fe8af499..e2b7ff1ff 100644 --- a/tests/unit/v2/test_metadata_routes.py +++ b/tests/unit/v2/test_metadata_routes.py @@ -1561,8 +1561,9 @@ def factory(): def test_parameter_children_route_reports_an_unwritten_tree_as_unavailable() -> None: results, _ids = _resource_results() message = ( - "catalog version 6.7.5 has no parameter tree: a tree is written when a " - "catalog version is published, and this version was published before" + "catalog version 6.7.5 has no parameter tree: catalog publication writes " + "trees only for the versions it publishes and for the installed version " + "with catalog format 2" ) reader = ResourceReader(results, error=ParameterTreeUnavailableError(message)) diff --git a/tests/unit/v2/test_metadata_service.py b/tests/unit/v2/test_metadata_service.py index ca5b81a05..4ff27c3ab 100644 --- a/tests/unit/v2/test_metadata_service.py +++ b/tests/unit/v2/test_metadata_service.py @@ -704,7 +704,7 @@ def test_an_unwritten_parameter_tree_is_reported_as_unavailable( ParameterTreeUnavailableError, match=( f"catalog version {POLICYENGINE_VERSION} has no parameter tree: " - "a tree is written when a catalog version is published" + "catalog publication writes trees only for the versions it publishes" ), ): service.list_parameter_children("us") From d6f4af31ba7698e282b16bd074b395345fae85d2 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:12:03 +0400 Subject: [PATCH 18/23] Name the catalog writer in the runtime-role rule and the Alembic skill Rule api.v2.runtime-privileges-tested-as-runtime-role's statement and the Alembic skill's privileges section now say the integration tests also run the metadata services as the runtime role and catalog publication as policyengine_v2_catalog_writer, and the skill's command runs the two newly scoped modules. The generated rules document is re-rendered. Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/alembic-migrations.md | 9 ++++++--- docs/generated/rules.md | 2 +- rules/registry.toml | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/docs/engineering/skills/alembic-migrations.md b/docs/engineering/skills/alembic-migrations.md index 093358e22..9ba11db86 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -146,9 +146,10 @@ check the declarations: - `api.v2.runtime-privileges-declared`: a unit test reads each connector module and fails when it needs a privilege its role isn't declared to hold. - `api.v2.runtime-privileges-tested-as-runtime-role`: the cross-database - integration tests run the v2 user services as `policyengine_v2_runtime` and - the comparison-run services as `policyengine_stage12_test`, holding only - their declared privileges. + integration tests run the v2 user and metadata services as + `policyengine_v2_runtime`, catalog publication as + `policyengine_v2_catalog_writer`, and the comparison-run services as + `policyengine_stage12_test`, holding only their declared privileges. - `api.v2.deployed-privileges-match`: every deploy runs `scripts/reconcile_v2_privileges.py apply` right after the schema upgrade, then `verify`, before the catalog is seeded. @@ -183,6 +184,8 @@ When a pull request adds or changes a v2 table, or a connector's use of one: uv run pytest -q tests/integration/test_v2_user_identity_persistence.py \ tests/integration/test_v2_saved_policy_view.py \ tests/integration/test_v2_stage12_comparison_persistence.py \ + tests/integration/test_v2_metadata_routes.py \ + tests/integration/test_v2_catalog_publication.py \ tests/integration/test_v2_privilege_reconciliation.py ``` diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 2d00bc6b6..73f6a439d 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -234,7 +234,7 @@ flowchart LR | `api.migration.cross-database-integration` | Cross-database integration tests pass against real MySQL and Postgres. | mechanical | enforced | yes | CI job `v2-integration-check` | `pr.yml:v2-integration-check` | — | | `api.v2.deployed-privileges-match` | Every v2 deploy grants the managed roles each privilege policyengine_api/data/v2/privileges.py declares right after the schema upgrade, reports privileges it doesn't declare without revoking them, and stops before the catalog is seeded if a declared privilege is still missing. | mechanical | enforced | yes | CI job `seed`, `catalog-runtime-integration` | `seed-v2-database.yml:seed`, `v2-integration-check.yml:catalog-runtime-integration` | yes | | `api.v2.runtime-privileges-declared` | Every v2 table has an entry for each managed database role in policyengine_api/data/v2/privileges.py, and every privilege a v2 connector module needs (from its reads, creates, updates or deletes module name and its row locks) is declared for the role that runs it. | mechanical | enforced | yes | pytest `tests/unit/v2/test_privilege_manifest.py` | `pr.yml:test` | yes | -| `api.v2.runtime-privileges-tested-as-runtime-role` | The cross-database integration tests run the v2 user services as policyengine_v2_runtime and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI. | mechanical | enforced | yes | CI job `v2-integration-check`, `catalog-runtime-integration` | `pr.yml:v2-integration-check`, `v2-integration-check.yml:catalog-runtime-integration` | — | +| `api.v2.runtime-privileges-tested-as-runtime-role` | The cross-database integration tests run the v2 user and metadata services as policyengine_v2_runtime, catalog publication as policyengine_v2_catalog_writer, and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI. | mechanical | enforced | yes | CI job `v2-integration-check`, `catalog-runtime-integration` | `pr.yml:v2-integration-check`, `v2-integration-check.yml:catalog-runtime-integration` | — | ```mermaid flowchart LR diff --git a/rules/registry.toml b/rules/registry.toml index 571f9aeb0..74f83f68f 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -669,7 +669,7 @@ test = "tests/unit/v2/test_privilege_manifest.py::test_rule_proof_runtime_privil [[rule]] id = "api.v2.runtime-privileges-tested-as-runtime-role" category = "migration" -statement = "The cross-database integration tests run the v2 user services as policyengine_v2_runtime and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI." +statement = "The cross-database integration tests run the v2 user and metadata services as policyengine_v2_runtime, catalog publication as policyengine_v2_catalog_writer, and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI." rationale = "docs/engineering/skills/alembic-migrations.md#table-privileges" scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_saved_policy_view.py", "tests/integration/test_v2_stage12_comparison_persistence.py", "tests/integration/test_v2_metadata_routes.py", "tests/integration/test_v2_catalog_publication.py"] tier = "mechanical" From 37ab9482fe2d9b7f09845caef1f7299261948c88 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:54:12 +0400 Subject: [PATCH 19/23] Look up a stored catalog version's ID and format in one place stored_version returns a country's stored version with its ID and catalog format (None before format 2). The stored-version comparison and the parameter tree writer both use it instead of their own copies of the same query and rule. Co-Authored-By: Claude Opus 5.5 --- .../v2/catalog/publication_parameter_tree.py | 42 ++++++++----------- .../v2/catalog/publication_reconciliation.py | 37 +++++++++++----- 2 files changed, 45 insertions(+), 34 deletions(-) diff --git a/policyengine_api/data/v2/catalog/publication_parameter_tree.py b/policyengine_api/data/v2/catalog/publication_parameter_tree.py index cbb4dcf62..5aa218a23 100644 --- a/policyengine_api/data/v2/catalog/publication_parameter_tree.py +++ b/policyengine_api/data/v2/catalog/publication_parameter_tree.py @@ -29,6 +29,10 @@ TreeParameterSource, build_parameter_tree, ) +from policyengine_api.data.v2.catalog.publication_reconciliation import ( + StoredVersion, + stored_version, +) from policyengine_api.data.v2.catalog.publication_staging import copy_rows from policyengine_api.data.v2.catalog.publication_types import ( CatalogPublicationError, @@ -36,8 +40,6 @@ from policyengine_api.data.v2.models import ( Parameter, ParameterTreeChild, - TaxBenefitModel, - TaxBenefitModelVersion, ) if TYPE_CHECKING: @@ -48,27 +50,19 @@ from policyengine_api.data.v2.catalog.records import CountryCatalog -MODELS = TaxBenefitModel.__table__ -MODEL_VERSIONS = TaxBenefitModelVersion.__table__ PARAMETERS = Parameter.__table__ PARAMETER_TREE_CHILDREN = ParameterTreeChild.__table__ -def _stored_version( - connection: Connection, country: CountryCatalog -) -> tuple[UUID, int | None]: - """Return the stored version's ID and catalog format (None before format 2).""" - row = connection.execute( - sa.select(MODEL_VERSIONS.c.id, MODEL_VERSIONS.c.catalog_format) - .select_from( - MODEL_VERSIONS.join(MODELS, MODELS.c.id == MODEL_VERSIONS.c.model_id) - ) - .where( - MODELS.c.name == country.model.name, - MODEL_VERSIONS.c.version == country.model_version.version, +def _version_for_tree(connection: Connection, country: CountryCatalog) -> StoredVersion: + stored = stored_version(connection, country) + if stored is None: + msg = ( + f"the {country.country_id} catalog version " + f"{country.model_version.version} is not stored" ) - ).one() - return row.id, row.catalog_format + raise CatalogPublicationError(msg) + return stored def _has_tree(connection: Connection, version_id: UUID) -> bool: @@ -171,16 +165,16 @@ def write_parameter_trees( """ written = [] for country in countries: - version_id, catalog_format = _stored_version(connection, country) - if catalog_format is None or _has_tree(connection, version_id): + stored = _version_for_tree(connection, country) + if stored.catalog_format is None or _has_tree(connection, stored.id): continue rows = catalog_parameter_tree( - country, _stored_parameter_ids(connection, version_id) + country, _stored_parameter_ids(connection, stored.id) ) count = copy_rows( connection, table=PARAMETER_TREE_CHILDREN, - rows=_table_rows(version_id, rows), + rows=_table_rows(stored.id, rows), ) written.append((country.model.name, country.model_version.version, count)) return tuple(sorted(written)) @@ -195,8 +189,8 @@ def assert_parameter_trees_written( Versions stored before catalog format 2 are exempt: they get no tree. """ for country in countries: - version_id, catalog_format = _stored_version(connection, country) - if catalog_format is not None and not _has_tree(connection, version_id): + stored = _version_for_tree(connection, country) + if stored.catalog_format is not None and not _has_tree(connection, stored.id): msg = ( f"the {country.country_id} catalog version " f"{country.model_version.version} has no parameter tree" diff --git a/policyengine_api/data/v2/catalog/publication_reconciliation.py b/policyengine_api/data/v2/catalog/publication_reconciliation.py index 14390b6ee..0000941d9 100644 --- a/policyengine_api/data/v2/catalog/publication_reconciliation.py +++ b/policyengine_api/data/v2/catalog/publication_reconciliation.py @@ -2,6 +2,7 @@ from __future__ import annotations +from dataclasses import dataclass from typing import TYPE_CHECKING import sqlalchemy as sa @@ -33,6 +34,8 @@ ) if TYPE_CHECKING: + from uuid import UUID + from policyengine_api.data.v2.catalog.records import CountryCatalog DATASETS = Dataset.__table__ @@ -76,16 +79,24 @@ def _versioned_table_from(table: sa.Table) -> sa.Join: ) -def stored_catalog_format( - connection: Connection, country: CountryCatalog -) -> int | None: - """Return the catalog format this country's stored version was published with. +@dataclass(frozen=True, slots=True) +class StoredVersion: + """A country's stored catalog version: its ID and catalog format. - None means it was published before catalog format 2, so its format 2 - fields are empty. + ``catalog_format`` is None for a version published before catalog format 2, + whose format 2 fields are empty. """ + + id: UUID + catalog_format: int | None + + +def stored_version( + connection: Connection, country: CountryCatalog +) -> StoredVersion | None: + """Return this country's stored catalog version, or None if it is not stored.""" statement = ( - sa.select(MODEL_VERSIONS.c.catalog_format) + sa.select(MODEL_VERSIONS.c.id, MODEL_VERSIONS.c.catalog_format) .select_from( MODEL_VERSIONS.join( MODELS, @@ -97,7 +108,10 @@ def stored_catalog_format( MODEL_VERSIONS.c.version == country.model_version.version, ) ) - return connection.execute(statement).scalar_one_or_none() + row = connection.execute(statement).one_or_none() + if row is None: + return None + return StoredVersion(id=row.id, catalog_format=row.catalog_format) def _comparison_pairs( @@ -375,8 +389,11 @@ def assert_country_matches( PolicyEngine.py version stays the same. A version published before catalog format 2 is compared without format 2's fields. """ - stored_format = stored_catalog_format(connection, country) - pairs = _comparison_pairs(country, with_format_2=stored_format is not None) + stored = stored_version(connection, country) + pairs = _comparison_pairs( + country, + with_format_2=stored is not None and stored.catalog_format is not None, + ) if include_regions: pairs = (*pairs, ("regions", *_region_comparison_pair(country))) for table_name, staged, actual in pairs: From 2e41451ea432503827aa003865e76aad88d672a6 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:54:35 +0400 Subject: [PATCH 20/23] Add the catalog format 2 condition to the tree writer's descriptions The publish_catalog docstring and the tree writer's module docstring now say the installed version gets a tree only when it is stored with catalog format 2, and the evidence field's comment says it is also empty when the installed version was stored before that format. Co-Authored-By: Claude Opus 5.5 --- policyengine_api/data/v2/catalog/publication.py | 5 +++-- .../data/v2/catalog/publication_parameter_tree.py | 8 +++++--- policyengine_api/data/v2/catalog/publication_types.py | 3 ++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/policyengine_api/data/v2/catalog/publication.py b/policyengine_api/data/v2/catalog/publication.py index b9a017904..5bbec59f1 100644 --- a/policyengine_api/data/v2/catalog/publication.py +++ b/policyengine_api/data/v2/catalog/publication.py @@ -127,8 +127,9 @@ def publish_catalog( In the same transaction it writes the parameter tree of each version in the catalog that has none, built from the catalog: the versions it - publishes, and the installed version when an earlier release stored it - without a tree. Other stored versions keep having none. + publishes, and the installed version when an earlier release stored it with + catalog format 2 but without a tree. Other stored versions, and an + installed version stored before catalog format 2, keep having none. """ started = time.monotonic() with engine.begin() as connection: diff --git a/policyengine_api/data/v2/catalog/publication_parameter_tree.py b/policyengine_api/data/v2/catalog/publication_parameter_tree.py index 5aa218a23..1147a18ae 100644 --- a/policyengine_api/data/v2/catalog/publication_parameter_tree.py +++ b/policyengine_api/data/v2/catalog/publication_parameter_tree.py @@ -2,9 +2,11 @@ Publication writes the tree of each country version in the catalog it publishes, which is extracted from the installed packages: versions it -publishes now, and the installed version when an earlier release stored it -without a tree. The rows are built from the catalog's own parameter and node -records (``parameter_tree``), never from rows an earlier release stored. Only +publishes now, and the installed version when an earlier release stored it with +catalog format 2 but without a tree. An installed version stored before catalog +format 2 gets none, since its parameter rows store no flags. The rows are built +from the catalog's own parameter and node records (``parameter_tree``), never +from rows an earlier release stored. Only the stored parameters' IDs are read, by name, so tree rows reference the stored parameters; publication has already checked that those stored rows match the catalog. Writing only inserts rows for a version that has no tree; no other diff --git a/policyengine_api/data/v2/catalog/publication_types.py b/policyengine_api/data/v2/catalog/publication_types.py index d0d62f19c..d547d0fa4 100644 --- a/policyengine_api/data/v2/catalog/publication_types.py +++ b/policyengine_api/data/v2/catalog/publication_types.py @@ -23,7 +23,8 @@ class PublicationEvidence: # regions differ: country, version, stored and staged region counts. kept_region_summaries: tuple[tuple[str, str, int, int], ...] = () # The catalog's versions whose parameter tree this publication wrote: - # model, version and tree rows. Empty once each has its tree. + # model, version and tree rows. Empty when none needed one: each already + # has its tree, or was stored before catalog format 2 and gets none. parameter_trees_written: tuple[tuple[str, str, int], ...] = () def as_dict(self) -> dict[str, object]: From edb0a8ec4d681638a3995ecad13126117a370288 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:55:39 +0400 Subject: [PATCH 21/23] Test the tree's parameter IDs with stored IDs unlike the catalog's The catalog's parameter IDs and the stored ones were both derived from the version and name, so the tests could not tell which the tree used. The unit test now gives each stored parameter a new ID, and the Postgres test first stores the version with new parameter IDs, so a tree built with the catalog's IDs fails both (a mismatch, and a foreign-key error). Co-Authored-By: Claude Opus 5.5 --- .../test_v2_catalog_publication.py | 37 ++++++++++++++++++- tests/unit/v2/test_catalog_publication.py | 14 +++++-- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/tests/integration/test_v2_catalog_publication.py b/tests/integration/test_v2_catalog_publication.py index a7ec27fe2..60077f905 100644 --- a/tests/integration/test_v2_catalog_publication.py +++ b/tests/integration/test_v2_catalog_publication.py @@ -6,6 +6,7 @@ from concurrent.futures import ThreadPoolExecutor from dataclasses import replace from pathlib import Path +from uuid import uuid4 import pytest from alembic import command @@ -895,17 +896,40 @@ def _stored_tree_with_parameter_ids( ) +def _with_other_parameter_ids(catalog): + """Return the catalog with a new ID for every parameter, and its values.""" + countries = [] + for country in catalog.countries: + parameters = [] + for parameter in country.parameters: + parameter_id = uuid4() + parameters.append( + replace( + parameter, + id=parameter_id, + values=tuple( + replace(value, id=uuid4(), parameter_id=parameter_id) + for value in parameter.values + ), + ) + ) + countries.append(replace(country, parameters=tuple(parameters))) + return replace(catalog, countries=tuple(countries)) + + def test_publication_gives_the_installed_stored_version_its_tree( publication_engine: Engine, ) -> None: """The installed version, stored by a release without the tree, gets one. It is built from the installed packages' catalog, the same as a fresh - publication's; writing it again changes nothing. + publication's; writing it again changes nothing. The version is first + stored with parameter IDs that differ from the catalog's, so the rebuilt + tree must reference the stored rows' IDs. """ catalog = normalized_catalog() current_version = catalog.country("us").model_version.version - publish_catalog(publication_engine, catalog) + publish_catalog(publication_engine, _with_other_parameter_ids(catalog)) fresh = { country.model.name: _stored_tree_with_parameter_ids( publication_engine, country.model.name, current_version @@ -931,6 +955,15 @@ def test_publication_gives_the_installed_stored_version_its_tree( == fresh[country.model.name] ) assert _identifiers(publication_engine) == before_ids + catalog_parameter_ids = { + parameter.id + for country in catalog.countries + for parameter in country.parameters + } + for rows in fresh.values(): + assert {row[8] for row in rows if row[8] is not None}.isdisjoint( + catalog_parameter_ids + ) def test_an_installed_version_stored_before_format_2_gets_no_tree( diff --git a/tests/unit/v2/test_catalog_publication.py b/tests/unit/v2/test_catalog_publication.py index 1d0c8042a..377c8c230 100644 --- a/tests/unit/v2/test_catalog_publication.py +++ b/tests/unit/v2/test_catalog_publication.py @@ -7,6 +7,7 @@ from io import StringIO from pathlib import Path from unittest.mock import patch +from uuid import uuid4 import pytest import sqlalchemy as sa @@ -407,16 +408,23 @@ def test_matching_kept_regions_log_nothing( def test_a_catalog_tree_needs_a_stored_row_for_every_parameter() -> None: - """Each tree parameter references the stored parameter of the same name.""" + """Each tree parameter references the stored parameter of the same name. + + The stored IDs differ from the catalog's, so the tree must take them from + the stored rows, not from the catalog records. + """ country = normalized_catalog().country("us") - stored_ids = {parameter.name: parameter.id for parameter in country.parameters} + stored_ids = {parameter.name: uuid4() for parameter in country.parameters} rows = catalog_parameter_tree(country, stored_ids) leaves = [row for row in rows if row.child_type == "parameter"] assert [(row.path, row.parameter_id) for row in leaves] == [ - (parameter.name, parameter.id) for parameter in country.parameters + (parameter.name, stored_ids[parameter.name]) for parameter in country.parameters ] + assert {row.parameter_id for row in leaves}.isdisjoint( + parameter.id for parameter in country.parameters + ) with pytest.raises( CatalogPublicationError, match=re.escape( From 1cc6f620fce3f4395a263190b3e404b17c86a587 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:56:12 +0400 Subject: [PATCH 22/23] Check that a version stored before catalog format 2 returns the tree 503 The Postgres route test stores the fixture catalog, makes it look published before catalog format 2, removes its tree and republishes it: publication writes no tree, and the route returns 503 with the message naming the version and no cache headers. Co-Authored-By: Claude Opus 5.5 --- tests/integration/test_v2_metadata_routes.py | 46 ++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/integration/test_v2_metadata_routes.py b/tests/integration/test_v2_metadata_routes.py index 1e78a6508..d3a248cec 100644 --- a/tests/integration/test_v2_metadata_routes.py +++ b/tests/integration/test_v2_metadata_routes.py @@ -1003,3 +1003,49 @@ def level(parent_path: str) -> list[tuple]: None, ), ] + + +def test_postgres_a_version_stored_before_format_2_returns_the_tree_503( + published_engine: Engine, +) -> None: + """A version stored before catalog format 2 gets no tree on republication. + + Its parameter tree request returns 503 with the message naming it. + """ + with published_engine.begin() as connection: + connection.execute( + text( + "UPDATE tax_benefit_model_versions SET catalog_format = NULL, " + "entities = NULL, basic_inputs = NULL" + ) + ) + connection.execute( + text( + "UPDATE variables SET is_input_variable = NULL, hidden_input = NULL, " + "module_name = NULL, adds_parameter_path = NULL, " + "subtracts_parameter_path = NULL, enum_values = NULL, " + "unit = NULL, definition_period = NULL" + ) + ) + connection.execute( + text("UPDATE parameters SET economy = NULL, household = NULL") + ) + connection.execute(text("DELETE FROM parameter_tree_children")) + evidence = publish_catalog(published_engine, normalized_catalog()) + + response = _client(published_engine).get( + "/v2/parameters/children", + params={"country_id": "us", "policyengine_version": POLICYENGINE_VERSION}, + ) + + assert evidence.parameter_trees_written == () + assert response.status_code == 503 + assert response.json() == { + "status": "error", + "message": ( + f"catalog version {POLICYENGINE_VERSION} has no parameter tree: " + "catalog publication writes trees only for the versions it publishes " + "and for the installed version with catalog format 2" + ), + } + assert "cache-control" not in response.headers From 4519b8e96e16158d9ebd10ebd942a285c4a229ce Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:56:30 +0400 Subject: [PATCH 23/23] Say which metadata read the runtime-role rule's tests cover Of the metadata reads, only the parameter tree read runs as policyengine_v2_runtime in the integration tests, so the rule's statement and the Alembic skill now name that read instead of the metadata services. The generated rules document is re-rendered. Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/alembic-migrations.md | 2 +- docs/generated/rules.md | 2 +- rules/registry.toml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/engineering/skills/alembic-migrations.md b/docs/engineering/skills/alembic-migrations.md index 9ba11db86..a2e36b84c 100644 --- a/docs/engineering/skills/alembic-migrations.md +++ b/docs/engineering/skills/alembic-migrations.md @@ -146,7 +146,7 @@ check the declarations: - `api.v2.runtime-privileges-declared`: a unit test reads each connector module and fails when it needs a privilege its role isn't declared to hold. - `api.v2.runtime-privileges-tested-as-runtime-role`: the cross-database - integration tests run the v2 user and metadata services as + integration tests run the v2 user services and the parameter tree read as `policyengine_v2_runtime`, catalog publication as `policyengine_v2_catalog_writer`, and the comparison-run services as `policyengine_stage12_test`, holding only their declared privileges. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 73f6a439d..e2507a1d2 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -234,7 +234,7 @@ flowchart LR | `api.migration.cross-database-integration` | Cross-database integration tests pass against real MySQL and Postgres. | mechanical | enforced | yes | CI job `v2-integration-check` | `pr.yml:v2-integration-check` | — | | `api.v2.deployed-privileges-match` | Every v2 deploy grants the managed roles each privilege policyengine_api/data/v2/privileges.py declares right after the schema upgrade, reports privileges it doesn't declare without revoking them, and stops before the catalog is seeded if a declared privilege is still missing. | mechanical | enforced | yes | CI job `seed`, `catalog-runtime-integration` | `seed-v2-database.yml:seed`, `v2-integration-check.yml:catalog-runtime-integration` | yes | | `api.v2.runtime-privileges-declared` | Every v2 table has an entry for each managed database role in policyengine_api/data/v2/privileges.py, and every privilege a v2 connector module needs (from its reads, creates, updates or deletes module name and its row locks) is declared for the role that runs it. | mechanical | enforced | yes | pytest `tests/unit/v2/test_privilege_manifest.py` | `pr.yml:test` | yes | -| `api.v2.runtime-privileges-tested-as-runtime-role` | The cross-database integration tests run the v2 user and metadata services as policyengine_v2_runtime, catalog publication as policyengine_v2_catalog_writer, and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI. | mechanical | enforced | yes | CI job `v2-integration-check`, `catalog-runtime-integration` | `pr.yml:v2-integration-check`, `v2-integration-check.yml:catalog-runtime-integration` | — | +| `api.v2.runtime-privileges-tested-as-runtime-role` | The cross-database integration tests run the v2 user services and the parameter tree read as policyengine_v2_runtime, catalog publication as policyengine_v2_catalog_writer, and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI. | mechanical | enforced | yes | CI job `v2-integration-check`, `catalog-runtime-integration` | `pr.yml:v2-integration-check`, `v2-integration-check.yml:catalog-runtime-integration` | — | ```mermaid flowchart LR diff --git a/rules/registry.toml b/rules/registry.toml index 74f83f68f..2f24327dc 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -669,7 +669,7 @@ test = "tests/unit/v2/test_privilege_manifest.py::test_rule_proof_runtime_privil [[rule]] id = "api.v2.runtime-privileges-tested-as-runtime-role" category = "migration" -statement = "The cross-database integration tests run the v2 user and metadata services as policyengine_v2_runtime, catalog publication as policyengine_v2_catalog_writer, and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI." +statement = "The cross-database integration tests run the v2 user services and the parameter tree read as policyengine_v2_runtime, catalog publication as policyengine_v2_catalog_writer, and the comparison-run services as policyengine_stage12_, holding only their declared privileges, so a statement those privileges don't allow fails CI." rationale = "docs/engineering/skills/alembic-migrations.md#table-privileges" scope = ["tests/integration/conftest.py", "tests/integration/test_v2_user_identity_persistence.py", "tests/integration/test_v2_saved_policy_view.py", "tests/integration/test_v2_stage12_comparison_persistence.py", "tests/integration/test_v2_metadata_routes.py", "tests/integration/test_v2_catalog_publication.py"] tier = "mechanical"