Unpack the payload in the dotrain TryFrom impls - #246
Conversation
DotrainSourceV1 and OrderBuilderStateV1 read value.payload directly, so a Deflate-encoded item that unpack() decodes correctly failed with a utf8 or cbor error. Every other TryFrom<RainMetaDocumentV1Item> honours content_encoding via unpack(). Closes #162 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 29 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 (2)
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 |
…_meta nesting bound Both sides appended to the tail of order_builder_state_v1.rs's test module. Kept the PR's test_try_from_unpacks_content_encoding and main's nest_document helper plus its three MAX_NESTED_DOCUMENT_DEPTH tests; the TryFrom impl keeps value.unpack()? and extract_from_meta keeps its depth-bounded walk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict in `order_builder_state_v1.rs`: this branch still carried `test_extract_from_meta_nested_rain_document`, which #303 deleted when it made an item claiming the document magic corrupt rather than a document to descend into. Resolved to main's side - the nesting tests are `..._nested_document_is_corrupt` and `..._deep_nesting_costs_one_decode` now - keeping this branch's `test_try_from_unpacks_content_encoding`. The two do not interact: one is about what an item's magic may be, the other about honouring its content_encoding. cargo test --workspace --lib 353 passed 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RoHV66knQy6ya7NNkFLpK
|
@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:
|
Closes #162
DotrainSourceV1andOrderBuilderStateV1built themselves fromvalue.payloaddirectly, ignoring the item'scontent_encoding. AContentEncoding::Deflateitem thatunpack()decodes correctly failed withFromUtf8Error/SerdeCborErroron the compressed bytes.Both now go through
value.unpack()?, which is what every otherTryFrom<RainMetaDocumentV1Item>in the crate does:OpMeta,AuthoringMeta,AuthoringMetaV2,SolidityAbiMeta,JsonAbi,InterpreterCallerMeta,RaindexSignedContextOracleV1,String,Vec<u8>.The issue flags a fork — either dotrain metas are defined to be
ContentEncoding::Nonealways (and the constraint should be checked), orunpack()should be used. This takes the second branch, because the crate has already ruled that way forRaindexSignedContextOracleV1:test_try_from_unpacks_content_encodingthere is on main asserting "TryFrom must unpack() the payload (honouring content_encoding), not read the raw bytes".ContentEncoding::NoneandIdentitydecode to the same bytes, so items produced by the encoders here (From<DotrainSourceV1>andTryFrom<OrderBuilderStateV1>, both emittingContentEncoding::None) round-trip unchanged. No caller outside these two files reaches these impls other than throughunpack_into, which routes both magics here already.QA
meta::types::dotrain::source_v1::tests::test_try_from_unpacks_content_encodingandmeta::types::dotrain::order_builder_state_v1::tests::test_try_from_unpacks_content_encoding— each fails on base (both impls reverted in the working tree to the pre-fixvalue.payloadreads, tests kept:test result: FAILED. 30 passed; 2 failed, panicking onFromUtf8Error { bytes: [120, 156, ...], valid_up_to: 1 }andSerdeCborError(InvalidUtf8, offset: 2)— the zlib78 9cheader). With the fix:test result: ok. 32 passed; 0 failed(cargo test -p rain-metadata --lib meta::types::dotrain::).crates/cli/src/meta/types/dotrain/source_v1.rs:94String::from_utf8(value.unpack()?)->String::from_utf8(value.payload.to_vec())-> killed bysource_v1::tests::test_try_from_unpacks_content_encoding;crates/cli/src/meta/types/dotrain/order_builder_state_v1.rs:147from_slice::<OrderBuilderStateV1>(&value.unpack()?)->from_slice::<OrderBuilderStateV1>(&value.payload)-> killed byorder_builder_state_v1::tests::test_try_from_unpacks_content_encoding. Both mutations restore exactly the reported defect; the 30 pre-existing dotrain tests survive them, which is why the two new ones are needed.ContentEncoding::Deflate.encode()before the item is built ("/* some dotrain code */"; thecreate_test_instance()struct), andRainMetaDocumentV1Item::unpack()is documented to decode the payload "based on the configuration". Every otherTryFrom<RainMetaDocumentV1Item>in the crate already honours it.DotrainSourceV1::try_fromreadingvalue.payloadandOrderBuilderStateV1::try_fromreadingvalue.payload; both covered, one discriminating test each. The issue's alternative branch (assertContentEncoding::Noneinstead of unpacking) is deliberately not taken, for the reason above.🤖 Generated with Claude Code