tdegrunt / jsonschema

JSON Schema validation
Other
1.83k stars 262 forks source link

Pattern handling breaks when passing patterns with flags #325

Open kyrandita opened 3 years ago

kyrandita commented 3 years ago

Recently updated to 1.3.0, the bug seems to have been introduced in 1.2.7 relating to commit https://github.com/tdegrunt/jsonschema/commit/35db08dac87a627f6e0ebcaaf72c15bfb4cf3fde (thanks to issue https://github.com/tdegrunt/jsonschema/issues/311 for finding this for me) confirmed still present in 1.4.0

The issue is that when passing an existing regex pattern /abc/i (matches 'abc', 'ABC' and various combinations) the existing flags are overridden by the addition of the 'u' flag making the resulting pattern /abc/u (matches ONLY 'abc'). By using the new RegExp(pattern, flags) constructor the flags parameter when provided overrides all existing flags if the pattern has any.

In my opinion forcing the u flag on all regex is wrong, if that is the default when passing as string patterns I don't see anything strictly wrong with that, but if I pass a pattern why are my flags being messed with? If adding the u flag is for some reason required even when I didn't set it myself my flags should at least be preserved.

The following would be my preferred solution

try {
  if(pattern instanceof RegExp) {
    regexp = pattern;
  } else {
    regexp = new RegExp(pattern, 'u');
  }
} catch(_e) {

If forcing the u flag is deemed necessary then substituting regexp = new RegExp(pattern, pattern.flags + 'u'); should work for the passing a regex object case

kyrandita commented 3 years ago

technically the entire try/catch could be moved inside this else block since it's only to provide backup behavior for when the forcing of the u flag breaks things on string patterns

awwright commented 3 years ago

Passing a RegExp literal is not currently supported (it's not described anywhere, and there's no test for it).

But this is a reasonable feature request, I'll take a look.

kyrandita commented 3 years ago

examples/all.js line 207 (last updated in 2016) is passing a RegExp literal, although not using flags to be sure... Though this was implemented long enough ago initially that I can't be sure it wasn't just "try and see if it works" on our side, in either case thank you for considering this :)

awwright commented 3 years ago

@kyrandita Can you please post a code sample that you think should work, but doesn't? This seems to work for me:

validate('0{123}', {'type': 'string', 'pattern': /.{1.3}/});
alanosullivancs commented 2 years ago

@awwright here is an example is the case sensitivity flag which should be observed but is overwritten in the RegExp constructor: validate('cAsEsenSitIve', {'type': 'string', 'pattern': /casesensitive/i}); this returns an error that is incorrect: 'instance does not match pattern "/casesensitive/i"'