Ignore unknown cbor map keys on meta item decode - #286
Conversation
The metadata-v1 map shape exists so later conventions can add indexes that older tooling adopts "or not" backwards compatibly, which a decoder that rejects unknown keys makes unreachable. Closes #216 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
1 participant
Closes #216.
The adjudication
#216 is a neutral flag. It reports that the
RainMetaDocumentV1Itemvisitorhard-errors on any cbor map key outside
{0, 1, 2, 3, 4, OaSchema}, notes thatthe spec's stated reason for the map shape is forward compatibility, and holds
back on a verdict because strictness might be protecting hash-addressed
round-tripping. This PR takes ignore the unknown key, on two grounds.
1. The spec text is one-sided. The map shape exists because "It would be
more difficult to represent new indexes that we MAY want to add in the future",
and "the map structure is chosen to facilitate future modifications to the
conventions in this document in a way that tooling can adopt (or not) in a
backwards compatible way". A decoder that rejects unknown keys makes "or not"
unreachable: every future index is a hard break for everything already
deployed. This repo has already run that experiment —
OaSchemawas added asan extra map key under exactly this mechanism, and every build of this crate
from before that addition rejects, outright, every document the SFT frontend
writes today.
2. The counter-argument does not hold on
main. "Strictness protectshash-addressed round-tripping" would require decode -> re-encode to be
byte-preserving. It is not, and unknown keys are not what breaks it — key order
is. Measured on
main's decode path (probe applied to the tree with the strictarm in place, run, then reverted; it passes):
RainMetaDocumentV1Itemis a canonicalising decoder already. An unknown key isone more thing it cannot carry, not a new class of hazard.
That consequence is now explicit instead of implied.
ignored_map_key_is_absent_from_the_reencodingpins it: an item that decodedfrom bytes carrying an unknown key re-encodes without it, and so hashes as what
this version can represent rather than as the bytes it read. The one caller that
re-encodes decoded items,
Store::store_content, therefore caches such an itemunder the hash of its re-encoding — which is strictly more than
maindoes withit, since on
mainthe unknown key failscbor_decodeand the whole documentgoes uncached.
What changed
The visitor's unknown-key arm consumes the value with
serde::de::IgnoredAnyinstead of raising
found unexpected key in the map. Nothing else about itemvalidity moves: keys 0 and 1 are still mandatory, an unknown magic number value
under key 1 is still an error, and the
OaSchemakey still decodes intoschema.Only unsigned integer keys are ignorable. A text key or a negative key still
errors. The spec rules the header names out as keys — "the HTTP string
representations of the keys such as
'Content-Encoding'are NOT supported asthis would allow encoders to produce data that decoders are explicitly trying to
avoid the complexity of reading" — and
next_key::<u64>()is also what lets thisgeneric
Deserializeimpl work on formats other than cbor.non_integer_map_key_errorspins that boundary, and mutation 3 below shows it is a decision rather than an
accident of the key type.
non_oa_schema_extra_map_key_errorsis deleted: it pinned the behaviour this PRchanges. Its handwritten bytes live on in
non_oa_schema_extra_map_key_is_ignoredwith the opposite assertion.
Composition with the other in-flight cbor_decode work
serde_cbor::Valuehop) — theclosest neighbour: same
match, adjacent lines, so a textual conflict iscertain. reject duplicate cbor map keys on meta item decode #276 rewrites the six known-key arms to
set_once(...)and leaves theunknown-key arm exactly as it found it; this PR rewrites only that arm. The
resolution is mechanical — keep reject duplicate cbor map keys on meta item decode #276's arms, keep this
_arm — and it wasmeasured, not assumed: PR reject duplicate cbor map keys on meta item decode #276's head merged with this branch, conflict
resolved that way,
cargo test -p rain-metadata --lib meta::tests::green — 57 passed, 0 failed — withboth sets of tests present. That run also covers the part of reject duplicate cbor map keys on meta item decode #276 that matters
most here: with the
Valuehop gone the visitor reads the cbor streamdirectly, so
next_value::<IgnoredAny>()is what keeps the stream in sync overa skipped key. Semantically the two are complementary — reject duplicate cbor map keys on meta item decode #276 makes a repeat of
a known key an error, and a repeat of an unknown key stays ignored because it
has no slot to overwrite. Enforcing RFC 8949 §5.6 over unknown keys too needs a
set of seen keys on top of reject duplicate cbor map keys on meta item decode #276's guard; that is a follow-up, not a conflict.
key 1 on the sequence element and skips the whole item before the visitor runs;
this diff is inside the visitor and touches no magic handling. An item carrying
both an unknown magic and an unknown key is dropped by fix: drop unknown-magic items in cbor_decode instead of failing the document #274's peek and never
reaches this arm.
opposite halves of one question. Drop cbor items missing mandatory keys 0/1 instead of failing the whole document #267 decides what happens to an item missing a
mandatory key, this decides what happens to an item carrying an extra one.
They meet at
unknown_map_key_does_not_stand_in_for_a_mandatory_key: if Drop cbor items missing mandatory keys 0/1 instead of failing the whole document #267lands first, that test's expectation becomes "the item is dropped" instead of
SerdeCborError— the expectation moves, the diff does not.sequence loop's guard; this diff does not touch
cbor_decodeat all.cbor_decode_documentdelegates to
cbor_decode, so it inherits this behaviour with nothing toresolve.
QA
crates/cli/src/meta/mod.rs):unknown_map_key_index_is_ignored(the issue's own repro, byte for byte),non_oa_schema_extra_map_key_is_ignored,unknown_map_key_consumes_its_whole_value,ignored_map_key_is_absent_from_the_reencoding. All four fail on base —verified by mutation 1, which restores the base arm verbatim:
51 passed; 4 failed, exactly these four.Two further tests bound the new path rather than exercising it, and both pass
on base by construction (base rejects those inputs too, for a different
reason):
non_integer_map_key_errors, killed by mutation 3;unknown_map_key_does_not_stand_in_for_a_mandatory_key, killed by no mutationof this diff — it is here because an item can now consist of ignorable keys
plus nothing else, and the mandatory-key requirement has to survive that.
reverted and the suite re-run green (
55 passed; 0 failed):_arm -> base'sother => Err(serde::de::Error::custom(...))?(never ignore) -> killed by all four discriminating tests (
51 passed; 4 failed)._ => { map.next_value::<IgnoredAny>()?; }->_ => {}(recognise the keyas ignorable but never consume its value) -> killed by the same four
(
51 passed; 4 failed), so the consume is load-bearing and not decoration.next_key::<u64>()->next_key::<serde_cbor::Value>()with the armsrewritten to
Value::Integer(..), i.e. the alternative that ignores keys ofevery cbor type -> killed by
non_integer_map_key_errorsalone(
54 passed; 1 failed), which is the evidence that the integer-only limitis a real decision and that nothing else in the suite depends on it.
(https://github.com/rainlanguage/specs/blob/main/metadata-v1.md), quoted above,
for both the ignore and the integer-only limit. Every fixture is handwritten
byte by byte from RFC 8949 major types (
0xa3map(3),0x40/0x41bytes,0x1bu64,0x61text(1),0x20-1), never produced bycbor_encode, andthe expected decoded items come from the existing
plain_itemhelper. The oneassertion that does compare against
cbor_encodeoutput isignored_map_key_is_absent_from_the_reencoding, which pins the re-encodingagainst the independently handwritten
handwritten_map()as well.set fails the decode of the whole sequence) and asks for one decision
(strict vs forward-compatible), flagging one trade-off (hash round-tripping).
All three are covered: the decision is taken and argued from the spec, the
repro decodes, and the trade-off is measured on
main, found already broken bykey order, and its remaining edge pinned by a test.
Verification
cargo test -p rain-metadata --lib— 313 passed, 0 failed.cargo clippy -p rain-metadata --all-targets -- -D warningsandcargo fmt --all --checkclean. The repo-wide suite was not run locally, byrequest — this machine is running many concurrent agents; CI covers it.
🤖 Generated with Claude Code