fix(resolver): give the printed QR its quiet zone - #244
Merged
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 54 |
| Duplication | 2 |
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.
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.
The PNG this node serves at
GET /dpp/{dppId}/qrcarried no quiet zone. Thatis the rendering an operator downloads to print onto a physical label.
The defect
ISO/IEC 18004 requires four modules of blank margin on all four sides of a QR
symbol. It is not decoration — the quiet zone is how a scanner finds the
symbol's edge, and a code printed flush to other artwork is out of spec.
Two renderers exist and they disagreed.
dpp-render's SVG, embedded in thepassport page:
dpp-resolver's PNG, served by the/qrroute:The divergence ran the wrong way round. The screen rendering — which a browser
surrounds with white page anyway — was the compliant one. The downloadable PNG,
the only output that ends up on a product, was the bare symbol.
Why nothing caught it
The route's only coverage was
qr_endpoint_returns_png, which asserts a PNG cameback. A solid white square satisfies that.
The reason it stayed that way is structural: PNG generation was inline in
resolve_qr_handler, reachable only after a vault fetch, a Redis lookup and DIDverification. Testing what it encoded meant standing all three up, so the
affordable assertion was
is_a_png— and that is what shipped, through asector→productGrouprename and every dependency bump since.qr_pngis now a free function. That is what makes the rest of this testable.The quiet zone is one value now
dpp_render::QR_QUIET_ZONE_MODULES, read by both renderers.The renderers themselves stay separate.
lib.rsrecords that they are split byoutput class and that print-grade carrier work may collapse them into a carrier
crate later; that decision is somebody's to make and this PR does not pre-empt
it. Only the property neither is allowed to get wrong is shared.
A round-trip decode alone would not have caught this
Worth stating plainly, because it is the obvious way to write this test and it
does not work.
Measured, not assumed. With the fix reverted to
quiet = 0:rqrrreads a symbol with no quiet zone perfectly well. Software decoders arefar more lenient than a hand scanner against a busy background, so a test that
only asks "does this decode?" goes green against the exact defect it was written
to find.
So the geometry is asserted directly — a blank border of the expected thickness
on all four sides — and the decode covers the payload. Neither is sufficient
alone.
Tests
dpp-resolver(new dev-dependencyrqrr, test-only):the_png_encodes_the_carrier_uri_it_was_given— decodes the PNG and assertsthe payload equals the carrier URI. This is what a
qrcodeorimagebumpcould previously have changed silently.
the_symbol_keeps_its_quiet_zone_on_every_side— the geometric assertionabove.
the_quiet_zone_check_cannot_pass_on_a_blank_image— the margin check issatisfied by an all-white image, so this pins that dark modules exist.
an_unencodable_uri_is_an_error_not_a_wrong_symbol— a URI too long for anyQR version errors rather than encoding something else.
dpp-render:build_qr_svg_reserves_the_quiet_zone—viewBoxexceeds the symbol byexactly two quiet zones, and no drawn module falls inside the margin.
build_qr_svg_encodes_the_carrier_uriis renamed tobuild_qr_svg_names_the_carrier_uri_in_its_title. It only ever checked the<title>text; the old name read as coverage that did not exist.Bearing on #204 and #205
Both bump this exact path (
qrcode0.12 → 0.14.1,image0.24 → 0.25.10) andboth passed CI on "bytes were returned". They should merge after this, so the
bumps are validated against what the label encodes rather than on faith.
Verification
just checkgreen — 934/934, clippy-D warnings, fmt, the nine scriptgates, integration compile, audit. Built against published
dpp-core0.19.0(no
.cargo/config.toml).The quiet-zone assertion was confirmed to bite by reverting the fix and watching
it go red, as quoted above.