From f1e190293b851bca3794c96a90751312f06fd842 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Sat, 18 Jul 2026 19:51:51 +0000 Subject: [PATCH] Address review: stream workbook to /tmp, repo the scenario-name SQL Dan's review on #1546: - Stream the workbook to a /tmp temp file and S3Client.upload_file it, instead of render_workbook -> bytes -> put_object. render_workbook now saves straight to a path; the orchestrator renders to a temp file, multipart-uploads it, and always cleans it up. Restores ADR-0065's "never an in-memory BytesIO" decision (the OOM path at the 100k-row cap). - Move the raw `SELECT ... FROM scenario` out of the Lambda handler into ScenarioNamesPostgresRepository, so the handler stays composition-only and all SQL lives in repositories/. - current_sap_points goes through _float, matching its Optional[float] read-model type and the sibling numeric facts. Co-Authored-By: Claude Opus 4.8 (1M context) --- applications/ara_export/handler.py | 28 +++-------------- .../xlsx/scenario_export_workbook.py | 22 ++++++++----- orchestration/scenario_export_orchestrator.py | 17 ++++++++-- .../scenario_export_postgres_repository.py | 4 ++- .../scenario_names_postgres_repository.py | 31 +++++++++++++++++++ .../xlsx/test_scenario_export_workbook.py | 27 +++++++++------- 6 files changed, 83 insertions(+), 46 deletions(-) create mode 100644 repositories/scenario_export/scenario_names_postgres_repository.py diff --git a/applications/ara_export/handler.py b/applications/ara_export/handler.py index 9e78e2a7b..230441683 100644 --- a/applications/ara_export/handler.py +++ b/applications/ara_export/handler.py @@ -19,8 +19,6 @@ import os from typing import Any import boto3 -from sqlalchemy import text -from sqlmodel import Session from applications.ara_export.ara_export_trigger_body import AraExportTriggerBody from domain.tasks.tasks import Source @@ -32,6 +30,9 @@ from orchestration.scenario_export_orchestrator import ScenarioExportOrchestrato from repositories.scenario_export.scenario_export_postgres_repository import ( ScenarioExportPostgresRepository, ) +from repositories.scenario_export.scenario_names_postgres_repository import ( + ScenarioNamesPostgresRepository, +) from repositories.tasks.subtask_postgres_repository import SubTaskPostgresRepository from utilities.aws_lambda.task_handler import task_handler @@ -42,27 +43,6 @@ logger = logging.getLogger(__name__) _URL_TTL_SECONDS = 3600 -class _ScenarioNames: - """Scenario id → display name, read from the ``scenario`` table (the domain - Scenario aggregate does not carry the name).""" - - def __init__(self, session: Session, scenario_ids: list[int]) -> None: - rows = ( - session.connection() - .execute( - text("SELECT id, name FROM scenario WHERE id = ANY(:ids)"), - {"ids": scenario_ids}, - ) - .all() - ) - self._names: dict[int, str] = { - row[0]: (row[1] or f"scenario_{row[0]}") for row in rows - } - - def name_for(self, scenario_id: int) -> str: - return self._names.get(scenario_id, f"scenario_{scenario_id}") - - def _email_sender() -> SesSmtpEmailSender: return SesSmtpEmailSender( host=os.environ["SES_SMTP_HOST"], @@ -98,7 +78,7 @@ def handler(body: dict[str, Any], context: Any) -> dict[str, Any]: boto_s3: Any = boto3.client("s3") # pyright: ignore[reportUnknownMemberType] orchestrator = ScenarioExportOrchestrator( rows=ScenarioExportPostgresRepository(session), - scenarios=_ScenarioNames(session, scenario_ids), + scenarios=ScenarioNamesPostgresRepository(session), exports=S3Client(boto_s3, os.environ["DOCUMENT_EXPORTS_BUCKET"]), email=_email_sender(), url_ttl_seconds=_URL_TTL_SECONDS, diff --git a/infrastructure/xlsx/scenario_export_workbook.py b/infrastructure/xlsx/scenario_export_workbook.py index 7dbc9bda8..d437ee0ba 100644 --- a/infrastructure/xlsx/scenario_export_workbook.py +++ b/infrastructure/xlsx/scenario_export_workbook.py @@ -9,7 +9,6 @@ dynamic number of scenario sheets is handled natively. from __future__ import annotations import re -from io import BytesIO from typing import Sequence from openpyxl import Workbook @@ -36,10 +35,19 @@ def _safe_sheet_name(name: str, used: set[str]) -> str: return clean -def render_workbook(sheets: Sequence[tuple[str, ExportSheet]]) -> bytes: - """Render the scenario sheets into a single branded ``.xlsx`` as bytes. Each - tuple is a Scenario name and its shaped sheet; sheet order follows the input - (the requested ``scenario_ids`` order).""" +def render_workbook( + sheets: Sequence[tuple[str, ExportSheet]], destination_path: str +) -> None: + """Render the scenario sheets into a single branded ``.xlsx`` at + ``destination_path``. Each tuple is a Scenario name and its shaped sheet; + sheet order follows the input (the requested ``scenario_ids`` order). + + Saves straight to disk (``Workbook.save(path)``) so the caller can + ``S3Client.upload_file`` it with multipart — the workbook is never also held + as an in-memory ``bytes`` copy (ADR-0065; the OOM path the 4 GB sizing was + against at the 100k-row cap). The further memory win is openpyxl + ``write_only`` mode; that switch would live at this seam. + """ workbook = Workbook() default = workbook.active if default is not None: @@ -53,6 +61,4 @@ def render_workbook(sheets: Sequence[tuple[str, ExportSheet]]) -> bytes: worksheet.append([row.get(column, "") for column in sheet.columns]) style_sheet(worksheet) - buffer = BytesIO() - workbook.save(buffer) - return buffer.getvalue() + workbook.save(destination_path) diff --git a/orchestration/scenario_export_orchestrator.py b/orchestration/scenario_export_orchestrator.py index 282751eb4..39d25eccb 100644 --- a/orchestration/scenario_export_orchestrator.py +++ b/orchestration/scenario_export_orchestrator.py @@ -12,6 +12,8 @@ from __future__ import annotations import html import logging +import os +import tempfile from dataclasses import dataclass from typing import Protocol, Sequence @@ -103,9 +105,20 @@ class ScenarioExportOrchestrator: }, ) - data = render_workbook(named_sheets) key = f"{self._key_prefix}/{export_name}.xlsx" - self._exports.put_object(key, data) + # Render to a /tmp file and multipart-upload it, so the workbook is never + # also held in memory as a bytes copy (ADR-0065 — the OOM path at the + # 100k-row cap). The temp file is always cleaned up. + handle, workbook_path = tempfile.mkstemp(suffix=".xlsx") + os.close(handle) + try: + render_workbook(named_sheets, workbook_path) + self._exports.upload_file(workbook_path, key) + finally: + try: + os.remove(workbook_path) + except OSError: + pass url = self._exports.generate_presigned_url(key, self._url_ttl_seconds) subject, body, html_body = self._compose_email( diff --git a/repositories/scenario_export/scenario_export_postgres_repository.py b/repositories/scenario_export/scenario_export_postgres_repository.py index 50e21dd66..c17c220d9 100644 --- a/repositories/scenario_export/scenario_export_postgres_repository.py +++ b/repositories/scenario_export/scenario_export_postgres_repository.py @@ -345,7 +345,9 @@ class ScenarioExportPostgresRepository(ScenarioExportRepository): for row in connection.execute(_EPC_PERF_SQL, source_params).all(): facts.setdefault(row[0], {}).update( { - "current_sap_points": row[1], + # _float to match the read-model's Optional[float] typing and + # the sibling numeric facts (the driver hands back an int). + "current_sap_points": _float(row[1]), "current_epc_rating": _str(row[2]), } ) diff --git a/repositories/scenario_export/scenario_names_postgres_repository.py b/repositories/scenario_export/scenario_names_postgres_repository.py new file mode 100644 index 000000000..690b48023 --- /dev/null +++ b/repositories/scenario_export/scenario_names_postgres_repository.py @@ -0,0 +1,31 @@ +from __future__ import annotations + +from sqlalchemy import text +from sqlmodel import Session + +# Scenario id → display name. The domain Scenario aggregate does not carry the +# name, so it is read from the ``scenario`` table; kept in the repository layer +# with the export's other reads (ADR-0065), so the Lambda handler stays +# composition-only. +_SCENARIO_NAME_SQL = text("SELECT name FROM scenario WHERE id = :scenario_id") + + +class ScenarioNamesPostgresRepository: + """Reads a Scenario's display name from Postgres, satisfying the + orchestrator's ``ScenarioNames`` port. Looked up lazily and cached (an export + names only its handful of requested Scenarios).""" + + def __init__(self, session: Session) -> None: + self._session = session + self._cache: dict[int, str] = {} + + def name_for(self, scenario_id: int) -> str: + if scenario_id not in self._cache: + row = ( + self._session.connection() + .execute(_SCENARIO_NAME_SQL, {"scenario_id": scenario_id}) + .first() + ) + name = row[0] if row is not None else None + self._cache[scenario_id] = name or f"scenario_{scenario_id}" + return self._cache[scenario_id] diff --git a/tests/infrastructure/xlsx/test_scenario_export_workbook.py b/tests/infrastructure/xlsx/test_scenario_export_workbook.py index 81e8a3cf8..a1f9e86e6 100644 --- a/tests/infrastructure/xlsx/test_scenario_export_workbook.py +++ b/tests/infrastructure/xlsx/test_scenario_export_workbook.py @@ -3,7 +3,7 @@ sheet per Scenario, headers then rows, verified by re-opening the bytes.""" from __future__ import annotations -from io import BytesIO +from pathlib import Path from openpyxl import load_workbook @@ -12,7 +12,9 @@ from infrastructure.xlsx.branding import HEADER_FILL from infrastructure.xlsx.scenario_export_workbook import render_workbook -def test_renders_a_sheet_named_for_the_scenario_with_headers_then_rows() -> None: +def test_renders_a_sheet_named_for_the_scenario_with_headers_then_rows( + tmp_path: Path, +) -> None: # arrange — one scenario's shaped sheet. sheet = ExportSheet( columns=("property_id", "loft_insulation"), @@ -20,18 +22,19 @@ def test_renders_a_sheet_named_for_the_scenario_with_headers_then_rows() -> None ) # act - data = render_workbook([("Fabric first", sheet)]) + path = str(tmp_path / "export.xlsx") + render_workbook([("Fabric first", sheet)], path) # assert — a real workbook with a sheet named for the scenario, the column # contract as the header row, and the data beneath it. - workbook = load_workbook(BytesIO(data)) + workbook = load_workbook(path) assert workbook.sheetnames == ["Fabric first"] worksheet = workbook["Fabric first"] assert [cell.value for cell in worksheet[1]] == ["property_id", "loft_insulation"] assert [cell.value for cell in worksheet[2]] == [1, 1200.0] -def test_sanitises_and_deduplicates_scenario_sheet_names() -> None: +def test_sanitises_and_deduplicates_scenario_sheet_names(tmp_path: Path) -> None: # arrange — a name over Excel's 31-char limit (used twice, so it must be # deduplicated) and one with characters Excel forbids in a sheet title. empty = ExportSheet(columns=("property_id",), rows=()) @@ -39,19 +42,20 @@ def test_sanitises_and_deduplicates_scenario_sheet_names() -> None: illegal_name = "Bad:/\\?*[]name" # act - data = render_workbook( - [(long_name, empty), (long_name, empty), (illegal_name, empty)] + path = str(tmp_path / "export.xlsx") + render_workbook( + [(long_name, empty), (long_name, empty), (illegal_name, empty)], path ) # assert — three distinct, Excel-legal sheet names. - names = load_workbook(BytesIO(data)).sheetnames + names = load_workbook(path).sheetnames assert len(names) == 3 assert len(set(names)) == 3 assert all(len(name) <= 31 for name in names) assert not any(ch in name for name in names for ch in r":/\?*[]") -def test_brands_the_header_row_and_freezes_it() -> None: +def test_brands_the_header_row_and_freezes_it(tmp_path: Path) -> None: # arrange sheet = ExportSheet( columns=("property_id", "loft_insulation"), @@ -59,11 +63,12 @@ def test_brands_the_header_row_and_freezes_it() -> None: ) # act - data = render_workbook([("Fabric first", sheet)]) + path = str(tmp_path / "export.xlsx") + render_workbook([("Fabric first", sheet)], path) # assert — the header is a bold, brand-filled band and the header row is # frozen so it stays visible while scrolling. - worksheet = load_workbook(BytesIO(data))["Fabric first"] + worksheet = load_workbook(path)["Fabric first"] header_cell = worksheet["A1"] assert header_cell.font.bold is True assert header_cell.fill.fgColor.rgb == HEADER_FILL