Skip to content

fix: two checks that could not do what they claimed - #193

Draft
LKSNDRTMLKV wants to merge 3 commits into
refactor/repoint-core-layoutfrom
fix/gtin-check-digit-every-product-group
Draft

fix: two checks that could not do what they claimed#193
LKSNDRTMLKV wants to merge 3 commits into
refactor/repoint-core-layoutfrom
fix/gtin-check-digit-every-product-group

Conversation

@LKSNDRTMLKV

Copy link
Copy Markdown
Member

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/dpp re-validated the GS1 check digit of ProductGroupData::Battery's GTIN. That value had already been through Gtin::parse, so the second check could only ever succeed — and by matching on one variant it implied the other ten product groups carrying a gtin were unprotected.

They are not. Every typed payload declares gtin: Gtin; Gtin's Deserialize is hand-written as Self::parse(&s).map_err(serde::de::Error::custom); and Gtin's inner field is private with parse as 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:

gtin_boundary::a_bad_check_digit_is_refused_while_the_body_is_parsed
gtin_boundary::a_valid_gtin_parses_and_the_request_is_accepted
gtin_boundary::the_handler_reads_the_gtin_generically_not_battery_only

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_gtin is now unused in this crate and its import is gone; the crate still uses dpp-digital-link for Digital Link URL building on the publish path.

The description assertion in the integrator

no_embedded_schema_keeps_a_description_after_stripping was failing on main-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 field description. Unsold-goods v2.0.0 does — the line description of Impl. Reg. (EU) 2026/2 Annex I note (e):

"description": {
  "type": "string",
  "minLength": 1,
  "description": "Note (e): established on the basis of the combined nomenclature, or a more detailed description.",
  "x-disclosure": "public"
}

strip_descriptions was 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_descriptions does, and reports the path of any surviving keyword rather than a yes/no. NAME_KEYED moved 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_survives already 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 check green end to end. Not run: the Docker tiers.

Stacking

Based on refactor/repoint-core-layout (#189), which is itself based on fix/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.

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.
@LKSNDRTMLKV LKSNDRTMLKV added the bug Something isn't working label Aug 26, 2026
@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant