Truncate bodyResponse to the actual number of bytes read #85
Conversation
Co-Authored-By: Andy Li [email protected]
Co-Authored-By: Andy Li [email protected]
Co-Authored-By: Andy Li [email protected]
Co-Authored-By: Andy Li [email protected]
WalkthroughThis change adds two new test cases to the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant KafkaServer (Python)
participant KafkaClient (Go)
TestRunner->>KafkaServer (Python): Start scenario server (main.py)
TestRunner->>KafkaClient (Go): Run client test (TestStages)
KafkaClient (Go)->>KafkaServer (Python): Connect and send ApiVersions request
KafkaServer (Python)->>KafkaClient (Go): Send crafted ApiVersions response (length field too large/small)
KafkaClient (Go)->>KafkaClient (Go): Validate response length field (checkLength)
KafkaClient (Go)->>TestRunner: Report test result (pass/fail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@andy1li can you please own this PR ? |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/test_helpers/scenarios/apiversions_length_field_too_small/main.py (1)
30-54: Apply the same error handling improvements as the companion scenario.This implementation has the same robustness concerns as the "too large" scenario regarding malformed request handling and graceful shutdown. Consider applying similar improvements for consistency.
🧹 Nitpick comments (1)
internal/test_helpers/scenarios/apiversions_length_field_too_large/main.py (1)
20-27: Consider adding graceful shutdown mechanism.The server runs indefinitely without a clean shutdown mechanism. For production-like test scenarios, consider adding signal handling or a shutdown flag.
import signal import sys def signal_handler(sig, frame): print('Shutting down gracefully...') sys.exit(0) def main(port=9092): signal.signal(signal.SIGINT, signal_handler) # ... rest of the implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/stages_test.go(1 hunks)internal/test_helpers/fixtures/scenarios/apiversions_length_field_too_large(1 hunks)internal/test_helpers/fixtures/scenarios/apiversions_length_field_too_small(1 hunks)internal/test_helpers/scenarios/apiversions_length_field_too_large/codecrafters.yml(1 hunks)internal/test_helpers/scenarios/apiversions_length_field_too_large/main.py(1 hunks)internal/test_helpers/scenarios/apiversions_length_field_too_large/your_program.sh(1 hunks)internal/test_helpers/scenarios/apiversions_length_field_too_small/codecrafters.yml(1 hunks)internal/test_helpers/scenarios/apiversions_length_field_too_small/main.py(1 hunks)internal/test_helpers/scenarios/apiversions_length_field_too_small/your_program.sh(1 hunks)protocol/kafka_client/client.go(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/test_helpers/fixtures/scenarios/apiversions_length_field_too_small (1)
internal/stage_4.go (1)
testAPIVersionErrorCase(20-116)
internal/test_helpers/scenarios/apiversions_length_field_too_small/main.py (1)
internal/test_helpers/scenarios/apiversions_length_field_too_large/main.py (4)
Request(8-17)from_bytes(15-17)main(20-27)handle_connection(30-53)
internal/test_helpers/scenarios/apiversions_length_field_too_large/main.py (1)
internal/test_helpers/scenarios/apiversions_length_field_too_small/main.py (4)
Request(8-17)from_bytes(15-17)main(20-27)handle_connection(30-53)
🪛 Shellcheck (0.10.0)
internal/test_helpers/scenarios/apiversions_length_field_too_small/your_program.sh
[warning] 2-2: Quote this to prevent word splitting.
(SC2046)
internal/test_helpers/scenarios/apiversions_length_field_too_large/your_program.sh
[warning] 2-2: Quote this to prevent word splitting.
(SC2046)
⏰ 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: Cursor Bugbot
🔇 Additional comments (20)
internal/test_helpers/scenarios/apiversions_length_field_too_large/your_program.sh (1)
1-2: Script logic is correct for test scenario execution.The shell script properly forwards arguments and locates the Python server in the same directory.
internal/test_helpers/scenarios/apiversions_length_field_too_small/codecrafters.yml (1)
1-1: Configuration is appropriate for test scenario.Enabling debug mode for this edge case scenario is a good practice for troubleshooting and development.
internal/test_helpers/scenarios/apiversions_length_field_too_large/codecrafters.yml (1)
1-1: Consistent configuration across test scenarios.The debug mode setting is appropriate and maintains consistency with the companion "too_small" scenario.
internal/test_helpers/scenarios/apiversions_length_field_too_small/your_program.sh (1)
1-2: Script structure is consistent and functional.The script maintains the same pattern as its companion, which is good for maintainability.
internal/stages_test.go (3)
43-49: Well-structured test case for "too_large" scenario.The test case follows the established pattern and correctly expects failure (exit code 1) for this edge case scenario.
50-56: Consistent test case for "too_small" scenario.The test case maintains consistency with its companion and the existing test structure, appropriately expecting failure for malformed responses.
43-56: Comprehensive edge case coverage added.These test cases provide valuable coverage for Kafka protocol edge cases involving malformed length fields, which aligns well with the PR objective of improving response validation.
internal/test_helpers/fixtures/scenarios/apiversions_length_field_too_small (1)
1-46: Comprehensive test fixture for length validation edge case.This fixture effectively captures the expected behavior when detecting malformed int32 fields in Kafka responses. The detailed error context (lines 37-42) clearly identifies the problematic field (
throttle_time_ms) and provides precise byte-level information about the length mismatch.internal/test_helpers/fixtures/scenarios/apiversions_length_field_too_large (1)
1-33: Well-designed complementary test fixture for length validation.This fixture effectively tests the opposite edge case from the "too small" scenario. The error message format (lines 21-29) is particularly well-crafted with visual indicators and a helpful hint about the common mistake of including the message size field in its own length calculation.
internal/test_helpers/scenarios/apiversions_length_field_too_large/main.py (2)
8-17: Request parsing implementation is clean and correct.The
Requestclass with itsfrom_bytesmethod effectively parses the Kafka request header using proper network byte order unpacking.
40-40: Verify the intentional length field error.The
19 + 4calculation deliberately creates a message length field that's too large, which is the intended behavior for this test scenario. This correctly simulates the edge case being tested.protocol/kafka_client/client.go (7)
30-52: Excellent length validation implementation with comprehensive error reporting.The
checkLength()method provides robust validation with detailed error messages including hex byte representation and contextual hints. The special case handling for the common mistake (line 42-47) is particularly helpful for debugging.
68-68: Good improvement: More descriptive constant name.Renaming
RETRIEStomaxRetriesimproves code readability and follows Go naming conventions.
76-76: Fix: Correct retry logic boundary condition.The change from
retries > maxRetriestoretries >= maxRetriesensures that exactlymaxRetriesattempts are made, which is the expected behavior.
136-139: Proper integration of length validation.The length check is appropriately placed after receiving the response but before returning it, ensuring validation occurs for all responses.
169-169: Enhanced robustness: Use io.ReadFull for length prefix.Using
io.ReadFullinstead ofconn.Readensures the complete 4-byte length prefix is read, preventing partial reads that could cause protocol errors.
183-184: Proper handling of partial reads.Capturing
numBytesReadand truncating the slice accordingly handles cases whereio.ReadFullreads fewer bytes than requested, improving robustness against network issues.
195-198: Good error handling for unexpected EOF.Handling
io.ErrUnexpectedEOFby returning partial responses allows for better error reporting and debugging, consistent with the timeout handling pattern.internal/test_helpers/scenarios/apiversions_length_field_too_small/main.py (2)
8-17: Consistent request parsing implementation.The
Requestclass implementation is identical to the "too large" scenario, maintaining consistency across test helpers.
40-40: Verify the intentional length field error for "too small" case.The
19 - 4calculation correctly creates a message length field that's too small, properly simulating the opposite edge case from the "too large" scenario.
internal/test_helpers/scenarios/apiversions_length_field_too_large/main.py
Show resolved
Hide resolved
internal/test_helpers/scenarios/apiversions_length_field_too_large/your_program.sh
Show resolved
Hide resolved
internal/test_helpers/scenarios/apiversions_length_field_too_small/your_program.sh
Show resolved
Hide resolved
| @@ -108,6 +133,10 @@ func (c *Client) SendAndReceive(request kafka_interface.RequestI, stageLogger *l | |||
|
|
|||
| stageLogger.Debugf("Hexdump of received \"%s\" response: \n%v\n", apiName, utils.GetFormattedHexdump(response.RawBytes)) | |||
|
|
|||
| if err := response.checkLength(); err != nil { | |||
There was a problem hiding this comment.
The usage of partial responses seems like a mistake to me, I think this should be an error returned from Receive(). There's no reason to only raise an error when calling SendAndReceive and not when calling just Receive().
I also think the whole refactor thing here was done pretty poorly (cc: @ryan-gang) - earlier this method (SendAndReceive) was just calling Send() followed by Receive(). Now it's got things like logging mixed in but the logs aren't placed in Send() and Receive() respectively, giving rise to patterns like this where we need to return partial responses to ensure logging happens. I think this should've followed the pattern we did in Redis where we separated out "instrumentable" connections that had hooks when things were being sent/received.
Might be a larger change so @andy1li you can ignore this one for now - we'll handle.
Previously: #50
Summary by CodeRabbit
New Features
Bug Fixes
Tests