fix(dal): preserve stored keys the struct does not model - #97
Merged
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
| 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.
A published passport could silently lose stored data, permanently, on the one
write that freezes it. This closes that, adds a guard that fails when the type
stops representing a stored key, and freezes one real document per sector so
partial coverage can't be mistaken for full coverage.
The defect
update_statusis a read-modify-write —find_by_id, mutate the struct,update— and publish takes the same path.update_passport_in_txwrote theserialised struct over the whole
doccolumn, so every stored key the structdoes not model was erased from the database, not merely absent in memory.
It bites hardest at publish. The retention guard tests
OLD.retention_locked,which is still false while the row is a draft, so the guard does not fire, the
lossy write lands, and
retention_lockedbecomes true in that same statement.Every later write is guarded — so the loss can never be repaired in place.
Real instance: the committed
battery/v2.0.0.jsonfixture carries"facilityId": "3212321321312", superseded in the type byfacility.Nothing has actually been lost: there are no deployed nodes and no production
data. That is what makes now the cheap moment.
A — the write preserves what it does not understand
doc = $2becomesdoc = doc || $2: a shallow top-level merge. The struct winson every key it models; keys it does not model survive.
That scope is deliberate and matches the model.
sectorDatais fully modelledand versioned through the lens chain, so replacing it wholesale is correct. The
envelope is the axis with no such mechanism, and it is the one that was losing
data.
Constraint this carries, recorded at the call site:
Passportisskip_serializing_if = "Option::is_none"throughout, so a field goingSome->Noneis absent from the payload and will no longer clear the stored key. Noproduction path does that today — every envelope-field assignment to
Noneinthis repo is inside a test. A field that genuinely needs clearing must write an
explicit JSON
null.Two other writers were checked and left alone:
repo_seal.rsusesjsonb_seton a single key, and
patch_fieldswrites the patched stored document. Bothalready preserve unknown keys.
B — a key that stops being represented has to be declared
no_frozen_doc_loses_an_envelope_key_unrecordedreads each frozen document,round-trips it through
Passport, and requires every top-level key to eithercome back or appear in
RETIRED_ENVELOPE_KEYSwith the reason it no longer is.This catches what the existing test structurally cannot. A renamed or removed
optional field does not fail deserialization at all — it is dropped,
Nonetakes its place, and nothing reports anything. That is worse than a loud break,
because a loud break is visible the first time anyone reads an old row.
Two design points:
nullis not a loss. Withskip_serializing_if, a nullround-trips to an absent key; counting that as loss would fire on nearly every
fixture and get the check switched off.
claim "old documents carrying this key are still correct on disk, the value is
simply not in the type".
facilityIdis the first entry. The failure messagesays explicitly not to edit the fixture — a frozen document edited to make a
test pass has stopped being evidence about anything.
C — one frozen document per sector, nested like the schemas are
Battery is the first sector to a binding date and the most versioned: six
schema versions, six fixtures, each carrying a field distinctive to its version
so it exercises that version rather than being interchangeable with its
neighbours. All eleven catalog sectors now have at least one frozen fixture —
the original version of this PR covered battery and textile only and left the
other nine as a named gap; that gap is closed here rather than carried forward.
Layout, changed in this revision: fixtures moved from a flat
{sector}_{version}.jsonnaming to one directory per sector —{catalog_key}/v{version}.json— mirroringdpp-core/crates/dpp-domain/schemas/{sector}/v{version}.jsonexactly. Elevensectors' versions interleaved in one flat listing was already hard to scan;
matching the convention the schemas themselves use means a reader who knows one
layout already knows the other.
passport_doc_compat.rs's fixture walk and itssector-identity check were updated to match — the check now compares against
Sector::catalog_key()(the directory name) rather than parsing a filename.Verification
Both halves are proven in both directions, not asserted:
pg_integration, real Postgres) seeds an unmodelled key on a draft,publishes, and asserts the key survived and the struct still won on the keys it
owns. Reverting
doc || $2todoc = $2makes it fail with "publish erased astored key the struct does not model".
facilityIdis removed from the retired list, naming the fileand the key.
just checkgreen. Docker tiers green.Carried incidentally, worth stating
Cargo.lockonmainresolveddpp-core0.15.0 whileCargo.tomlpinned0.16.0 — the lock was never regenerated when the repin landed, because local
verification runs with the sibling-path override active, which masks it. This
branch was verified with that override off, so the lock regenerates with real
0.16.0 registry checksums.