diff --git a/changelog.d/uk-breakdown-withheld-codes.added.md b/changelog.d/uk-breakdown-withheld-codes.added.md new file mode 100644 index 000000000..fabb580f2 --- /dev/null +++ b/changelog.d/uk-breakdown-withheld-codes.added.md @@ -0,0 +1 @@ +A `breakdown=local_areas` result of `GET /{country_id}/economy/{policy_id}/over/{baseline_policy_id}` carries `constituency_withheld_codes` and `local_authority_withheld_codes` unchanged from the simulation API, and the SPM guide documents them: the sorted ONS codes of each level's areas that the run estimated but disclosure control withheld, `[]` when none were, and null or absent when the result doesn't report them. diff --git a/changelog.d/uk-breakdown-withheld-codes.fixed.md b/changelog.d/uk-breakdown-withheld-codes.fixed.md new file mode 100644 index 000000000..50d33fa24 --- /dev/null +++ b/changelog.d/uk-breakdown-withheld-codes.fixed.md @@ -0,0 +1 @@ +UK `breakdown=local_areas` results cached before the simulation API listed each level's withheld area codes (`constituency_withheld_codes` and `local_authority_withheld_codes`) are no longer served: a breakdown request's options hash now includes `breakdown_schema=2`, so the next request computes the breakdown again. Hashes and cached results of requests without a breakdown are unchanged. diff --git a/docs/canonical-spm.md b/docs/canonical-spm.md index 49700344f..10dcb4e23 100644 --- a/docs/canonical-spm.md +++ b/docs/canonical-spm.md @@ -343,7 +343,19 @@ accepted only for the UK, with `region` exactly `uk` or one UK country requests, including other spellings of those regions, return HTTP 400 before anything is read from the cache or submitted. The API sends `"breakdown": "local_areas"` to the simulation API, which computes the impacts in a run separate from the national result, and -returns its result (`kind: "local_areas"`) like any other economy result. If +returns its result (`kind: "local_areas"`) like any other economy result. +Besides `constituency_impact` and `local_authority_impact`, the result has +`constituency_withheld_codes` and `local_authority_withheld_codes`: the ONS +codes, sorted, of each level's areas that the run estimated but disclosure +control withheld, which the impact lists leave out. `[]` means none were +withheld. `null` or an absent field means the result doesn't report them, as +in results computed before the simulation API reported them. An area the run +never estimated, such as one with no households in the dataset, is not listed. +The API stores and returns these fields unchanged. A breakdown's options hash +also includes `breakdown_schema=2`, the version of the breakdown result, so a +breakdown cached before the result reported withheld codes is not returned and +the next request computes it again; hashes of requests without `breakdown` +don't include it. If the simulation API refuses the request with HTTP 400 or 422, because the deployed simulation API or the .py release it routes to cannot run breakdowns, the route returns HTTP 502 with the simulation API's reason and stores diff --git a/policyengine_api/services/economy_impact_types.py b/policyengine_api/services/economy_impact_types.py index 18ee3fe13..c93709011 100644 --- a/policyengine_api/services/economy_impact_types.py +++ b/policyengine_api/services/economy_impact_types.py @@ -61,6 +61,17 @@ class ImpactStatus(Enum): # telemetry envelope forbids extra fields, so the run is marked here rather # than by a new field; geography_type and geography_code still name its region. LOCAL_AREA_BREAKDOWN_SIMULATION_KIND = "local_area_breakdown" +# The version of the local-area breakdown result that a breakdown request's +# options hash names, as breakdown_schema. A breakdown result cached under an +# earlier version, or before the hash named one, is not served, and the next +# request computes it again. Requests without a breakdown have no such field, +# so their hashes and cache entries don't change. Raise it when the simulation +# API's local-area result gains or changes fields, since a simulation API +# deploy that keeps the same .py version doesn't change any other part of the +# cache identity. +# 2: each level lists the ONS codes of the areas the run estimated but +# disclosure control withheld (PolicyEngine/policyengine-sim-api#968). +LOCAL_AREA_BREAKDOWN_RESULT_SCHEMA_VERSION = 2 BUDGET_WINDOW_CLAIM_ATTEMPTS = 3 REFORM_IMPACT_START_CLAIM_ATTEMPTS = 3 # A budget-window progress message names this many computing years and counts diff --git a/policyengine_api/services/economy_stored_impacts.py b/policyengine_api/services/economy_stored_impacts.py index 0ec81fb75..5473d7629 100644 --- a/policyengine_api/services/economy_stored_impacts.py +++ b/policyengine_api/services/economy_stored_impacts.py @@ -13,6 +13,7 @@ ReformImpactStartClaim, ) from policyengine_api.services.economy_impact_types import ( + LOCAL_AREA_BREAKDOWN_RESULT_SCHEMA_VERSION, EconomicImpactSetupOptions, ImpactAction, ImpactStatus, @@ -234,6 +235,10 @@ def _build_options_hash( bundle_parts.append(f"policyengine_version={bundle.policyengine_version}") if options.get("spm") is not None: bundle_parts.append("spm_provenance_schema=canonical-spm-comparison-v2") + if options.get("breakdown") is not None: + bundle_parts.append( + f"breakdown_schema={LOCAL_AREA_BREAKDOWN_RESULT_SCHEMA_VERSION}" + ) if bundle.runtime_app_name: bundle_parts.append(f"runtime_app_name={bundle.runtime_app_name}") return "[" + "&".join([option_pairs, *bundle_parts]).strip("&") + "]" diff --git a/tests/contract/test_simulation_gateway_contract.py b/tests/contract/test_simulation_gateway_contract.py index 217037170..8126e5902 100644 --- a/tests/contract/test_simulation_gateway_contract.py +++ b/tests/contract/test_simulation_gateway_contract.py @@ -16,6 +16,7 @@ MOCK_BATCH_SUBMIT_RESPONSE_SUCCESS, MOCK_HEALTH_RESPONSE, MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_WITHHELD_CODES, MOCK_MODAL_JOB_ID, MOCK_POLL_RESPONSE_COMPLETE, MOCK_RESOLVED_APP_NAME, @@ -203,9 +204,14 @@ def _uk_submission(options: dict) -> dict: "local_areas", MOCK_LOCAL_AREA_BREAKDOWN_RESULT, ), + ( + {"breakdown": "local_areas"}, + "local_areas", + MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_WITHHELD_CODES, + ), ({}, None, MOCK_POLL_RESPONSE_COMPLETE["result"]), ], - ids=["local-area-breakdown", "national"], + ids=["local-area-breakdown", "local-area-breakdown-withheld-codes", "national"], ) def test_gateway_uk_comparison_sends_breakdown_only_when_requested( monkeypatch, options, expected_breakdown, result diff --git a/tests/fixtures/libs/simulation_entrypoint.py b/tests/fixtures/libs/simulation_entrypoint.py index 11a6c4d50..fa1d8cf21 100644 --- a/tests/fixtures/libs/simulation_entrypoint.py +++ b/tests/fixtures/libs/simulation_entrypoint.py @@ -83,6 +83,20 @@ } ], } +# The same result from a simulation API that lists, for each level, the ONS +# codes of the areas the run estimated but disclosure control withheld, sorted +# (PolicyEngine/policyengine-sim-api#968). An empty list means none were. +MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_WITHHELD_CODES = { + **MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + "constituency_withheld_codes": ["E14001172", "S14000021"], + "local_authority_withheld_codes": [], +} +# A result that doesn't report its withheld areas, with null for each level. +MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_NULL_WITHHELD_CODES = { + **MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + "constituency_withheld_codes": None, + "local_authority_withheld_codes": None, +} MOCK_POLICYENGINE_BUNDLE = { "model_version": "1.459.0", "policyengine_version": "3.4.0", diff --git a/tests/unit/routes/test_economy_spm_query.py b/tests/unit/routes/test_economy_spm_query.py index 9a57270ab..d4aa17c90 100644 --- a/tests/unit/routes/test_economy_spm_query.py +++ b/tests/unit/routes/test_economy_spm_query.py @@ -17,7 +17,11 @@ from policyengine_api.services.budget_window_cache import BudgetWindowCache from policyengine_api.services.economy_service import EconomyService from policyengine_api.services.reform_impacts_service import ReformImpactsService -from tests.fixtures.libs.simulation_entrypoint import MOCK_LOCAL_AREA_BREAKDOWN_RESULT +from tests.fixtures.libs.simulation_entrypoint import ( + MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_NULL_WITHHELD_CODES, + MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_WITHHELD_CODES, +) @pytest.fixture(params=[False, True], ids=["annual", "budget-window"]) @@ -542,20 +546,32 @@ def test_annual_route_returns_502_when_the_simulation_api_refuses_a_breakdown( assert gateway.run.call_args.args[0]["breakdown"] == "local_areas" +@pytest.mark.parametrize( + "simulation_result", + [ + MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_WITHHELD_CODES, + MOCK_LOCAL_AREA_BREAKDOWN_RESULT_WITH_NULL_WITHHELD_CODES, + MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + ], + ids=["withheld-codes", "null-withheld-codes", "no-withheld-codes"], +) def test_annual_route_stores_and_returns_a_completed_breakdown_unchanged( - economy_service_http, + economy_service_http, simulation_result ): """A finished breakdown job is polled once, stored, and then read back. The second request polls the job (``_handle_execution_state``); the third reads the stored record (``_handle_completed_impact``) without polling. + The withheld area codes come back as the simulation API sent them, and a + result without them, from a simulation API that doesn't report them, + comes back without them. """ client, gateway = economy_service_http breakdown_query = [*UK_ANNUAL_QUERY, ("breakdown", "local_areas")] gateway.get_execution_by_id.return_value = ModalSimulationExecution( job_id="submitted-job", status="complete", - result=MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + result=simulation_result, resolved_app_name="policyengine-simulation-test", ) gateway.get_execution_status.side_effect = lambda execution: execution.status @@ -571,13 +587,11 @@ def test_annual_route_stores_and_returns_a_completed_breakdown_unchanged( assert response.status_code == 200, response.get_json() body = response.get_json() assert body["status"] == "ok" - result = body["result"] + returned = body["result"] # The breakdown's own fields come back unchanged; the service adds # only the bundle identity it adds to every economy result. - assert { - key: result[key] for key in MOCK_LOCAL_AREA_BREAKDOWN_RESULT - } == MOCK_LOCAL_AREA_BREAKDOWN_RESULT - assert set(result) - set(MOCK_LOCAL_AREA_BREAKDOWN_RESULT) == { + assert {key: returned[key] for key in simulation_result} == simulation_result + assert set(returned) - set(simulation_result) == { "policyengine_bundle", "resolved_app_name", } diff --git a/tests/unit/services/test_economy_service_request_plumbing.py b/tests/unit/services/test_economy_service_request_plumbing.py index c9d14cc45..575798149 100644 --- a/tests/unit/services/test_economy_service_request_plumbing.py +++ b/tests/unit/services/test_economy_service_request_plumbing.py @@ -7,7 +7,7 @@ import json from dataclasses import replace -from datetime import timedelta +from datetime import datetime, timedelta from unittest.mock import patch import pytest @@ -32,7 +32,10 @@ ImpactStatus, RuntimeBundleIdentity, ) -from policyengine_api.services.reform_impacts_service import ReformImpactsService +from policyengine_api.services.reform_impacts_service import ( + ReformImpactExecution, + ReformImpactsService, +) from tests.fixtures.libs.simulation_entrypoint import MOCK_LOCAL_AREA_BREAKDOWN_RESULT from tests.fixtures.services.economy_service import ( MOCK_API_VERSION, @@ -412,7 +415,16 @@ def test_budget_window_poll_reports_running_years( f"&policyengine_version={MOCK_POLICYENGINE_VERSION}" ) UK_NATIONAL_LOOKUP_HASH = f"[{UK_BUNDLE_HASH_FIELDS}]" -UK_BREAKDOWN_LOOKUP_HASH = f"[breakdown=local_areas&{UK_BUNDLE_HASH_FIELDS}]" +# A breakdown's hash names the version of the breakdown result it holds, so +# breakdowns cached before the result listed its withheld area codes are not +# served. Hashes without a breakdown have no such field and keep their values. +UK_BREAKDOWN_LOOKUP_HASH = ( + f"[breakdown=local_areas&{UK_BUNDLE_HASH_FIELDS}&breakdown_schema=2]" +) +# The hash a breakdown had before that version was added. +UK_BREAKDOWN_LOOKUP_HASH_BEFORE_WITHHELD_CODES = ( + f"[breakdown=local_areas&{UK_BUNDLE_HASH_FIELDS}]" +) def _stored_options_hash(lookup_hash: str) -> str: @@ -573,6 +585,139 @@ def test_national_and_breakdown_hashes_never_match_each_others_lookup( assert not _like_matches(_stored_options_hash(other_hash), pattern) +# The hashes a request without a breakdown stored before breakdowns had a +# result version, written out so that any change to them shows here. +UK_STORED_HASH_WITHOUT_BREAKDOWN = ( + "[dataset=default&model_version=1.2.3&policyengine_version=3.4.0" + "&runtime_app_name=policyengine-simulation-us1-2-3-uk2-7-8]" +) + + +@pytest.mark.usefixtures("uk_package_versions") +@pytest.mark.parametrize( + ("country_id", "region", "options", "stored_hash"), + [ + ("uk", "uk", {}, UK_STORED_HASH_WITHOUT_BREAKDOWN), + ("uk", "country/scotland", {}, UK_STORED_HASH_WITHOUT_BREAKDOWN), + ("uk", "country/northern_ireland", {}, UK_STORED_HASH_WITHOUT_BREAKDOWN), + ("us", "us", MOCK_OPTIONS, MOCK_OPTIONS_HASH), + ("us", "ca", MOCK_OPTIONS, MOCK_OPTIONS_HASH), + ], + ids=[ + "uk-national", + "uk-nation-scotland", + "uk-nation-northern-ireland", + "us-national", + "us-state", + ], +) +def test_requests_without_a_breakdown_keep_their_stored_hashes( + economy_service, country_id, region, options, stored_hash +): + economy_service.get_economic_impact( + EconomicImpactRequest( + country_id=country_id, + policy_id=MOCK_POLICY_ID, + baseline_policy_id=MOCK_BASELINE_POLICY_ID, + region=region, + dataset="default", + options=options, + api_version=MOCK_API_VERSION, + ), + time_period="2025", + ) + + stored = economy_service._reform_impacts._cache.recent(5) + assert [impact.options_hash for impact in stored] == [stored_hash] + + +def _store_completed_uk_impact( + service: EconomyService, + *, + options: dict, + options_hash: str, + job_id: str, + result: dict, +) -> None: + """Store a finished impact for the UK in 2026 under this options hash.""" + service._reform_impacts.set_reform_impact( + identity=ReformImpactIdentity( + scope=ReformImpactScope( + country_id="uk", + reform_policy_id=MOCK_POLICY_ID, + baseline_policy_id=MOCK_BASELINE_POLICY_ID, + region="uk", + dataset="default", + time_period="2026", + ), + api_version=CACHE_VERSION, + options_hash=options_hash, + ), + execution=ReformImpactExecution( + execution_id=job_id, + start_time=datetime(2026, 10, 1, 12, 0), + observability_id=MOCK_OBSERVABILITY_ID, + ), + options=options, + status="computing", + reform_impact_json={}, + ) + service._reform_impacts.set_complete_reform_impact( + execution_id=job_id, + reform_impact_json={**result, "resolved_app_name": MOCK_RESOLVED_APP_NAME}, + ) + + +UK_NATIONAL_RESULT = {"budget": {"budgetary_impact": 1.0}} + + +@pytest.mark.usefixtures("uk_package_versions") +def test_a_breakdown_cached_before_the_withheld_codes_is_recomputed_but_not_national( + economy_service, mock_simulation_entrypoint +): + _store_completed_uk_impact( + economy_service, + options={"breakdown": "local_areas"}, + options_hash=_stored_options_hash( + UK_BREAKDOWN_LOOKUP_HASH_BEFORE_WITHHELD_CODES + ), + job_id="breakdown-job-before-withheld-codes", + result=MOCK_LOCAL_AREA_BREAKDOWN_RESULT, + ) + _store_completed_uk_impact( + economy_service, + options={}, + options_hash=_stored_options_hash(UK_NATIONAL_LOOKUP_HASH), + job_id="national-job", + result=UK_NATIONAL_RESULT, + ) + + national = economy_service.get_economic_impact(_uk_request({}), time_period="2026") + breakdown = economy_service.get_economic_impact( + _uk_request({"breakdown": "local_areas"}), time_period="2026" + ) + + # The national result is still read from its cache entry. + assert national.status == ImpactStatus.OK + assert national.data is not None + assert national.data["budget"] == UK_NATIONAL_RESULT["budget"] + # The breakdown stored without the withheld codes is not: a new job runs. + assert breakdown == EconomicImpactResult.computing() + submitted = [ + call.args[0].get("breakdown") + for call in mock_simulation_entrypoint.run.call_args_list + ] + assert submitted == ["local_areas"] + stored = economy_service._reform_impacts._cache.recent(5) + assert {impact.execution_id: impact.options_hash for impact in stored} == { + "breakdown-job-before-withheld-codes": _stored_options_hash( + UK_BREAKDOWN_LOOKUP_HASH_BEFORE_WITHHELD_CODES + ), + "national-job": _stored_options_hash(UK_NATIONAL_LOOKUP_HASH), + MOCK_MODAL_JOB_ID: _stored_options_hash(UK_BREAKDOWN_LOOKUP_HASH), + } + + @pytest.mark.usefixtures("uk_package_versions") def test_a_breakdown_request_never_reuses_the_national_job( economy_service, mock_simulation_entrypoint @@ -988,6 +1133,20 @@ def test_options_hash_marks_spm_provenance_schema(): ) +def test_options_hash_marks_the_breakdown_result_schema(): + options_hash = EconomyService()._build_options_hash( + options={"breakdown": "local_areas"}, + bundle=RuntimeBundleIdentity( + dataset="default", model_version="1", runtime_app_name="app" + ), + ) + + assert options_hash == ( + "[breakdown=local_areas&dataset=default&model_version=1" + "&breakdown_schema=2&runtime_app_name=app]" + ) + + def test_empty_options_hash_drops_the_leading_separator(): options_hash = EconomyService()._build_options_hash( options={}, bundle=RuntimeBundleIdentity(dataset="default", model_version="1")