feat: add IPv4 validator for strings #192
Conversation
- Added IPv4() method to StringSchema in string.go - Added ErrCodeIPv4 constant in zconst/zconst.go - Added default error message in i18n/en/en.go - Added comprehensive tests in string_validate_test.go and string_test.go - Updated documentation in docs/reference.md The validator uses net.ParseIP() and To4() to ensure the address is a valid IPv4 format and not IPv6.
WalkthroughAdded a String IPv4 validator: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Validator caller
participant Schema as StringSchema.IPv4
participant Net as net.ParseIP/To4
Caller->>Schema: Validate(value)
Schema->>Net: ParseIP(value)
Net-->>Schema: IP or nil
alt IP != nil and To4() != nil
Schema-->>Caller: success (no issue)
else not IPv4
Schema-->>Caller: return issue (IssueCodeIP, param "IPv4")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
zconst/consts.go (1)
137-138: Constant definition is correct.The IPv4 issue code follows the established naming pattern and is positioned logically after the URL constant.
Consider removing the extra blank line at Line 138 for consistency with other constant declarations in this block.
IssueCodeIPv4 ZogIssueCode = "ipv4" -string_test.go (1)
201-221: Good test coverage for basic IPv4 validation.The tests cover valid addresses, invalid formats, out-of-range octets, and coercion behavior.
Consider adding the following test cases for more comprehensive coverage:
- IPv6 rejection test: Verify that IPv6 addresses like "2001:db8::1" or "::1" are properly rejected
- Negation test: Add test for
String().Not().IPv4()for consistency with Email and URL validators (see TestStringNot lines 408-427 for examples)Example additions:
// Add to TestStringIPv4 errs = field.Parse("2001:db8::1", &dest) assert.NotEmpty(t, errs) tutils.VerifyDefaultIssueMessages(t, errs) errs = field.Parse("::1", &dest) assert.NotEmpty(t, errs) tutils.VerifyDefaultIssueMessages(t, errs)// Add to TestStringNot around line 458 "not ipv4": { schema: String().Not().IPv4(), strVal: "not-an-ip", expectedErrMap: nil, }, "not ipv4 failure": { schema: String().Not().IPv4(), strVal: "192.168.1.1", expectedErrMap: internals.ZogIssueList{ &internals.ZogIssue{ Code: "not_ipv4", Dtype: "string", Value: tutils.PtrOf("192.168.1.1"), Message: "must not be a valid IPv4 address", Err: nil, }, }, },string_validate_test.go (1)
210-245: Comprehensive validation test coverage.The test covers valid IPv4 addresses, invalid formats, optional/required field behavior, and correct error code reporting.
Consider adding an IPv6 rejection test case to explicitly verify that IPv6 addresses are not accepted:
// Invalid cases dest = "256.1.1.1" errs = field.Validate(&dest) assert.NotEmpty(t, errs) assert.Equal(t, zconst.IssueCodeIPv4, errs[0].Code) dest = "not-an-ip" errs = field.Validate(&dest) assert.NotEmpty(t, errs) assert.Equal(t, zconst.IssueCodeIPv4, errs[0].Code) + + // IPv6 should be rejected + dest = "2001:db8::1" + errs = field.Validate(&dest) + assert.NotEmpty(t, errs) + assert.Equal(t, zconst.IssueCodeIPv4, errs[0].Code) // Required field with empty stringAlso consider adding a negation test case in TestValidateStringNot (similar to the recommendation for string_test.go).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/reference.md(1 hunks)i18n/en/en.go(1 hunks)string.go(2 hunks)string_test.go(1 hunks)string_validate_test.go(1 hunks)zconst/consts.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
string_test.go (2)
string.go (1)
String(83-91)tutils/testIssueMessages.go (1)
VerifyDefaultIssueMessages(14-32)
i18n/en/en.go (1)
zconst/consts.go (1)
IssueCodeIPv4(137-137)
string_validate_test.go (3)
string.go (1)
String(83-91)zconst/consts.go (2)
IssueCodeIPv4(137-137)IssueCodeRequired(63-63)internals/tests.go (1)
Required(114-121)
string.go (5)
internals/tests.go (2)
TestOption(17-17)Test(64-75)utilsOptions.go (2)
TestOption(10-10)IssueCode(32-36)utils.go (2)
Test(42-42)Ctx(12-12)zconst/consts.go (1)
IssueCodeIPv4(137-137)internals/contexts.go (1)
Ctx(10-44)
🔇 Additional comments (3)
docs/docs/reference.md (1)
111-111: Documentation looks good.The IPv4 validator is properly documented and positioned logically between URL and UUID validators.
i18n/en/en.go (1)
31-31: Translation entry looks good.The error message is clear and consistent with other validators (Email, URL, UUID).
string.go (1)
4-4: Import addition is correct.The
netpackage is the appropriate standard library choice for IP validation.
Oudwins
left a comment
Oudwins
left a comment
There was a problem hiding this comment.
Awesome work! Thank you so much for this! As it sometimes happens AI got confused here.
I don't think there is a use case for wanting this validator in the not schema. At least from briefly thinking about it doesn't really seem useful. Happy to keep it in if you can think of a use case that wouldn't be better served with a different validator. Otherwise, I would prefer we remove it.
But other than that it looks great!
Co-authored-by: Tristan M. <[email protected]>
Co-authored-by: Tristan M. <[email protected]>
rshelekhov
left a comment
rshelekhov
left a comment
There was a problem hiding this comment.
Thanks for the feedback! Adjusted as suggested.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
string.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
string.go (5)
internals/tests.go (2)
TestOption(17-17)Test(64-75)utilsOptions.go (2)
TestOption(10-10)IssueCode(32-36)utils.go (2)
Test(42-42)Ctx(12-12)zconst/consts.go (1)
IssueCodeIPv4(137-137)internals/contexts.go (1)
Ctx(10-44)
🔇 Additional comments (1)
string.go (1)
4-4: LGTM! Import is necessary for IPv4 validation.The
netpackage import is correctly added to support the newIPv4()validator method.
Oudwins
left a comment
Oudwins
left a comment
There was a problem hiding this comment.
Hey @rshelekhov thank you for making the changes.
I must apologize because I said it looked good. But went on a walk thought a little bit more about it and I think to future proof the validation errors a bit more it would make sense to do a minor change on how the errors are generated. Hopefully its not too much trouble. Let me know if the explanations are not clear enough!
Oudwins
left a comment
Oudwins
left a comment
There was a problem hiding this comment.
Hey! With this changes and considerations I feel pretty confident that this is the right path forward. Left a few comments in the older threads beyond this I think we are good. Once this merges I will immediately do a release so if you are wanting to use this you can get access immediately.
Again apologies for all the back and forth
… messages and validation
No worries at all! I completely understand and really value your thorough approach. It’s definitely better to fine-tune the implementation now to make sure it’s done right. I just pushed the changes. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
i18n/en/en.go(1 hunks)i18n/es/es.go(1 hunks)string.go(2 hunks)string_validate_test.go(1 hunks)zconst/consts.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- string_validate_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
i18n/en/en.go (1)
zconst/consts.go (1)
IssueCodeIP(137-137)
string.go (5)
zconst/consts.go (2)
IPv4(140-140)IssueCodeIP(137-137)internals/tests.go (2)
TestOption(17-17)Test(64-75)utilsOptions.go (3)
TestOption(10-10)IssueCode(32-36)Params(57-61)utils.go (2)
Test(42-42)Ctx(12-12)internals/contexts.go (1)
Ctx(10-44)
i18n/es/es.go (1)
zconst/consts.go (1)
IssueCodeIP(137-137)
2 participants
Description
Added IPv4() validator for string schema to validate IPv4 address format.
Motivation
IPv4 validation is a common requirement for:
This validator is present in similar libraries (Zod, go-playground/validator)
and would be a valuable addition to Zog.
Implementation Details
net.ParseIP()functionTo4()method to ensure IPv4 format (not IPv6)Changes
IPv4()method toStringSchemainstring.goErrCodeIPv4constant inzconst/zconst.goi18n/en/en.gostring_validate_test.gostring_test.godocs/reference.mdTesting
Example Usage
Closes issue #191
Summary by CodeRabbit
New Features
Documentation
Localization
Tests