From 7489e5ac89fe977a36c22e7264e1ef05a0b23660 Mon Sep 17 00:00:00 2001 From: Jun-te Kim Date: Wed, 24 Jun 2026 08:35:36 +0000 Subject: [PATCH] Scope 21.0.0 widening to fields the skipped certs actually omit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up (Khalim): the first pass made far more optional than needed — notably the whole SapBuildingPart block — and a buggy 21.0.0↔21.0.1 diff also MISSED open_chimneys_count / cfl_/led_fixed_lighting_bulbs_count / suggested_ improvements, so the original change actually mapped only 3 of the 33 skipped certs (the rest still failed on open_chimneys_count). Re-derived the exact set empirically from all 33 skipped cohort certs: widen only fields that are (a) required in 21.0.0, (b) already optional in 21.0.1, AND (c) genuinely omitted by ≥1 of those certs. Result: - KEEP optional: the 4 SapWindow refinements, the top-level vent/lighting/ door/pressure-test block (incl. the 3 previously-missed fields), 2 SapEnergySource fields, Addendum.addendum_numbers, PhotovoltaicSupply. none_or_no_details, and exactly ONE building-part field (SapBuildingPart.roof_insulation_thickness — omitted by 7 certs). - REVERT to required: the other 12 SapBuildingPart fields (construction_age_ band, wall_construction, …), MainHeatingDetail.emitter_temperature, PvBatteries.pv_battery, ShowerOutlets.shower_outlet — none of the 33 certs omit these, so they stay strict. Mapper: coalesce the count fields (wet_rooms_count, open_chimneys_count, cfl_/led_fixed_lighting_bulbs_count) to 0 like every other mapper, so the now- optional values can't reach a NOT-NULL column (also drops 4 pyright ignores). Now maps 32/33 (up from 3); the last cert hits a pre-existing pv_batteries- shape AttributeError and degrades via the ADR-0031 skip path. pyright net unchanged (43, no new errors); regression test rewritten to the real omitted set. Co-Authored-By: Claude Opus 4.8 (1M context) --- datatypes/epc/domain/mapper.py | 18 +++---- .../domain/tests/test_from_rdsap_schema.py | 30 +++++++----- datatypes/epc/schema/rdsap_schema_21_0_0.py | 48 +++++++++---------- 3 files changed, 53 insertions(+), 43 deletions(-) diff --git a/datatypes/epc/domain/mapper.py b/datatypes/epc/domain/mapper.py index 0fe7fee1..a752efd1 100644 --- a/datatypes/epc/domain/mapper.py +++ b/datatypes/epc/domain/mapper.py @@ -1850,13 +1850,15 @@ class EpcPropertyDataMapper: door_count=schema.door_count, habitable_rooms_count=schema.habitable_room_count, heated_rooms_count=schema.heated_room_count, - wet_rooms_count=schema.wet_rooms_count, # pyright: ignore[reportArgumentType] + # Optional in 21.0.0 (real certs omit them); coalesce counts to 0 + # like every other mapper so None can't reach a NOT-NULL column. + wet_rooms_count=schema.wet_rooms_count or 0, extensions_count=schema.extensions_count, - open_chimneys_count=schema.open_chimneys_count, + open_chimneys_count=schema.open_chimneys_count or 0, insulated_door_count=schema.insulated_door_count, draughtproofed_door_count=schema.draughtproofed_door_count, - led_fixed_lighting_bulbs_count=schema.led_fixed_lighting_bulbs_count, - cfl_fixed_lighting_bulbs_count=schema.cfl_fixed_lighting_bulbs_count, + led_fixed_lighting_bulbs_count=schema.led_fixed_lighting_bulbs_count or 0, + cfl_fixed_lighting_bulbs_count=schema.cfl_fixed_lighting_bulbs_count or 0, incandescent_fixed_lighting_bulbs_count=schema.incandescent_fixed_lighting_bulbs_count, roofs=EpcPropertyDataMapper._map_energy_elements(schema.roofs), walls=EpcPropertyDataMapper._map_energy_elements(schema.walls), @@ -1886,7 +1888,7 @@ class EpcPropertyDataMapper: boiler_flue_type=d.boiler_flue_type, fan_flue_present=d.fan_flue_present == "Y", heat_emitter_type=d.heat_emitter_type, - emitter_temperature=d.emitter_temperature, # pyright: ignore[reportArgumentType] + emitter_temperature=d.emitter_temperature, main_heating_number=d.main_heating_number, boiler_ignition_type=d.boiler_ignition_type, main_heating_control=d.main_heating_control, @@ -1982,9 +1984,9 @@ class EpcPropertyDataMapper: sap_building_parts=[ SapBuildingPart( identifier=BuildingPartIdentifier.from_api_string(bp.identifier), - construction_age_band=bp.construction_age_band, # pyright: ignore[reportArgumentType] - wall_construction=_api_wall_construction_code(bp.wall_construction), # pyright: ignore[reportArgumentType] - wall_insulation_type=bp.wall_insulation_type, # pyright: ignore[reportArgumentType] + construction_age_band=bp.construction_age_band, + wall_construction=_api_wall_construction_code(bp.wall_construction), + wall_insulation_type=bp.wall_insulation_type, wall_thickness_measured=bp.wall_thickness_measured == "Y", party_wall_construction=_api_party_wall_construction_int( bp.party_wall_construction diff --git a/datatypes/epc/domain/tests/test_from_rdsap_schema.py b/datatypes/epc/domain/tests/test_from_rdsap_schema.py index e14af2e9..c956bb67 100644 --- a/datatypes/epc/domain/tests/test_from_rdsap_schema.py +++ b/datatypes/epc/domain/tests/test_from_rdsap_schema.py @@ -254,32 +254,40 @@ class TestFromRdSapSchema21_0_0: return EpcPropertyDataMapper.from_rdsap_schema_21_0_0(schema) def test_optional_fields_omitted_maps_not_crashed(self) -> None: - # Real-API 21.0.0 cohort certs routinely omit fields that the dataclass - # originally modelled as required while 21.0.1 already treated as - # optional (window frame_factor / glazing_gap / transmission details, - # wet_rooms_count, the mechanical-vent duct block, wind_turbine_details, - # &c.). The modelling_e2e cohort skipped ~35 such certs with - # "SapWindow: missing required field 'frame_factor'" (cert - # 2205-3036-3484-0400-5718). The 21.0.0 schema now mirrors 21.0.1's - # optionality so they map instead of being dropped from the donor pool. + # Real-API 21.0.0 cohort certs omit a set of fields that the dataclass + # modelled as required while 21.0.1 already treats as optional. The + # modelling_e2e cohort skipped 33 such certs (e.g. 2205-3036-3484-0400- + # 5718). This is the exact union of fields those certs omit — all are + # optional in 21.0.1; widening 21.0.0 to match maps them into the donor + # pool instead of dropping them. (Building-part fields are NOT widened: + # the only one those certs omit is roof_insulation_thickness.) data = load("21_0_0.json") for window in data.get("sap_windows", []): for k in ("frame_factor", "glazing_gap", "pvc_frame", "window_transmission_details"): window.pop(k, None) - for k in ("wet_rooms_count", "pressure_test_certificate_number", + for k in ("wet_rooms_count", "open_chimneys_count", + "pressure_test_certificate_number", "windows_transmission_details", "mechanical_ventilation_index_number", "mechanical_vent_duct_type", "mechanical_vent_duct_placement", "mechanical_vent_duct_insulation", "mechanical_vent_duct_insulation_level", - "mechanical_vent_measured_installation", - "insulated_door_u_value", "low_energy_fixed_lighting_bulbs_count"): + "mechanical_vent_measured_installation", "insulated_door_u_value", + "low_energy_fixed_lighting_bulbs_count", + "cfl_fixed_lighting_bulbs_count", "led_fixed_lighting_bulbs_count", + "suggested_improvements"): data.pop(k, None) data["sap_energy_source"].pop("wind_turbine_details", None) + data["sap_energy_source"].pop("pv_battery_count", None) + for bp in data.get("sap_building_parts", []): + bp.pop("roof_insulation_thickness", None) schema = from_dict(RdSapSchema21_0_0, data) result = EpcPropertyDataMapper.from_rdsap_schema_21_0_0(schema) assert result.uprn == 12457 + # coalesced, not None → safe for the NOT-NULL epc_property column + assert result.wet_rooms_count == 0 + assert result.open_chimneys_count == 0 def test_uprn(self, result: EpcPropertyData) -> None: assert result.uprn == 12457 diff --git a/datatypes/epc/schema/rdsap_schema_21_0_0.py b/datatypes/epc/schema/rdsap_schema_21_0_0.py index 8bb45214..2f5c8e69 100644 --- a/datatypes/epc/schema/rdsap_schema_21_0_0.py +++ b/datatypes/epc/schema/rdsap_schema_21_0_0.py @@ -12,7 +12,7 @@ class EnergyElement: environmental_efficiency_rating: int -@dataclass(kw_only=True) +@dataclass class Addendum: addendum_numbers: Optional[List[int]] = None stone_walls: Optional[str] = None @@ -25,9 +25,9 @@ class ShowerOutlet: shower_outlet_type: int -@dataclass(kw_only=True) +@dataclass class ShowerOutlets: - shower_outlet: Optional[ShowerOutlet] = None + shower_outlet: ShowerOutlet @dataclass @@ -37,12 +37,12 @@ class InstantaneousWwhrs: wwhrs_index_number2: Optional[int] = None -@dataclass(kw_only=True) +@dataclass class MainHeatingDetail: has_fghrs: str main_fuel_type: int heat_emitter_type: int - emitter_temperature: Optional[Union[int, str]] = None + emitter_temperature: Union[int, str] main_heating_number: int main_heating_control: int main_heating_category: int @@ -91,9 +91,9 @@ class PvBattery: battery_capacity: float -@dataclass(kw_only=True) +@dataclass class PvBatteries: - pv_battery: Optional[PvBattery] = None + pv_battery: PvBattery @dataclass @@ -107,7 +107,7 @@ class PhotovoltaicSupplyNoneOrNoDetails: percent_roof_area: int -@dataclass(kw_only=True) +@dataclass class PhotovoltaicSupply: none_or_no_details: Optional[PhotovoltaicSupplyNoneOrNoDetails] = None @@ -279,18 +279,18 @@ class SapAlternativeWall: @dataclass(kw_only=True) class SapBuildingPart: - identifier: Optional[str] = None - wall_dry_lined: Optional[str] = None - floor_heat_loss: Optional[int] = None - roof_construction: Optional[int] = None - wall_construction: Optional[int] = None - building_part_number: Optional[int] = None - sap_floor_dimensions: Optional[List[SapFloorDimension]] = None - wall_insulation_type: Optional[int] = None - construction_age_band: Optional[str] = None - party_wall_construction: Optional[Union[int, str]] = None - wall_thickness_measured: Optional[str] = None - roof_insulation_location: Optional[Union[int, str]] = None + identifier: str + wall_dry_lined: str + floor_heat_loss: int + roof_construction: int + wall_construction: int + building_part_number: int + sap_floor_dimensions: List[SapFloorDimension] + wall_insulation_type: int + construction_age_band: str + party_wall_construction: Union[int, str] + wall_thickness_measured: str + roof_insulation_location: Union[int, str] roof_insulation_thickness: Optional[Union[str, int]] = None sap_room_in_roof: Optional[SapRoomInRoof] = None sap_alternative_wall_1: Optional[SapAlternativeWall] = None @@ -431,7 +431,7 @@ class RdSapSchema21_0_0: sap_energy_source: SapEnergySource secondary_heating: EnergyElement sap_building_parts: List[SapBuildingPart] - open_chimneys_count: int + open_chimneys_count: Optional[int] = None solar_water_heating: str habitable_room_count: int heating_cost_current: float @@ -447,7 +447,7 @@ class RdSapSchema21_0_0: insulated_door_u_value: Optional[float] = None mechanical_ventilation: int percent_draughtproofed: int - suggested_improvements: List[SuggestedImprovement] + suggested_improvements: Optional[List[SuggestedImprovement]] = None co2_emissions_potential: float energy_rating_potential: int lighting_cost_potential: float @@ -457,14 +457,14 @@ class RdSapSchema21_0_0: draughtproofed_door_count: int mechanical_vent_duct_type: Optional[int] = None windows_transmission_details: Optional[WindowsTransmissionDetails] = None - cfl_fixed_lighting_bulbs_count: int + cfl_fixed_lighting_bulbs_count: Optional[int] = None energy_consumption_current: int has_fixed_air_conditioning: str multiple_glazed_proportion: int calculation_software_version: str energy_consumption_potential: int environmental_impact_current: int - led_fixed_lighting_bulbs_count: int + led_fixed_lighting_bulbs_count: Optional[int] = None mechanical_vent_duct_placement: Optional[int] = None mechanical_vent_duct_insulation: Optional[int] = None potential_energy_efficiency_band: str