-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add MAC Address validation (#3970) #3972
base: main
Are you sure you want to change the base?
feat: Add MAC Address validation (#3970) #3972
Conversation
✅ Deploy Preview for guileless-rolypoly-866f8a ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Co-authored-by: Maarten Knijnenberg <[email protected]>
Co-authored-by: Maarten Knijnenberg <[email protected]>
@coderabbitai review this |
🧩 Analysis chain@colinhacks I'll review the changes for the MAC address validation feature. 🏁 Script executed:
Length of output: 10746 @colinhacks Thank you for the PR. I've reviewed the changes for MAC address validation. The implementation looks solid with good test coverage. Here are a few suggestions for improvement:
// Matches 6 groups of 2 hexadecimal characters
// Each group is separated by either all colons or all dashes
const macRegex = /^
(?:
([0-9A-Fa-f]{2}(:)){5}[0-9A-Fa-f]{2} | // colon-separated
([0-9A-Fa-f]{2}(-)){5}[0-9A-Fa-f]{2} // dash-separated
)$/x;
const macRegex = /^([0-9A-Fa-f]{2}(:)){5}[0-9A-Fa-f]{2}$|^([0-9A-Fa-f]{2}(-)){5}[0-9A-Fa-f]{2}$/i;
The implementation is good to go, but these changes would make it even better. Let me know if you'd like me to explain any of these suggestions in more detail. ✅ Actions performedReview triggered.
|
WalkthroughThis pull request extends the string validation library with MAC address validation. Documentation in the README is updated with a new section explaining MAC validation and clarifies IP ranges with a "(CIDR)" note. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Z as ZodString
participant V as isValidMac
U->>Z: Call mac() with input string
Z->>V: Validate input using regex
V-->>Z: Return true/false
alt Valid MAC input
Z-->>U: Return success result
else Invalid MAC input
Z-->>U: Return error message with issue details
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/__tests__/string.test.ts (1)
1-976
:⚠️ Potential issueRun Prettier to fix code style issues.
The test pipeline indicates code style issues. Please run Prettier on the test file to ensure consistent formatting.
🧰 Tools
🪛 GitHub Actions: test
[warning] 1-1: Code style issues found. Forgot to run Prettier?
🧹 Nitpick comments (5)
src/__tests__/string.test.ts (1)
942-976
: Consider enhancing the MAC validation test suite.While the test cases are comprehensive, consider these improvements:
- Add case-insensitive validation tests
- Add comments explaining the test cases
- Include additional edge cases:
- Mixed case (e.g., "00:1a:2B:3c:4D:5e")
- Leading/trailing whitespace
- Invalid separators (e.g., "00/1A/2B/3C/4D/5E")
Here's a suggested enhancement:
test("MAC validation", () => { const mac = z.string().mac(); + // Basic validation expect(mac.safeParse("00:1A:2B:3C:4D:5E").success).toBe(true); expect(() => mac.parse("00:1A-2B:3C:4D:5E")).toThrow(); + // Test cases for valid MAC addresses const validMacs = [ + // Standard uppercase format with colons "00:1A:2B:3C:4D:5E", "FF:FF:FF:FF:FF:FF", + // Standard uppercase format with hyphens "01-23-45-67-89-AB", + // Mixed case format + "00:1a:2B:3c:4D:5e", + // Lowercase format "0a:1b:2c:3d:4e:5f", + // With whitespace (if supported) + " 00:1A:2B:3C:4D:5E ", // ... rest of the valid cases ]; + // Test cases for invalid MAC addresses const invalidMacs = [ + // Mixed separators "00:1A-2B:3C-4D:5E", + // No separators "001A2B3C4D5E", + // Too short "00:1A:2B:3C:4D", + // Too long "00:1A:2B:3C:4D:5E:FF", + // Invalid characters "00:1A:2B:3C:4D:GZ", + // Invalid separators + "00/1A/2B/3C/4D/5E", // ... rest of the invalid cases ]; const macSchema = z.string().mac(); expect(validMacs.every((mac) => macSchema.safeParse(mac).success)).toBe(true); expect(invalidMacs.every((mac) => macSchema.safeParse(mac).success)).toBe(false); });src/types.ts (2)
699-702
: Improve MAC address regex readability and case-insensitivity.The MAC address regex pattern could be improved in the following ways:
- Add comments to explain the components
- Add case-insensitive flag
- Fix typo in comment ("seperators" → "separators")
Apply this diff to improve the regex pattern:
-// no support for mac without seperators -// enforces all seperators should be either ':' or '-' -const macRegex=/^([0-9A-Fa-f]{2}(:)){5}[0-9A-Fa-f]{2}$|^([0-9A-Fa-f]{2}(-)){5}[0-9A-Fa-f]{2}$/; +// No support for MAC addresses without separators +// Enforces all separators must be either ':' or '-' +const macRegex = /^([0-9A-Fa-f]{2}(:)){5}[0-9A-Fa-f]{2}$|^([0-9A-Fa-f]{2}(-)){5}[0-9A-Fa-f]{2}$/i; +// Explanation: +// ^ Start of string +// [0-9A-Fa-f]{2} Two hex digits +// (:) Colon separator (captured) +// {5} Repeat 5 times +// [0-9A-Fa-f]{2} Final two hex digits +// $ End of string +// | OR +// Same pattern but with hyphen separators
779-785
: Simplify the MAC address validation function.The validation function can be simplified by directly returning the test result.
Apply this diff to simplify the function:
-function isValidMac(mac:string) { - if (macRegex.test(mac)){ - return true; - } - - return false; -} +function isValidMac(mac: string): boolean { + return macRegex.test(mac); +}README.md (2)
813-813
: MAC Address Validation Example Added
The examplez.string().mac(); // Validate 48-bit MAC
clearly illustrates the basic usage of the new MAC address validation method. Consider adding a brief note mentioning that the validator supports both colon-separated and dash-separated formats and that it applies case-insensitive matching to accommodate lower- and uppercase letters.
977-988
: Detailed MAC Addresses Section
The new "MAC addresses" section provides clear examples of how to validate standard 48-bit MAC addresses. The examples include:
- Valid cases with both colon (
"00:1A:2B:3C:4D:5E"
) and dash ("98-76-54-32-10-FF"
) separations.- Invalid cases, including the case with an extra segment (
"00:1A:2B:3C:4D:5E:FF"
) and a mixed separator format ("00:1A:2B-3C:4D:5E"
).For improved clarity, consider including a comment (or a link to the implementation) explaining the underlying regex—for example, highlighting that the regex is designed to be case-insensitive and only allows one consistent separator throughout the string. Also, ensure that the heading starts at the beginning of the line as required by markdown lint rules.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
977-977: Headings must start at the beginning of the line
null(MD023, heading-start-left)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md
(5 hunks)src/ZodError.ts
(1 hunks)src/__tests__/string.test.ts
(10 hunks)src/types.ts
(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: test
src/__tests__/string.test.ts
[warning] 1-1: Code style issues found. Forgot to run Prettier?
README.md
[warning] 1-1: Code style issues found. Forgot to run Prettier?
🪛 markdownlint-cli2 (0.17.2)
README.md
977-977: Headings must start at the beginning of the line
null
(MD023, heading-start-left)
🔇 Additional comments (8)
src/ZodError.ts (1)
110-110
: LGTM!The addition of "mac" to the
StringValidation
type is well-integrated with the existing validation types and follows the established pattern.src/__tests__/string.test.ts (1)
467-566
: LGTM!The isMac property checks are well-implemented and follow the established pattern for other string validation types. Each validation type correctly reports its isMac status.
src/types.ts (4)
628-629
: LGTM!The type definition for MAC address validation follows the established pattern for string validations.
1089-1099
: LGTM!The MAC address validation logic is well-integrated into the existing validation framework and follows the established pattern.
1178-1180
: LGTM!The mac() method is correctly implemented following the established pattern for string validation methods.
1384-1386
: LGTM!The isMac getter is correctly implemented following the established pattern for string validation getters.
README.md (2)
78-79
: New Table of Contents Entry for MAC Addresses
The addition of the "MAC addresses" link in the table of contents ensures that the new validation feature is easily discoverable. This update is clear and consistent with the surrounding entries.
845-845
: Custom Error Message for MAC Address Validation
The examplez.string().mac({ message: "Invalid MAC address" });
shows how to override the default error message. This is a good practice for enhancing usability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
1002-1002
: Markdown Heading AlignmentThe heading “### MAC addresses” on line 1002 appears to have leading whitespace. Please remove any extra spaces so that the heading starts at the beginning of the line in order to comply with markdownlint MD023.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1002-1002: Headings must start at the beginning of the line
null(MD023, heading-start-left)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md
(5 hunks)src/types.ts
(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
1002-1002: Headings must start at the beginning of the line
null
(MD023, heading-start-left)
🔇 Additional comments (2)
README.md (2)
78-79
: TOC Update for MAC Addresses FeatureThe addition of the new TOC item “MAC addresses” (along with "IP ranges (CIDR)") aligns well with the PR objectives. Please verify that the anchor "#mac-addresses" correctly links to the corresponding section below.
1006-1013
: Comprehensive MAC Address ExamplesThe code snippet clearly demonstrates the usage of the new
mac()
validation method. It shows valid cases with both colon-separated and dash-separated MAC addresses as well as failure cases for improper formats (extra group and mixed separator use).Suggestion: If not already implemented, consider explicitly noting (in the documentation or inline comments) that the validation is case-insensitive so that both uppercase and lowercase hexadecimal digits are accepted.
@colinhacks I’ve reviewed the suggestions from CodeRabbit AI and I’m aware of the lint stage failure in actions. I’ll fix the lint issues, but I’d like to confirm whether I should also incorporate the AI-suggested changes or just focus on resolving the lint errors before updating the PR. |
This PR adds support for validating MAC addresses in Zod, addressing issue #3970. The validation ensures that input values conform to standard MAC address formats.
Changes Introduced:
XX:XX:XX:XX:XX:XX
) and dash-separated (XX-XX-XX-XX-XX-XX
) formats.Testing Done:
yarn test
to ensure all existing and new tests pass.Checklist:
Issue Reference:
Closes #3970.
Summary by CodeRabbit
New Features
Documentation
Tests