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/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 94362152a..515691849 100644 --- a/policyengine_api/services/report_output_service.py +++ b/policyengine_api/services/report_output_service.py @@ -717,9 +717,16 @@ 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 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 @@ -741,13 +748,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): + response_id = report_output.id + report_output = self._get_or_create_current_report_output( + session, report_output + ) 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/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 ae0d85d8a..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) @@ -486,3 +478,139 @@ 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_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) + ) + return insert_outdated_report_output( + orm_session_factory, + monkeypatch, + country_id=country_id, + simulation_1_id=baseline.id, + simulation_2_id=reform.id, + year=year, + output={"outdated": True}, + ) + + +def test_update_with_outdated_id_completes_the_served_current_report( + service, + 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 + + 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_economy_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 + } + + +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 9962d124b..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() @@ -354,3 +355,71 @@ 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 +): + 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() + 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} + + +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