fix: two checks that could not do what they claimed - #193
Draft
LKSNDRTMLKV wants to merge 3 commits into
Draft
Conversation
A substring search could not tell a description keyword from a property named description, which unsold-goods v2.0.0 legitimately has.
Gtin validates on deserialize, so the battery-only re-check was unreachable while implying ten other product groups went unchecked.
Up to standards ✅🟢 Issues
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two checks that read as though they were enforcing something and were not. Neither change alters behaviour; both replace a check that could not fail with a test that can.
Closes #190.
The GTIN branch on the create path
POST /api/v1/dppre-validated the GS1 check digit ofProductGroupData::Battery's GTIN. That value had already been throughGtin::parse, so the second check could only ever succeed — and by matching on one variant it implied the other ten product groups carrying agtinwere unprotected.They are not. Every typed payload declares
gtin: Gtin;Gtin'sDeserializeis hand-written asSelf::parse(&s).map_err(serde::de::Error::custom); andGtin's inner field is private withparseas its only other constructor. An invalid GTIN cannot be deserialised, cannot be constructed, and never reaches a handler — for all eleven product groups at once.This is why #190, which I filed, was wrong. It claimed ten product groups could be created with a malformed GTIN. The dead branch is removed and three tests take its place:
The first submits a tyre payload — deliberately not battery — carrying
09506000134353: a well-formed 14-digit GTIN with the wrong check digit, and asserts the rejection names the check digit.dpp_digital_link::validate_gtinis now unused in this crate and its import is gone; the crate still usesdpp-digital-linkfor Digital Link URL building on the publish path.The description assertion in the integrator
no_embedded_schema_keeps_a_description_after_strippingwas failing onmain-line branches before any change here — confirmed by stashing this work and re-running it.The assertion was
!serde_json::to_string(&schema).contains("\"description\""). A substring search, which broke the moment a regulated schema legitimately named a fielddescription. Unsold-goods v2.0.0 does — the line description of Impl. Reg. (EU) 2026/2 Annex I note (e):strip_descriptionswas right all along: it removes the keyword and keeps the field name, which is what the contract requires. Only the test was wrong.It now walks the tree structurally, the same way
strip_descriptionsdoes, and reports the path of any surviving keyword rather than a yes/no.NAME_KEYEDmoved to module scope so the check and the code cannot drift onto different lists.One new test,
the_detector_reports_keywords_and_ignores_a_property_of_that_name, feeds it an un-stripped schema holding both cases and asserts it reports exactly the two real keywords and not the field name — because a green check proves nothing until it has been seen to discriminate.a_property_named_description_survivesalready covered the stripping half, so nothing here duplicates it.CLAUDE.md
A new subsection under Testing: establish a fact about behaviour with a
#[test]you commit, not a throwaway probe. The GTIN branch above is the worked example — a probe would have answered the question once, for one person, and left the dead branch in place.Verification
just checkgreen end to end. Not run: the Docker tiers.Stacking
Based on
refactor/repoint-core-layout(#189), which is itself based onfix/access-filter-path-aware(#188). Retarget before either parent merges. The integrator test fix is independent of both and could be lifted out if you would rather it landed first — it is the commit unblocking the gate.