From bfb8a0f419dd5cbdc5d297e75c8ccd957ff102ce Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 2 Nov 2026 17:59:57 +0400 Subject: [PATCH] Accept UK area records with or without x and y Make x and y optional on ConstituencyImpactRecord and LocalAuthorityImpactRecord, the first of three deploys that remove them. Both default to None and are left out of every dump while None (pydantic's field-level exclude_if), so a record without them validates and its JSON has no x or y keys rather than "x": null. The executor still sends both. A push to main deploys the executor before the gateway, and the gateway validates every result it returns against the contract. This deploy lets the gateway accept records without x and y before #918's executor change stops sending them; #923 then removes the fields. The gateway's /jobs/{job_id} route already drops null fields (response_model_exclude_none), and the new tests check the JSON it returns for a breakdown and a national result whose records carry no x or y, and for records that send them as null. Raise the contract's pydantic floor to 2.12, the first release with exclude_if; only that specifier line changes in each of the five locks that record the contract's metadata. Regenerate the gateway's golden OpenAPI, which no longer lists x and y as required. Written red-green: the new contract, gateway and generated-client tests failed first because x and y were required ("Field required", and a 500 from the gateway), and because a null x or y was written to the dump. Made from the contract commit of #919 (cherry picked from commit 5b5fbde0), with its gateway tests moved to test_simulation_job_status.py after main split test_endpoints.py, its changelog entry turned into changelog.d/984.changed.md, and wording that kept x and y for older apps replaced. Fixes #984 Co-Authored-By: Claude Opus 5.5 --- changelog.d/984.changed.md | 1 + .../pyproject.toml | 2 +- .../macro_output.py | 21 +++-- .../tests/test_local_area_breakdown.py | 42 ++++++++++ .../tests/test_macro_output_uk_geography.py | 84 +++++++++++++++++++ libs/policyengine-simulation-contract/uv.lock | 2 +- libs/policyengine-stage12-persistence/uv.lock | 2 +- .../tests/simulation/test_calculate.py | 18 ++++ .../policyengine-simulation-entry/uv.lock | 2 +- .../policyengine-simulation-executor/uv.lock | 2 +- .../tests/golden/openapi.json | 6 +- .../tests/test_simulation_job_status.py | 66 +++++++++++++++ .../policyengine-simulation-gateway/uv.lock | 2 +- 13 files changed, 232 insertions(+), 18 deletions(-) create mode 100644 changelog.d/984.changed.md diff --git a/changelog.d/984.changed.md b/changelog.d/984.changed.md new file mode 100644 index 000000000..f9c5fdfad --- /dev/null +++ b/changelog.d/984.changed.md @@ -0,0 +1 @@ +The `x` and `y` fields of UK constituency and local-authority records, each area's integer position on the hex map, are now optional in the contract, as the first of three steps that remove them. A record validates with or without them, and one without them leaves both keys out of its JSON instead of writing `null`. The executor still sends both. The gateway's OpenAPI document no longer lists them as required, and the contract now requires pydantic 2.12 or later, the first release with field-level `exclude_if`. diff --git a/libs/policyengine-simulation-contract/pyproject.toml b/libs/policyengine-simulation-contract/pyproject.toml index 7214eee3b..1de678f91 100644 --- a/libs/policyengine-simulation-contract/pyproject.toml +++ b/libs/policyengine-simulation-contract/pyproject.toml @@ -8,7 +8,7 @@ authors = [ license = {file = "../../LICENSE"} requires-python = ">=3.13" dependencies = [ - "pydantic>=2.0", + "pydantic>=2.12", "packaging>=24", # gateway_models embeds the TelemetryEnvelope from the observability lib. "policyengine-simulation-observability", diff --git a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py index 7ec8bc815..3592d6442 100644 --- a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py +++ b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/macro_output.py @@ -4,7 +4,7 @@ from typing import Generic, Literal, TypeVar -from pydantic import BaseModel, ConfigDict, RootModel +from pydantic import BaseModel, ConfigDict, Field, RootModel from policyengine_simulation_contract.spm import SPMComparisonProvenance @@ -240,6 +240,10 @@ class CliffImpactOutput(MacroOutputModel): UKGeographyDisclosureTier = Literal["reported", "approximate"] +def _is_none(value: object) -> bool: + return value is None + + class UKAreaDisclosureFields(MacroOutputModel): """Disclosure tier and 90% intervals of a published UK area's income changes. @@ -267,14 +271,17 @@ class ConstituencyImpactRecord(UKAreaDisclosureFields): ``average_household_income_change`` is the average change per household and ``relative_household_income_change`` the relative change in the area's total household net income, both rounded to the order of their standard error. - ``population`` is the weighted number of households. ``x`` and ``y`` are - integer map-rendering coordinates. + ``population`` is the weighted number of households. + + ``x`` and ``y`` are the area's integer map position, which is being + removed: a record validates with or without them, and one without them + leaves both out of its JSON rather than writing null. """ constituency_code: str constituency_name: str - x: int | None - y: int | None + x: int | None = Field(default=None, exclude_if=_is_none) + y: int | None = Field(default=None, exclude_if=_is_none) average_household_income_change: float relative_household_income_change: float population: float @@ -288,8 +295,8 @@ class LocalAuthorityImpactRecord(UKAreaDisclosureFields): local_authority_code: str local_authority_name: str - x: int | None - y: int | None + x: int | None = Field(default=None, exclude_if=_is_none) + y: int | None = Field(default=None, exclude_if=_is_none) average_household_income_change: float relative_household_income_change: float population: float diff --git a/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py b/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py index cdd80eaf1..d2cacccfd 100644 --- a/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py +++ b/libs/policyengine-simulation-contract/tests/test_local_area_breakdown.py @@ -201,3 +201,45 @@ def test_each_area_level_reads_its_records_as_its_own_type(): assert [type(record) for record in output.local_authority_impact] == [ LocalAuthorityImpactRecord ] + + +def _without_map_position(record: dict[str, object]) -> dict[str, object]: + return {key: value for key, value in record.items() if key not in {"x", "y"}} + + +# Records from an executor that no longer sends each area's map position. +BREAKDOWN_WITHOUT_MAP_POSITIONS = { + **BREAKDOWN, + "constituency_impact": [_without_map_position(CONSTITUENCY)], + "local_authority_impact": [_without_map_position(LOCAL_AUTHORITY)], +} + + +def test_a_breakdown_without_map_positions_round_trips_without_them(): + output = LocalAreaBreakdownOutput.model_validate(BREAKDOWN_WITHOUT_MAP_POSITIONS) + + assert output.model_dump(mode="json") == BREAKDOWN_WITHOUT_MAP_POSITIONS + + +def test_a_job_status_reads_a_breakdown_without_map_positions(): + response = JobStatusResponse( + status="complete", result=BREAKDOWN_WITHOUT_MAP_POSITIONS + ) + + assert isinstance(response.result, LocalAreaBreakdownOutput) + assert response.model_dump(mode="json")["result"] == ( + BREAKDOWN_WITHOUT_MAP_POSITIONS + ) + + +def test_a_national_result_reads_area_lists_without_map_positions(): + national = { + **NATIONAL, + "constituency_impact": [_without_map_position(CONSTITUENCY)], + "local_authority_impact": [_without_map_position(LOCAL_AUTHORITY)], + } + + response = JobStatusResponse(status="complete", result=national) + + assert isinstance(response.result, SingleYearMacroOutput) + assert response.model_dump(mode="json")["result"] == national diff --git a/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py b/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py index 87d8ae48a..7a7b66640 100644 --- a/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py +++ b/libs/policyengine-simulation-contract/tests/test_macro_output_uk_geography.py @@ -2,6 +2,8 @@ from __future__ import annotations +import json + import pytest from policyengine_simulation_contract.macro_output import ( ConstituencyImpactRecord, @@ -72,3 +74,85 @@ def test_published_values_stay_required() -> None: def test_an_empty_area_list_is_valid() -> None: assert GeographicImpactOutput.model_validate([]).root == [] + + +# x and y, each area's integer map position, are being removed. The executor +# sends them until a later change stops it, so a record validates with or +# without them. +CONSTITUENCY_WITHOUT_MAP_POSITION = { + **{ + key: value + for key, value in STORED_BEFORE_DISCLOSURE.items() + if key not in {"x", "y"} + }, + **DISCLOSURE_FIELDS, +} +LOCAL_AUTHORITY_WITHOUT_MAP_POSITION = { + "local_authority_code": "E06000063", + "local_authority_name": "Cumberland", + "average_household_income_change": -150.0, + "relative_household_income_change": -0.0026, + "population": 120_000.0, + **DISCLOSURE_FIELDS, +} +AREA_RECORDS_WITHOUT_MAP_POSITION = [ + pytest.param( + ConstituencyImpactRecord, CONSTITUENCY_WITHOUT_MAP_POSITION, id="constituency" + ), + pytest.param( + LocalAuthorityImpactRecord, + LOCAL_AUTHORITY_WITHOUT_MAP_POSITION, + id="local-authority", + ), +] + + +@pytest.mark.parametrize(("record_type", "payload"), AREA_RECORDS_WITHOUT_MAP_POSITION) +def test_records_without_a_map_position_validate_and_dump_without_one( + record_type: type[ConstituencyImpactRecord | LocalAuthorityImpactRecord], + payload: dict[str, object], +) -> None: + record = record_type.model_validate(payload) + + assert record.x is None + assert record.y is None + assert record.model_dump(mode="python") == payload + assert record.model_dump(mode="json") == payload + assert json.loads(record.model_dump_json()) == payload + + +@pytest.mark.parametrize(("record_type", "payload"), AREA_RECORDS_WITHOUT_MAP_POSITION) +def test_records_with_a_map_position_keep_it( + record_type: type[ConstituencyImpactRecord | LocalAuthorityImpactRecord], + payload: dict[str, object], +) -> None: + with_map_position = {**payload, "x": 56, "y": -40} + + record = record_type.model_validate(with_map_position) + + assert (record.x, record.y) == (56, -40) + assert record.model_dump(mode="json") == with_map_position + + +@pytest.mark.parametrize(("record_type", "payload"), AREA_RECORDS_WITHOUT_MAP_POSITION) +def test_a_null_map_position_is_left_out_of_the_dump( + record_type: type[ConstituencyImpactRecord | LocalAuthorityImpactRecord], + payload: dict[str, object], +) -> None: + record = record_type.model_validate({**payload, "x": None, "y": None}) + + assert record.model_dump(mode="json") == payload + assert json.loads(record.model_dump_json()) == payload + + +def test_an_area_list_without_map_positions_dumps_without_them() -> None: + payload = [CONSTITUENCY_WITHOUT_MAP_POSITION, LOCAL_AUTHORITY_WITHOUT_MAP_POSITION] + + output = GeographicImpactOutput.model_validate(payload) + + assert [type(record) for record in output.root] == [ + ConstituencyImpactRecord, + LocalAuthorityImpactRecord, + ] + assert output.model_dump(mode="json") == payload + assert json.loads(output.model_dump_json()) == payload diff --git a/libs/policyengine-simulation-contract/uv.lock b/libs/policyengine-simulation-contract/uv.lock index 8915800ea..3fa7e7d15 100644 --- a/libs/policyengine-simulation-contract/uv.lock +++ b/libs/policyengine-simulation-contract/uv.lock @@ -1122,7 +1122,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.0" }, + { name = "pydantic", specifier = ">=2.12" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/libs/policyengine-stage12-persistence/uv.lock b/libs/policyengine-stage12-persistence/uv.lock index 5e3c5dca8..20d14c19b 100644 --- a/libs/policyengine-stage12-persistence/uv.lock +++ b/libs/policyengine-stage12-persistence/uv.lock @@ -604,7 +604,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.0" }, + { name = "pydantic", specifier = ">=2.12" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/projects/policyengine-apis-integ/tests/simulation/test_calculate.py b/projects/policyengine-apis-integ/tests/simulation/test_calculate.py index 105d7bcb6..10c659678 100644 --- a/projects/policyengine-apis-integ/tests/simulation/test_calculate.py +++ b/projects/policyengine-apis-integ/tests/simulation/test_calculate.py @@ -362,6 +362,10 @@ def test_result_assertions_accept_generated_client_objects() -> None: } +def _without_x_and_y(record: Mapping) -> dict: + return {key: value for key, value in record.items() if key not in {"x", "y"}} + + def test_breakdown_assertions_accept_generated_client_objects() -> None: status = JobStatusResponse.from_dict( {"status": "complete", "result": _BREAKDOWN_RESULT} @@ -371,6 +375,20 @@ def test_breakdown_assertions_accept_generated_client_objects() -> None: assert_uk_local_area_breakdown(status.result) +def test_generated_client_reads_area_records_without_x_and_y() -> None: + # Once the executor stops sending x and y, records arrive without them. + breakdown = { + **_BREAKDOWN_RESULT, + "constituency_impact": [_without_x_and_y(_BREAKDOWN_CONSTITUENCY)], + "local_authority_impact": [_without_x_and_y(_BREAKDOWN_LOCAL_AUTHORITY)], + } + + status = JobStatusResponse.from_dict({"status": "complete", "result": breakdown}) + + assert isinstance(status.result, LocalAreaBreakdownOutput) + assert status.result.to_dict() == breakdown + + @pytest.mark.parametrize( ("change", "message"), [ diff --git a/projects/policyengine-simulation-entry/uv.lock b/projects/policyengine-simulation-entry/uv.lock index c3bfa7d53..e9a587c24 100644 --- a/projects/policyengine-simulation-entry/uv.lock +++ b/projects/policyengine-simulation-entry/uv.lock @@ -881,7 +881,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../../libs/policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.0" }, + { name = "pydantic", specifier = ">=2.12" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/projects/policyengine-simulation-executor/uv.lock b/projects/policyengine-simulation-executor/uv.lock index 2bd2f7c36..a9b4bb246 100644 --- a/projects/policyengine-simulation-executor/uv.lock +++ b/projects/policyengine-simulation-executor/uv.lock @@ -1883,7 +1883,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../../libs/policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.0" }, + { name = "pydantic", specifier = ">=2.12" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" }, diff --git a/projects/policyengine-simulation-gateway/tests/golden/openapi.json b/projects/policyengine-simulation-gateway/tests/golden/openapi.json index ae9c9d9c5..edf892ffb 100644 --- a/projects/policyengine-simulation-gateway/tests/golden/openapi.json +++ b/projects/policyengine-simulation-gateway/tests/golden/openapi.json @@ -1146,14 +1146,12 @@ "required": [ "constituency_code", "constituency_name", - "x", - "y", "average_household_income_change", "relative_household_income_change", "population" ], "title": "ConstituencyImpactRecord", - "description": "Reform impact on household net income in one UK parliamentary constituency.\n\n``average_household_income_change`` is the average change per household and\n``relative_household_income_change`` the relative change in the area's total\nhousehold net income, both rounded to the order of their standard error.\n``population`` is the weighted number of households. ``x`` and ``y`` are\ninteger map-rendering coordinates." + "description": "Reform impact on household net income in one UK parliamentary constituency.\n\n``average_household_income_change`` is the average change per household and\n``relative_household_income_change`` the relative change in the area's total\nhousehold net income, both rounded to the order of their standard error.\n``population`` is the weighted number of households.\n\n``x`` and ``y`` are the area's integer map position, which is being\nremoved: a record validates with or without them, and one without them\nleaves both out of its JSON rather than writing null." }, "DecileOutput": { "properties": { @@ -1921,8 +1919,6 @@ "required": [ "local_authority_code", "local_authority_name", - "x", - "y", "average_household_income_change", "relative_household_income_change", "population" diff --git a/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py b/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py index 627748b50..48f2c3ea1 100644 --- a/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py +++ b/projects/policyengine-simulation-gateway/tests/test_simulation_job_status.py @@ -10,6 +10,10 @@ from policyengine_simulation_observability.identifiers import OBSERVABILITY_ID_HEADER +def _without_x_and_y(record: dict[str, object]) -> dict[str, object]: + return {key: value for key, value in record.items() if key not in {"x", "y"}} + + class TestSubmitSimulationEndpoint: """Tests for POST /simulate/economy/comparison endpoint.""" @@ -162,6 +166,68 @@ def test__given_complete_local_area_breakdown__then_polling_returns_it( assert response.json()["status"] == "complete" assert response.json()["result"] == LOCAL_AREA_BREAKDOWN_RESULT + @pytest.mark.parametrize( + "map_position", + [ + pytest.param({}, id="left-out"), + pytest.param({"x": None, "y": None}, id="null"), + ], + ) + def test__given_breakdown_records_without_x_and_y__then_json_has_no_x_or_y( + self, mock_modal, client: TestClient, map_position: dict[str, None] + ): + # Once the executor stops sending x and y on UK area records, the + # JSON must not invent null keys. + constituency = _without_x_and_y( + LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] + ) + local_authority = { + "local_authority_code": "E06000063", + "local_authority_name": "Cumberland", + "average_household_income_change": -120.0, + "relative_household_income_change": -0.002, + "population": 120000.0, + "disclosure_tier": "approximate", + "average_household_income_change_lower": -160.0, + "average_household_income_change_upper": -80.0, + "relative_household_income_change_lower": -0.003, + "relative_household_income_change_upper": -0.001, + } + submit_response = client.post( + "/simulate/economy/comparison", + json={"country": "uk", "scope": "macro", "region": "uk", "reform": {}}, + ) + mock_modal["func"].last_call.result = { + **LOCAL_AREA_BREAKDOWN_RESULT, + "constituency_impact": [{**constituency, **map_position}], + "local_authority_impact": [{**local_authority, **map_position}], + } + + response = client.get(f"/jobs/{submit_response.json()['job_id']}") + + assert response.status_code == 200 + result = response.json()["result"] + assert result["constituency_impact"] == [constituency] + assert result["local_authority_impact"] == [local_authority] + + def test__given_national_records_without_x_and_y__then_json_has_no_x_or_y( + self, mock_modal, client: TestClient + ): + constituency = _without_x_and_y( + LOCAL_AREA_BREAKDOWN_RESULT["constituency_impact"][0] + ) + submit_response = client.post( + "/simulate/economy/comparison", + json={"country": "uk", "scope": "macro", "region": "uk", "reform": {}}, + ) + call = mock_modal["func"].last_call + call.result = {**call.result, "constituency_impact": [constituency]} + + response = client.get(f"/jobs/{submit_response.json()['job_id']}") + + assert response.status_code == 200 + assert response.json()["result"]["constituency_impact"] == [constituency] + def test__given_running_job__then_polling_returns_202( self, mock_modal, client: TestClient ): diff --git a/projects/policyengine-simulation-gateway/uv.lock b/projects/policyengine-simulation-gateway/uv.lock index 082898381..8e709799d 100644 --- a/projects/policyengine-simulation-gateway/uv.lock +++ b/projects/policyengine-simulation-gateway/uv.lock @@ -1304,7 +1304,7 @@ requires-dist = [ { name = "modal", marker = "extra == 'test'", specifier = ">=1.4,<2" }, { name = "packaging", specifier = ">=24" }, { name = "policyengine-simulation-observability", editable = "../../libs/policyengine-simulation-observability" }, - { name = "pydantic", specifier = ">=2.0" }, + { name = "pydantic", specifier = ">=2.12" }, { name = "pyright", marker = "extra == 'build'", specifier = ">=1.1.401" }, { name = "pytest", marker = "extra == 'test'", specifier = ">=8.3.4" }, { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.25.3" },