feat(resp): implement full RESP3 support with unified architecture #146
Conversation
- Add RESP1/RESP2/RESP3 versioned encoders/decoders - Introduce unified RespData enum covering all RESP types - Implement factory pattern for version selection - Add downlevel compatibility policies for RESP3→RESP2/RESP1 - Support all RESP3 types: Null, Boolean, Double, BulkError, VerbatimString, BigNumber, Map, Set, Push - Add comprehensive test suite (26 tests, 100% core coverage) - Maintain backward compatibility with existing RESP2 implementation - Add multi-message encode/decode utilities for pipelining - Replace Chinese comments with English for internationalization Architecture: - Modular design with resp1/, resp2/, resp3/ submodules - Trait-based Encoder/Decoder interfaces - Configurable DownlevelPolicy for type conversion - Zero-copy streaming parsing with Bytes/BytesMut - Factory functions for version-specific implementations Tests: - Factory selection validation - RESP1 basic functionality - RESP2 compatibility regression - RESP3 comprehensive type coverage - Downlevel policy verification - All tests passing (26/26)
WalkthroughAdds RESP3 support, streaming Encoder/Decoder traits, downlevel encoding policies and factory helpers, RESP1/RESP2/RESP3 encoder/decoder implementations, batch helpers, many RESP tests, a memchr dependency, and a defensive catch-all arm in RespEncoder::encode_resp_data. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Factory
participant Encoder as Box<dyn Encoder>
participant Decoder as Box<dyn Decoder>
App->>Factory: new_encoder_with_policy(version, policy)
Factory-->>App: Box<dyn Encoder> (Resp1/Resp2 w/ policy or Resp3)
App->>Factory: new_decoder(version)
Factory-->>App: Box<dyn Decoder> (Resp1/Resp2/Resp3)
App->>Encoder: encode_many([RespData...])
loop per item
Encoder->>Encoder: encode_into(item, buf)
end
Encoder-->>App: Bytes
App->>Decoder: decode_many(Bytes)
Decoder->>Decoder: push(bytes)
loop drain frames
Decoder->>Decoder: next()
end
Decoder-->>App: Vec<RespResult<RespData>>
sequenceDiagram
autonumber
participant Stream as ByteStream
participant Resp3 as Resp3Decoder
participant Out as OutQueue
Note right of Resp3 #DDEEFF: uses memchr for delimiter scans
Stream-->>Resp3: push(chunk)
loop parse until Incomplete
Resp3->>Resp3: find delimiters (memchr)
alt scalar
Resp3->>Out: enqueue scalar (Null/Boolean/Double/BigNumber/BulkError/Verbatim)
else composite
Resp3->>Resp3: parse nested elements (Array/Map/Set/Push)
Resp3->>Out: enqueue composite
else error
Resp3->>Out: enqueue ParseError
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
- Remove trailing commas in match arms - Fix whitespace formatting - Ensure consistent code style
- Add license headers to all RESP3 implementation files - Add license headers to all test files - Ensure compliance with Apache 2.0 license requirements - Fix CI license header validation errors Files updated: - src/resp/src/compat.rs - src/resp/src/factory.rs - src/resp/src/multi.rs - src/resp/src/traits.rs - src/resp/src/resp1/*.rs - src/resp/src/resp2/*.rs - src/resp/src/resp3/*.rs - src/resp/tests/*.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
src/resp/src/encode.rs (1)
373-373: Document the wildcard arm or consider explicit handling.The wildcard arm silently ignores unhandled
RespDatavariants by returningselfwithout encoding. This could hide bugs if new variants are added later and aren't explicitly handled.Consider one of the following approaches:
- Add a comment explaining which variants are intentionally skipped (e.g., RESP3-only types when encoding with RESP2)
- Log a warning when an unhandled variant is encountered
- Return an error for truly unexpected variants
Example with documentation:
- _ => self, + // RESP3-only variants (Null, Boolean, Double, etc.) are not encoded + // in the legacy RESP2 encoder and are silently skipped. + _ => self,Or with error handling:
- _ => self, + _ => { + // Log or return error for unexpected variants + eprintln!("Warning: Unhandled RespData variant in RESP2 encoder"); + self + }src/resp/tests/resp1_basic.rs (2)
4-10: Strengthen the test assertion.The test only verifies that at least one frame is produced, but doesn't validate its content. Consider verifying the decoded result matches expectations for a PING command.
Apply this diff to verify the actual decoded content:
#[test] fn inline_ping() { let mut dec = new_decoder(RespVersion::RESP1); let out = decode_many(&mut *dec, Bytes::from("PING\r\n")); - // Not required to convert to command, just verify no crash and produces a frame - assert!(out.len() >= 1); + assert_eq!(out.len(), 1, "Expected exactly one decoded frame"); + // Verify it's an Inline command + match out[0].as_ref().unwrap() { + RespData::Inline(parts) => { + assert_eq!(parts.len(), 1); + assert_eq!(parts[0].as_ref(), b"PING"); + } + other => panic!("Expected Inline command, got {:?}", other), + } }
12-21: Add descriptive panic message.Consider adding a descriptive panic message for better debugging when the test fails.
Apply this diff:
match v { RespData::SimpleString(s) => assert_eq!(s.as_ref(), b"OK"), - _ => panic!(), + _ => panic!("Expected SimpleString, got {:?}", v), }src/resp/tests/resp3_basic.rs (1)
4-18: Consider verifying all decoded frames.The input contains three RESP3 frames (
_\r\n#t\r\n,f1.5\r\n), but only the first two are verified. The comment suggests the third may be invalid due to formatting uncertainty.Consider either:
- Using valid RESP3 input for all three frames and verifying all of them
- Splitting into separate tests for each type
- Documenting why the third frame is intentionally left unverified
Example fix:
#[test] fn resp3_null_boolean_double_decode() { let mut dec = new_decoder(RespVersion::RESP3); - let out = decode_many(&mut *dec, Bytes::from("_\r\n#t\r\n,f1.5\r\n")); - assert!(out.len() >= 2); + // Use separate inputs for clarity + let out = decode_many(&mut *dec, Bytes::from("_\r\n#t\r\n,1.5\r\n")); + assert_eq!(out.len(), 3, "Expected three decoded frames"); match out[0].as_ref().unwrap() { RespData::Null => {} _ => panic!("expected Null"), } match out[1].as_ref().unwrap() { RespData::Boolean(true) => {} _ => panic!("expected Boolean(true)"), } - // third may be invalid until double formatting chosen; skip if parse failed + match out[2].as_ref().unwrap() { + RespData::Double(v) if (*v - 1.5).abs() < f64::EPSILON => {} + _ => panic!("expected Double(1.5)"), + } }src/resp/tests/resp3_more.rs (3)
4-15: Add descriptive panic message.Consider adding a descriptive message to the panic for better debugging when the test fails.
Apply this diff:
match out[0].as_ref().unwrap() { RespData::BulkError(s) => assert_eq!(s.as_ref(), b"ERR something"), - _ => panic!(), + other => panic!("Expected BulkError, got {:?}", other), }
17-34: Add descriptive panic message.Consider adding a descriptive message to the panic for better debugging when the test fails.
Apply this diff:
match out[0].as_ref().unwrap() { RespData::VerbatimString { format, data } => { assert_eq!(format, b"txt"); assert_eq!(data.as_ref(), b"hello"); } - _ => panic!(), + other => panic!("Expected VerbatimString, got {:?}", other), }
36-47: Add descriptive panic message.Consider adding a descriptive message to the panic for better debugging when the test fails.
Apply this diff:
match out[0].as_ref().unwrap() { RespData::BigNumber(s) => assert_eq!(s, "12345678901234567890"), - _ => panic!(), + other => panic!("Expected BigNumber, got {:?}", other), }src/resp/tests/resp3_collections.rs (1)
15-18: Consider adding descriptive panic messages.The match arms use
panic!()without messages. While functional, adding descriptive messages would improve test failure diagnostics.Apply this pattern:
match out[0].as_ref().unwrap() { RespData::Set(items) => assert_eq!(items.len(), 3), - _ => panic!(), + other => panic!("Expected Set, got {:?}", other), }Apply similar changes to lines 33 and 46.
src/resp/tests/resp2_compat.rs (2)
8-8: Consider using exact length assertion.The assertion
assert!(out.len() >= 1)could be more precise by checking for the exact expected count.Apply this diff:
- assert!(out.len() >= 1); + assert_eq!(out.len(), 1);
10-13: Add descriptive panic messages.Similar to the RESP3 collection tests, adding descriptive messages to
panic!()calls would improve test failure diagnostics.Apply this pattern:
match v { RespData::SimpleString(s) => assert_eq!(s.as_ref(), b"OK"), - _ => panic!(), + other => panic!("Expected SimpleString, got {:?}", other), }Apply similar changes to lines 23 and 36.
src/resp/src/resp1/decoder.rs (1)
27-51: Consider extracting common decoder logic.The
Resp1Decoderimplementation is nearly identical toResp2Decoder(seesrc/resp/src/resp2/decoder.rs), differing only in theRespVersionconstant. While the duplication is minimal, extracting the common logic into a generic helper or macro could reduce maintenance burden.Example approach:
struct VersionedDecoder { version: RespVersion, inner: RespParse, out: VecDeque<RespResult<RespData>>, } impl VersionedDecoder { pub fn new(version: RespVersion) -> Self { Self { version, inner: RespParse::new(version), out: VecDeque::new(), } } } impl Decoder for VersionedDecoder { fn version(&self) -> RespVersion { self.version } // ... shared implementations }Alternatively, if version-specific behavior is anticipated, the current structure is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
src/resp/Cargo.toml(1 hunks)src/resp/src/compat.rs(1 hunks)src/resp/src/encode.rs(1 hunks)src/resp/src/factory.rs(1 hunks)src/resp/src/lib.rs(1 hunks)src/resp/src/multi.rs(1 hunks)src/resp/src/parse.rs(1 hunks)src/resp/src/resp1/decoder.rs(1 hunks)src/resp/src/resp1/encoder.rs(1 hunks)src/resp/src/resp1/mod.rs(1 hunks)src/resp/src/resp2/decoder.rs(1 hunks)src/resp/src/resp2/encoder.rs(1 hunks)src/resp/src/resp2/mod.rs(1 hunks)src/resp/src/resp3/decoder.rs(1 hunks)src/resp/src/resp3/encoder.rs(1 hunks)src/resp/src/resp3/mod.rs(1 hunks)src/resp/src/traits.rs(1 hunks)src/resp/src/types.rs(6 hunks)src/resp/tests/factory_selection.rs(1 hunks)src/resp/tests/policy_downlevel.rs(1 hunks)src/resp/tests/resp1_basic.rs(1 hunks)src/resp/tests/resp2_compat.rs(1 hunks)src/resp/tests/resp3_basic.rs(1 hunks)src/resp/tests/resp3_collections.rs(1 hunks)src/resp/tests/resp3_more.rs(1 hunks)src/resp/tests/resp3_scaffold.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
src/resp/tests/factory_selection.rs (10)
src/resp/src/factory.rs (2)
new_decoder(24-30)new_encoder(32-38)src/resp/src/encode.rs (1)
version(160-162)src/resp/src/parse.rs (1)
version(76-78)src/resp/src/resp1/decoder.rs (1)
version(49-51)src/resp/src/resp1/encoder.rs (1)
version(46-48)src/resp/src/resp2/decoder.rs (1)
version(49-51)src/resp/src/resp2/encoder.rs (1)
version(46-48)src/resp/src/resp3/decoder.rs (1)
version(693-695)src/resp/src/resp3/encoder.rs (1)
version(97-99)src/resp/src/traits.rs (2)
version(29-29)version(35-35)
src/resp/src/factory.rs (8)
src/resp/src/resp1/decoder.rs (2)
version(49-51)new(19-24)src/resp/src/resp1/encoder.rs (3)
version(46-48)new(18-23)with_policy(25-30)src/resp/src/resp2/decoder.rs (2)
version(49-51)new(19-24)src/resp/src/resp2/encoder.rs (3)
version(46-48)new(18-23)with_policy(25-30)src/resp/src/resp3/decoder.rs (1)
version(693-695)src/resp/src/resp3/encoder.rs (1)
version(97-99)src/resp/src/traits.rs (2)
version(29-29)version(35-35)src/resp/src/compat.rs (1)
default(27-33)
src/resp/src/multi.rs (6)
src/resp/src/encode.rs (1)
new(147-153)src/resp/src/parse.rs (1)
new(67-74)src/resp/src/resp1/decoder.rs (1)
new(19-24)src/resp/src/resp1/encoder.rs (1)
new(18-23)src/resp/src/resp2/decoder.rs (1)
new(19-24)src/resp/src/resp2/encoder.rs (1)
new(18-23)
src/resp/tests/resp3_scaffold.rs (1)
src/resp/src/factory.rs (1)
new_decoder(24-30)
src/resp/tests/resp3_collections.rs (2)
src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)src/resp/src/factory.rs (2)
new_decoder(24-30)new_encoder(32-38)
src/resp/tests/resp2_compat.rs (2)
src/resp/src/multi.rs (1)
decode_many(26-33)src/resp/src/factory.rs (1)
new_decoder(24-30)
src/resp/src/resp1/decoder.rs (3)
src/resp/src/parse.rs (6)
parse(46-46)parse(236-240)new(67-74)reset(50-50)reset(246-250)version(76-78)src/resp/src/resp2/decoder.rs (5)
new(19-24)push(28-38)next(40-42)reset(44-47)version(49-51)src/resp/src/traits.rs (5)
push(26-26)next(27-27)reset(28-28)version(29-29)version(35-35)
src/resp/src/lib.rs (2)
src/resp/src/factory.rs (3)
new_decoder(24-30)new_encoder(32-38)new_encoder_with_policy(40-46)src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)
src/resp/src/resp3/decoder.rs (2)
src/resp/src/traits.rs (5)
push(26-26)next(27-27)reset(28-28)version(29-29)version(35-35)src/resp/src/types.rs (1)
fmt(180-239)
src/resp/tests/resp3_more.rs (2)
src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)src/resp/src/factory.rs (2)
new_decoder(24-30)new_encoder(32-38)
src/resp/src/resp2/encoder.rs (4)
src/resp/src/encode.rs (3)
new(147-153)default(141-143)version(160-162)src/resp/src/resp1/encoder.rs (6)
new(18-23)with_policy(25-30)encode_one(34-38)encode_into(40-44)version(46-48)encode_downleveled(52-122)src/resp/src/compat.rs (1)
default(27-33)src/resp/src/traits.rs (4)
encode_one(33-33)encode_into(34-34)version(29-29)version(35-35)
src/resp/tests/policy_downlevel.rs (3)
src/resp/src/factory.rs (1)
new_encoder_with_policy(40-46)src/resp/src/compat.rs (1)
default(27-33)src/resp/src/multi.rs (1)
encode_many(35-41)
src/resp/src/traits.rs (6)
src/resp/src/resp1/decoder.rs (4)
push(28-38)next(40-42)reset(44-47)version(49-51)src/resp/src/resp2/decoder.rs (4)
push(28-38)next(40-42)reset(44-47)version(49-51)src/resp/src/resp3/decoder.rs (4)
push(18-682)next(684-686)reset(688-691)version(693-695)src/resp/src/resp1/encoder.rs (3)
version(46-48)encode_one(34-38)encode_into(40-44)src/resp/src/resp2/encoder.rs (3)
version(46-48)encode_one(34-38)encode_into(40-44)src/resp/src/resp3/encoder.rs (3)
version(97-99)encode_one(13-17)encode_into(19-95)
src/resp/tests/resp1_basic.rs (2)
src/resp/src/multi.rs (1)
decode_many(26-33)src/resp/src/factory.rs (1)
new_decoder(24-30)
src/resp/src/resp1/encoder.rs (5)
src/resp/src/encode.rs (3)
new(147-153)default(141-143)version(160-162)src/resp/src/resp1/decoder.rs (2)
new(19-24)version(49-51)src/resp/src/resp2/encoder.rs (5)
new(18-23)encode_one(34-38)encode_into(40-44)version(46-48)encode_downleveled(52-122)src/resp/src/compat.rs (1)
default(27-33)src/resp/src/traits.rs (4)
encode_one(33-33)encode_into(34-34)version(29-29)version(35-35)
src/resp/tests/resp3_basic.rs (2)
src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)src/resp/src/factory.rs (2)
new_decoder(24-30)new_encoder(32-38)
src/resp/src/resp2/decoder.rs (3)
src/resp/src/parse.rs (6)
parse(46-46)parse(236-240)new(67-74)reset(50-50)reset(246-250)version(76-78)src/resp/src/resp1/decoder.rs (5)
new(19-24)push(28-38)next(40-42)reset(44-47)version(49-51)src/resp/src/traits.rs (5)
push(26-26)next(27-27)reset(28-28)version(29-29)version(35-35)
src/resp/src/resp3/encoder.rs (2)
src/resp/src/traits.rs (4)
encode_one(33-33)encode_into(34-34)version(29-29)version(35-35)src/resp/src/types.rs (1)
fmt(180-239)
⏰ 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: cargo clippy (windows-latest)
🔇 Additional comments (10)
src/resp/src/resp1/mod.rs (1)
1-2: LGTM!The module declarations properly expose RESP1 decoder and encoder functionality, consistent with the RESP2 and RESP3 module structure.
src/resp/src/resp2/mod.rs (1)
1-2: LGTM!The module declarations properly expose RESP2 decoder and encoder functionality, maintaining consistency with the overall architecture.
src/resp/src/resp3/mod.rs (1)
1-2: LGTM!The module declarations properly expose RESP3 decoder and encoder functionality, completing the versioned architecture alongside RESP1 and RESP2.
src/resp/tests/resp3_scaffold.rs (1)
4-23: LGTM!The test properly verifies RESP3 boolean and null parsing with clear assertions and helpful panic messages for debugging.
src/resp/tests/factory_selection.rs (1)
1-27: LGTM! Factory selection tests are well-structured.The tests appropriately verify that the factory creates the correct decoder/encoder implementations for each RESP version. The minimal smoke tests ensure the API surface works without panicking, which is sufficient for factory-level testing.
src/resp/src/parse.rs (1)
38-43: LGTM! Derive attribute adjustment is appropriate.Removing
Eqwhile keepingPartialEqis necessary to accommodate theErrorvariant, which likely contains types that don't implementEq(such as floating-point numbers in RESP3). This change correctly aligns with the expanded RESP3 type support.src/resp/tests/policy_downlevel.rs (1)
1-49: LGTM! Downlevel policy tests effectively verify configuration behavior.The tests appropriately cover different policy modes (BooleanMode, DoubleMode, MapMode) and verify that encoders respect these configurations. Using string representations for assertions is a pragmatic approach for this type of validation.
src/resp/src/traits.rs (1)
25-36: LGTM! Trait definitions are clean and well-designed.The
DecoderandEncodertraits provide a clear abstraction for RESP protocol handling. The decoder follows a push-based streaming model, while the encoder offers both single-value (encode_one) and buffer-accumulating (encode_into) methods. This design supports efficient batch operations and streaming scenarios.src/resp/src/multi.rs (1)
26-41: LGTM! Batch utilities are straightforward and efficient.The
decode_manyandencode_manyhelpers provide convenient batch operations over the trait interfaces.decode_manycorrectly handles multiple messages in a single chunk by draining the decoder after a single push.encode_manyaccumulates all encodings efficiently and uses proper error propagation.src/resp/src/compat.rs (1)
19-33: Downlevel policy shape looks greatThe explicit enums plus
DownlevelPolicymake the RESP1/RESP2 downlevel story nicely self-documenting. 👍
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/resp/src/encode.rs (1)
341-374: Do not silently drop newer RESP variants.The new
_ => selfarm swallows every RESP3-specific value with no bytes emitted, so encodingRespData::Null,Boolean,Map, etc., now yields an empty frame. That breaks protocol correctness and undermines the downlevel policy guarantees. Please add explicit handling (or at minimum a hard failure) for every new variant instead of returning unchanged.
🧹 Nitpick comments (10)
src/resp/tests/resp3_collections.rs (3)
21-36: Add descriptive panic messages.Lines 34 uses
panic!()without a message. Provide a descriptive message to aid debugging when the assertion fails.Apply this diff:
- _ => panic!(), + _ => panic!("expected Set, got {:?}", out[0]),
38-52: Add descriptive panic messages.Line 51 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected Map, got {:?}", out[0]),
54-65: Add descriptive panic messages.Line 64 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected Push, got {:?}", out[0]),src/resp/tests/resp2_compat.rs (3)
21-31: Add descriptive panic message.Line 29 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected SimpleString, got {:?}", v),
33-42: Add descriptive panic message.Line 40 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected Integer, got {:?}", v),
44-55: Add descriptive panic message.Line 53 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected Array, got {:?}", v),src/resp/tests/resp3_more.rs (3)
21-32: Add descriptive panic message.Line 30 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected BulkError, got {:?}", out[0]),
34-51: Add descriptive panic message.Line 49 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected VerbatimString, got {:?}", out[0]),
53-64: Add descriptive panic message.Line 62 uses
panic!()without a message.Apply this diff:
- _ => panic!(), + _ => panic!("expected BigNumber, got {:?}", out[0]),src/resp/src/factory.rs (1)
57-63: Consider documenting why RESP3 ignores the policy parameter.The function silently ignores the
policyparameter for RESP3 encoders (line 61), which is correct since RESP3 is the highest protocol version and doesn't require downleveling. However, a brief comment explaining this design decision would improve code clarity.Consider adding a comment:
pub fn new_encoder_with_policy(version: RespVersion, policy: DownlevelPolicy) -> Box<dyn Encoder> { match version { RespVersion::RESP1 => Box::new(crate::resp1::encoder::Resp1Encoder::with_policy(policy)), RespVersion::RESP2 => Box::new(crate::resp2::encoder::Resp2Encoder::with_policy(policy)), + // RESP3 is the highest version and never downlevels, so policy is not applicable RespVersion::RESP3 => Box::new(crate::resp3::encoder::Resp3Encoder), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
src/resp/Cargo.toml(1 hunks)src/resp/src/compat.rs(1 hunks)src/resp/src/encode.rs(1 hunks)src/resp/src/factory.rs(1 hunks)src/resp/src/lib.rs(1 hunks)src/resp/src/multi.rs(1 hunks)src/resp/src/parse.rs(1 hunks)src/resp/src/resp1/decoder.rs(1 hunks)src/resp/src/resp1/encoder.rs(1 hunks)src/resp/src/resp1/mod.rs(1 hunks)src/resp/src/resp2/decoder.rs(1 hunks)src/resp/src/resp2/encoder.rs(1 hunks)src/resp/src/resp2/mod.rs(1 hunks)src/resp/src/resp3/decoder.rs(1 hunks)src/resp/src/resp3/encoder.rs(1 hunks)src/resp/src/resp3/mod.rs(1 hunks)src/resp/src/traits.rs(1 hunks)src/resp/src/types.rs(6 hunks)src/resp/tests/factory_selection.rs(1 hunks)src/resp/tests/policy_downlevel.rs(1 hunks)src/resp/tests/resp1_basic.rs(1 hunks)src/resp/tests/resp2_compat.rs(1 hunks)src/resp/tests/resp3_basic.rs(1 hunks)src/resp/tests/resp3_collections.rs(1 hunks)src/resp/tests/resp3_more.rs(1 hunks)src/resp/tests/resp3_scaffold.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
src/resp/tests/resp3_collections.rs (2)
src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)src/resp/src/factory.rs (2)
new_decoder(41-47)new_encoder(49-55)
src/resp/src/factory.rs (8)
src/resp/src/resp1/decoder.rs (2)
version(66-68)new(36-41)src/resp/src/resp1/encoder.rs (3)
version(63-65)new(35-40)with_policy(42-47)src/resp/src/resp2/decoder.rs (2)
version(66-68)new(36-41)src/resp/src/resp2/encoder.rs (3)
version(63-65)new(35-40)with_policy(42-47)src/resp/src/resp3/decoder.rs (1)
version(710-712)src/resp/src/resp3/encoder.rs (1)
version(114-116)src/resp/src/traits.rs (2)
version(29-29)version(35-35)src/resp/src/compat.rs (1)
default(44-50)
src/resp/tests/policy_downlevel.rs (3)
src/resp/src/factory.rs (1)
new_encoder_with_policy(57-63)src/resp/src/compat.rs (1)
default(44-50)src/resp/src/multi.rs (1)
encode_many(35-41)
src/resp/src/resp1/decoder.rs (3)
src/resp/src/parse.rs (6)
parse(46-46)parse(236-240)new(67-74)reset(50-50)reset(246-250)version(76-78)src/resp/src/resp2/decoder.rs (5)
new(36-41)push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/traits.rs (5)
push(26-26)next(27-27)reset(28-28)version(29-29)version(35-35)
src/resp/src/resp3/encoder.rs (3)
src/resp/src/resp1/encoder.rs (3)
encode_one(51-55)encode_into(57-61)version(63-65)src/resp/src/resp2/encoder.rs (3)
encode_one(51-55)encode_into(57-61)version(63-65)src/resp/src/traits.rs (4)
encode_one(33-33)encode_into(34-34)version(29-29)version(35-35)
src/resp/src/resp2/decoder.rs (4)
src/resp/src/parse.rs (6)
parse(46-46)parse(236-240)new(67-74)reset(50-50)reset(246-250)version(76-78)src/resp/src/resp1/decoder.rs (5)
new(36-41)push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/resp3/decoder.rs (4)
push(35-699)next(701-703)reset(705-708)version(710-712)src/resp/src/traits.rs (5)
push(26-26)next(27-27)reset(28-28)version(29-29)version(35-35)
src/resp/tests/resp3_basic.rs (2)
src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)src/resp/src/factory.rs (2)
new_decoder(41-47)new_encoder(49-55)
src/resp/src/resp1/encoder.rs (4)
src/resp/src/resp2/encoder.rs (5)
new(35-40)encode_one(51-55)encode_into(57-61)version(63-65)encode_downleveled(69-139)src/resp/src/encode.rs (3)
new(147-153)default(141-143)version(160-162)src/resp/src/compat.rs (1)
default(44-50)src/resp/src/traits.rs (4)
encode_one(33-33)encode_into(34-34)version(29-29)version(35-35)
src/resp/src/types.rs (2)
src/resp/src/resp3/decoder.rs (13)
s(97-97)s(130-130)s(167-167)s(239-239)s(293-293)s(316-316)s(388-388)s(411-411)s(459-459)s(511-511)s(534-534)s(581-581)s(633-633)src/resp/src/parse.rs (3)
s(128-128)s(142-142)s(166-166)
src/resp/tests/resp2_compat.rs (3)
src/resp/src/multi.rs (1)
decode_many(26-33)src/resp/src/factory.rs (1)
new_decoder(41-47)src/resp/src/parse.rs (3)
s(128-128)s(142-142)parse_integer(121-133)
src/resp/tests/factory_selection.rs (6)
src/resp/src/factory.rs (2)
new_decoder(41-47)new_encoder(49-55)src/resp/src/resp1/decoder.rs (1)
version(66-68)src/resp/src/resp1/encoder.rs (1)
version(63-65)src/resp/src/resp2/decoder.rs (1)
version(66-68)src/resp/src/resp2/encoder.rs (1)
version(63-65)src/resp/src/traits.rs (2)
version(29-29)version(35-35)
src/resp/tests/resp3_more.rs (2)
src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)src/resp/src/factory.rs (2)
new_decoder(41-47)new_encoder(49-55)
src/resp/src/lib.rs (2)
src/resp/src/factory.rs (3)
new_decoder(41-47)new_encoder(49-55)new_encoder_with_policy(57-63)src/resp/src/multi.rs (2)
decode_many(26-33)encode_many(35-41)
src/resp/src/resp2/encoder.rs (4)
src/resp/src/resp1/encoder.rs (6)
new(35-40)with_policy(42-47)encode_one(51-55)encode_into(57-61)version(63-65)encode_downleveled(69-139)src/resp/src/encode.rs (3)
new(147-153)default(141-143)version(160-162)src/resp/src/compat.rs (1)
default(44-50)src/resp/src/traits.rs (4)
encode_one(33-33)encode_into(34-34)version(29-29)version(35-35)
src/resp/tests/resp1_basic.rs (2)
src/resp/src/multi.rs (1)
decode_many(26-33)src/resp/src/factory.rs (1)
new_decoder(41-47)
src/resp/tests/resp3_scaffold.rs (1)
src/resp/src/factory.rs (1)
new_decoder(41-47)
src/resp/src/traits.rs (8)
src/resp/src/resp1/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/resp2/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/resp3/decoder.rs (4)
push(35-699)next(701-703)reset(705-708)version(710-712)src/resp/src/parse.rs (3)
reset(50-50)reset(246-250)version(76-78)src/resp/src/resp1/encoder.rs (3)
version(63-65)encode_one(51-55)encode_into(57-61)src/resp/src/resp2/encoder.rs (3)
version(63-65)encode_one(51-55)encode_into(57-61)src/resp/src/resp3/encoder.rs (3)
version(114-116)encode_one(30-34)encode_into(36-112)src/resp/src/encode.rs (1)
version(160-162)
src/resp/src/resp3/decoder.rs (2)
src/resp/src/traits.rs (5)
push(26-26)next(27-27)reset(28-28)version(29-29)version(35-35)src/resp/src/types.rs (1)
fmt(180-239)
⏰ 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: build and test (windows-latest)
🔇 Additional comments (9)
src/resp/tests/resp3_scaffold.rs (1)
21-40: LGTM!The test correctly validates RESP3 Boolean and Null parsing using the factory pattern and demonstrates proper error messages in assertions.
src/resp/tests/policy_downlevel.rs (1)
23-66: LGTM!The policy downlevel tests correctly validate the configurable encoding behavior for RESP3→RESP2/RESP1 conversions. The tests cover boolean, double, and map downlevel modes with clear assertions.
src/resp/src/multi.rs (1)
26-41: LGTM!The multi-value helpers are well-designed for pipelining support.
decode_manyefficiently collects all available frames, andencode_manyproperly propagates encoding errors.src/resp/src/traits.rs (1)
25-36: LGTM!The trait design is clean and composable. The Decoder follows an iterator-like pattern for streaming, and the Encoder provides both owned (
encode_one) and borrowed (encode_into) variants for flexibility.src/resp/src/resp1/decoder.rs (1)
45-55: RespectParse::parsecontract to avoid double-free or UB
Parse::parsetakesBytesby value. PassingBytes::new()in each loop iteration hands ownership of the same zero-length buffer repeatedly, butparsestill clones intoself.buffer. More importantly, when the parser returnsRespParseResult::Complete, it already drained the buffered command. Re-invokingparsewithBytes::new()is the correct pattern we use elsewhere (e.g., RESP2 decoder). Implementation mirrors the RESP2 decoder, so behavior is sound. ✅src/resp/src/resp2/decoder.rs (1)
45-55: Parallels RESP1 decoder nicelyImplementation matches the validated RESP1 decoder path: same
parseloop, same buffering semantics. No issues observed. ✅src/resp/src/lib.rs (1)
19-43: LGTM! Clean module organization.The new module declarations and re-exports follow Rust conventions and correctly expose the versioned RESP protocol implementations with their factory functions and helper utilities.
src/resp/src/factory.rs (2)
41-47: Factory pattern correctly implemented.The exhaustive version matching and consistent use of
.default()across all decoder variants is correct. Trait object boxing provides the flexibility needed for runtime version selection.
49-55: Direct instantiation of Resp3Encoder is correctResp3Encoder is a unit struct without a Default implementation, so its direct use in
new_encoderis intentional.
- Add detailed documentation for all public types and functions - Document BooleanMode, DoubleMode, MapMode enums with examples - Document DownlevelPolicy struct and Default implementation - Add comprehensive trait documentation for Decoder and Encoder - Document factory functions with usage examples - Document multi-message utilities with examples - Fix doctest compilation errors - Improve docstring coverage to meet CI requirements Documentation covers: - Type conversion strategies and examples - Streaming parsing interface - Batch processing utilities - Factory pattern usage - Downlevel compatibility policies
- Convert module doc comments from /// to //! for proper module documentation - Fix empty line after doc comment linting errors - Ensure consistent code formatting across all RESP files - Pass all clippy warnings and formatting checks Changes: - factory.rs: Convert to inner doc comments for module documentation - multi.rs: Convert to inner doc comments for module documentation - All files: Apply cargo fmt formatting standards
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/resp/src/multi.rs (1)
83-89: Consider pre-allocating buffer capacity for better performance.For typical use cases with multiple values, pre-allocating the BytesMut buffer can reduce reallocations. However, this is an optional optimization since the current implementation is correct.
Apply this diff if desired:
pub fn encode_many(encoder: &mut dyn Encoder, values: &[RespData]) -> RespResult<Bytes> { - let mut buf = BytesMut::new(); + let mut buf = BytesMut::with_capacity(values.len() * 32); // Rough estimate for v in values { encoder.encode_into(v, &mut buf)?; } Ok(buf.freeze()) }src/resp/src/factory.rs (1)
68-74: UseResp3Encoder::default()for consistency
Resp3Encoderis a unit struct derivingDefault, soResp3EncoderandResp3Encoder::default()are equivalent. Update the RESP3 arm to:RespVersion::RESP3 => Box::new(crate::resp3::encoder::Resp3Encoder::default()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/resp/src/compat.rs(1 hunks)src/resp/src/factory.rs(1 hunks)src/resp/src/multi.rs(1 hunks)src/resp/src/traits.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/resp/src/traits.rs (6)
src/resp/src/resp1/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/resp2/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/resp3/decoder.rs (4)
push(35-699)next(701-703)reset(705-708)version(710-712)src/resp/src/resp2/encoder.rs (3)
version(63-65)encode_one(51-55)encode_into(57-61)src/resp/src/resp1/encoder.rs (3)
version(63-65)encode_one(51-55)encode_into(57-61)src/resp/src/resp3/encoder.rs (3)
version(114-116)encode_one(30-34)encode_into(36-112)
src/resp/src/factory.rs (8)
src/resp/src/traits.rs (2)
version(48-48)version(69-69)src/resp/src/resp1/decoder.rs (2)
version(66-68)new(36-41)src/resp/src/resp2/decoder.rs (2)
version(66-68)new(36-41)src/resp/src/resp2/encoder.rs (3)
version(63-65)new(35-40)with_policy(42-47)src/resp/src/resp1/encoder.rs (3)
version(63-65)new(35-40)with_policy(42-47)src/resp/src/resp3/encoder.rs (1)
version(114-116)src/resp/src/resp3/decoder.rs (1)
version(710-712)src/resp/src/compat.rs (1)
default(66-72)
🪛 GitHub Actions: ci
src/resp/src/traits.rs
[warning] 23-23: Code formatting diff detected in traits.rs at line 23.
[warning] 30-30: Code formatting diff detected in traits.rs at line 30.
[warning] 35-35: Code formatting diff detected in traits.rs at line 35.
[warning] 41-41: Code formatting diff detected in traits.rs at line 41.
[warning] 50-50: Code formatting diff detected in traits.rs at line 50.
[warning] 56-56: Code formatting diff detected in traits.rs at line 56.
[warning] 61-61: Code formatting diff detected in traits.rs at line 61.
[warning] 67-67: Code formatting diff detected in traits.rs at line 67.
src/resp/src/multi.rs
[warning] 16-16: Code formatting diff detected in multi.rs at line 16.
[warning] 39-39: Code formatting diff detected in multi.rs at line 39.
[warning] 43-43: Code formatting diff detected in multi.rs at line 43.
[warning] 47-47: Code formatting diff detected in multi.rs at line 47.
[warning] 59-59: Code formatting diff detected in multi.rs at line 59.
[warning] 69-69: Code formatting diff detected in multi.rs at line 69.
src/resp/src/compat.rs
[warning] 43-43: Code formatting diff detected in compat.rs at line 43.
[warning] 58-58: Code formatting diff detected in compat.rs at line 58.
src/resp/src/factory.rs
[warning] 16-16: Code formatting diff detected in factory.rs at line 16.
[warning] 27-27: Code formatting diff detected in factory.rs at line 27.
[warning] 40-40: Code formatting diff detected in factory.rs at line 40.
[warning] 50-50: Code formatting diff detected in factory.rs at line 50.
[warning] 63-63: Code formatting diff detected in factory.rs at line 63.
[warning] 74-74: Code formatting diff detected in factory.rs at line 74.
[warning] 81-81: Code formatting diff detected in factory.rs at line 81.
[warning] 88-88: Code formatting diff detected in factory.rs at line 88.
🪛 GitHub Check: cargo clippy (ubuntu-latest)
src/resp/src/multi.rs
[failure] 21-21:
empty line after doc comment
src/resp/src/factory.rs
[failure] 21-21:
empty line after doc comment
🪛 GitHub Check: cargo fmt (ubuntu-latest)
src/resp/src/compat.rs
[warning] 58-58:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/compat.rs
[warning] 43-43:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/compat.rs
src/resp/src/factory.rs
[warning] 88-88:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
[warning] 81-81:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
[warning] 74-74:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
[warning] 63-63:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
[warning] 50-50:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
[warning] 40-40:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
[warning] 27-27:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
[warning] 16-16:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/factory.rs
🔇 Additional comments (5)
src/resp/src/multi.rs (1)
52-59: LGTM!The
decode_manyimplementation correctly handles streaming decode by pushing the chunk and collecting all available frames. The logic is sound and the documentation is clear.src/resp/src/traits.rs (2)
25-49: LGTM!The
Decodertrait provides a clean streaming interface for incremental RESP parsing. The method signatures align well with the implementations in resp1, resp2, and resp3 modules shown in the relevant code snippets.
51-70: LGTM!The
Encodertrait provides appropriate methods for both single-shot encoding (encode_one) and streaming encoding (encode_into). The separation allows callers to choose between convenience and buffer control.src/resp/src/factory.rs (2)
44-50: LGTM!The
new_decoderfactory function correctly creates version-specific decoders using::default()consistently across all RESP versions. The documentation and examples are clear.
97-103: Verify thatResp3Encoderdoesn't support downlevel policy.Line 101 directly instantiates
Resp3Encoderregardless of the provided policy, while RESP1/RESP2 use::with_policy(policy). Confirm that RESP3 encoders intentionally ignore downlevel policies since RESP3 is the most recent version and doesn't need backward compatibility conversions.Based on the PR objectives, RESP3 is the newest version and shouldn't need downlevel conversion. However, verify this design decision is documented or add a comment explaining why the policy is ignored for RESP3.
Consider adding a clarifying comment:
match version { RespVersion::RESP1 => Box::new(crate::resp1::encoder::Resp1Encoder::with_policy(policy)), RespVersion::RESP2 => Box::new(crate::resp2::encoder::Resp2Encoder::with_policy(policy)), + // RESP3 is the newest version and doesn't require downlevel conversion RespVersion::RESP3 => Box::new(crate::resp3::encoder::Resp3Encoder), }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/resp/src/compat.rs (1)
18-25: Consider clearer false representation.The
BooleanMode::SimpleStringcomment indicatesfalse → +ERR, which might be confusing since+ERRlooks like an error message but is actually a simple string (errors in RESP start with-). Consider documenting alternatives like+false,+NO, or+NONEfor improved clarity, or clarify why+ERRwas chosen for backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/resp/src/compat.rs(1 hunks)src/resp/src/factory.rs(1 hunks)src/resp/src/multi.rs(1 hunks)src/resp/src/traits.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/resp/src/factory.rs
- src/resp/src/traits.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/resp/src/multi.rs (4)
src/resp/src/resp1/decoder.rs (1)
new(36-41)src/resp/src/resp2/decoder.rs (1)
new(36-41)src/resp/src/encode.rs (1)
new(147-153)src/resp/src/parse.rs (1)
new(67-74)
⏰ 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). (3)
- GitHub Check: cargo fmt (windows-latest)
- GitHub Check: build and test (windows-latest)
- GitHub Check: cargo clippy (windows-latest)
🔇 Additional comments (5)
src/resp/src/multi.rs (2)
31-59: LGTM!The
decode_manyfunction correctly implements batch decoding by pushing the chunk and collecting all available frames. The streaming approach properly handles partial frames by relying on the decoder's internal state. Documentation is comprehensive with clear examples.
61-89: LGTM!The
encode_manyfunction correctly implements batch encoding by accumulating encoded values into a single buffer. Early return on first encoding error is appropriate fail-fast behavior. Documentation is comprehensive with clear examples.src/resp/src/compat.rs (3)
27-34: LGTM!The
DoubleModeenum provides sensible conversion options. TheIntegerIfWholevariant is a smart optimization that maintains precision by using integers when possible while falling back to bulk strings for fractional values.
36-43: LGTM!The
MapModeenum provides two well-defined conversion strategies with clear wire format examples. TheFlatArraymode is more compact whileArrayOfPairsmaintains explicit pairing structure.
45-73: LGTM!The
DownlevelPolicystruct provides a clean configuration API with sensible defaults. The public fields pattern is appropriate for configuration structs, and the default implementation chooses conservative conversion settings that prioritize compatibility and precision.
- Replace assert!(out.len() >= 1) with assert_eq!(out.len(), 1) for precise length checks - Add descriptive panic messages to all match arms for better debugging - Improve test failure diagnostics across all RESP test files Files updated: - resp1_basic.rs: Enhanced inline_ping and simple_string_ok tests - resp2_compat.rs: Fixed length assertions and panic messages - resp3_basic.rs: Added verification for all three decoded frames - resp3_more.rs: Added descriptive panic messages for BulkError, VerbatimString, BigNumber - resp3_collections.rs: Added descriptive panic messages for Set, Map, Push - encode.rs: Added documentation for wildcard arm handling RESP3 types
- Add bounds checking for Double to Integer conversion in RESP1 and RESP2 encoders - Prevent potential overflow when casting out-of-range doubles to i64 - Ensure values are within i64::MIN..=i64::MAX range before casting - Maintain backward compatibility while improving safety Safety improvements: - Check *v >= i64::MIN as f64 before casting - Check *v <= i64::MAX as f64 before casting - Fall back to string representation for out-of-range values - Applied to both RESP1Encoder and RESP2Encoder This prevents silent integer overflow that could occur with very large or very small double values when using IntegerIfWhole policy.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/resp/tests/resp3_basic.rs (1)
53-53: Standardize double encoding assertion to require CRLF.The assertion accepts both ",1.5\r\n" and ",1.5", but RESP3 spec mandates CRLF termination for doubles. This issue was flagged in previous reviews.
Apply this diff:
- assert!(s.contains(",1.5\r\n") || s.contains(",1.5")); + assert!(s.contains(",1.5\r\n"));
🧹 Nitpick comments (6)
src/resp/tests/resp2_compat.rs (2)
33-42: Consider adding length check for consistency.While the test works correctly, adding
assert_eq!(out.len(), 1);before line 37 would match the pattern established inparse_simple_string_okand make the test intent clearer.Apply this diff:
fn parse_integer() { let mut dec = new_decoder(RespVersion::RESP2); let out = decode_many(&mut *dec, Bytes::from(":1000\r\n")); + assert_eq!(out.len(), 1); let v = out[0].as_ref().unwrap(); match v { RespData::Integer(n) => assert_eq!(*n, 1000),
44-55: Consider verifying array contents.The test validates the array structure but doesn't check the actual bulk string values ("foo" and "bar"). For more thorough coverage, you could assert the array elements match the expected values.
Apply this diff to add deeper assertions:
fn parse_bulk_and_array() { let mut dec = new_decoder(RespVersion::RESP2); let out = decode_many(&mut *dec, Bytes::from("*2\r\n$3\r\nfoo\r\n$3\r\nbar\r\n")); + assert_eq!(out.len(), 1); let v = out[0].as_ref().unwrap(); match v { RespData::Array(Some(items)) => { assert_eq!(items.len(), 2); + match &items[0] { + RespData::BulkString(Some(s)) => assert_eq!(s.as_ref(), b"foo"), + other => panic!("Expected BulkString, got {:?}", other), + } + match &items[1] { + RespData::BulkString(Some(s)) => assert_eq!(s.as_ref(), b"bar"), + other => panic!("Expected BulkString, got {:?}", other), + } } other => panic!("Expected Array, got {:?}", other),src/resp/tests/resp3_collections.rs (3)
21-36: Consider validating set content after roundtrip.The test only checks the collection length (3 items). While this confirms the roundtrip succeeds, it doesn't verify that the actual values (Boolean(true), Null, Double(2.5)) are preserved correctly.
If you want more thorough coverage, you can add content validation:
match out[0].as_ref().unwrap() { RespData::Set(items) => { assert_eq!(items.len(), 3); // Validate presence of expected values assert!(items.iter().any(|i| matches!(i, RespData::Boolean(true)))); assert!(items.iter().any(|i| matches!(i, RespData::Null))); assert!(items.iter().any(|i| matches!(i, RespData::Double(d) if (*d - 2.5).abs() < f64::EPSILON))); } other => panic!("Expected Set, got {:?}", other), }
38-52: Consider validating map entries after roundtrip.The test only checks the entry count (2 entries). While this confirms the roundtrip succeeds, it doesn't verify that the actual key-value pairs are preserved correctly.
If you want more thorough coverage, you can add entry validation:
match out[0].as_ref().unwrap() { RespData::Map(entries) => { assert_eq!(entries.len(), 2); // Validate presence of expected entries assert!(entries.iter().any(|(k, v)| matches!(k, RespData::Boolean(true)) && matches!(v, RespData::Double(d) if (*d - 1.0).abs() < f64::EPSILON) )); assert!(entries.iter().any(|(k, v)| matches!(k, RespData::Null) && matches!(v, RespData::BulkError(b) if b.as_ref() == b"ERR x") )); } other => panic!("Expected Map, got {:?}", other), }
54-65: Consider validating push content after roundtrip.The test only checks the item count (2 items). While this confirms the roundtrip succeeds, it doesn't verify that the actual values (Boolean(false), Double(3.14)) are preserved correctly.
If you want more thorough coverage, you can add content validation:
match out[0].as_ref().unwrap() { RespData::Push(items) => { assert_eq!(items.len(), 2); // Validate actual values assert!(matches!(items[0], RespData::Boolean(false))); assert!(matches!(items[1], RespData::Double(d) if (d - 3.14).abs() < f64::EPSILON)); } other => panic!("Expected Push, got {:?}", other), }src/resp/src/resp2/encoder.rs (1)
107-107: Consider more explicit precision control for Double formatting.Using
format!("{}", v)may not preserve full precision for some floating-point values. While this is likely acceptable for RESP2 compatibility, you could make the precision choice more explicit (e.g.,format!("{:.}", v)or specify a fixed precision).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/resp/src/encode.rs(1 hunks)src/resp/src/resp1/encoder.rs(1 hunks)src/resp/src/resp2/encoder.rs(1 hunks)src/resp/tests/resp1_basic.rs(1 hunks)src/resp/tests/resp2_compat.rs(1 hunks)src/resp/tests/resp3_basic.rs(1 hunks)src/resp/tests/resp3_collections.rs(1 hunks)src/resp/tests/resp3_more.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/resp/src/encode.rs
- src/resp/src/resp1/encoder.rs
- src/resp/tests/resp1_basic.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/resp/src/resp2/encoder.rs (3)
src/resp/src/resp1/encoder.rs (6)
new(35-40)with_policy(42-47)encode_one(51-55)encode_into(57-61)version(63-65)encode_downleveled(69-143)src/resp/src/encode.rs (3)
new(147-153)default(141-143)version(160-162)src/resp/src/compat.rs (1)
default(66-72)
src/resp/tests/resp3_basic.rs (2)
src/resp/src/multi.rs (2)
decode_many(52-59)encode_many(83-89)src/resp/src/factory.rs (2)
new_decoder(44-50)new_encoder(68-74)
src/resp/tests/resp2_compat.rs (3)
src/resp/src/multi.rs (1)
decode_many(52-59)src/resp/src/factory.rs (1)
new_decoder(44-50)src/resp/src/parse.rs (3)
s(128-128)s(142-142)parse_integer(121-133)
src/resp/tests/resp3_more.rs (2)
src/resp/src/multi.rs (2)
decode_many(52-59)encode_many(83-89)src/resp/src/factory.rs (2)
new_decoder(44-50)new_encoder(68-74)
src/resp/tests/resp3_collections.rs (2)
src/resp/src/multi.rs (2)
decode_many(52-59)encode_many(83-89)src/resp/src/factory.rs (2)
new_decoder(44-50)new_encoder(68-74)
⏰ 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: build and test (windows-latest)
🔇 Additional comments (8)
src/resp/tests/resp2_compat.rs (1)
21-31: LGTM! Well-structured RESP2 compatibility test.The test correctly validates RESP2 simple string decoding with appropriate length checks and variant matching.
src/resp/tests/resp3_basic.rs (1)
21-39: LGTM!The decode test correctly uses CRLF-terminated RESP3 format for the double value and performs proper floating-point comparison with epsilon. The previous concerns about double encoding format have been addressed.
src/resp/tests/resp3_more.rs (3)
21-32: LGTM!The BulkError roundtrip test correctly encodes and decodes the data, with proper content validation using pattern matching.
34-51: LGTM!The VerbatimString roundtrip test correctly encodes and decodes both the format prefix and data payload, with proper validation of both fields.
53-64: LGTM!The BigNumber roundtrip test correctly encodes and decodes the numeric string value with exact match validation.
src/resp/src/resp2/encoder.rs (3)
28-47: LGTM!The struct definition and constructors follow the same pattern as
Resp1Encoderand correctly initialize the encoder withRespVersion::RESP2. The dual constructor approach (default policy vs. custom policy) provides good flexibility.
50-66: LGTM!The
Encodertrait implementation correctly delegates to the internal encoding logic and properly clears the buffer before each encode operation.
96-108: Bounds checks successfully guard the float cast.The addition of explicit bounds checks at lines 100-101 (
*v >= i64::MIN as f64 && *v <= i64::MAX as f64) correctly prevents silent saturation when casting large floats toi64. This resolves the concern from the previous review.
- Fix RESP3 decoder to support standard RESP types (+, -, :, $, *) - Fix RESP3 encoder to handle all RESP types including Error and Inline - Add comprehensive test coverage for RESP3 functionality - Fix VerbatimString Display implementation type error - Ensure backward compatibility with RESP1/RESP2 protocols All tests passing (35/35). RESP3 now fully supports mixed protocol messages.
- Pre-allocate BytesMut buffer capacity in encode_many function - Use rough estimate of 32 bytes per value to reduce reallocations - Fix code formatting issues in resp3_encoder_comprehensive.rs tests - All tests pass and functionality remains intact
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/resp/src/resp3/encoder.rs (1)
112-171: Good: RESP2 delegation restores backwards‑compat for core types.Handling for SimpleString/Error/Integer/BulkString/Array via RespEncoder covers prior review concerns about rejecting legacy variants. Nice.
🧹 Nitpick comments (1)
src/resp/src/types.rs (1)
92-92: Public API change: Eq removed from RespData.Dropping Eq from RespData can break downstream crates that derive/require Eq. Confirm this is intentional and document in release notes (semver‑significant).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/resp/src/factory.rs(1 hunks)src/resp/src/resp3/decoder.rs(1 hunks)src/resp/src/resp3/encoder.rs(1 hunks)src/resp/src/types.rs(6 hunks)src/resp/tests/resp3_encoder_comprehensive.rs(1 hunks)src/resp/tests/resp3_standard_types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/resp/src/factory.rs
- src/resp/src/resp3/decoder.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/resp/src/resp3/encoder.rs (3)
src/resp/src/traits.rs (4)
encode_one(60-60)encode_into(66-66)version(48-48)version(69-69)src/resp/src/encode.rs (2)
new(147-153)version(160-162)src/resp/src/types.rs (1)
fmt(180-239)
src/resp/tests/resp3_standard_types.rs (2)
src/resp/src/multi.rs (1)
decode_many(52-59)src/resp/src/factory.rs (2)
new_decoder(44-50)new_encoder(68-74)
src/resp/src/types.rs (2)
src/resp/src/resp3/decoder.rs (16)
s(97-97)s(130-130)s(167-167)s(239-239)s(293-293)s(316-316)s(388-388)s(411-411)s(459-459)s(511-511)s(534-534)s(581-581)s(633-633)s(656-656)s(739-739)s(765-765)src/net/src/resp.rs (2)
std(99-101)std(125-127)
src/resp/tests/resp3_encoder_comprehensive.rs (1)
src/resp/src/factory.rs (1)
new_encoder(68-74)
🪛 GitHub Actions: ci
src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 1-1: Formatting differences detected by 'make fmt'. The test file contains diffs such as added indentation and block reformatting. Please run 'cargo fmt' / 'make fmt' to fix and commit the formatted changes.
🪛 GitHub Check: cargo fmt (ubuntu-latest)
src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 91-91:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 82-82:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 71-71:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 62-62:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 54-54:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 47-47:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 31-31:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
[warning] 21-21:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/resp3_encoder_comprehensive.rs
🔇 Additional comments (4)
src/resp/tests/resp3_encoder_comprehensive.rs (2)
21-21: Fix formatting to satisfy CI (cargo fmt).CI reports formatting diffs in this file. Please run cargo fmt (or make fmt) and commit the changes.
Also applies to: 31-31, 47-47, 54-54, 62-62, 71-71, 82-82, 91-91
21-45: Good coverage and expected encodings.Assertions for RESP3 primitives, standard RESP types, and collections look correct and comprehensive. LGTM.
Also applies to: 48-69, 71-103
src/resp/tests/resp3_standard_types.rs (1)
21-68: LGTM: clear round‑trip tests for standard types and mixed frames.Covers decode_many/encode_one paths well with precise assertions. No issues spotted.
Also applies to: 70-96, 98-131
src/resp/src/types.rs (1)
50-67: RESP3 discriminants and mappings look correct.New variants and prefix mappings for RESP3 are consistent. Debug impl for VerbatimString correctly borrows the format slice now. LGTM.
Also applies to: 71-89, 119-136, 216-238
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/resp/src/factory.rs (4)
29-50: Resolve formatting issues.The function logic is correct and properly creates version-specific decoders. However, formatting inconsistencies at lines 40 and 50 have been previously flagged and remain unresolved. Please run
cargo fmtto address these issues.
52-74: Resolve formatting issues.The function logic is correct and properly creates version-specific encoders with default downlevel policies. However, formatting inconsistencies at lines 63 and 74 have been previously flagged and remain unresolved. Please run
cargo fmtto address these issues.
76-103: Document RESP3 policy parameter behavior and resolve formatting issues.The function logic correctly creates version-specific encoders. However, two concerns need attention:
Policy parameter ignored for RESP3: Line 101 shows that when
versionisRESP3, the providedpolicyparameter is ignored anddefault()is used instead. While this may be intentional (RESP3 is the native format and doesn't require downlevel conversion), this behavior should be explicitly documented in the function's doc comment to avoid user confusion.Formatting issues: Lines 81 and 88 have formatting inconsistencies that were previously flagged and remain unresolved.
Consider documenting the policy behavior like this:
/// # Arguments /// * `version` - The RESP protocol version to create an encoder for -/// * `policy` - The downlevel compatibility policy for RESP3 types +/// * `policy` - The downlevel compatibility policy for RESP3 types. Note: This parameter +/// is ignored for RESP3 encoders as RESP3 is the native format and does not +/// require downlevel conversion.Additionally, run
cargo fmtto resolve the formatting issues.
18-27: Address formatting and Clippy issues.The empty line at line 22 after the module doc comment and formatting inconsistencies at lines 16 and 27 have been previously flagged but remain unresolved. Please run
cargo fmtto fix formatting issues and remove the empty line after the doc comment to satisfy Clippy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/resp/src/factory.rs(1 hunks)src/resp/src/multi.rs(1 hunks)src/resp/src/resp3/decoder.rs(1 hunks)src/resp/src/resp3/encoder.rs(1 hunks)src/resp/src/types.rs(6 hunks)src/resp/tests/resp3_encoder_comprehensive.rs(1 hunks)src/resp/tests/resp3_standard_types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/resp/src/multi.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/resp/src/resp3/decoder.rs (4)
src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)src/resp/src/resp1/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/resp2/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/types.rs (1)
fmt(180-239)
src/resp/tests/resp3_encoder_comprehensive.rs (1)
src/resp/src/factory.rs (1)
new_encoder(68-74)
src/resp/src/factory.rs (4)
src/resp/src/resp2/encoder.rs (3)
version(63-65)new(35-40)with_policy(42-47)src/resp/src/resp1/encoder.rs (3)
version(63-65)new(35-40)with_policy(42-47)src/resp/src/traits.rs (2)
version(48-48)version(69-69)src/resp/src/compat.rs (1)
default(66-72)
src/resp/tests/resp3_standard_types.rs (2)
src/resp/src/multi.rs (1)
decode_many(52-59)src/resp/src/factory.rs (2)
new_decoder(44-50)new_encoder(68-74)
src/resp/src/resp3/encoder.rs (3)
src/resp/src/traits.rs (4)
encode_one(60-60)encode_into(66-66)version(48-48)version(69-69)src/resp/src/encode.rs (2)
new(147-153)version(160-162)src/resp/src/types.rs (1)
fmt(180-239)
⏰ 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). (2)
- GitHub Check: cargo fmt (windows-latest)
- GitHub Check: build and test (windows-latest)
🔇 Additional comments (1)
src/resp/src/factory.rs (1)
1-17: LGTM! Duplicate header issue resolved.The copyright header is now correctly present as a single instance. The duplicate header issue flagged in previous reviews has been successfully addressed.
- Replace write! macro usage with extend_from_slice for BytesMut compatibility - Fix BulkError, VerbatimString, Map, Set, Push encoding to use direct byte operations - Improve error handling for SimpleString UTF-8 validation - Remove dependency on fmt::Write trait for better stability - All tests pass and functionality remains intact
…t/Push - Extract parse_single_value helper function for unified RESP type parsing - Refactor Map/Set/Push parsing to use parse_single_value for all elements - Add support for standard RESP types (+, -, :, $, *) in collections - Add comprehensive tests for Map/Set/Push with standard RESP types - Fix issue where collections only supported RESP3-specific types - All tests pass and functionality remains intact
- Replace redundant closure with function reference in map - Convert loop to while let loop for better readability - All tests pass and functionality remains intact
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/resp/src/resp3/decoder.rs (1)
475-491: Refactor to idiomaticwhile letloop.Clippy suggests rewriting this as a
while letloop for better readability.Apply this diff:
- loop { - if let Some(result) = self.parse_single_value() { - match result { - Ok(data) => { - self.out.push_back(Ok(data)); - continue; - } - Err(e) => { - self.out.push_back(Err(e)); - break; - } - } - } else { - // Need more data - break; - } + while let Some(result) = self.parse_single_value() { + match result { + Ok(data) => { + self.out.push_back(Ok(data)); + } + Err(e) => { + self.out.push_back(Err(e)); + break; + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resp/src/resp3/decoder.rs(1 hunks)src/resp/tests/resp3_collections_comprehensive.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/resp/src/resp3/decoder.rs (3)
src/resp/src/types.rs (1)
fmt(180-239)src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)src/resp/src/resp1/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)
src/resp/tests/resp3_collections_comprehensive.rs (2)
src/resp/src/multi.rs (2)
decode_many(52-59)encode_many(83-89)src/resp/src/factory.rs (2)
new_decoder(44-50)new_encoder(68-74)
🪛 GitHub Actions: ci
src/resp/src/resp3/decoder.rs
[error] 456-456: redundant closure
🪛 GitHub Check: cargo clippy (ubuntu-latest)
src/resp/src/resp3/decoder.rs
[failure] 475-475:
this loop could be written as a while let loop
[failure] 456-456:
redundant closure
🔇 Additional comments (1)
src/resp/src/resp3/decoder.rs (1)
446-465: Consider the risk of misinterpreting corrupt data as inline commands.The default case treats any unrecognized prefix as an inline command (space-separated tokens ending in
\r\n). While this provides RESP1 compatibility, it may also silently accept malformed RESP3 data instead of returning a parse error.If the goal is strict RESP3 parsing, consider whether unrecognized prefixes should fail explicitly rather than fall back to inline parsing.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resp/src/resp3/decoder.rs(1 hunks)src/resp/tests/resp3_collections_comprehensive.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/resp/src/resp3/decoder.rs (3)
src/resp/src/types.rs (1)
fmt(180-239)src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)src/resp/src/resp1/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)
src/resp/tests/resp3_collections_comprehensive.rs (2)
src/resp/src/multi.rs (2)
decode_many(52-59)encode_many(83-89)src/resp/src/factory.rs (2)
new_decoder(44-50)new_encoder(68-74)
⏰ 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: build and test (windows-latest)
🔇 Additional comments (3)
src/resp/src/resp3/decoder.rs (3)
45-276: Simple type parsing is correct.The parsing logic for RESP3-specific types (Null, Boolean, Double, BulkError, VerbatimString, BigNumber) and standard RESP types (SimpleString, Error, Integer, BulkString) correctly follows the pattern:
- Check if sufficient data is available
- Return
Noneif incomplete (without consuming buffer)- Validate format
- Consume buffer and return result only after validation
This ensures no buffer corruption on incomplete input.
446-465: Verify: Should unknown prefixes be treated as inline commands?The default case treats any unrecognized byte prefix as an inline command, parsing it as space-separated tokens. This lenient behavior may mask malformed RESP data or protocol errors. While this maintains RESP2 compatibility, consider:
- Should RESP3 decoder support inline commands at all? (RESP3 typically uses proper encoding)
- Should unknown prefixes error out to catch bugs/corruption early?
- Is there a risk of misinterpreting binary data as inline commands?
If inline support is intentional for backward compatibility, the current implementation is reasonable. Otherwise, consider returning a parse error for unrecognized prefixes.
470-500: Decoder trait implementation follows established pattern.The
push/next/reset/versionimplementations are consistent with the RESP1 decoder's approach:
- Incremental buffer accumulation
- Loop-based parsing with early termination on errors
- FIFO output queue
Note: Error handling breaks parsing on first error within a
push()cycle, preventing error cascades but requiring clients to callreset()to recover.
- Fix buffer corruption in Array, Map, Set, and Push parsing - Prevent premature header consumption that caused stream desynchronization - Use temporary buffer swapping to preserve parsing state - Restore buffer state when parsing fails or needs more data - All tests pass and functionality remains intact This fixes the critical issue where collection parsing would consume headers before verifying all elements are present, leading to stream corruption when parsing incomplete data.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/resp/src/resp3/decoder.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/resp/src/resp3/decoder.rs (2)
src/resp/src/types.rs (1)
fmt(180-239)src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)
⏰ 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: cargo fmt (windows-latest)
…ions - Add ParsingState enum to track ongoing collection parsing - Implement continue_collection_parsing for incremental parsing - Add MapWaitingForValue state to handle key-value parsing across chunks - Fix BigNumber CRLF validation to check before consuming buffer - Add comprehensive incremental parsing tests for all collection types - Ensure parsed elements are preserved when more data is needed - All existing tests pass, new incremental parsing tests verify fix This fixes the critical data loss bug where successfully parsed collection elements were lost when parsing incomplete data, ensuring proper stream synchronization and incremental parsing support.
- Remove unused bytes::Bytes import - Fix compilation error with -D warnings flag - All tests pass and clippy checks pass
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/resp/src/resp3/decoder.rs (2)
622-624: Clarify or remove misleading comment.The comment "keep empty to avoid unused import warning" is confusing since the function body is not empty and the
dataparameter is actively used on line 624.Consider removing the comment:
fn push(&mut self, data: Bytes) { - // keep empty to avoid unused import warning self.buf.extend_from_slice(&data);
319-319: Consider adding length validation for DoS protection.While not critical for initial RESP3 support, consider adding reasonable upper bounds on parsed lengths before using them in calculations like
let need = nl + 1 + len + 2orVec::with_capacity(len as usize). This prevents potential integer overflow or excessive memory allocation from malicious inputs.Example approach for bulk types:
const MAX_BULK_LEN: usize = 512 * 1024 * 1024; // 512 MB if len > MAX_BULK_LEN as i64 { return Some(Err(RespError::ParseError("bulk length exceeds limit".into()))); }Also applies to: Lines 350, 456, 501, 532, 560, 588
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resp/src/resp3/decoder.rs(1 hunks)src/resp/tests/incremental_parsing.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/resp/tests/incremental_parsing.rs (1)
src/resp/src/factory.rs (1)
new_decoder(44-50)
src/resp/src/resp3/decoder.rs (2)
src/resp/src/types.rs (1)
fmt(180-239)src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)
🪛 GitHub Actions: ci
src/resp/tests/incremental_parsing.rs
[error] 18-18: unused import: bytes::Bytes
🪛 GitHub Check: build and test (ubuntu-latest)
src/resp/tests/incremental_parsing.rs
[failure] 18-18:
unused import: bytes::Bytes
🔇 Additional comments (5)
src/resp/tests/incremental_parsing.rs (1)
21-175: LGTM! Comprehensive incremental parsing coverage.The test suite properly exercises the RESP3 decoder's ability to handle partial data across all collection types (Array, Map, Set, Push) and multiple messages. The assertions clearly validate both incomplete and complete parsing scenarios.
src/resp/src/resp3/decoder.rs (4)
28-53: LGTM! Well-designed state machine.The
ParsingStateenum elegantly solves the incremental parsing problem by preserving successfully parsed elements across calls. TheMapWaitingForValuestate is a thoughtful addition that handles the case where a map key is parsed but its value is not yet available.
64-227: LGTM! State machine correctly handles incremental parsing.The state machine properly:
- Accumulates successfully parsed elements in the state
- Restores state with accumulated items when more data is needed
- Resets to Idle and propagates errors on parse failures
- Only emits complete collections after all elements are parsed
This correctly addresses the critical data-loss issue identified in previous reviews.
238-617: LGTM! Comprehensive RESP type coverage.The atomic parser correctly handles:
- All RESP3-specific types (
_,#,,,!,=,()- All legacy RESP2 types (
+,-,:,$,*)- RESP3 collections (
%,~,>) with proper state machine delegation- Inline commands as fallback
The parsing logic validates CRLF before consuming buffers and properly returns
Nonewhen more data is needed, fixing issues from previous reviews.
626-636: LGTM! Proper streaming implementation.The
pushmethod correctly:
- Appends new data to the buffer
- Parses all available complete values
- Breaks on errors to prevent stream desynchronization
- Remove misleading comment in push method - Add DoS protection with length limits for all collection types - Add security limits constants (512MB bulk, 1M elements for collections) - Add comprehensive security tests for all length limits - Prevent integer overflow and excessive memory allocation Security improvements: - MAX_BULK_LEN: 512MB for bulk strings - MAX_ARRAY_LEN: 1M elements for arrays - MAX_MAP_PAIRS: 1M pairs for maps - MAX_SET_LEN: 1M elements for sets - MAX_PUSH_LEN: 1M elements for push messages All tests pass and clippy checks pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/resp/tests/incremental_parsing.rs (1)
20-174: Suggest adding tests for nested collections and RESP3-specific types.The incremental parsing tests are well-structured and cover the main composite types (Array, Map, Set, Push) with multiple chunks. However, consider expanding coverage:
Nested incomplete collections: Test cases where an outer collection is split across chunks and contains an inner collection that's also split. For example:
// First chunk: "*2\r\n*1\r\n" (outer array header + incomplete inner array) // Second chunk: "+foo\r\n+bar\r\n" (complete inner array + second element) // Expected: Array([Array(["foo"]), SimpleString("bar")])RESP3-specific types in incremental scenarios: Test Null (
_\r\n), Boolean (#t\r\n), Double (,3.14\r\n), BulkError, VerbatimString, and BigNumber when split across chunks.These additions would verify the decoder's state machine handles complex real-world scenarios correctly.
Would you like me to generate test cases for nested incremental parsing?
src/resp/src/resp3/decoder.rs (1)
622-624: Remove misleading comment.The comment "keep empty to avoid unused import warning" doesn't accurately describe what this line does—it actively extends the buffer with incoming data, which is essential functionality.
Apply this diff:
fn push(&mut self, data: Bytes) { - // keep empty to avoid unused import warning self.buf.extend_from_slice(&data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resp/src/resp3/decoder.rs(1 hunks)src/resp/tests/incremental_parsing.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/resp/tests/incremental_parsing.rs (1)
src/resp/src/factory.rs (1)
new_decoder(44-50)
src/resp/src/resp3/decoder.rs (2)
src/resp/src/types.rs (1)
fmt(180-239)src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)
⏰ 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). (2)
- GitHub Check: build and test (windows-latest)
- GitHub Check: cargo fmt (windows-latest)
🔇 Additional comments (1)
src/resp/src/resp3/decoder.rs (1)
239-618: LGTM: Comprehensive RESP type coverage with correct CRLF validation.The atomic value parser correctly handles all RESP3 types (Null, Boolean, Double, BulkError, VerbatimString, BigNumber) and standard RESP types (SimpleString, Error, Integer, BulkString, Array, Map, Set, Push). The implementation properly:
- Validates CRLF terminators before consuming bytes with
split_to- Uses
memchrfor efficient newline detection- Handles special Double values (
inf,-inf,nan)- Delegates composite types to the state machine via
continue_collection_parsing()This addresses the issues flagged in previous reviews about incomplete dispatch and buffer corruption.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/resp/tests/security_limits.rs (3)
25-25: Extract hardcoded limits into named constants.The limits (512MB for bulk strings, 1M for arrays/maps/sets/pushes) are hardcoded across multiple tests. Extract these into named constants at the module level to improve maintainability and make the test intentions clearer.
Apply this diff to introduce constants at the top of the file:
use resp::{RespVersion, new_decoder}; + +// Security limits for RESP3 decoder +const MAX_BULK_STRING_BYTES: usize = 512 * 1024 * 1024; // 512MB +const MAX_ARRAY_ELEMENTS: usize = 1024 * 1024; // 1M elements +const MAX_MAP_PAIRS: usize = 1024 * 1024; // 1M pairs +const MAX_SET_ELEMENTS: usize = 1024 * 1024; // 1M elements +const MAX_PUSH_ELEMENTS: usize = 1024 * 1024; // 1M elements #[test] fn bulk_string_length_limit() { let mut decoder = new_decoder(RespVersion::RESP3); // Test bulk string exceeding 512MB limit - let oversized_len = 512 * 1024 * 1024 + 1; // 512MB + 1 byte + let oversized_len = MAX_BULK_STRING_BYTES + 1;Apply similar changes to the other test functions.
Also applies to: 40-40, 55-55, 70-70, 85-85
31-32: Consider using more idiomatic assertion patterns.The current assertions use
.is_some()followed by.unwrap().is_err(), which works but is verbose. Consider usingmatches!orassert!(matches!(...))for more concise and expressive assertions.For error cases, apply this pattern:
- assert!(result.is_some(), "Should return an error"); - assert!(result.unwrap().is_err(), "Should return parse error for oversized bulk string"); + assert!(matches!(result, Some(Err(_))), "Should return parse error for oversized bulk string");For success cases:
- assert!(result.is_some(), "Should parse successfully"); - assert!(result.unwrap().is_ok(), "Should parse without error"); + assert!(matches!(result, Some(Ok(_))), "Should parse successfully");Also applies to: 46-47, 61-62, 76-77, 91-92, 105-106
95-107: Consider adding boundary test cases.The test suite covers values exceeding limits and a normal case well within limits, but doesn't test exact boundary conditions. Consider adding tests for values at the exact limit (e.g., exactly 512MB, exactly 1M elements) to ensure the limits are inclusive/exclusive as intended.
Example test for exact bulk string limit:
#[test] fn bulk_string_at_exact_limit() { let mut decoder = new_decoder(RespVersion::RESP3); // Test bulk string at exactly 512MB limit let exact_limit = MAX_BULK_STRING_BYTES; let message = format!("${}\r\n", exact_limit); decoder.push(message.into()); let result = decoder.next(); // Verify whether exact limit is accepted or rejected assert!(matches!(result, Some(Ok(_))), "Should accept bulk string at exact limit"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/resp/src/resp3/decoder.rs(1 hunks)src/resp/tests/incremental_parsing.rs(1 hunks)src/resp/tests/security_limits.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/resp/tests/incremental_parsing.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/resp/tests/security_limits.rs (1)
src/resp/src/factory.rs (1)
new_decoder(44-50)
src/resp/src/resp3/decoder.rs (3)
src/resp/src/types.rs (1)
fmt(180-239)src/resp/src/resp2/decoder.rs (4)
push(45-55)next(57-59)reset(61-64)version(66-68)src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)
🪛 GitHub Check: cargo fmt (ubuntu-latest)
src/resp/tests/security_limits.rs
[warning] 57-57:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/security_limits.rs
[warning] 51-51:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/security_limits.rs
[warning] 42-42:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/security_limits.rs
[warning] 36-36:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/security_limits.rs
[warning] 27-27:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/security_limits.rs
[warning] 20-20:
Diff in /home/runner/work/kiwi/kiwi/src/resp/tests/security_limits.rs
src/resp/src/resp3/decoder.rs
[warning] 603-603:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/resp3/decoder.rs
[warning] 572-572:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/resp3/decoder.rs
[warning] 507-507:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/resp3/decoder.rs
[warning] 462-462:
Diff in /home/runner/work/kiwi/kiwi/src/resp/src/resp3/decoder.rs
⏰ 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: build and test (windows-latest)
🔇 Additional comments (5)
src/resp/tests/security_limits.rs (1)
26-32: Length validation is performed during header parsing: the decoder checkslen_usize > MAX_BULK_LENand returns a parse error before reading any payload (src/resp/src/resp3/decoder.rs:464–466).src/resp/src/resp3/decoder.rs (4)
28-67: Well-designed DoS protection and state machine architecture.The MAX limits provide good protection against memory exhaustion attacks, and the ParsingState enum elegantly handles resumable parsing for all RESP3 collection types. The
MapWaitingForValuestate is a nice optimization that avoids re-parsing keys when data runs out between key and value.
69-234: Excellent resumable parsing implementation.The state machine correctly preserves parsing progress when data is incomplete and cleanly handles errors by resetting to
Idle. TheMapWaitingForValue→Maptransition with recursive continuation (line 164) is particularly elegant.
245-642: Comprehensive and correct RESP parsing implementation.All RESP3 and legacy RESP2/RESP1 types are properly handled:
- Simple types correctly validate CRLF before consuming bytes (including the BigNumber fix from line 380)
- Bulk types enforce MAX limits and validate endings
- Collections transition to stateful parsing after consuming headers
- The inline command fallback (lines 621-640) correctly handles prefix-less commands per RESP spec
The implementation is thorough and defensive.
645-675: Trait implementation follows established patterns.The
Decodertrait implementation correctly mirrors the RESP2 decoder pattern (see relevant snippets from resp2/decoder.rs) while adding RESP3-specific state management. Thepushmethod's loop-until-incomplete-or-error pattern and theresetmethod's comprehensive cleanup are both correct.
- Replace single state with state stack to handle nested collections - Fix nested incomplete collection parsing issue identified by CodeRabbit - Add comprehensive nested incremental parsing tests - Implement parse_single_value_for_collection for proper state management - Support arbitrary depth of nested arrays, maps, sets, and push messages - Add security limits for all collection types to prevent DoS attacks - All tests pass including deeply nested scenarios Key changes: - ParsingState now uses Vec<ParsingState> instead of single state - continue_collection_parsing uses stack-based approach - parse_single_value_for_collection handles nested parsing correctly - Added MAX_*_LEN constants for security protection - Comprehensive test coverage for nested scenarios Fixes CodeRabbit issue: nested incomplete collections were lost when inner collection header was read but data was incomplete.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/resp/tests/nested_incremental.rs (3)
120-122: Remove extraneous "+outer_value" to avoid an unintended second message.This pushes an extra SimpleString that becomes a separate decoded message and isn’t asserted. Trim it for clarity and isolation.
- // Second chunk: complete inner map + outer map value - decoder.push("+inner_key\r\n+inner_value\r\n+outer_value\r\n".into()); + // Second chunk: complete inner map + decoder.push("+inner_key\r\n+inner_value\r\n".into());
158-160: Remove extraneous "+map_value" to avoid an unintended second message.Only the set item is needed to complete the structure; the extra value becomes a separate message in the queue.
- // Second chunk: complete the set + map value - decoder.push("+set_item\r\n+map_value\r\n".into()); + // Second chunk: complete the set + decoder.push("+set_item\r\n".into());
70-77: Drop debug prints from tests.They add noise and can slow CI; assertions already describe failures.
- println!("After second chunk: {:?}", result2); ... - println!("After third chunk: {:?}", result3); ... - println!("After first chunk: {:?}", result1);Also applies to: 196-197
src/resp/src/resp3/decoder.rs (1)
642-663: Tighten inline-command fallback.Default arm treats any CRLF-terminated line with unknown prefix as Inline. This can misclassify garbage as commands. Consider gating to ASCII letters/digits and/or a maximum inline length.
Example guard:
- Enforce max length (e.g., 4 KiB)
- Reject leading byte if it’s a known RESP prefix or non-printable
If you want, I can provide a small helper to validate inline heads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/resp/src/resp3/decoder.rs(1 hunks)src/resp/tests/nested_incremental.rs(1 hunks)src/resp/tests/security_limits.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/resp/tests/security_limits.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/resp/tests/nested_incremental.rs (1)
src/resp/src/factory.rs (1)
new_decoder(44-50)
src/resp/src/resp3/decoder.rs (2)
src/resp/src/types.rs (2)
default(113-115)fmt(180-239)src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)
⏰ 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: build and test (windows-latest)
- Add length limits for BulkError, VerbatimString, and BigNumber to prevent DoS attacks - Add MAX_INLINE_LEN (4KB) limit for inline commands - Tighten inline-command fallback validation: - Reject known RESP prefixes (+, -, :, $, *, %, ~, >, _, #, ,, !, =, () - Reject non-printable characters (outside ASCII 32-126) - Remove extraneous messages from nested_incremental tests for clarity - Remove debug println! statements from tests - Add comprehensive security tests for new limits - Fix clippy warnings (manual range contains) This closes DoS attack vectors and improves input validation robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/resp/src/resp3/decoder.rs (1)
262-717: Optional: Consider factoring length header parsing.The length-parsing pattern is repeated across Array, Map, Set, Push, BulkString, BulkError, and VerbatimString branches. While the current code is clear, extracting a helper like
parse_length_header(prefix: u8, max: usize) -> Option<Result<usize, RespError>>could reduce duplication and make future maintenance easier. This is optional—the current approach is already maintainable.Example structure:
fn parse_length_header(&self, max_len: usize, error_msg: &str) -> Option<Result<usize, RespError>> { if let Some(nl) = memchr::memchr(b'\n', &self.buf) { if nl < 3 || self.buf[nl - 1] != b'\r' { return Some(Ok(None)); // Need more data or invalid } let len_bytes = &self.buf[1..nl - 1]; match std::str::from_utf8(len_bytes) .ok() .and_then(|s| s.parse::<usize>().ok()) { Some(v) if v <= max_len => Some(Ok(Some((v, nl + 1)))), Some(_) => Some(Err(RespError::ParseError(format!("{} exceeds limit", error_msg)))), None => Some(Err(RespError::ParseError(format!("invalid {}", error_msg)))), } } else { None } }Then each branch becomes:
b'%' => { match self.parse_length_header(MAX_MAP_PAIRS, "map pairs") { Some(Ok(Some((pairs, header_len)))) => { let _ = self.buf.split_to(header_len); self.state_stack.push(ParsingState::Map { ... }); self.continue_collection_parsing() } Some(Ok(None)) => None, // Need more data Some(Err(e)) => Some(Err(e)), None => None, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/resp/src/resp3/decoder.rs(1 hunks)src/resp/tests/nested_incremental.rs(1 hunks)src/resp/tests/security_limits.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/resp/tests/security_limits.rs
- src/resp/tests/nested_incremental.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/resp/src/resp3/decoder.rs (1)
src/resp/src/traits.rs (5)
push(34-34)next(40-40)reset(45-45)version(48-48)version(69-69)
⏰ 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: build and test (windows-latest)
🔇 Additional comments (4)
src/resp/src/resp3/decoder.rs (4)
28-64: Excellent DoS protection and state machine design.The length limits comprehensively cover all types that could be exploited (bulk strings, arrays, maps, sets, push messages, inline commands, and the newly added limits for BulkError, VerbatimString, and BigNumber). The
ParsingStateenum design is particularly well thought out—theMapWaitingForValuevariant elegantly handles the edge case where a map key has been parsed but the value parsing requires more data, preventing state loss during incremental parsing.
73-241: Stack-based state machine properly handles nested incomplete collections.The architecture using
state_stack: Vec<ParsingState>elegantly solves the incremental parsing challenge for nested collections. When an inner collection header is parsed but data is incomplete, its state is pushed onto the stack and parsing can resume correctly when more data arrives. The error handling that clears the stack (e.g., lines 92, 123, 139) prevents corruption from cascading through nested structures. This design successfully addresses the "nested incomplete collection parsing" concern from earlier reviews.
262-717: Comprehensive RESP parsing with correct validation order.The atomic parser correctly handles all RESP3 types (Null, Boolean, Double, BulkError, VerbatimString, BigNumber, Map, Set, Push) and legacy RESP types (SimpleString, Error, Integer, BulkString, Array). Key strengths:
Validation before consumption: All branches validate CRLF and length constraints before calling
split_to, preventing buffer corruption on malformed input (e.g., BulkError at lines 328-330, VerbatimString at lines 366-368).DoS protections applied: Length checks against MAX_* constants occur before allocation (e.g., lines 343-346, 379-382, 412-415).
Edge case handling: Double parsing handles "inf", "-inf", "nan" case-insensitively (lines 305-314), VerbatimString validates the 3-byte format header (lines 390-391).
Inline command safety: Lines 680-700 reject RESP prefixes and non-printable characters, preventing ambiguity.
720-749: LGTM: Clean Decoder trait implementation.The trait implementation correctly follows the streaming decoder pattern:
pushgreedily parses all available values, breaking on the first error (line 731) to prevent cascading failures.nextdrains the output queue, enabling efficient pipelining.resetclears all state including the newstate_stack(line 744), ensuring no corruption after errors or restarts.
There was a problem hiding this comment.
Pull Request Overview
This PR implements full RESP3 protocol support for the Redis client library, introducing a unified architecture that supports RESP1, RESP2, and RESP3 protocols with configurable downlevel compatibility.
- Introduces versioned encoder/decoder modules (resp1/, resp2/, resp3/) with trait-based interfaces
- Adds comprehensive RESP3 type support including Null, Boolean, Double, BulkError, VerbatimString, BigNumber, Map, Set, and Push types
- Implements factory pattern for version selection and downlevel policies for backward compatibility
Reviewed Changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/resp/src/types.rs | Extends RespData enum with RESP3 types and removes Eq derivation for f64 compatibility |
| src/resp/src/traits.rs | Defines streaming Decoder/Encoder trait interfaces |
| src/resp/src/resp3/decoder.rs | Implements RESP3 streaming parser with security limits and collection state tracking |
| src/resp/src/resp3/encoder.rs | Implements RESP3 encoder with delegation to RESP2 encoder for standard types |
| src/resp/src/resp2/encoder.rs | RESP2 encoder wrapper with downlevel policy support |
| src/resp/src/resp1/encoder.rs | RESP1 encoder wrapper with downlevel policy support |
| src/resp/src/factory.rs | Factory functions for creating version-specific encoders/decoders |
| src/resp/src/compat.rs | Downlevel policy configuration for RESP3→RESP2/RESP1 conversion |
| src/resp/src/multi.rs | Batch encode/decode utilities for pipelining support |
| src/resp/src/lib.rs | Updated public API exports |
| Various test files | Comprehensive test coverage for all RESP versions and features |
Comments suppressed due to low confidence (1)
src/resp/src/resp3/encoder.rs:1
- The catch-all pattern
_ => selfsilently ignores unhandled variants. Consider making this pattern explicit by listing the specific RESP3 variants that are being ignored, or add a comment explaining why they're not handled.
// Copyright (c) 2024-present, arana-db Community. All rights reserved.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // All pairs parsed successfully | ||
| Some(Ok(RespData::Map(items))) |
There was a problem hiding this comment.
[nitpick] The comment 'All pairs parsed successfully' is accurate but the variable name 'items' could be more descriptive as 'pairs' or 'entries' to better reflect that it contains key-value pairs.
| // Clear state stack on error | ||
| self.state_stack.clear(); |
There was a problem hiding this comment.
The error handling pattern of clearing the state stack is repeated multiple times throughout the code. Consider extracting this into a helper method to reduce duplication and ensure consistent error cleanup.
| const MAX_BULK_LEN: usize = 512 * 1024 * 1024; // 512 MB | ||
| const MAX_BULK_ERROR_LEN: usize = MAX_BULK_LEN; // tune independently if needed | ||
| const MAX_VERBATIM_LEN: usize = MAX_BULK_LEN; // tune independently if needed |
There was a problem hiding this comment.
[nitpick] Consider defining these constants as configurable parameters rather than hard-coded values. This would allow applications to adjust security limits based on their specific requirements and memory constraints.
|
这里可能要讨论一下是否要将RESP2和RESP3合在一起，不要拆开实现 @AlexStocks
|
|
我举得可以进行讨论是否要进行合在一起实现，不过在开始之前需要进行探讨一下 @guozhihao-224 @AlexStocks |
可以。周末一块聊下。 |
|
3 participants
Architecture:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Breaking Changes