Fix IETF parameter parsing. #651
Conversation
WalkthroughThe Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code graph analysis (1)js/moq/src/ietf/parameters.ts (1)
🔇 Additional comments (4)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq/src/ietf/publish_namespace.rs (1)
213-223: Update test to expect successful decoding with parameters.The test
test_announce_rejects_parametersexpects decoding to fail (line 221:assert!(result.is_err())), but this expectation is outdated. TheParameters::decodeimplementation at rs/moq/src/ietf/parameters.rs:10-49 successfully decodes non-empty parameter counts. It only returns errors when the count exceedsMAX_PARAMSor duplicate keys are encountered—not for num_params = 1.Since the PublishNamespace decoder at line 35 calls
Parameters::decode(r)?without special rejection logic, the test should be updated to expectOkand verify the decoded message structure:fn test_announce_rejects_parameters() { #[rustfmt::skip] let invalid_bytes = vec![ 0x01, // namespace length 0x04, 0x74, 0x65, 0x73, 0x74, // "test" 0x01, // num_params = 1 ]; let result: Result<PublishNamespace, _> = decode_message(&invalid_bytes); assert!(result.is_ok()); }Alternatively, rename the test to reflect that parameters are successfully ignored, or remove it if parameter decoding is no longer a concern.
🧹 Nitpick comments (1)
rs/moq/src/ietf/parameters.rs (1)
71-77: Add typed setters for even/odd parameter kinds
setcurrently trusts callers to hand-encode the Value field, even when the kind is even. The IETF draft requires even kinds to be a single varint with canonical QUIC encoding (correct prefix bits, minimum length). Hand-rolling those bytes is very easy to get wrong, and one stray slice like[0x00, 0x3f]will trip a KEY_VALUE_FORMATTING_ERROR on the wire. Let’s give callers a safe helper that does the varint encoding internally (and a separate helper for odd kinds) so misuse isn’t possible.- pub fn set(&mut self, kind: u64, value: Vec<u8>) { - self.0.insert(kind, value); - } + pub fn set_varint(&mut self, kind: u64, value: u64) { + debug_assert_eq!(kind % 2, 0, "set_varint expects an even parameter kind"); + + let mut encoded = Vec::new(); + value.encode(&mut encoded); + self.0.insert(kind, encoded); + } + + pub fn set_bytes(&mut self, kind: u64, value: Vec<u8>) { + debug_assert_eq!(kind % 2, 1, "set_bytes expects an odd parameter kind"); + self.0.insert(kind, value); + }(After adding these, migrate call sites like
Session::connect/acceptto useset_varintfor even code points andset_bytesfor odd ones.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
rs/moq/src/coding/mod.rs(0 hunks)rs/moq/src/ietf/fetch.rs(1 hunks)rs/moq/src/ietf/mod.rs(2 hunks)rs/moq/src/ietf/parameters.rs(1 hunks)rs/moq/src/ietf/publish.rs(1 hunks)rs/moq/src/ietf/publish_namespace.rs(1 hunks)rs/moq/src/ietf/setup.rs(1 hunks)rs/moq/src/ietf/subscribe.rs(1 hunks)rs/moq/src/ietf/subscribe_namespace.rs(1 hunks)rs/moq/src/ietf/track.rs(1 hunks)rs/moq/src/lite/mod.rs(2 hunks)rs/moq/src/lite/parameters.rs(1 hunks)rs/moq/src/lite/setup.rs(1 hunks)rs/moq/src/session.rs(4 hunks)
💤 Files with no reviewable changes (1)
- rs/moq/src/coding/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
rs/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write Rust tests integrated within source files (unit tests alongside code).
Files:
rs/moq/src/ietf/setup.rsrs/moq/src/ietf/mod.rsrs/moq/src/lite/parameters.rsrs/moq/src/ietf/subscribe.rsrs/moq/src/ietf/parameters.rsrs/moq/src/ietf/publish_namespace.rsrs/moq/src/lite/mod.rsrs/moq/src/ietf/fetch.rsrs/moq/src/ietf/subscribe_namespace.rsrs/moq/src/ietf/track.rsrs/moq/src/lite/setup.rsrs/moq/src/session.rsrs/moq/src/ietf/publish.rs
🧬 Code graph analysis (11)
rs/moq/src/ietf/setup.rs (1)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/lite/parameters.rs (1)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/ietf/subscribe.rs (1)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/ietf/parameters.rs (5)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)rs/moq/src/ietf/fetch.rs (12)
decode(67-100)decode(125-138)decode(161-174)decode(193-202)decode(216-219)decode(237-240)encode(32-63)encode(116-123)encode(152-159)encode(187-191)encode(212-214)encode(231-233)rs/moq/src/ietf/publish.rs (6)
decode(136-147)decode(182-205)decode(236-245)encode(129-134)encode(164-180)encode(230-234)rs/moq/src/ietf/publish_namespace.rs (10)
decode(30-41)decode(57-60)decode(80-90)decode(105-108)decode(128-137)encode(24-28)encode(53-55)encode(74-78)encode(101-103)encode(122-126)rs/moq/src/ietf/setup.rs (4)
decode(20-25)decode(52-57)encode(28-31)encode(47-50)
rs/moq/src/ietf/publish_namespace.rs (1)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/ietf/fetch.rs (2)
rs/moq/src/ietf/namespace.rs (2)
decode_namespace(19-33)encode_namespace(4-16)js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/ietf/subscribe_namespace.rs (1)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/ietf/track.rs (1)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/lite/setup.rs (1)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
rs/moq/src/session.rs (2)
js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)js/moq/src/ietf/setup.ts (1)
ServerSetup(72-123)
rs/moq/src/ietf/publish.rs (2)
rs/moq/src/ietf/namespace.rs (2)
decode_namespace(19-33)encode_namespace(4-16)js/moq/src/ietf/parameters.ts (1)
Parameters(3-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (8)
rs/moq/src/ietf/mod.rs (1)
8-8: LGTM! Parameters module properly exposed.The module declaration and public re-export follow standard Rust patterns and correctly expose the new Parameters type from the ietf module.
Also applies to: 26-26
rs/moq/src/lite/setup.rs (1)
1-4: LGTM! Import path correctly updated.The import statement has been properly updated to reference Parameters from the lite module, aligning with the module reorganization.
rs/moq/src/ietf/track.rs (1)
9-9: LGTM! Import path correctly updated.Parameters now correctly imported from
crate::ietf, aligning with the module reorganization. The decode logic remains unchanged.rs/moq/src/ietf/subscribe_namespace.rs (1)
5-9: LGTM! Import reorganization correctly applied.The import statement properly references Parameters from
crate::ietf, maintaining consistency with the broader module reorganization.rs/moq/src/ietf/setup.rs (1)
1-4: LGTM! Import path correctly updated.Parameters now properly imported from the ietf module, aligning with the refactoring to centralize parameter handling in the ietf namespace.
rs/moq/src/ietf/publish_namespace.rs (1)
5-9: LGTM! Import path correctly updated.Parameters now imported from
crate::ietf, consistent with the module reorganization across the ietf namespace.rs/moq/src/lite/mod.rs (1)
5-5: LGTM! Parameters module properly exposed in lite namespace.The module declaration and public re-export correctly expose the lite-specific Parameters type, following standard Rust module patterns.
Also applies to: 17-17
rs/moq/src/ietf/fetch.rs (1)
4-7: LGTM! Import reorganization correctly applied.Parameters has been properly moved from the coding module imports to the ietf module imports. The decode logic for Fetch and FetchOk remains functionally unchanged.
Summary by CodeRabbit