Implement String.regex "invalidate" configuration #1035

WesTyler · 2016-11-14T17:02:43Z

Per discussions in #1033 and #1020, refactored string().regex(pattern, [name]) to support string().regex(pattern, [name | config]) as follows:

If name is a string it behaves as currently implemented. No change.
If config is an object, it supports the following properties:

name: {string}, behaves as currently implemented.
invalidate: {boolean}, defaults to false behavior; if true, disallows the provided regex pattern.

Usage:

const schema = Joi.string().regex(/[a-z]/, {
    name      : 'lowercase',
    invalidate: true
});

schema.validate('bad', (err, value) => {
    if (err) {
        // ValidationError: "value" with value "bad" matches the invalidated lowercase pattern
    }
});
schema.validate('BAD', (err, value) => {
    if (err) {} // err === null
});

schema.describe();
/*
{
  type: 'string',
  invalids: [ '' ],
  rules: [ { name: 'invalidatedRegex', arg: /[a-z]/ } ] 
}
*/

WesTyler · 2016-11-14T17:03:34Z

(Realized I missed the API doc update. Pushing now)

Marsup · 2016-11-14T18:21:48Z

lib/string.js

+
+            const patternMatch = pattern.test(value);
+
+            if ((patternMatch && !patternIsInvalidated) || (!patternMatch && patternIsInvalidated)) {


Clearer with patternMatch ^ patternIsInvalidated.

Marsup · 2016-11-14T18:23:52Z

lib/string.js


-            if (pattern.test(value)) {
+        return this._test(testName, pattern, function (value, state, options) {


pattern should now become an object with every details in it (name, pattern, invalidated), I wouldn't change the name of the test depending on the options.

I changed the test name to distinguish between a normal regex test and an inverted regex test in the describe'd value. Using the revision you requested, should the name/pattern/inverted flag just become part of the arg property on the describe?

{ type: 'string', invalids: [''], rules: [ { name: 'regex', arg: { pattern: /[a-z]/, inverted: true } } ] }

It will automatically if you do.

pattern.pattern gets a bit weird. Especially at this.createError (end up with a pattern: pattern.pattern). How about patternObject with a pattern property?

(Running low on time on my lunch break. Going to move forward with patternObject, but willing to change if you disagree)

Marsup · 2016-11-14T18:25:04Z

lib/string.js

@@ -91,19 +91,30 @@ internals.String = class extends Any {
        });
    }

-    regex(pattern, name) {
+    regex(pattern, config) {


I prefer the term options, in readme too.

I was going to use "options", but then I have a variable name clash with the test callback defined on line 103.

Oh, didn't see that one. patternOptions then ?

Marsup · 2016-11-14T18:26:56Z

API.md


 Defines a regular expression rule where:
 - `pattern` - a regular expression object the string value must match against.
 - `name` - optional name for patterns (useful with multiple patterns). Defaults to 'required'.
+- `config` - an optional configuration object with the following supported properties:
+  - `name` - optional pattern name. Defaults to `required`.
+  - `invalidate` - optional boolean flag. Defaults to `false` behavior. If specified as `true`, the provided pattern will be disallowed instead of required.


Not a big fan of invalidate, it makes me think of caching. What about invert ?

Marsup · 2016-11-14T18:29:08Z

API.md


 Defines a regular expression rule where:
 - `pattern` - a regular expression object the string value must match against.
 - `name` - optional name for patterns (useful with multiple patterns). Defaults to 'required'.
+- `config` - an optional configuration object with the following supported properties:
+  - `name` - optional pattern name. Defaults to `required`.


Same as the original, I have absolutely no idea where that Defaults to required came from, it isn't :)

Marsup · 2016-11-14T18:31:19Z

lib/string.js


        Hoek.assert(pattern instanceof RegExp, 'pattern must be a RegExp');

        pattern = new RegExp(pattern.source, pattern.ignoreCase ? 'i' : undefined);         // Future version should break this and forbid unsupported regex flags

-        return this._test('regex', pattern, function (value, state, options) {
+        const patternIsInvalidated = (typeof config === 'object' && Hoek.reach(config, 'invalidate'));


Why use hoek to access it since it's a direct property and you just checked it's an object ?

Habit. Updating along with other changes. :)

Marsup

You're close !

Marsup · 2016-11-14T20:18:40Z

lib/string.js

+            pattern: new RegExp(pattern.source, pattern.ignoreCase ? 'i' : undefined)         // Future version should break this and forbid unsupported regex flags
+        };
+
+        const patternIsInverted = (typeof patternOptions === 'object' && patternOptions.invert);


if (typeof patternOptions === 'string') { patternOjbect.name = patternOptions; } else if (typeof patternOptions === 'object') { patternOjbect.name = patternOptions.name; patternObject.invert = !!patternOptions.invert; }

For patternObject.name = patternOptions.name; it looks like I need to have either a conditional check or a fallback value for describe when name isn't provided:

"name": "[undefined]"

Preference here?

Where are you getting this ? It's not supposed to be stringified.

I was actually just taking a second look at that. The stringified piece is just because I copied the Lab test output. But I am seeing this:

const schema = Joi.string().regex(/[a-z]/, { invert: true }); const description = schema.describe(); console.log(description.rules[0]); /* { name: 'regex', arg: { pattern: /[a-z]/, name: undefined, invert: true } } */

If having name: undefined is okay, then no problem. I just wouldn't expect any property at all in this case.

Just a if then, no ternary.

Marsup · 2016-11-14T20:20:32Z

lib/string.js

                return value;
            }

-            return this.createError((name ? 'string.regex.name' : 'string.regex.base'), { name, pattern, value }, state, options);
+            const name = typeof patternOptions === 'string' ?


This becomes useless with upper snippet.

Marsup · 2016-11-14T20:21:47Z

lib/string.js

+            const name = typeof patternOptions === 'string' ?
+                patternOptions :
+                Hoek.reach(patternOptions, 'name');
+            const baseRegex = patternIsInverted ? 'string.regex.inverted' : 'string.regex.base';


The error code can be precalculated outside of the test, we should do that.

Marsup · 2016-11-14T20:22:46Z

API.md


 ```js
 const schema = Joi.string().regex(/^[abc]+$/);
+


Missing the simpler inlined string.

Marsup · 2016-11-14T20:23:20Z

API.md

@@ -1542,14 +1542,26 @@ const schema = Joi.object({
 });
 ```

-#### `string.regex(pattern, [name])`
+#### `string.regex(pattern, [name | config])`


Marsup · 2016-11-14T21:05:31Z

Enough work for you, I have other ideas but I'll try them out locally, thanks a lot !

WesTyler · 2016-11-14T21:06:41Z

Awesome, thanks for the feedback and guidance!

Marsup · 2016-11-14T21:24:35Z

Just pushed a small fix, just so you know what I was aiming for.

WesTyler · 2016-11-14T21:25:49Z

Ah, I see. Cool 👍

lock · 2020-01-09T10:03:28Z

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

WesTyler added 2 commits November 14, 2016 10:41

Implement regex "invalidate" configuration

fe35fcc

Supports describe API for string.regex invalidate config

83b1aa7

Update API.md with string.regex invalidate config

c5f95af

Marsup requested changes Nov 14, 2016

View reviewed changes

Marsup added the feature New functionality or improvement label Nov 14, 2016

Marsup self-assigned this Nov 14, 2016

Refactor string regex implementation per review on hapijs#1035

ea40559

Marsup requested changes Nov 14, 2016

View reviewed changes

WesTyler added 2 commits November 14, 2016 14:51

Code cleanup hapijs#1035

0f578f4

Fix API.md TOC for string.regex

80f06eb

Marsup added this to the 10.0.0 milestone Nov 14, 2016

Marsup merged commit e60376b into hapijs:master Nov 14, 2016

Marsup added a commit that referenced this pull request Nov 14, 2016

Cleanup for #1035

678e7a3

Marsup mentioned this pull request Nov 16, 2016

Joi.string().invalid (not, disallow) doesn't support regex? #1020

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement String.regex "invalidate" configuration #1035

Implement String.regex "invalidate" configuration #1035

WesTyler commented Nov 14, 2016

WesTyler commented Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

WesTyler Nov 14, 2016 •

edited

Loading

Marsup Nov 14, 2016

WesTyler Nov 14, 2016 •

edited

Loading

Marsup Nov 14, 2016

WesTyler Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

WesTyler Nov 14, 2016

Marsup left a comment

Marsup Nov 14, 2016

WesTyler Nov 14, 2016 •

edited

Loading

Marsup Nov 14, 2016

WesTyler Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

Marsup Nov 14, 2016

WesTyler Nov 14, 2016

Marsup commented Nov 14, 2016

WesTyler commented Nov 14, 2016

Marsup commented Nov 14, 2016

WesTyler commented Nov 14, 2016

lock bot commented Jan 9, 2020


		const patternMatch = pattern.test(value);

		if ((patternMatch && !patternIsInvalidated) \|\| (!patternMatch && patternIsInvalidated)) {


		if (pattern.test(value)) {
		return this._test(testName, pattern, function (value, state, options) {

Implement String.regex "invalidate" configuration #1035

Implement String.regex "invalidate" configuration #1035

Conversation

WesTyler commented Nov 14, 2016

WesTyler commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marsup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marsup commented Nov 14, 2016

WesTyler commented Nov 14, 2016

Marsup commented Nov 14, 2016

WesTyler commented Nov 14, 2016

lock bot commented Jan 9, 2020

WesTyler Nov 14, 2016 •

edited

Loading

WesTyler Nov 14, 2016 •

edited

Loading

WesTyler Nov 14, 2016 •

edited

Loading