Skip to content
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.

Sign up for GitHub

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

Add alphaspace validator #1343

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+40 −0
Open

Add alphaspace validator #1343

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf1b127
add alphaspace validator
takaaa220 Dec 7, 2024
da6de87
Fix doc for alpha space
takaaa220 Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
@@ -134,6 +134,7 @@ validate := validator.New(validator.WithRequiredStructEnabled())
| Tag | Description |
| - | - |
| alpha | Alpha Only |
| alphaspace | Alpha Space |
Copy link

Choose a reason for hiding this comment

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

I'm a bit annoyed, this could lead to obesity

I mean, someone will then ask for alphanumspace, alphaunicodespace, and so on

The regexps are computed by the lib even if no one use it, so lib becomes slower and slower.

The PR addresses the issue raised. So it's OK.

But I would like to step out a bit.

The regexp rule idea was already declined, for good reasons

So I think the way to consider doing this is via the custom validator

#1191 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

@ccoVeille
Thank you for your detailed feedback. I understand and agree your concerns regarding the potential growth and its impact on library performance.

If you feel a custom validator is the better path for this functionality, I’m happy to close this pull request if necessary.

Copy link

Choose a reason for hiding this comment

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

I'm a random Gopher,user of the lib

Let's wait for a maintainer's feedback

Copy link
Author

Choose a reason for hiding this comment

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

I'm a random Gopher,user of the lib

oh, I mistakenly thought you were the maintainer.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the regex's are lazy loaded, so if anyone doesn't use them, it doesn't affect them as much.

In this case if people think this is a good addition, I don't think we should block it because we worry about it not being used and slightly bigger binary.

Copy link

Choose a reason for hiding this comment

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

Thanks for your explanation

| alphanum | Alphanumeric |
| alphanumunicode | Alphanumeric Unicode |
| alphaunicode | Alpha Unicode |
6 changes: 6 additions & 0 deletions baked_in.go
View file
Original file line number Diff line number Diff line change
@@ -114,6 +114,7 @@ var (
"fieldcontains": fieldContains,
"fieldexcludes": fieldExcludes,
"alpha": isAlpha,
"alphaspace": isAlphaSpace,
"alphanum": isAlphanum,
"alphaunicode": isAlphaUnicode,
"alphanumunicode": isAlphanumUnicode,
@@ -1761,6 +1762,11 @@ func isAlphanumUnicode(fl FieldLevel) bool {
return alphaUnicodeNumericRegex().MatchString(fl.Field().String())
}

// isAlphaSpace is the validation function for validating if the current field's value is a valid alpha value with spaces.
func isAlphaSpace(fl FieldLevel) bool {
return alphaSpaceRegex().MatchString(fl.Field().String())
}

// isAlphaUnicode is the validation function for validating if the current field's value is a valid alpha unicode value.
func isAlphaUnicode(fl FieldLevel) bool {
return alphaUnicodeRegex().MatchString(fl.Field().String())
6 changes: 6 additions & 0 deletions doc.go
View file
Original file line number Diff line number Diff line change
@@ -762,6 +762,12 @@ This validates that a string value contains ASCII alpha characters only

Usage: alpha

# Alpha Space

This validates that a string value contains ASCII alpha characters and spaces only

Usage: alphaspace

# Alphanumeric

This validates that a string value contains ASCII alphanumeric characters only
2 changes: 2 additions & 0 deletions regexes.go
View file
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ import (

const (
alphaRegexString = "^[a-zA-Z]+$"
alphaSpaceRegexString = "^[a-zA-Z ]+$"
Copy link
Author

Choose a reason for hiding this comment

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

Would it be better for the regex to not match if the input contains only spaces?

Copy link

Choose a reason for hiding this comment

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

I would leave it like that

But people already asked for what you suggest, so I don't know

#1191 (comment)

But then, should leading and trailing space are valid?

  • foo, trailing space, valid or invalid?
  • foo , leading space, valid or invalid
  • `` empty, invalid?
  • only space, invalid?
  • foo bar

I'm asking because the regexp will need to be adapted

Copy link
Author

@takaaa220 takaaa220 Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I reviewed #1191 (comment) before asking the question. My question was more for clarification to ensure we’re aligned on the intended behavior.
I think that making the regex more complex to handle edge cases like these is unnecessary. Current implementation avoids unnecessary complexity while meeting the expected use cases.

Copy link
Member

Choose a reason for hiding this comment

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

I think the validator should do what it says and nothing else. If a string contains any of the alpha characters or an empty space it should be valid.

Anything about spaces before or after should then be dealt with by the user IMO. Unless someone can give a great example why " a" is invalid and "a a" should be valid I think this is good.

alphaNumericRegexString = "^[a-zA-Z0-9]+$"
alphaUnicodeRegexString = "^[\\p{L}]+$"
alphaUnicodeNumericRegexString = "^[\\p{L}\\p{N}]+$"
@@ -92,6 +93,7 @@ func lazyRegexCompile(str string) func() *regexp.Regexp {

var (
alphaRegex = lazyRegexCompile(alphaRegexString)
alphaSpaceRegex = lazyRegexCompile(alphaSpaceRegexString)
alphaNumericRegex = lazyRegexCompile(alphaNumericRegexString)
alphaUnicodeRegex = lazyRegexCompile(alphaUnicodeRegexString)
alphaUnicodeNumericRegex = lazyRegexCompile(alphaUnicodeNumericRegexString)
25 changes: 25 additions & 0 deletions validator_test.go
View file
Original file line number Diff line number Diff line change
@@ -8990,6 +8990,31 @@ func TestAlpha(t *testing.T) {
AssertError(t, errs, "", "", "", "", "alpha")
}

func TestAlphaSpace(t *testing.T) {
validate := New()

s := "abcd"
errs := validate.Var(s, "alphaspace")
Equal(t, errs, nil)

s = "abc def"
errs = validate.Var(s, "alphaspace")
Equal(t, errs, nil)

s = " "
errs = validate.Var(s, "alphaspace")
Equal(t, errs, nil)

s = "abc!"
errs = validate.Var(s, "alphaspace")
NotEqual(t, errs, nil)
AssertError(t, errs, "", "", "", "", "alphaspace")

errs = validate.Var(1, "alphaspace")
NotEqual(t, errs, nil)
AssertError(t, errs, "", "", "", "", "alphaspace")
}

func TestStructStringValidation(t *testing.T) {
validate := New()