fix(transactions): bound the batch patch's import-id identity at the spec's 36 - #61
Merged
Merged
Conversation
…spec's 36 The last entry in wireBoundsUnenforced was a decision, not a gap: the spec bounds SaveTransactionWithIdOrImportId.import_id at 36 characters, but PatchByImportID's key reached the wire unchecked because TransactionPatch only had the validate it inherited from the embedded TransactionUpdate, which has no import-id field. An over-long key cost a round trip and came back a server 400 instead of the *ArgumentError every other transcribed bound answers with. PR #55 recorded the waiver and named enforcing it a behavior change belonging in its own commit — this is that commit. TransactionPatch now carries its own validate, shadowing the promoted one: the import-id identity through checkRuneMax — runes, not bytes, the unit JSON Schema's maxLength counts — then the embedded update's bounds unchanged. UpdateBatch's per-element loop already annotates the failing index, so the new rejection reads "(patch N)" like every other. The id identity stays deliberately unbounded, and a test pins that too: the spec declares no maxLength on transaction ids, and inventing one would reject payloads the server accepts — the same over-reach this package's validate doctrine (spec-stated invariants, and only those) exists to prevent. The boundary is pinned from both sides: 36 runes of multibyte import_id pass pre-flight AND arrive in the request body intact — the fake captures the body, because "unbounded" means sent, not merely attempted — while 37 fail with the field and patch index named before any I/O. The wireBoundsUnenforced table stays, empty: the mechanism is the point — the completeness assertion still forces every spec bound to be mapped or waived, so the next declared-but-unenforced bound must be written down, not forgotten. Its import_id row moves to wireBounds, making ImportIDMax enforced on every path that carries the field. The review's mutation pass proved the honest limit of that table: a row moved back to the waiver side with a bogus reason passes this file, because the table pins the spec side only — each row's teeth are its behavior test, and the table's comment now says so where the next row will be added. CHANGELOG records the user-visible half under the existing Unreleased Changed section: a batch that previously failed server-side with a 400 now fails client-side with *ArgumentError before any I/O. No wire payload changes for inputs the server accepts.
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.
Summary
Closes the last entry in
wireBoundsUnenforced: the import-id identity of aPatchByImportIDpatch is now pre-flighted against the spec's 36-character bound inUpdateBatch. Previously an over-long key cost a round trip and came back a server 400; now it fails client-side with the*ArgumentErrorevery other transcribed bound answers with, naming the field and the patch index ((patch N)).PR #55 recorded this exact waiver and named enforcing it "a behavior change belonging in its own commit" — this is that commit.
What changed
transactions.goTransactionPatchgains its ownvalidate, shadowing the promotedTransactionUpdate.validate:checkRuneMaxon the import-id identity (runes, not bytes — the unit JSON Schema'smaxLengthcounts), then the embedded update's bounds unchanged.contract_wire_test.go{SaveTransactionWithIdOrImportId, import_id}row moves fromwireBoundsUnenforced(now empty — the table stays, the mapped-or-waived mechanism is the point) intowireBounds. The table doc now states its honest limit: a row claims enforcement, its teeth are the behavior test.transactions_test.goField == "import_id"and(patch 1); a 100-charPatchByIDid plus exactly 36 runes of import_id pass pre-flight and are asserted to arrive in the request body intact (the fake captures the body — "unbounded" means sent, not merely attempted).CHANGELOG.md[Unreleased]→Changed.Deliberate non-changes
ididentity stays unbounded, and a test pins that too — the spec declares nomaxLengthon transaction ids, and inventing one would reject payloads the server accepts, the exact over-reach the validate doctrine (spec-stated invariants, and only those) exists to prevent.Update,Create,CreateBatch, andPatchByIDbehavior is untouched — verified call-site by call-site in review;MarshalJSONis untouched, so wire bytes for accepted inputs are unchanged.Adversarial review (2 rounds, 3 personas each)
### Changedheading under[Unreleased]; "request went out" test would miss silent id truncation; waiver-table move with a bogus reason survives mutation (table ≠ gate)-race -count=20), truncation and dropped-field mutants die by name, production code byte-identical across roundsMutation campaign: 6/7 killed in round 1 (validate removal, byte-count swap, both off-by-ones, delegation drop, index annotation) + both new classes in round 2 (silent truncation, dropped
import_id). The sole survivor is the documented table-move mutant, guarded independently by the behavior test.Testing
make local-ciexit 0; coverage 97.1%; newTransactionPatch.validateat 100%, no stranded statements.CHANGELOGentry included.