Skip to content

Conversation

@loks0n
Copy link
Contributor

@loks0n loks0n commented Nov 6, 2025

Summary by CodeRabbit

  • New Features

    • Enhanced DNS name handling with case-insensitive normalization for domain names.
    • Improved DNS Record property accessibility.
  • Tests

    • Added comprehensive test coverage for DNS question name assignment and normalization.
    • Extended test coverage for zone export/import with template records.

@coderabbitai
Copy link

coderabbitai bot commented Nov 6, 2025

Walkthrough

This pull request modifies the DNS Message classes to use promoted properties in constructors. The Question class changes from a promoted property constructor to explicitly assigning a lowercase-normalized name to a public property. The Record class introduces promoted public properties for type, class, ttl, rdata, and optional priority, weight, and port fields. A class-level docblock is added to Record. Four new unit tests validate constructor behavior for Questions with name normalization, Zone/File export and import operations with template records, and Zone constructor acceptance of template records.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Focus on the lowercase normalization logic in Question constructor to confirm it handles DNS name case-insensitivity correctly
  • Verify that promoted property syntax in Record doesn't affect existing encoding/decoding logic that references these properties
  • Confirm test assertions accurately reflect the intended behavior of name normalization and template record handling

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'fix: lowercase questions' accurately reflects the main change: converting Question constructor from promoted property to explicit lowercase assignment of $name.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-lowercase-questions

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
tests/unit/DNS/Message/QuestionTest.php (1)

11-16: Consider testing with mixed case input.

The first test uses already-lowercase input, so it doesn't verify case normalization. Consider using mixed case like 'Www.Example.Com' to make the test more meaningful, or remove it since testConstructorSetsNameCaseInsensitive already validates the lowercase behavior.

Example:

     public function testConstructorSetsName(): void
     {
-        $question = new Question('www.example.com', Record::TYPE_A, Record::CLASS_IN);
+        $question = new Question('Www.Example.Com', Record::TYPE_A, Record::CLASS_IN);

         $this->assertSame('www.example.com', $question->name);
     }
tests/unit/DNS/Zone/FileTest.php (1)

550-567: Use conventional IPv6 address format.

While 'b:b::b:b:b' is technically valid IPv6, it's unconventional. Use standard test addresses like '2001:db8::1' for better readability and consistency with other tests.

 $ORIGIN example.com.
 %s
-www 600 IN AAAA b:b::b:b:b
+www 600 IN AAAA 2001:db8::1
 ZONE,
             self::DEFAULT_SOA
         );
 
         $zone = File::import($contents);
 
         $this->assertInstanceOf(Zone::class, $zone);
         $this->assertCount(1, $zone->records);
         $this->assertSame('www.example.com', $zone->records[0]->name);
-        $this->assertSame('b:b::b:b:b', $zone->records[0]->rdata);
+        $this->assertSame('2001:db8::1', $zone->records[0]->rdata);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b69202f and ea87bbb.

📒 Files selected for processing (5)
  • src/DNS/Message/Question.php (1 hunks)
  • src/DNS/Message/Record.php (2 hunks)
  • tests/unit/DNS/Message/QuestionTest.php (1 hunks)
  • tests/unit/DNS/Zone/FileTest.php (1 hunks)
  • tests/unit/DNS/ZoneTest.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/unit/DNS/Message/QuestionTest.php (2)
src/DNS/Message/Question.php (1)
  • Question (7-54)
src/DNS/Message/Record.php (1)
  • Record (10-542)
tests/unit/DNS/ZoneTest.php (2)
src/DNS/Message/Record.php (1)
  • Record (10-542)
src/DNS/Zone.php (1)
  • Zone (10-72)
tests/unit/DNS/Zone/FileTest.php (2)
src/DNS/Message/Record.php (1)
  • Record (10-542)
src/DNS/Zone/File.php (2)
  • File (13-815)
  • export (130-188)
src/DNS/Message/Question.php (2)
src/DNS/Message/Record.php (2)
  • __construct (113-124)
  • Record (10-542)
src/DNS/Message.php (1)
  • __construct (32-68)
⏰ 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: Unit & E2E
🔇 Additional comments (3)
src/DNS/Message/Record.php (2)

7-9: LGTM! Clear class documentation.

The class-level docblock appropriately describes the Record class purpose.


101-124: Excellent constructor documentation.

The detailed docblock clearly documents all parameters, their types, default values, and usage context. The implementation correctly normalizes the domain name to lowercase while using promoted properties for other fields.

src/DNS/Message/Question.php (1)

9-17: LGTM! Correct implementation of case-insensitive name handling.

The change from promoted property to explicit assignment enables lowercase normalization of domain names, which is essential for case-insensitive DNS lookups. The pattern is consistent with the Record class implementation.

@loks0n loks0n merged commit d1ed3f9 into main Nov 6, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants