Conversation
WalkthroughThe pull request modifies DNS message truncation handling to address NODATA/NXDOMAIN edge cases. In Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/DNS/Message.php (1)
306-320: Correct coverage for the answer-truncation edge case.Using
$fittingAnswers === []rather than$this->answers === []is intentional: it also guards the case where the original response had answers but none fit individually withinmaxSize, which would otherwise produce aNOERROR + answers:[] + authority:[]response that still triggers the NODATA constructor exception whenauthoritativeistrue.Optional: The NODATA/NXDOMAIN condition is spelled out twice (lines 262-263 and 307-308). Extracting a small private helper (e.g.,
isNodataOrNxdomain(array $answers): bool) would eliminate the duplication and make the intent explicit at both call sites.♻️ Proposed helper extraction
+ private function isNodataOrNxdomain(array $answers): bool + { + return ($this->header->responseCode === self::RCODE_NOERROR && $answers === []) + || $this->header->responseCode === self::RCODE_NXDOMAIN; + }Then in
encodeWithTruncation:- $isNodataOrNxdomain = ($this->header->responseCode === self::RCODE_NOERROR && $this->answers === []) - || $this->header->responseCode === self::RCODE_NXDOMAIN; + $isNodataOrNxdomain = $this->isNodataOrNxdomain($this->answers);- $isNodataOrNxdomainTruncated = ($this->header->responseCode === self::RCODE_NOERROR && $fittingAnswers === []) - || $this->header->responseCode === self::RCODE_NXDOMAIN; + $isNodataOrNxdomainTruncated = $this->isNodataOrNxdomain($fittingAnswers);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DNS/Message.php` around lines 306 - 320, The code correctly uses $fittingAnswers rather than $this->answers to decide NODATA/NXDOMAIN authoritative behavior in encodeWithTruncation, but the same condition is duplicated; extract a small private helper (e.g., isNodataOrNxdomain(array $answers): bool) and replace both uses with calls to that helper, then compute $isNodataOrNxdomainTruncated = $this->isNodataOrNxdomain($fittingAnswers) and pass authoritative: $isNodataOrNxdomainTruncated ? false : $this->header->authoritative when building the truncated response to eliminate duplication and make intent explicit.tests/unit/DNS/MessageTest.php (1)
463-493: Test logic is correct; consider adding NXDOMAIN parity coverage.The setup correctly targets Step 2 truncation:
header (12B) + question for 'empty.example.com' TXT (~23B) ≈ 35B, well under the 80B limit, while the full packet with the SOA record exceeds it. The assertions confirm the expected non-authoritative, authority-dropped outcome.The symmetric scenario — an NXDOMAIN response with SOA in authority where truncation drops the authority section — is not currently covered. That path follows the same
$isNodataOrNxdomainbranch in Step 2 and is worth a test case.🧪 Suggested NXDOMAIN parity test
public function testEncodeNxdomainWithTruncationDroppingAuthority(): void { $question = new Question('missing.example.com', Record::TYPE_A); $query = Message::query($question, id: 0x5678); $soa = new Record( 'example.com', Record::TYPE_SOA, Record::CLASS_IN, 300, 'ns.example.com. hostmaster.example.com. 2024010101 3600 600 86400 300' ); $response = Message::response( $query->header, Message::RCODE_NXDOMAIN, questions: $query->questions, answers: [], authority: [$soa], additional: [], authoritative: true ); $truncated = $response->encode(80); $decoded = Message::decode($truncated); $this->assertSame(Message::RCODE_NXDOMAIN, $decoded->header->responseCode); $this->assertCount(0, $decoded->answers); $this->assertCount(0, $decoded->authority); $this->assertFalse($decoded->header->authoritative, 'Dropped authority => non-authoritative'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/DNS/MessageTest.php` around lines 463 - 493, Add a symmetric test case covering NXDOMAIN truncation similar to the existing testEncodeNodataWithTruncationDroppingAuthority: create a Question (e.g. 'missing.example.com' A), build a query via Message::query, create the SOA Record, construct a response with Message::response using Message::RCODE_NXDOMAIN, authoritative=true and authority containing the SOA, call Message::encode with the same small size (80) to force truncation, decode via Message::decode and assert header->responseCode is Message::RCODE_NXDOMAIN, answers and authority are empty, and header->authoritative is false; name the test method testEncodeNxdomainWithTruncationDroppingAuthority to mirror the existing test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/DNS/Message.php`:
- Around line 306-320: The code correctly uses $fittingAnswers rather than
$this->answers to decide NODATA/NXDOMAIN authoritative behavior in
encodeWithTruncation, but the same condition is duplicated; extract a small
private helper (e.g., isNodataOrNxdomain(array $answers): bool) and replace both
uses with calls to that helper, then compute $isNodataOrNxdomainTruncated =
$this->isNodataOrNxdomain($fittingAnswers) and pass authoritative:
$isNodataOrNxdomainTruncated ? false : $this->header->authoritative when
building the truncated response to eliminate duplication and make intent
explicit.
In `@tests/unit/DNS/MessageTest.php`:
- Around line 463-493: Add a symmetric test case covering NXDOMAIN truncation
similar to the existing testEncodeNodataWithTruncationDroppingAuthority: create
a Question (e.g. 'missing.example.com' A), build a query via Message::query,
create the SOA Record, construct a response with Message::response using
Message::RCODE_NXDOMAIN, authoritative=true and authority containing the SOA,
call Message::encode with the same small size (80) to force truncation, decode
via Message::decode and assert header->responseCode is Message::RCODE_NXDOMAIN,
answers and authority are empty, and header->authoritative is false; name the
test method testEncodeNxdomainWithTruncationDroppingAuthority to mirror the
existing test.
eldadfux
force-pushed
the
fix-import-exception
branch
from
143e12d to
31f8969
Compare
2 participants
Summary by CodeRabbit
Bug Fixes
Tests