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

Added nanoid() validation action #789

Merged
merged 8 commits into from
Aug 27, 2024

Conversation

jasperteo
Copy link
Contributor

Added pipeline validation action for Nano ID. v.nanoid() should work similarly to the other validation actions for ID generators like v.cuid2(), v.ulid() and v.uuid().

By default, Nano ID uses URL-friendly symbols (A-Za-z0-9_-) and returns an ID with 21 characters, but I modified it to a variable length of 2 to 36 characters instead (https://zelark.github.io/nano-id-cc/) while keeping the default alphabet/symbol set. This flexibility allows for people who want to use Nano ID with a reduced ID length and use v.length() to adjust.

@fabian-hiller fabian-hiller self-assigned this Aug 18, 2024
@fabian-hiller fabian-hiller added the enhancement New feature or request label Aug 18, 2024
@fabian-hiller
Copy link
Owner

Thank you for this PR! I will try to review and merge it next week.

@fabian-hiller
Copy link
Owner

I would recommend removing the 2 to 36 character limit from the regex and instead we could add the option to pass the length via the first argument of the function. If no argument is passed, it defaults to 21. What do you think?

@jasperteo
Copy link
Contributor Author

I did take precedence from the existing v.cuid2() action, which didn't have the option to pass length. Correct me if I am wrong, Cuid2 and Nano ID are similar in that their lengths can be adjusted (default 32 for Cuid2 and default 21 for Nano ID).

I do agree with you though, I like

const NanoIDSchema = v.pipe(
  v.string(),
  v.nanoid(21, 'The Nano ID is badly formatted.')
);

over

const NanoIDSchema = v.pipe(
  v.string(),
  v.nanoid('The Nano ID is badly formatted.'),
  v.length(21)
);

I will make changes with your suggestions

@fabian-hiller
Copy link
Owner

I think I would try to create the regex dynamically using the passed length.

@jasperteo
Copy link
Contributor Author

I made some changes so that it will accept length as the first argument of the function.

ESLint was complaining about the dynamic regex https://github.com/eslint-community/eslint-plugin-security/blob/main/docs/rules/detect-non-literal-regexp.md so I opted to do it another way.

I am a little unsure how the generics should be set up now that length is the first argument.

@fabian-hiller
Copy link
Owner

I thought about what I wrote and I am no longer sure if we should bake the length into the action. Since the length is variable, it might be better to recommend using our length actions for more flexibility. This would also allow you to specify a length range. What do you think?

@jasperteo
Copy link
Contributor Author

jasperteo commented Aug 24, 2024

This would make it more in line with the rest of the other existing validation actions.

I think using v.length() to control length might be better in a long run to keep things standardized and not confuse users, which was what I had in mind initially, although it's convenient to have length parameter inside v.nanoid() itself.

I will probably reverse some changes of my previous commit, but I think I will still remove the 2 to 36 character limit, so it just checks for valid characters in the regex, leaving the option to check length entirely to the v.length() action if the user wants to do so. It will be similar to v.cuid2().

No length check

const NanoIDSchema = v.pipe(
  v.string(),
  v.nanoid('The Nano ID is badly formatted.')
);

With length check

const NanoIDSchema = v.pipe(
  v.string(),
  v.nanoid('The Nano ID is badly formatted.'),
  v.length(21)
);

Does it sound good to you?

@fabian-hiller
Copy link
Owner

Thank you for your changes. I will review everything soon and probably release it with our next minor release.

@fabian-hiller
Copy link
Owner

Thank you so much! You did a great job with this PR! Are you interested in helping me add more common string validators for JWT (#692) as well as Bitcoin and Ethereum addresses?

@fabian-hiller fabian-hiller force-pushed the feat-nanoid-validation branch from 90d4c20 to 9f25d78 Compare August 27, 2024 19:14
@fabian-hiller fabian-hiller merged commit 97b33f4 into fabian-hiller:main Aug 27, 2024
6 checks passed
@jasperteo jasperteo deleted the feat-nanoid-validation branch August 28, 2024 04:14
@jasperteo
Copy link
Contributor Author

Thank you so much! You did a great job with this PR! Are you interested in helping me add more common string validators for JWT (#692) as well as Bitcoin and Ethereum addresses?

Thanks! Unable to help for now but I will look into it when I have more time.

@fabian-hiller
Copy link
Owner

v0.40.0 is available 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants