fix: correctly run `.matches` when passing regex object #1150

tonysamperi · 2022-05-23T13:48:38Z

Forcing pattern to be string, to cope with lib validator which doesn't reset the RegExp instance
Fixes #1127

Description

Fixes matches validation failing one other time, passing regex value with g flag.

I have added tests for what I changed.

I didn't add any tests, because it works exactly as before, but simply extracts the source when passing the regex.

This pull request is ready to merge.

…t reset the RegExp instance

fedeci · 2022-05-23T20:14:33Z

src/chain/validators-impl.ts

+      typeof pattern === 'string' ? pattern : pattern.source,
+      modifiers,


Isn't this counter-intuitive?
I mean this means that a user specifying a regex object with flags need to also set the optional modifiers separately
e.g.

.matches(/foo/g) // => becomes /foo/ .matches(/foo/g, 'g') // => is correctly /foo/g

Hi, yes, you're right! I forgot about the flags...I'll fix it tomorrow...

fedeci

Hey @tonysamperi thanks for the PR, I am unsure how this fixes the linked issue, validator.js correctly handles passed in regex objects (ref)

coveralls · 2022-05-23T20:29:23Z

Coverage remained the same at 100.0% when pulling c803450 on tonysamperi:bugfix/regex-instance into 82e4d84 on express-validator:master.

tonysamperi · 2022-05-23T21:53:36Z

Hey @tonysamperi thanks for the PR, I am unsure how this fixes the linked issue, validator.js correctly handles passed in regex objects (ref)

Hi Federico, it fixes (apart from the flags matter, that I'm fixing tomorrow) the problem I discovered (yes, I'm KikoCosmetics) in #1127.
Basically when you pass a RegExp instance to express-validator, its instance gets preserved.
I noticed that in lib "validator", if pattern is string, a new RegExp gets instantiated, but if it's not (which is the case), it gets used as it is. Which means that after the first match, the lastIndex on the instance won't be 0, causing the second test to fail (one test passes, one fails...and so on...).
The idea of my fix was passing always a string, so that the test is done with a fresh RegExp instance every time.
This for sure is how express-validator should be intended to work.
I don't know if the behaviour of lib "validator" should be considered faulty.

Let me know what you think...we can also chat in Italian if it helps to better understand each other :)

fedeci · 2022-05-24T09:47:11Z

Don't worry I got that later, I totally missed that explanation in the issue! I've also already updated the validatorjs issue to ask them to start using .match. Thanks for the fix!

tonysamperi · 2022-05-24T10:15:05Z

Hi @fedeci I just added a commit...
I had to do something to merge regex flags with additional modifiers...
That's the cleanest way I could think of...
How does it look?
Just had to do some other fixing after running the standard-validation.spec...now it runs fine!

gustavohenke

Looks pretty solid and flexible to me 🙂 happy if @fedeci is

fedeci · 2022-05-25T20:59:27Z

src/chain/validators-impl.ts

+    return this.addStandardValidation.apply(this, [
+      validator.matches,
+      ...(typeof pattern === 'string'
+        ? [pattern, modifiers]
+        : [pattern.source, ...[new Set((modifiers || '') + pattern.flags)].join('')]),
+    ]);


Just a super nitpick, can we use a ternary expression or an if statement, instead of the apply? That's definitely correct but makes the code harder to read for new developers.

And can you add a couple tests to check that the flags added after the regex are effectively passed to the addStandatdValidation function?

No problem for the tests,
But removing the apply would mean duplicate entirely the call to addStandardValidation...
I'd say that new developers shouldn't be supposed to touch such a piece of code!
I was reluctant myself despite several years of experience! 😅
Also remember that if they fix validator library we can revert this change! 😉

That's a nice opinion and I'm fine to keep it as is, I'll open a PR and push to get that merged in asap!

And can you add a couple tests to check that the flags added after the regex are effectively passed to the addStandatdValidation function?

An even better testing way would be to reproduce the same regex in the issue, calling the method twice and asserting that the validation is passed both times.

Ok I'll add the tests tomorrow!
Thanks!!

Great! Thank you!

Good thing you asked for the tests...The spread was misplaced...

fedeci · 2022-06-08T20:15:12Z

Has this been closed for any particular reason? I'm sorry for the long delay in re-reviewing this PR!

tonysamperi · 2022-06-08T20:17:47Z

Hi I needed to delete the repo, I did some cleanup of my github account.
At this point maybe they'll fix on validator.js side and we won't need this!

tonysamperi · 2022-06-08T20:23:55Z

I can create another if you guys want btw

fedeci · 2022-06-08T20:25:07Z

I see it really though they will fix soon, they are likely slower then us because they tend to wait to merge all PRs at once. If you prefer I can create this PR from your commit again tomorrow (you will get the commit credits so it should be fine). Let me know!

tonysamperi · 2022-06-08T20:27:26Z

Don't worry I'll do some magic and maybe it will let me reopen this (I don't know if I can trick github by using the same branch name 😂)

Bye!

tonysamperi · 2022-06-09T19:42:32Z

Sadly, no, it won't let me reopen this PR. :(

Forcing pattern to be string, to cope with lib validator which doesn'…

74f376f

…t reset the RegExp instance

fedeci changed the title ~~Fix #1127~~ fix: correctly run .matches when passing regex object May 23, 2022

fedeci reviewed May 23, 2022

View reviewed changes

Handling regex flags, merging with additional flags passed as argument

9fbde32

tonysamperi added 2 commits May 24, 2022 12:16

Fix thisArg

83fb803

Fix apply args

c803450

gustavohenke approved these changes May 24, 2022

View reviewed changes

fedeci reviewed May 25, 2022

View reviewed changes

fedeci mentioned this pull request May 25, 2022

fix(matches): prevent regex state from breaking following validations validatorjs/validator.js#1975

Merged

3 tasks

Added tests and fixed spread position

40028c7

tonysamperi closed this Jun 7, 2022

tonysamperi deleted the bugfix/regex-instance branch June 7, 2022 19:15

		typeof pattern === 'string' ? pattern : pattern.source,
		modifiers,

Uh oh!

fix: correctly run .matches when passing regex object #1150

fix: correctly run .matches when passing regex object #1150

Uh oh!

Conversation

tonysamperi commented May 23, 2022 • edited by fedeci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonysamperi May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedeci left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonysamperi commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fedeci commented May 24, 2022

Uh oh!

tonysamperi commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gustavohenke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedeci commented Jun 8, 2022

Uh oh!

tonysamperi commented Jun 8, 2022

Uh oh!

tonysamperi commented Jun 8, 2022

Uh oh!

fedeci commented Jun 8, 2022

Uh oh!

tonysamperi commented Jun 8, 2022

Uh oh!

tonysamperi commented Jun 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: correctly run `.matches` when passing regex object #1150

fix: correctly run `.matches` when passing regex object #1150

tonysamperi commented May 23, 2022 •

edited by fedeci

Loading

tonysamperi May 23, 2022 •

edited

Loading

coveralls commented May 23, 2022 •

edited

Loading

tonysamperi commented May 23, 2022 •

edited

Loading

tonysamperi commented May 24, 2022 •

edited

Loading