feat: importexceptions by loks0n · Pull Request #20 · utopia-php/dns

loks0n · 2025-11-10T14:13:58Z

Summary by CodeRabbit

Bug Fixes
- Enhanced error reporting for DNS zone file imports with improved context and line numbers for invalid records
- Better error messages for zone file parsing failures
Refactoring
- Reorganized exception handling structure for improved code maintainability
- Added type safety improvements to internal constants

coderabbitai · 2025-11-10T14:14:08Z

Walkthrough

This pull request reorganizes exception class namespaces and refactors DNS zone file import error handling. Two existing exception classes (DecodingException and PartialDecodingException) are moved from Utopia\DNS\Exception to Utopia\DNS\Exception\Message namespace. A new ImportException class is introduced in Utopia\DNS\Exception\Zone to encapsulate zone file content during import failures. Related files are updated to import from the new namespaces. The Zone/File parser is refactored to catch InvalidArgumentException and wrap it with ImportException, which carries zone content context. Additionally, string type annotations are added to four protected constants in the DNS validator class. Corresponding test files are updated to reflect the new exception namespaces and types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

src/DNS/Zone/File.php — Most complex file requiring careful review of exception handling refactoring. Verify that all InvalidArgumentException catches are properly wrapped with ImportException, and confirm the new Record::typeNameToCode usage replaces the removed parseType method correctly.
Zone file parsing error paths — Multiple locations where error messages are modified (e.g., "Unknown record type" → "Invalid record type (line X)"). Ensure error context is accurate and helpful for debugging.
ImportException constructor and usage — Verify that zone file content is correctly passed and stored through the exception hierarchy.
Test updates across multiple Message and Zone test files — Confirm that all expected exception types and error messages align with the implementation changes.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The PR title 'feat: importexceptions' is vague and does not clearly summarize the main changes; it lacks clarity about what functionality is being added or modified.	Consider a more descriptive title that clearly indicates the main purpose, such as 'feat: add ImportException for DNS zone file parsing errors' or 'feat: reorganize exceptions into Message and Zone namespaces'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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 feat-importexceptions

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e6b4ba and f7f17d0.

📒 Files selected for processing (15)

src/DNS/Exception/Message/DecodingException.php (1 hunks)
src/DNS/Exception/Message/PartialDecodingException.php (1 hunks)
src/DNS/Exception/Zone/ImportException.php (1 hunks)
src/DNS/Message.php (1 hunks)
src/DNS/Message/Domain.php (1 hunks)
src/DNS/Message/Header.php (1 hunks)
src/DNS/Message/Question.php (1 hunks)
src/DNS/Message/Record.php (1 hunks)
src/DNS/Server.php (1 hunks)
src/DNS/Validator/DNS.php (1 hunks)
src/DNS/Zone/File.php (8 hunks)
tests/unit/DNS/Message/DomainTest.php (1 hunks)
tests/unit/DNS/Message/HeaderTest.php (1 hunks)
tests/unit/DNS/MessageTest.php (5 hunks)
tests/unit/DNS/Zone/FileTest.php (10 hunks)

🧰 Additional context used

🧬 Code graph analysis (13)

src/DNS/Server.php (2)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/PartialDecodingException.php (1)

PartialDecodingException (11-25)

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

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

src/DNS/Exception/Message/PartialDecodingException.php (1)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Message/Domain.php (2)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

tests/unit/DNS/Message/HeaderTest.php (2)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

src/DNS/Message/Record.php (2)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

src/DNS/Exception/Message/DecodingException.php (1)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Message.php (2)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

src/DNS/Exception/Message/PartialDecodingException.php (1)

PartialDecodingException (11-25)

tests/unit/DNS/Message/DomainTest.php (2)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

src/DNS/Message/Header.php (2)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

src/DNS/Zone/File.php (3)

src/DNS/Zone.php (1)

Zone (10-72)

src/DNS/Exception/Zone/ImportException.php (1)

ImportException (8-25)

src/DNS/Message/Record.php (2)

Record (10-542)

typeNameToCode (326-329)

tests/unit/DNS/Zone/FileTest.php (2)

src/DNS/Exception/Zone/ImportException.php (1)

ImportException (8-25)

src/DNS/Zone/File.php (2)

File (14-820)

import (37-135)

tests/unit/DNS/MessageTest.php (3)

src/DNS/Message.php (1)

Message (11-208)

src/DNS/Exception/Message/DecodingException.php (1)

DecodingException (8-10)

src/DNS/Exception/Message/PartialDecodingException.php (1)

PartialDecodingException (11-25)

⏰ 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 (29)

src/DNS/Validator/DNS.php (1)

14-17: LGTM! Type annotations improve type safety.

Adding explicit string type declarations to these constants follows modern PHP best practices and provides compile-time type checking without affecting runtime behavior.

tests/unit/DNS/Message/DomainTest.php (1)

6-6: LGTM: Import updated to reflect new exception namespace.

The namespace change correctly updates the import to the new Message sub-namespace while maintaining all test functionality.

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

5-5: LGTM: Namespace refactoring aligns exception organization.

The import correctly references the new Message sub-namespace, improving code organization by grouping message-related exceptions together.

src/DNS/Message.php (1)

5-6: LGTM: Both exception imports updated consistently.

The namespace refactoring correctly moves both DecodingException and PartialDecodingException to the Message sub-namespace, maintaining consistency across the codebase.

tests/unit/DNS/MessageTest.php (1)

6-7: LGTM: Test imports aligned with new exception namespace.

Both exception imports correctly reference the Message sub-namespace, and all test assertions are properly updated.

src/DNS/Exception/Message/DecodingException.php (1)

3-3: LGTM: Exception moved to Message sub-namespace.

The namespace change improves code organization by grouping message-related exceptions together under Utopia\DNS\Exception\Message.

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

5-5: LGTM: Import updated for consistency.

The namespace change correctly aligns with the exception reorganization across the codebase.

src/DNS/Exception/Message/PartialDecodingException.php (1)

3-3: LGTM: PartialDecodingException relocated to Message sub-namespace.

The namespace change is correct. Since DecodingException is now in the same namespace, the parent class extension works without requiring an explicit import.

src/DNS/Server.php (1)

7-7: LGTM: Server exception handling updated for new namespace.

The import correctly references PartialDecodingException from the new Message sub-namespace, maintaining consistent error handling.

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

5-5: LGTM!

The namespace update aligns with the exception reorganization moving DecodingException to Utopia\DNS\Exception\Message.

tests/unit/DNS/Message/HeaderTest.php (1)

6-6: LGTM!

Test import updated consistently with the exception namespace reorganization.

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

5-5: LGTM!

Namespace update is consistent with the exception reorganization.

src/DNS/Exception/Zone/ImportException.php (1)

1-25: LGTM!

The ImportException implementation is clean and well-designed. The class properly stores zone content for debugging context, uses readonly for immutability, and is marked final to prevent unintended subclassing.

tests/unit/DNS/Zone/FileTest.php (7)

6-6: LGTM!

Import added to support the new ImportException type.

106-127: LGTM!

Test expectations correctly updated to expect ImportException instead of InvalidArgumentException. Error messages are preserved, maintaining test coverage while aligning with the new exception hierarchy.

132-158: LGTM!

Test assertions properly updated to use ImportException with unchanged error message expectations.

207-211: LGTM!

Exception type updated consistently with preserved error message validation.

595-608: LGTM!

CAA validation test correctly updated to expect ImportException.

626-653: LGTM!

Multiple SOA and TTL validation tests properly updated with ImportException expectations and line-specific error messages.

708-732: LGTM!

Remaining test cases correctly updated to expect ImportException for SOA validation and owner omission errors.

src/DNS/Zone/File.php (9)

6-6: LGTM!

Import added to support the new ImportException type for zone file parsing errors.

35-35: LGTM!

Documentation updated to reflect the new exception type thrown by the import method.

50-50: LGTM!

Default origin validation now throws ImportException with zone content, providing better debugging context.

66-70: LGTM!

Directive handling errors are properly wrapped in ImportException, preserving the original error message and exception chain while adding zone content context.

75-75: LGTM!

Empty $ORIGIN directive validation correctly throws ImportException with zone content.

93-97: LGTM!

Resource record parsing errors are consistently wrapped in ImportException, maintaining the exception chain and adding zone content for debugging.

117-117: LGTM!

Multiple SOA detection now throws ImportException with line number and zone content.

127-132: LGTM!

Zone-level validations (missing SOA, undeterminable zone name) correctly throw ImportException with zone content context.

370-373: LGTM!

Refactored to use Record::typeNameToCode() instead of a private helper, eliminating code duplication and improving maintainability.

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.}

Meldiron · 2025-11-10T14:26:15Z

-        $type = self::parseType($typeString);
+        $type = Record::typeNameToCode($typeString);
+        if ($type === null) {
+            throw new InvalidArgumentException("Invalid record type '$typeString' (line $lineNum).");


Intentional, or ImportException too?

Intentional, it's within a static method that doesn't have the $content variable in scope.

We throw this exception and it's caught by a try catch in the import() method 👍

feat: importexceptions

f7f17d0

Meldiron reviewed Nov 10, 2025

View reviewed changes

Meldiron approved these changes Nov 10, 2025

View reviewed changes

loks0n merged commit 14e9b33 into main Nov 10, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: importexceptions#20

feat: importexceptions#20
loks0n merged 1 commit into
mainfrom
feat-importexceptions

loks0n commented Nov 10, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Nov 10, 2025 •

edited

Loading

Uh oh!

Meldiron Nov 10, 2025

Uh oh!

loks0n Nov 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

loks0n commented Nov 10, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

Meldiron Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

loks0n Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

loks0n commented Nov 10, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Nov 10, 2025 •

edited

Loading