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

Implement String.regex "invalidate" configuration #1035

Merged
merged 6 commits into from
Nov 14, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion API.md
Original file line number Diff line number Diff line change
Expand Up @@ -1542,14 +1542,26 @@ const schema = Joi.object({
});
```

#### `string.regex(pattern, [name])`
#### `string.regex(pattern, [name | config])`
Copy link
Collaborator

Choose a reason for hiding this comment

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

options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops XD


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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

- `invalidate` - optional boolean flag. Defaults to `false` behavior. If specified as `true`, the provided pattern will be disallowed instead of required.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing the simpler inlined string.

const namedSchema = Joi.string().regex(/[0-9]/, { name: 'numbers'});
namedSchema.validate('alpha'); // ValidationError: "value" with value "alpha" fails to match the numbers pattern

const invalidatedSchema = Joi.string().regex(/[a-z]/, { invalidate: true });
invalidatedSchema.validate('lowercase'); // ValidationError: "value" with value "lowercase" matches the invalidated pattern: [a-z]

const invalidatedNamedSchema = Joi.string().regex(/[a-z]/, { name: 'alpha', invalidate: true });
invalidatedNamedSchema.validate('lowercase'); // ValidationError: "value" with value "lowercase" matches the invalidated alpha pattern
```

#### `string.replace(pattern, replacement)`
Expand Down
4 changes: 3 additions & 1 deletion lib/language.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@ exports.errors = {
token: 'must only contain alpha-numeric and underscore characters',
regex: {
base: 'with value "{{!value}}" fails to match the required pattern: {{pattern}}',
name: 'with value "{{!value}}" fails to match the {{name}} pattern'
invalidated: 'with value "{{!value}}" matches the invalidated pattern: {{pattern}}',
name: 'with value "{{!value}}" fails to match the {{name}} pattern',
invalidatedName: 'with value "{{!value}}" matches the invalidated {{name}} pattern'
},
email: 'must be a valid email',
uri: 'must be a valid uri',
Expand Down
19 changes: 15 additions & 4 deletions lib/string.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,19 +91,30 @@ internals.String = class extends Any {
});
}

regex(pattern, name) {
regex(pattern, config) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer the term options, in readme too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Habit. Updating along with other changes. :)

const testName = patternIsInvalidated ? 'invalidatedRegex' : 'regex';

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@WesTyler WesTyler Nov 14, 2016

Choose a reason for hiding this comment

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

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
                        }
                    }
                ]
            }

Copy link
Collaborator

Choose a reason for hiding this comment

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

It will automatically if you do.

Copy link
Contributor Author

@WesTyler WesTyler Nov 14, 2016

Choose a reason for hiding this comment

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

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)


const patternMatch = pattern.test(value);

if ((patternMatch && !patternIsInvalidated) || (!patternMatch && patternIsInvalidated)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clearer with patternMatch ^ patternIsInvalidated.

return value;
}

return this.createError((name ? 'string.regex.name' : 'string.regex.base'), { name, pattern, value }, state, options);
const name = typeof config === 'string' ?
config :
Hoek.reach(config, 'name');
const baseRegex = patternIsInvalidated ? 'string.regex.invalidated' : 'string.regex.base';
const nameRegex = patternIsInvalidated ? 'string.regex.invalidatedName' : 'string.regex.name';

return this.createError((name ? nameRegex : baseRegex), { name, pattern, value }, state, options);
});
}

Expand Down
50 changes: 50 additions & 0 deletions test/string.js
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,37 @@ describe('string', () => {
done();
});
});

it('should include a pattern name in options object', (done) => {

const schema = Joi.string().regex(/[a-z]+/, { name: 'letters' }).regex(/[0-9]+/, { name: 'numbers' });
schema.validate('abcd', (err, value) => {

expect(err.message).to.contain('numbers pattern');
done();
});
});

it('should "invalidate" regex pattern if specified in options object', (done) => {

const schema = Joi.string().regex(/[a-z]/, { invalidate: true });
Helper.validate(schema, [
['0123456789', true],
['abcdefg', false, null, '"value" with value "abcdefg" matches the invalidated pattern: /[a-z]/']
], done);
});

it('should include invalidated pattern name if specified', (done) => {

const schema = Joi.string().regex(/[a-z]/, {
name : 'lowercase',
invalidate: true
});
Helper.validate(schema, [
['0123456789', true],
['abcdefg', false, null, '"value" with value "abcdefg" matches the invalidated lowercase pattern']
], done);
});
});

describe('ip()', () => {
Expand Down Expand Up @@ -3628,5 +3659,24 @@ describe('string', () => {
});
done();
});

it('describes invalidate regex pattern', (done) => {

const schema = Joi.string().regex(/[a-z]/, {
invalidate: true
});
const description = schema.describe();
expect(description).to.equal({
type: 'string',
invalids: [''],
rules: [
{
name: 'invalidatedRegex',
arg: /[a-z]/
}
]
});
done();
});
});
});