From 418c9e17cc1d635ce77d5e659997f6162056fe55 Mon Sep 17 00:00:00 2001 From: PavelMakarchuk Date: Mon, 26 Oct 2026 11:34:36 -0400 Subject: [PATCH 1/2] Update the served current report when a PATCH names an outdated report ID GET serves an outdated report ID as the current report for the same simulations and year. PATCH now does the same, so the result a client saves under that ID completes the report it was served, instead of the outdated record. Fixes #4043. Co-Authored-By: Claude Opus 5.5 --- changelog.d/report-patch-outdated-id.fixed.md | 1 + .../services/report_output_service.py | 14 +++- .../services/test_report_output_service.py | 80 +++++++++++++++++++ .../unit/test_simulation_and_report_routes.py | 44 ++++++++++ 4 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 changelog.d/report-patch-outdated-id.fixed.md diff --git a/changelog.d/report-patch-outdated-id.fixed.md b/changelog.d/report-patch-outdated-id.fixed.md new file mode 100644 index 000000000..4e7661852 --- /dev/null +++ b/changelog.d/report-patch-outdated-id.fixed.md @@ -0,0 +1 @@ +`PATCH /{country_id}/report` with a report ID stored under an outdated cache version now updates the current report that `GET /{country_id}/report/{id}` serves for that ID, so a recomputed report no longer stays pending. diff --git a/policyengine_api/services/report_output_service.py b/policyengine_api/services/report_output_service.py index 94362152a..2350f09db 100644 --- a/policyengine_api/services/report_output_service.py +++ b/policyengine_api/services/report_output_service.py @@ -717,6 +717,12 @@ def update_report_output( ) -> ReportOutputView | None: """Apply the given status, output or error message and sync the run history. + An ID stored under an outdated cache version updates the current report for + the same simulations and year, the one ``resolve_report_output`` serves for + that ID, and the view keeps the requested ID as ``response_id``. Otherwise + a client that saves the result it was served under the old ID would update + the outdated record, and the served report would stay pending. + Return None when nothing was given. Raise LookupError if the report does not exist, and ValueError when marking it running without an active run or completing a US economy report whose SPM provenance is invalid. @@ -741,13 +747,19 @@ def update_report_output( if report_output is None: msg = f"Report output #{report_id} not found" raise LookupError(msg) + response_id = None + if not self._is_current_report_output(report_output): + report_output = self._get_or_create_current_report_output( + session, report_output + ) + response_id = report_id self._update_report_output( session, report_output, values, requested_status=status, ) - return self._build_view(session, report_output) + return self._build_view(session, report_output, response_id=response_id) def _update_report_output( self, diff --git a/tests/unit/services/test_report_output_service.py b/tests/unit/services/test_report_output_service.py index ae0d85d8a..ee5b859bd 100644 --- a/tests/unit/services/test_report_output_service.py +++ b/tests/unit/services/test_report_output_service.py @@ -486,3 +486,83 @@ def test_noop_and_missing_updates(service, orm_session_factory): assert service.update_report_output("us", report.id) is None with pytest.raises(LookupError, match="Report output #999 not found"): service.update_report_output("us", 999, status="pending") + + +def create_outdated_uk_report(orm_session_factory, monkeypatch) -> int: + """Save a UK economy report under an older report-output cache version.""" + baseline = create_simulation( + orm_session_factory, + country_id="uk", + policy_id=1, + population_id="uk", + population_type="geography", + ) + reform = create_simulation( + orm_session_factory, + country_id="uk", + policy_id=2, + population_id="uk", + population_type="geography", + ) + monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 2) + outdated_cache_version = get_report_output_cache_version("uk") + monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 3) + with orm_session_factory.begin() as session: + outdated = ReportOutput( + country_id="uk", + simulation_1_id=baseline.id, + simulation_2_id=reform.id, + api_version=outdated_cache_version, + status="complete", + output={"outdated": True}, + year="2027", + ) + session.add(outdated) + session.flush() + return outdated.id + + +def test_update_with_outdated_id_completes_the_served_current_report( + service, + orm_session_factory, + monkeypatch, +): + outdated_id = create_outdated_uk_report(orm_session_factory, monkeypatch) + served = service.resolve_report_output("uk", outdated_id) + current_id = served.report_output.id + + view = service.update_report_output( + "uk", outdated_id, status="complete", output={"current": True} + ) + + assert view.response_id == outdated_id + assert view.report_output.id == current_id + assert view.report_output.status == "complete" + reopened = service.resolve_report_output("uk", outdated_id) + assert reopened.report_output.id == current_id + assert reopened.report_output.status == "complete" + assert reopened.report_output.output == {"current": True} + assert reopened.report_output.active_run_id is None + assert reopened.report_output.latest_successful_run_id is not None + with orm_session_factory() as session: + outdated = session.get(ReportOutput, outdated_id) + assert outdated.output == {"outdated": True} + + +def test_update_with_outdated_id_creates_the_current_report_when_none_was_served( + service, + orm_session_factory, + monkeypatch, +): + outdated_id = create_outdated_uk_report(orm_session_factory, monkeypatch) + + view = service.update_report_output( + "uk", outdated_id, status="complete", output={"current": True} + ) + + assert view.response_id == outdated_id + assert view.report_output.id != outdated_id + assert view.report_output.api_version == get_report_output_cache_version("uk") + assert service.resolve_report_output("uk", outdated_id).report_output.output == { + "current": True + } diff --git a/tests/unit/test_simulation_and_report_routes.py b/tests/unit/test_simulation_and_report_routes.py index 9962d124b..f2dd63660 100644 --- a/tests/unit/test_simulation_and_report_routes.py +++ b/tests/unit/test_simulation_and_report_routes.py @@ -354,3 +354,47 @@ def test_simulation_v1_routes_keep_json_fields_as_strings(orm_session_factory): simulation = session.get(Simulation, simulation_id) assert simulation.output == output assert isinstance(simulation.simulation_spec_json, dict) + + +def test_report_patch_with_outdated_id_completes_the_report_get_serves( + orm_session_factory, monkeypatch +): + from policyengine_api import constants + + simulation_ids = [ + SimulationService(orm_session_factory) + .get_or_create_simulation("uk", "uk", "geography", policy_id) + .simulation.id + for policy_id in (1, 2) + ] + monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 2) + outdated_cache_version = get_report_output_cache_version("uk") + monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 3) + with orm_session_factory.begin() as session: + outdated = ReportOutput( + country_id="uk", + simulation_1_id=simulation_ids[0], + simulation_2_id=simulation_ids[1], + api_version=outdated_cache_version, + status="complete", + output={"outdated": True}, + year="2027", + ) + session.add(outdated) + session.flush() + outdated_id = outdated.id + client = create_test_client() + served = client.get(f"/uk/report/{outdated_id}").get_json()["result"] + assert served["status"] == "pending" + + response = client.patch( + "/uk/report", + json={"id": outdated_id, "status": "complete", "output": {"current": True}}, + ) + + assert response.status_code == 200 + assert response.get_json()["result"]["id"] == outdated_id + reopened = client.get(f"/uk/report/{outdated_id}").get_json()["result"] + assert reopened["id"] == outdated_id + assert reopened["status"] == "complete" + assert json.loads(reopened["output"]) == {"current": True} From 9ac4902971de7da8ebfe0304b36abdf2bf4f4c25 Mon Sep 17 00:00:00 2001 From: PavelMakarchuk Date: Tue, 27 Oct 2026 08:36:27 -0400 Subject: [PATCH 2/2] Address review: record ID, docs, error and US cases, shared setup - Take response_id from the outdated record's own id, so a PATCH that sends the ID as a string still returns the stored integer. - Document the new ValueError (an outdated record whose simulation no longer exists) and the outdated-ID behaviour in the PATCH and GET route docstrings and OpenAPI descriptions. - Test status="error" on an outdated ID, and a US economy report on an outdated ID, with valid and missing SPM provenance. - Move the outdated-record setup to one helper, insert_outdated_report_output, used by all three tests that need it. Co-Authored-By: Claude Opus 5.5 --- policyengine_api/openapi_spec.yaml | 9 +- .../routes/report_output_routes.py | 6 + .../services/report_output_service.py | 13 +- .../services/report_output_fixtures.py | 29 ++++ .../services/test_report_output_service.py | 156 ++++++++++++------ .../unit/test_simulation_and_report_routes.py | 61 +++++-- 6 files changed, 194 insertions(+), 80 deletions(-) create mode 100644 tests/fixtures/services/report_output_fixtures.py diff --git a/policyengine_api/openapi_spec.yaml b/policyengine_api/openapi_spec.yaml index 6c2f5672d..31940fd8d 100644 --- a/policyengine_api/openapi_spec.yaml +++ b/policyengine_api/openapi_spec.yaml @@ -757,7 +757,9 @@ paths: summary: Update a report output operationId: update_report_output description: >- - Update a report output status, result, or error message. Completed US + Update a report output status, result, or error message. An ID stored + under an outdated cache version updates the current report that GET + serves for that ID, and the response keeps the requested ID. Completed US economy outputs require compact v2 comparison provenance. Each receipt contains the complete resolved configuration; a sibling spm_config is rejected. parameters: @@ -813,7 +815,10 @@ paths: get: summary: Get a report output operationId: get_report_output - description: Get a report output by ID. Timestamp fields are projected from the selected base report run. + description: >- + Get a report output by ID. An ID stored under an outdated cache version + serves the current report for the same simulations and year, under the + requested ID. Timestamp fields are projected from the selected base report run. parameters: - name: country_id in: path diff --git a/policyengine_api/routes/report_output_routes.py b/policyengine_api/routes/report_output_routes.py index 98cefed86..026e9c724 100644 --- a/policyengine_api/routes/report_output_routes.py +++ b/policyengine_api/routes/report_output_routes.py @@ -139,6 +139,9 @@ def create_report_output(country_id: str) -> Response: def get_report_output(country_id: str, report_id: int) -> Response: """Get a report output record by ID. + An ID stored under an outdated cache version serves the current report for + the same simulations and year, under the requested ID. + The response result may include requested_at, started_at, and finished_at values projected from the selected report_output_runs row. Those fields are base report execution metadata, not user-specific user-report association @@ -173,6 +176,9 @@ def get_report_output(country_id: str, report_id: int) -> Response: def update_report_output(country_id: str) -> Response: """Update a report output record with results or error. + An ID stored under an outdated cache version updates the current report that + GET serves for that ID, and the response keeps the requested ID. + Args: country_id (str): The country ID. diff --git a/policyengine_api/services/report_output_service.py b/policyengine_api/services/report_output_service.py index 2350f09db..515691849 100644 --- a/policyengine_api/services/report_output_service.py +++ b/policyengine_api/services/report_output_service.py @@ -719,13 +719,14 @@ def update_report_output( An ID stored under an outdated cache version updates the current report for the same simulations and year, the one ``resolve_report_output`` serves for - that ID, and the view keeps the requested ID as ``response_id``. Otherwise - a client that saves the result it was served under the old ID would update - the outdated record, and the served report would stay pending. + that ID, and the view keeps the outdated record's ID as ``response_id``. + Otherwise a client that saves the result it was served under the old ID + would update the outdated record, and the served report would stay pending. Return None when nothing was given. Raise LookupError if the report does not - exist, and ValueError when marking it running without an active run or - completing a US economy report whose SPM provenance is invalid. + exist, and ValueError when marking it running without an active run, + completing a US economy report whose SPM provenance is invalid, or when an + outdated record names a simulation that no longer exists. """ values = { key: value @@ -749,10 +750,10 @@ def update_report_output( raise LookupError(msg) response_id = None if not self._is_current_report_output(report_output): + response_id = report_output.id report_output = self._get_or_create_current_report_output( session, report_output ) - response_id = report_id self._update_report_output( session, report_output, diff --git a/tests/fixtures/services/report_output_fixtures.py b/tests/fixtures/services/report_output_fixtures.py new file mode 100644 index 000000000..bd3bf921b --- /dev/null +++ b/tests/fixtures/services/report_output_fixtures.py @@ -0,0 +1,29 @@ +from policyengine_api import constants +from policyengine_api.constants import get_report_output_cache_version +from policyengine_api.data.v1_models import ReportOutput + + +def insert_outdated_report_output( + orm_session_factory, monkeypatch, country_id: str, **fields +) -> int: + """Save a report output under the previous report-output cache version. + + The cache version is computed with the ``report_output`` schema version set + to 2, and the schema version is set back to 3 before the record is saved, + so the record reads as outdated against the current cache version. + ``fields`` are the record's other columns, e.g. its simulation IDs, year + and output; its status is "complete" unless given. + """ + monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 2) + outdated_cache_version = get_report_output_cache_version(country_id) + monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 3) + assert outdated_cache_version != get_report_output_cache_version(country_id) + with orm_session_factory.begin() as session: + outdated = ReportOutput( + country_id=country_id, + api_version=outdated_cache_version, + **{"status": "complete", **fields}, + ) + session.add(outdated) + session.flush() + return outdated.id diff --git a/tests/unit/services/test_report_output_service.py b/tests/unit/services/test_report_output_service.py index ee5b859bd..c6a2e3bb1 100644 --- a/tests/unit/services/test_report_output_service.py +++ b/tests/unit/services/test_report_output_service.py @@ -1,7 +1,6 @@ import pytest from sqlalchemy import func, select -from policyengine_api import constants from policyengine_api.constants import get_report_output_cache_version from policyengine_api.data.v1_models import ReportOutput, ReportOutputRun from policyengine_api.services.report_output_service import ReportOutputService @@ -10,6 +9,7 @@ ReportRunService, ) from policyengine_api.services.simulation_service import SimulationService +from tests.fixtures.services.report_output_fixtures import insert_outdated_report_output run_service = ReportRunService() SPM_CONFIG = { @@ -385,29 +385,21 @@ def test_resolve_legacy_spm_report_returns_new_empty_cache_record( population_id="us", population_type="geography", ) - monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 2) - legacy_cache_version = get_report_output_cache_version("us") - monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 3) - assert legacy_cache_version != get_report_output_cache_version("us") - with orm_session_factory.begin() as session: - legacy = ReportOutput( - country_id="us", - simulation_1_id=baseline.id, - simulation_2_id=reform.id, - api_version=legacy_cache_version, - status="complete", - output={ - "spm_config": SPM_CONFIG, - "spm_provenance": { - "baseline": [SPM_RECEIPT], - "reform": [SPM_RECEIPT], - }, + legacy_id = insert_outdated_report_output( + orm_session_factory, + monkeypatch, + country_id="us", + simulation_1_id=baseline.id, + simulation_2_id=reform.id, + year="2025", + output={ + "spm_config": SPM_CONFIG, + "spm_provenance": { + "baseline": [SPM_RECEIPT], + "reform": [SPM_RECEIPT], }, - year="2025", - ) - session.add(legacy) - session.flush() - legacy_id = legacy.id + }, + ) view = service.resolve_report_output("us", legacy_id) @@ -488,38 +480,29 @@ def test_noop_and_missing_updates(service, orm_session_factory): service.update_report_output("us", 999, status="pending") -def create_outdated_uk_report(orm_session_factory, monkeypatch) -> int: - """Save a UK economy report under an older report-output cache version.""" - baseline = create_simulation( - orm_session_factory, - country_id="uk", - policy_id=1, - population_id="uk", - population_type="geography", +def create_outdated_economy_report( + orm_session_factory, monkeypatch, country_id="uk", year="2027" +) -> int: + """Save a nationwide economy report under an outdated cache version.""" + baseline, reform = ( + create_simulation( + orm_session_factory, + country_id=country_id, + policy_id=policy_id, + population_id=country_id, + population_type="geography", + ) + for policy_id in (1, 2) ) - reform = create_simulation( + return insert_outdated_report_output( orm_session_factory, - country_id="uk", - policy_id=2, - population_id="uk", - population_type="geography", + monkeypatch, + country_id=country_id, + simulation_1_id=baseline.id, + simulation_2_id=reform.id, + year=year, + output={"outdated": True}, ) - monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 2) - outdated_cache_version = get_report_output_cache_version("uk") - monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 3) - with orm_session_factory.begin() as session: - outdated = ReportOutput( - country_id="uk", - simulation_1_id=baseline.id, - simulation_2_id=reform.id, - api_version=outdated_cache_version, - status="complete", - output={"outdated": True}, - year="2027", - ) - session.add(outdated) - session.flush() - return outdated.id def test_update_with_outdated_id_completes_the_served_current_report( @@ -527,7 +510,7 @@ def test_update_with_outdated_id_completes_the_served_current_report( orm_session_factory, monkeypatch, ): - outdated_id = create_outdated_uk_report(orm_session_factory, monkeypatch) + outdated_id = create_outdated_economy_report(orm_session_factory, monkeypatch) served = service.resolve_report_output("uk", outdated_id) current_id = served.report_output.id @@ -554,7 +537,7 @@ def test_update_with_outdated_id_creates_the_current_report_when_none_was_served orm_session_factory, monkeypatch, ): - outdated_id = create_outdated_uk_report(orm_session_factory, monkeypatch) + outdated_id = create_outdated_economy_report(orm_session_factory, monkeypatch) view = service.update_report_output( "uk", outdated_id, status="complete", output={"current": True} @@ -566,3 +549,68 @@ def test_update_with_outdated_id_creates_the_current_report_when_none_was_served assert service.resolve_report_output("uk", outdated_id).report_output.output == { "current": True } + + +def test_update_with_outdated_id_records_an_error_on_the_served_current_report( + service, + orm_session_factory, + monkeypatch, +): + outdated_id = create_outdated_economy_report(orm_session_factory, monkeypatch) + current_id = service.resolve_report_output("uk", outdated_id).report_output.id + + view = service.update_report_output( + "uk", outdated_id, status="error", error_message="The run failed" + ) + + assert view.response_id == outdated_id + assert view.report_output.id == current_id + reopened = service.resolve_report_output("uk", outdated_id).report_output + assert reopened.id == current_id + assert reopened.status == "error" + assert reopened.error_message == "The run failed" + with orm_session_factory() as session: + outdated = session.get(ReportOutput, outdated_id) + assert outdated.status == "complete" + assert outdated.error_message is None + + +def test_update_with_outdated_id_completes_a_us_economy_report_with_valid_provenance( + service, + orm_session_factory, + monkeypatch, +): + outdated_id = create_outdated_economy_report( + orm_session_factory, monkeypatch, country_id="us", year="2025" + ) + current_id = service.resolve_report_output("us", outdated_id).report_output.id + + view = service.update_report_output( + "us", outdated_id, status="complete", output=compact_economy_output() + ) + + assert view.response_id == outdated_id + assert view.report_output.id == current_id + assert view.report_output.output == compact_economy_output() + assert view.display_run.output == compact_economy_output() + + +def test_update_with_outdated_id_checks_spm_provenance_on_the_current_report( + service, + orm_session_factory, + monkeypatch, +): + outdated_id = create_outdated_economy_report( + orm_session_factory, monkeypatch, country_id="us", year="2025" + ) + current_id = service.resolve_report_output("us", outdated_id).report_output.id + + with pytest.raises(ValueError, match="requires SPM provenance"): + service.update_report_output( + "us", outdated_id, status="complete", output={"budgetary_impact": 42} + ) + + with orm_session_factory() as session: + current = session.get(ReportOutput, current_id) + assert current.status == "pending" + assert current.output is None diff --git a/tests/unit/test_simulation_and_report_routes.py b/tests/unit/test_simulation_and_report_routes.py index f2dd63660..db4548dff 100644 --- a/tests/unit/test_simulation_and_report_routes.py +++ b/tests/unit/test_simulation_and_report_routes.py @@ -15,6 +15,7 @@ ReportRunService, ) from policyengine_api.services.simulation_service import SimulationService +from tests.fixtures.services.report_output_fixtures import insert_outdated_report_output simulation_service = SimulationService() report_service = ReportOutputService() @@ -359,30 +360,21 @@ def test_simulation_v1_routes_keep_json_fields_as_strings(orm_session_factory): def test_report_patch_with_outdated_id_completes_the_report_get_serves( orm_session_factory, monkeypatch ): - from policyengine_api import constants - simulation_ids = [ SimulationService(orm_session_factory) .get_or_create_simulation("uk", "uk", "geography", policy_id) .simulation.id for policy_id in (1, 2) ] - monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 2) - outdated_cache_version = get_report_output_cache_version("uk") - monkeypatch.setitem(constants.RUNTIME_CACHE_SCHEMA_VERSIONS, "report_output", 3) - with orm_session_factory.begin() as session: - outdated = ReportOutput( - country_id="uk", - simulation_1_id=simulation_ids[0], - simulation_2_id=simulation_ids[1], - api_version=outdated_cache_version, - status="complete", - output={"outdated": True}, - year="2027", - ) - session.add(outdated) - session.flush() - outdated_id = outdated.id + outdated_id = insert_outdated_report_output( + orm_session_factory, + monkeypatch, + country_id="uk", + simulation_1_id=simulation_ids[0], + simulation_2_id=simulation_ids[1], + year="2027", + output={"outdated": True}, + ) client = create_test_client() served = client.get(f"/uk/report/{outdated_id}").get_json()["result"] assert served["status"] == "pending" @@ -398,3 +390,36 @@ def test_report_patch_with_outdated_id_completes_the_report_get_serves( assert reopened["id"] == outdated_id assert reopened["status"] == "complete" assert json.loads(reopened["output"]) == {"current": True} + + +def test_report_patch_with_outdated_id_as_a_string_returns_the_integer_id( + orm_session_factory, monkeypatch +): + simulation_ids = [ + SimulationService(orm_session_factory) + .get_or_create_simulation("uk", "uk", "geography", policy_id) + .simulation.id + for policy_id in (1, 2) + ] + outdated_id = insert_outdated_report_output( + orm_session_factory, + monkeypatch, + country_id="uk", + simulation_1_id=simulation_ids[0], + simulation_2_id=simulation_ids[1], + year="2027", + output={"outdated": True}, + ) + client = create_test_client() + + response = client.patch( + "/uk/report", + json={ + "id": str(outdated_id), + "status": "complete", + "output": {"current": True}, + }, + ) + + assert response.status_code == 200 + assert response.get_json()["result"]["id"] == outdated_id