From 77f90e144edd01ca0b04183aab514541bc8f7667 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Fri, 5 Jun 2026 10:07:24 +0000 Subject: [PATCH] review: store epc_building_part.wall_insulation_thickness as JSONB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR feedback (dancafc): the SQLModel column was Optional[str], but the domain `SapBuildingPart.wall_insulation_thickness` is Optional[Union[str, int]] — `_api_resolve_wall_insulation_thickness` returns an int mm when the API lodges `wall_insulation_thickness == "measured"` (SAP 10.2 §5.7 / Table 8). The plain str column round-trips that int back as the string "100", corrupting the Table 8 insulated-wall U-value lookup. This column was missed in the round-trip-fidelity §1 JSONB sweep (#1129) — its `Union[str, int]` sibling `roof_insulation_thickness` was converted, but `wall_insulation_thickness` was not, and no 21.0.0/21.0.1 fixture lodges "measured" so the gap stayed latent. Convert to JSONB (matching `roof_insulation_thickness` / `flat_roof_insulation_thickness`), align the column type to Optional[Union[str, int]] (also removes a pyright type-mismatch), record it in the migration doc §1, and add a round-trip guard test asserting an int survives as an int (fails as '100' == 100 on the old str column). Co-Authored-By: Claude Opus 4.8 --- .../epc-property-round-trip-fidelity.md | 7 ++-- infrastructure/postgres/epc_property_table.py | 10 +++++- tests/repositories/epc/test_epc_round_trip.py | 34 +++++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/docs/migrations/epc-property-round-trip-fidelity.md b/docs/migrations/epc-property-round-trip-fidelity.md index d9ed6557..53590a2f 100644 --- a/docs/migrations/epc-property-round-trip-fidelity.md +++ b/docs/migrations/epc-property-round-trip-fidelity.md @@ -32,8 +32,9 @@ and **still require the matching DB migration** wherever the physical tables liv `heating_secondary_heating_type`, `heating_shower_outlet_type`, `energy_pv_connection`; `epc_main_heating_detail`: `main_fuel_type`, `heat_emitter_type`, `emitter_temperature`, `main_heating_control`; `epc_building_part`: `wall_construction`, `wall_insulation_type`, - `party_wall_construction`, `flat_roof_insulation_thickness`, `roof_insulation_location`, - `roof_insulation_thickness`; `epc_window`: `glazing_gap`, `orientation`, `window_type`, + `party_wall_construction`, `wall_insulation_thickness`, `flat_roof_insulation_thickness`, + `roof_insulation_location`, `roof_insulation_thickness`; `epc_window`: `glazing_gap`, + `orientation`, `window_type`, `glazing_type`, `window_location`, `window_wall_type`, `draught_proofed`, `permanent_shutters_present`, `transmission_data_source`). - **New scalar columns** — `epc_property`: `heating_number_baths`, `heating_number_baths_wwhrs`, @@ -68,7 +69,7 @@ in the forward mapper so the Python type round-trips exactly (JSON scalars prese |---|---| | `epc_property` | `heating_cylinder_size`, `heating_immersion_heating_type`, `heating_cylinder_insulation_type`, `heating_secondary_heating_type`, `heating_shower_outlet_type`, `energy_pv_connection` | | `epc_main_heating_detail` | `main_fuel_type`, `heat_emitter_type`, `emitter_temperature`, `main_heating_control` | -| `epc_building_part` | `wall_construction`, `wall_insulation_type`, `party_wall_construction`, `flat_roof_insulation_thickness`, `roof_insulation_location`, `roof_insulation_thickness` | +| `epc_building_part` | `wall_construction`, `wall_insulation_type`, `party_wall_construction`, `wall_insulation_thickness`, `flat_roof_insulation_thickness`, `roof_insulation_location`, `roof_insulation_thickness` | | `epc_window` | `glazing_gap`, `orientation`, `window_type`, `glazing_type`, `window_location`, `window_wall_type`, `draught_proofed`, `permanent_shutters_present` | (`energy_meter_type` and `energy_wind_turbines_terrain_type` are `str` in the domain — leave as diff --git a/infrastructure/postgres/epc_property_table.py b/infrastructure/postgres/epc_property_table.py index 539628bd..4b235d34 100644 --- a/infrastructure/postgres/epc_property_table.py +++ b/infrastructure/postgres/epc_property_table.py @@ -549,7 +549,15 @@ class EpcBuildingPartModel(SQLModel, table=True): building_part_number: Optional[int] = Field(default=None) wall_dry_lined: Optional[bool] = Field(default=None) wall_thickness_mm: Optional[int] = Field(default=None) - wall_insulation_thickness: Optional[str] = Field(default=None) + # Union[str, int] — int mm when the API lodges + # `wall_insulation_thickness == "measured"` (resolved by + # `_api_resolve_wall_insulation_thickness`), else the lodged string + # ("NI", a numeric string, ...). JSONB to preserve int vs str on + # round-trip, exactly like the sibling `roof_insulation_thickness` / + # `flat_roof_insulation_thickness`. + wall_insulation_thickness: Optional[Union[str, int]] = Field( + default=None, sa_column=Column(JSONB, nullable=True) + ) floor_heat_loss: Optional[int] = Field(default=None) floor_insulation_thickness: Optional[str] = Field(default=None) flat_roof_insulation_thickness: Optional[Union[str, int]] = Field( diff --git a/tests/repositories/epc/test_epc_round_trip.py b/tests/repositories/epc/test_epc_round_trip.py index 192027f7..8233bda3 100644 --- a/tests/repositories/epc/test_epc_round_trip.py +++ b/tests/repositories/epc/test_epc_round_trip.py @@ -48,3 +48,37 @@ def test_epc_property_data_round_trips(schema_dir: str, db_engine: Engine) -> No # Assert assert reloaded == original + + +def test_building_part_wall_insulation_thickness_preserves_int( + db_engine: Engine, +) -> None: + # SAP 10.2 §5.7/Table 8: when the API lodges + # `wall_insulation_thickness == "measured"`, the mapper resolves the + # value to an int mm. The `epc_building_part.wall_insulation_thickness` + # column must therefore preserve int vs str on round-trip (JSONB), like + # its `roof_insulation_thickness` sibling — a plain str column would + # round-trip the int 100 back as "100" and corrupt the Table 8 lookup. + from dataclasses import replace + + # Arrange — take a green fixture and force the measured-int case. + original = _load_epc("RdSAP-Schema-21.0.0") + assert original.sap_building_parts, "fixture must have a building part" + bp0 = replace(original.sap_building_parts[0], wall_insulation_thickness=100) + original = replace( + original, + sap_building_parts=[bp0, *original.sap_building_parts[1:]], + ) + + # Act + with Session(db_engine) as session: + epc_property_id = EpcPostgresRepository(session).save(original) + session.commit() + with Session(db_engine) as session: + reloaded = EpcPostgresRepository(session).get(epc_property_id) + + # Assert — the int survives as an int, not the string "100". + assert reloaded is not None + value = reloaded.sap_building_parts[0].wall_insulation_thickness + assert value == 100 + assert isinstance(value, int)