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.

Sign up for GitHub

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

Jump to bottom

Allow validate options to be passed through assert/attempt #1722 #1723

Merged
merged 1 commit into from
May 29, 2019
Merged

Allow validate options to be passed through assert/attempt #1722 #1723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
- [`validate(value, schema, [options], [callback])`](#validatevalue-schema-options-callback)
- [`compile(schema)`](#compileschema)
- [`describe(schema)`](#describeschema)
- [`assert(value, schema, [message])`](#assertvalue-schema-message)
- [`attempt(value, schema, [message])`](#attemptvalue-schema-message)
- [`assert(value, schema, [message], [options])`](#assertvalue-schema-message-options)
Copy link

Choose a reason for hiding this comment

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

I think this is not clear that options can come in place of message. There is a notation for this case but I can't recall it right now.

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 just following the formatting for validate which has a similar flexible contract with options and callback both being optional.

Copy link

Choose a reason for hiding this comment

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

True. Then I guess that consistency is a better option here, let's leave it as is 😃

- [`attempt(value, schema, [message], [options])`](#attemptvalue-schema-message-options)
- [`ref(key, [options])`](#refkey-options)
- [`isRef(ref)`](#isrefref)
- [`reach(schema, path)`](#reachschema-path)
Expand Down Expand Up @@ -371,23 +371,25 @@ Results in:
valids: [ 'foo', 'bar' ] }
```

### `assert(value, schema, [message])`
### `assert(value, schema, [message], [options])`

Validates a value against a schema and [throws](#errors) if validation fails where:
- `value` - the value to validate.
- `schema` - the validation schema. Can be a **joi** type object or a plain object where every key is assigned a **joi** type object using [`Joi.compile`](#compileschema) (be careful of the cost of compiling repeatedly the same schemas).
- `message` - optional message string prefix added in front of the error message. may also be an Error object.
- `options` - optional options object, passed in to [`Joi.validate`](##validatevalue-schema-options-callback)

```js
Joi.assert('x', Joi.number());
```

### `attempt(value, schema, [message])`
### `attempt(value, schema, [message], [options])`

Validates a value against a schema, returns valid object, and [throws](#errors) if validation fails where:
- `value` - the value to validate.
- `schema` - the validation schema. Can be a **joi** type object or a plain object where every key is assigned a **joi** type object using [`Joi.compile`](#compileschema) (be careful of the cost of compiling repeatedly the same schemas).
- `message` - optional message string prefix added in front of the error message. may also be an Error object.
- `options` - optional options object, passed in to [`Joi.validate`](##validatevalue-schema-options-callback)

```js
Joi.attempt('x', Joi.number()); // throws error
Expand Down
15 changes: 11 additions & 4 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,21 @@ internals.root = function () {
}
};

root.assert = function (value, schema, message) {
root.assert = function (...args) {

this.attempt(value, schema, message);
this.attempt(...args);
};

root.attempt = function (value, schema, message) {
root.attempt = function (value, schema, ...args/* [message], [options]*/) {
Copy link

Choose a reason for hiding this comment

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

One thought: how about making it an [message | options] like in string regex function? This would allow to use validate options AND the custom message. The third argument would be then either 'custom message' or { message: 'custom message', ...otherValidateOptions }.

What do you think about that?

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 implemented it in a way that message, options, both, or neither are acceptable. I even added a couple of tests around using both message and options in a few different scenarios.

As far as following the regex() param usage pattern, that's definitely an option. I was trying to keep the impact on current usage as minimal as possible, because attempt is actually used in quite a few places by internals behind the scenes.

Copy link

Choose a reason for hiding this comment

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

Yeah, I think we can keep this implementation. I just wanted to share my thought :)


const result = this.validate(value, schema);
const first = args[0];
const message = (
Copy link

Choose a reason for hiding this comment

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

Wouldn't it be easier to just check if args[0] is an object?

Like:

const firstArgAsOptions = typeof args[0] === 'object';
const message = firstArgAsOptions ? null : args[0];
const options = firstArgAsOptions ? args[0] : args[1];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the possible values of message is a new Error instance, as new Error().

Unfortunately, that has the type 'object'

> typeof (new Error())
'object'

Copy link

Choose a reason for hiding this comment

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

Oh, I missed that case, you're right!

first instanceof Error ||
typeof first === 'string'
) ? first : null;

const options = message ? args[1] : args[0];
const result = this.validate(value, schema, options);
const error = result.error;
if (error) {
if (!message) {
Expand Down
27 changes: 25 additions & 2 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2637,6 +2637,17 @@ describe('Joi', () => {
}).to.not.throw();
expect(result).to.not.exist();
});

it('respects abortEarly option', () => {

try {
Joi.assert({}, Joi.object().keys({ a: Joi.required(), b: Joi.required() }), { abortEarly: false });
throw new Error('should not reach that');
}
catch (err) {
expect(err.details.length).to.equal(2);
}
});
});

describe('attempt()', () => {
Expand Down Expand Up @@ -2675,11 +2686,23 @@ describe('Joi', () => {
}).to.throw('the reason is "value" must be a number');
});

it('throws on invalid value with message as error', () => {
it('throws on invalid value with message and abortEarly: false', () => {

try {
Joi.attempt({}, Joi.object().keys({ a: Joi.required(), b: Joi.required() }), 'the reasons are', { abortEarly: false });
throw new Error('should not reach that');
}
catch (err) {
expect(err.message.match(/the reasons are/)).to.not.equal(null);
expect(err.details.length).to.equal(2);
}
});

it('throws on invalid value with message as error even with abortEarly: false', () => {

expect(() => {

Joi.attempt('x', Joi.number(), new Error('invalid value'));
Joi.attempt({}, Joi.object().keys({ a: Joi.required(), b: Joi.required() }), new Error('invalid value'), { abortEarly: false });
}).to.throw('invalid value');
});

Expand Down