fix(domain): preserve carbon class label verbatim - #55
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 15 |
| Duplication | 2 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR successfully transitions the CarbonFootprintClass from an enumeration to a validated newtype, enabling support for arbitrary class labels as required by schema v2.1.0. All core acceptance criteria, including validation logic (length, whitespace, and control characters) and the addition of mandatory provenance fields, have been addressed.
While the implementation is functionally correct and meets standards, there is a significant opportunity to improve data integrity. Currently, the carbon footprint class and its associated ruleset ID/version are independent optional fields. The documentation itself notes that the class label is meaningless without this context; therefore, grouping these into a single optional structure is recommended to prevent inconsistent data states. No security flaws or major logic bugs were identified that would block merging, but the structural refactor is highly encouraged for long-term maintainability.
About this PR
- The current implementation of
BatteryDataallows for a state where acarbon_footprint_classis provided without its required provenance (ruleset ID and version). Since these fields are logically inseparable for compliance (Art. 7(2)), consider grouping them into a unified optional struct to enforce this dependency at the type level.
1 comment outside of the diff
benches/src/validation.rs
line 8⚪ LOW RISK
Suggestion: Consolidate this test data. Sincedpp-domainalready providessample_battery_data, consider making that helper public (perhaps under atest-supportfeature) and using it here to reduce the friction of future schema updates.
Test suggestions
- Verify round-trip preserves arbitrary labels (e.g., 'F', 'A+', 'CLASS-1') that weren't in the previous enum.
- Verify rejection of empty and blank (whitespace-only) strings.
- Verify rejection of labels containing control characters.
- Verify rejection of labels exceeding 8 code points (checking length in chars, not bytes).
- Verify labels are stored verbatim without case folding or trimming.
- Verify BatteryData includes provenance fields in its serialized JSON representation.
- Drift guard: assert that Rust MAX_LEN and lack of enum match the embedded v2.1.0 JSON schema.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| pub carbon_footprint_class: Option<CarbonFootprintClass>, | ||
|
|
||
| /// Identifier of the ruleset whose class boundaries produced | ||
| /// `carbon_footprint_class`. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub carbon_footprint_class_ruleset_id: Option<String>, | ||
|
|
||
| /// Version of the ruleset identified by `carbon_footprint_class_ruleset_id`. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub carbon_footprint_class_ruleset_version: Option<String>, |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The documentation notes that the carbon footprint class is 'meaningless' without the accompanying ruleset ID and version. To enforce this dependency and prevent inconsistent data states, group these three fields into a single optional struct.
Try running the following prompt in your coding agent:
In
crates/dpp-domain/src/domain/sector/data/battery.rs, groupcarbon_footprint_class,carbon_footprint_class_ruleset_id, andcarbon_footprint_class_ruleset_versioninto a newCarbonFootprintClassProvenancestruct. Use#[serde(flatten)]on anOption<CarbonFootprintClassProvenance>field inBatteryData. Ensure the fields in the new struct use#[serde(rename = "...")]to preserve the current camelCase wire names.
| } | ||
| } | ||
|
|
||
| impl TryFrom<String> for CarbonFootprintClass { |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Consider implementing FromStr for CarbonFootprintClass. This is a low-effort addition that improves ergonomics by allowing the type to be used with .parse::<CarbonFootprintClass>().
| len: usize, | ||
| max: usize, | ||
| }, | ||
| #[error("carbon footprint class label '{0}' contains a control character")] |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Use the debug representation {:?} for the label in the ControlCharacter error message to ensure control characters are escaped (e.g., shown as \n instead of an actual newline).
| #[error("carbon footprint class label '{0}' contains a control character")] | |
| #[error("carbon footprint class label '{0:?}' contains a control character")] |
Replaces
CarbonFootprintClass's A-E enum with a validated newtype over the declared label.Reopens #54, which GitHub auto-closed when its base branch was deleted on the merge of #52. Same content, now based on
main.Why
#52 removed the A-E enumeration from battery schema v2.1.0: Art. 7(2) of Reg. (EU) 2023/1542 defines no class labels, defers them to an unadopted delegated act, and requires the Commission to "review the number of performance classes and the thresholds between them, every three years".
The Rust type was not updated in that PR, so the schema permitted values the typed path corrupted.
#[serde(other)]mapped any unrecognised label toOtherand discarded the original string — under a future seven-class scale, "F" and "A+" would both have been stored, and re-served, asOther. A published passport carries a qualified electronic seal, so a lossy round-trip is a correctness defect rather than a cosmetic one.The change
MAX_LEN = 8matches the schema'smaxLength. Rejects empty/blank, over-length, and control characters. The label is stored verbatim — no case folding, no trimming — because the declared value is what an auditor re-checks against the delegated act.try_fromrather than a plain newtype is deliberate: deserialization now validates instead of silently accepting, so a bad label is rejected at the boundary where a field path can still be reported.Also fixed
BatteryDatanever carried the provenance fields. #52 addedcarbonFootprintClassRulesetId/Versionto the schema but not to the struct, so withadditionalProperties: falsethey were unreachable through the typed path entirely. Both are now onBatteryData, which is what makes pairing a label with its ruleset possible rather than aspirational.Breaking
Pre-1.0, landing in the unreleased 0.11.0.
CarbonFootprintClass::{A,B,C,D,E,Other}->CarbonFootprintClass::new(label) -> Result<_, CarbonFootprintClassError>BatteryDatagains twoOption<String>fieldsNot changed
EnergyEfficiencyClass(same file, A-G) is left alone: that scale is fixed by Regulation 2017/1369, and I did not verify it against primary text in this pass.Verification
just checkgreen: 805 tests, plugin suites included.just build-pluginsgreen.Seven new tests. The one that pins the regression asserts "A", "F", "A+", "A+++" and "CLASS-1" each survive a serialize/deserialize round trip. Plus a drift guard that parses the embedded v2.1.0 schema and asserts
maxLengthequalsMAX_LENand that the field has not regained anenum— the two bounds are one fact stated twice, and should fail loudly if they diverge.