fix(domain): align product categories with schema enums - #58
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| Duplication | 0 |
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
The PR successfully aligns the battery and unsold-goods sectors with schema enums and implements a drift-guard test to prevent future regressions. Codacy quality results are up to standards. The review identified two primary improvements for the new test logic in crates/dpp-domain/src/catalog/tests.rs: the test currently assumes a flat JSON schema structure which may break if schemas evolve to use $ref or allOf, and it relies on a manual mapping table that could allow new sectors to bypass validation if omitted. Overall, the logic is sound and meets the primary acceptance criteria.
About this PR
- The new drift-guard test is a positive step for data integrity. To ensure its long-term effectiveness, consider the risk that new sectors added to the catalog might be silently ignored if the mapping table is not updated. Ensuring a default-fail or explicit-exemption policy for all sectors would make this check more robust.
Test suggestions
- Verify battery sector categories match the 'batteryType' schema enum
- Verify unsold-goods sector categories match the 'productCategory' schema enum
- Ensure drift guard fails when a catalog category is missing from the schema enum
- Ensure drift guard correctly maps sectors to their specific schema property names (e.g., tyreClass, productionRoute)
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| /// `SectorDescriptor::product_categories` has no reader in Rust today; both | ||
| /// would have become load-bearing the moment one appeared. | ||
| #[test] | ||
| fn product_categories_are_legal_values_of_their_schema_enum() { |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: This drift-guard test ensures consistency between the catalog and schemas. To prevent new sectors from silently bypassing this check, consider adding a secondary check that verifies every sector in the catalog with categories is either present in CATEGORY_ENUM_PROPERTY or explicitly exempted.
Try running the following prompt in your IDE agent:
Update the test
product_categories_are_legal_values_of_their_schema_enumincrates/dpp-domain/src/catalog/tests.rsto include a validation step that ensures all sectors in the catalog which defineproduct_categoriesare either included in theCATEGORY_ENUM_PROPERTYtable or are explicitly marked as exempt (e.g., 'textile'). This will ensure that future additions to the domain model are automatically validated against their respective schemas.
| let schema: serde_json::Value = | ||
| serde_json::from_str(schema_json).expect("schema is valid JSON"); | ||
|
|
||
| let allowed: Vec<&str> = schema["properties"][property]["enum"] |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The direct lookup of the enum property assumes a flat schema structure. If schemas are updated to use $ref or allOf to define these properties, the test will fail to find the valid values. Consider adding a comment about this structural dependency or confirming that the registry provides dereferenced schemas.
Try running the following prompt in your coding agent:
In
crates/dpp-domain/src/catalog/tests.rs, enhance theproduct_categories_are_legal_values_of_their_schema_enumtest to handle cases where the enum might be nested within a$refor anallOfblock, or verify if theVersionedSchemaRegistryprovides dereferenced JSON.
One concept, two spellings — in two sectors. Flagged by Codacy on #56.
The drift
slistarting-lighting-ignitionbatteryTypeclothing_accessoriesaccessoriesproductCategoryCodacy found the first. The drift guard added here found the second.
Both catalogs now use the schema spelling, on the rule that the schema is what real data validates against — and for battery it also has a typed home,
BatteryType::Slialready carrying#[serde(rename = "starting-lighting-ignition")].Why nothing was broken
SectorDescriptor::product_categorieshas no reader anywhere in Rust. (verify.rs:156readscredential.credential_subject.product_categories— the credential's own scope, a different field.) So today it is declared data with no consumer, which is why the mismatch was invisible. It becomes load-bearing the moment a first reader appears, which is the worst moment to discover the spellings disagree.The actual fix
product_categories_are_legal_values_of_their_schema_enumincatalog/tests.rs. The rename alone just resets the clock; this is what stops it recurring.The correspondence is not derivable — categories live under
productCategory,productType,batteryType,productFamily,productionRouteortyreClassdepending on sector — so it is declared in a 9-row table rather than inferred by convention. A sector absent from the table is skipped;textileis the real case, declaring categories with no schema enum to check against. If a schema property is renamed the row panics loudly rather than silently skipping.Verified non-vacuous by reintroducing
sliand confirming the failure names both the offending value and the legal set:Found, not fixed
textiledeclareshome_textilein snake_case while comparable sectors use kebab-case, and the unsold-goods schema spells the same concepthome-textile. It has no schema enum, so the guard cannot reach it. That is a naming-convention decision across the whole catalog rather than a correctness fix.Verification
just checkgreen: 814 tests, plugin suites included.just build-pluginsgreen.