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.

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 string regex and uuid validators #40

Merged
merged 7 commits into from
Oct 31, 2024
Merged

Conversation

amaro0
Copy link
Contributor

@amaro0 amaro0 commented Oct 30, 2024

Hi, I just started using Zog and I love it so far. During development, I noticed that there are no validators for both UUID and regex. Let me know what you think about adding these two. I hope I didn't mess up the Spanish translations. Feel free to close this if you prefer to keep the interface lean before v1.0.

@Oudwins
Copy link
Owner

Oudwins commented Oct 31, 2024

This is awesome @amaro0 thanks a lot. These were on my TODO but I'm have been busy and I am currently focusing on implementing pointers. Overall it looks good. If you don't mind doing a few small changes I am more than happy to merge this:

  • fix the linting error
  • spanish uuid error should be "Debe ser un UUID válido"
  • I would like the regex to be a parameter for the regex test. You can look at the contains test for en example of how to do this. Such that the error is not "string is invalid" but rather "string does not match regex {{regex}}" or something like that.

Lastly, one thing I would like your thoughts on:

  • Should the name for the test be "Matches" or "Regex"?
  • "Matches" feels a little more golang since the methods in stdlib are regexp.Match() & regexp.MatchString()
  • "Regex" is more Zod

I can honestly go either way

Also how did you get the uuid regex? Here are some other ones unsure if any are better, regexes are impossible to review XD:

# used by Yup
/^(?:[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}|00000000-0000-0000-0000-000000000000)$/i
# used by zod. This one looks like the one you used and it looks like zod used the Yup version but switched
/^[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12}$/i

# Golang Validator project has multiple:
	uUID3RegexString                 = "^[0-9a-f]{8}-[0-9a-f]{4}-3[0-9a-f]{3}-[0-9a-f]{4}-[0-9a-f]{12}$"
	uUID4RegexString                 = "^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$"
	uUID5RegexString                 = "^[0-9a-f]{8}-[0-9a-f]{4}-5[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$"
	uUIDRegexString                  = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"
	uUID3RFC4122RegexString          = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-3[0-9a-fA-F]{3}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"
	uUID4RFC4122RegexString          = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$"
	uUID5RFC4122RegexString          = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-5[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$"
	uUIDRFC4122RegexString           = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"

anyways. After looking at it I think its fine. We'll just use what Zod uses.

@Oudwins
Copy link
Owner

Oudwins commented Oct 31, 2024

Oh, btw how was the contributing process? I still don't have a guide up but I hope the code is quite readable and easy to understand. I'm curious if you had trouble understanding any specific parts of the code when implementing this. If you did I would love to hear them so I can explain those in the contributing guide

@amaro0
Copy link
Contributor Author

amaro0 commented Oct 31, 2024

I would like the regex to be a parameter for the regex test. You can look at the contains test for en example of how to do this. Such that the error is not "string is invalid" but rather "string does not match regex {{regex}}" or something like that.

I did this at first but decided to ditch that approach. There are two problems with this approach that I can think of:

  • Bad actor can use regex from error message for ReDoS attack
  • The end user would not understand what needs to be fixed based on that error message

I also reviewed the zod library, which does not expose the regex in the error message but instead provides a simple string is invalid error message. I believe not exposing the regex is a sensible default. If needed, you can manually include the regex in a custom error message.

Let me know your thoughts on this approach.

Should the name for the test be "Matches" or "Regex"?

For me regex is more natural as I am using zod for few years. Also I am rarely developing in Go.

Also how did you get the uuid regex?

Taken from the zod source code. As I am primarily working in web dev and never used non-v4 UUIDs, this is all I need. Also since this regex allows UPPERCASE characters it might be beneficial to add ToLowerCase/ToUpperCase methods

Contributing was a breeze. I looked at implementation of URL validator and everything was immediately clear. I spent more time picking and testing UUID regex than writing the code. I was bit surprised by the presence of Spanish translations.

@Oudwins
Copy link
Owner

Oudwins commented Oct 31, 2024

I did this at first but decided to ditch that approach. There are two problems with this approach that I can think of:

You are right and I am wrong. This is the better approach.

For me regex is more natural as I am using zod for few years. Also I am rarely developing in Go.

Lets rename it to match then. Its a little more specific and doesn't close the door to implementing other regex methods (such as find, etc). I think regex is more intuitive but overall match is better. To be clear "Match" not "Matches" like I said before. "Match" is the actual name of the method being used under the hood so I think this makes more sense.

Contributing was a breeze. I looked at implementation of URL validator and everything was immediately clear. I spent more time picking and testing UUID regex than writing the code. I was bit surprised by the presence of Spanish translations.

I can't put into words how happy this makes me feel! Thanks!

string.go Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry I forgot to mention. My bad. I would like just the string as the param. So users can add it to the error message very easily if they want to by just modifying the error message to "String does not match {{match}}".

You can get the string out of the regex by doing regex.String()

@Oudwins
Copy link
Owner

Oudwins commented Oct 31, 2024

Thanks a lot for everything! I know its annoying doing small changes like this.

I forgot to say that Zog supports spanish mostly because I am spanish and need the translations. If you ever want to make another PR spanish is not required I'm happy to come in and add the translations as needed.

@amaro0
Copy link
Contributor Author

amaro0 commented Oct 31, 2024

Done :)

@Oudwins Oudwins merged commit 8853d76 into Oudwins:master Oct 31, 2024
3 checks passed
@Oudwins
Copy link
Owner

Oudwins commented Oct 31, 2024

Merged. Thank you! You are awesome

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