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

Add support for truthy/falsy boolean values #998

Merged
merged 8 commits into from Oct 22, 2016
20 changes: 20 additions & 0 deletions API.md
Expand Up @@ -52,6 +52,8 @@
- [`array.length(limit)`](#arraylengthlimit)
- [`array.unique([comparator])`](#arrayuniquecomparator)
- [`boolean`](#boolean)
- [`boolean.truthy(value)`](#booleantruthyvalue)
- [`boolean.falsy(value)`](#booleanfalsyvalue)
- [`binary`](#binary)
- [`binary.encoding(encoding)`](#binaryencodingencoding)
- [`binary.min(limit)`](#binaryminlimit)
Expand Down Expand Up @@ -808,6 +810,24 @@ const boolean = Joi.boolean();
boolean.validate(true, (err, value) => { });
```

#### `boolean.truthy(value)`

Allows for additional strings to be considered valid boolean values in addition to default 'truthy' strings: ['true', 'yes', '1', 'on']. Accepts a string or an array of strings.
Copy link
Contributor

@DavidTPate DavidTPate Oct 5, 2016

Choose a reason for hiding this comment

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

Personally, I don't think this should be additional as I want to be able to be explicit about it. Instead I think this should override the valid truthy values instead of just add new ones.

The same goes for the falsey one below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I get that. It is currently (in this PR) implemented as additive, so that I didn't nuke the strings that are already type-coerced by the "vanilla" boolean schema type. Here's where the "additive" piece is in the code: https://github.com/hapijs/joi/pull/998/files#diff-3b86748616af947ebd7969eedec0062aR76

I'm totally down for overriding the default type-coerced strings. I was just trying to play nice with what was already in place. Just let me know what would be preferred and I will update accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I figured that was the case @WesTyler. It's definitely just my personal preference here not sure that one method is more correct than the other.

Not sure of the opinion that @Marsup has on this, but I definitely defer to him. Just wanted to state my preference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like I said in the issue, I don't mind it being a breaking change, so default true/false and additive calls to stay consistent with valid/invalid makes the most sense to me.


```js
const boolean = Joi.boolean().truthy('Y');
boolean.validate('Y', (err, value) => { });
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing boolean.validate('Y', (err, value) => { }); // valid or something else might help to show that this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. On it, thanks.

```

#### `boolean.falsy(value)`

Allows for additional strings to be considered valid boolean values in addition to default 'falsy' strings: ['false', 'no', '0', 'off']. Accepts a string or an array of strings.

```js
const boolean = Joi.boolean().falsy('N');
boolean.validate('N', (err, value) => { });
```

### `binary`

Generates a schema object that matches a Buffer data type (as well as the strings which will be converted to Buffers).
Expand Down
61 changes: 59 additions & 2 deletions lib/boolean.js
Expand Up @@ -3,6 +3,7 @@
// Load modules

const Any = require('./any');
const Hoek = require('hoek');


// Declare internals
Expand All @@ -15,6 +16,18 @@ internals.Boolean = class extends Any {

super();
this._type = 'boolean';
this._inner._truthySet = [
'true',
'yes',
'on',
'1'
];
this._inner._falsySet = [
'false',
'no',
'off',
'0'
];
}

_base(value, state, options) {
Expand All @@ -27,8 +40,8 @@ internals.Boolean = class extends Any {
options.convert) {

const lower = value.toLowerCase();
result.value = (lower === 'true' || lower === 'yes' || lower === 'on' || lower === '1' ? true
: (lower === 'false' || lower === 'no' || lower === 'off' || lower === '0' ? false : value));
result.value = (this._inner._truthySet.indexOf(lower) !== -1 ? true
: (this._inner._falsySet.indexOf(lower) !== -1 ? false : value));
}

if (typeof value === 'number' &&
Expand All @@ -41,6 +54,50 @@ internals.Boolean = class extends Any {
result.errors = (typeof result.value === 'boolean') ? null : this.createError('boolean.base', null, state, options);
return result;
}

truthy(truthyValue) {

let truthySet = [];

if (Array.isArray(truthyValue)) {
truthySet = truthyValue.map((value) => {

Hoek.assert(typeof value === 'string', 'truthy value must be a string');

return value.toLowerCase();
});
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just cast the value to an array if it isn't one currently? That way you don't have the code repeated. So before line 62 something like: truthyValue = Array.isArray(truthyValue) ? truthyValue : [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I'll do that along with the API.md update above. Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually inconsistent with how we do it everywhere else, see https://github.com/hapijs/joi/blob/master/lib/any.js#L235.

Hoek.assert(typeof truthyValue === 'string', 'truthy value must be a string');
truthySet.push(truthyValue.toLowerCase());
}

const obj = this.clone();
obj._inner._truthySet = truthySet.concat(obj._truthySet);
return obj;
}

falsy(falsyValue) {

let falsySet = [];

if (Array.isArray(falsyValue)) {
falsySet = falsyValue.map((value) => {

Hoek.assert(typeof value === 'string', 'falsy value must be a string');

return value.toLowerCase();
});
}
else {
Hoek.assert(typeof falsyValue === 'string', 'falsy value must be a string');
falsySet.push(falsyValue.toLowerCase());
}

const obj = this.clone();
obj._inner._falsySet = falsySet.concat(obj._falsySet);
return obj;
}
};


Expand Down
101 changes: 101 additions & 0 deletions test/boolean.js
Expand Up @@ -159,5 +159,106 @@ describe('boolean', () => {
[null, true]
], done);
});

it('should handle work with additional truthy value', (done) => {

const rule = Joi.boolean().truthy('Y');
Helper.validate(rule, [
['Y', true],
['y', true],
[true, true],
[false, true],
['N', false, null, '"value" must be a boolean']
], done);
});

it('should handle work with additional truthy array', (done) => {

const rule = Joi.boolean().truthy(['Y', 'Si']);
Helper.validate(rule, [
['Si', true],
['si', true],
['Y', true],
['y', true],
[true, true],
[false, true],
['N', false, null, '"value" must be a boolean'],
[null, false, null, '"value" must be a boolean']
], done);
});

it('should handle work with additional falsy value', (done) => {

const rule = Joi.boolean().falsy('N');
Helper.validate(rule, [
['N', true],
['n', true],
['Y', false, null, '"value" must be a boolean'],
[true, true],
[false, true]
], done);
});

it('should handle work with additional falsy array', (done) => {

const rule = Joi.boolean().falsy(['N', 'Never']);
Helper.validate(rule, [
['N', true],
['n', true],
['Never', true],
['never', true],
['Y', false, null, '"value" must be a boolean'],
[null, false, null, '"value" must be a boolean'],
[true, true],
[false, true]
], done);
});

it('should handle work with required, null allowed, and both additional truthy and falsy values', (done) => {

const rule = Joi.boolean().truthy(['Y', 'Si']).falsy(['N', 'Never']).allow(null).required();
Helper.validate(rule, [
['N', true],
['n', true],
['Never', true],
['never', true],
['Y', true],
['y', true],
['Si', true],
['si', true],
[true, true],
[false, true],
[null, true],
['M', false, null, '"value" must be a boolean']
], done);
});

it('errors on non-string truthy value', (done) => {

expect(() => {

Joi.boolean().truthy({});
}).to.throw('truthy value must be a string');

expect(() => {

Joi.boolean().truthy(['valid', {}]);
}).to.throw('truthy value must be a string');
done();
});

it('errors on non-string falsy value', (done) => {

expect(() => {

Joi.boolean().falsy({});
}).to.throw('falsy value must be a string');

expect(() => {

Joi.boolean().falsy(['valid', {}]);
}).to.throw('falsy value must be a string');
done();
});
});
});