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

strictBool option for schema type boolean (clean) #5344

Merged
merged 3 commits into from
Nov 24, 2017
Merged

strictBool option for schema type boolean (clean) #5344

merged 3 commits into from
Nov 24, 2017

Conversation

c0d0g3n
Copy link
Contributor

@c0d0g3n c0d0g3n commented Jun 10, 2017

Summary
See #5288, #5211 and #4245. Adds an option for schema type Boolean, called strictBool (disabled by default to ensure compatibility). When strictBool:true is passed, path input is restricted to true, false, 'true', 'false', 1, 0, '1' and '0'. Other values throw a castError. null is also accepted (as before) so the field can be marked as unspecified.

Test plan
0fb09d9 (second one) includes a test which verifies that the values we expect to be accepted are indeed accepted. (Need to test that other values are rejected?)

npm test -- -g 'strictBool': test passed

TODO

  • Document new option.

vkarpov15
vkarpov15 requested changes Jun 16, 2017
View reviewed changes
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Couple small details but looks great overall 👍

test/schema.boolean.test.js Outdated
return m1.validate();
});

return global.Promise.all(validatePromises);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail on node < 4 because no promises :p . Just use a for loop to chain all the promises together.

test/schema.boolean.test.js Outdated
@@ -29,5 +29,21 @@ describe('schematype', function() {
assert.strictEqual(true, m3.b);
done();
});
it('strictBool option (gh-5211)', function() {
console.log('chekc');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this please

@ekulabuhov
Copy link
Contributor

ekulabuhov commented Nov 24, 2017 •
edited
Loading

@vkarpov15 @c0d0g3n is there anything else preventing this from being merged except for failing style tests and missing documentation? I have a need for this feature and can help to complete it if you're ok with this?

@c0d0g3n
Copy link
Contributor Author

c0d0g3n commented Nov 24, 2017

The issues mentioned by @vkarpov15 should have been adressed in last commit. (But now I think of it, the Array.forEach I used may not be compatible with node <4 as well.) You're more than welcome to add this feature to the docs though.🙂 (Last time I tried, I couldn't get the build system to work.)

@vkarpov15 vkarpov15 added this to the 4.13.5 milestone Nov 24, 2017
vkarpov15
vkarpov15 approved these changes Nov 24, 2017
View reviewed changes
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks 🍻

@vkarpov15 vkarpov15 merged commit 38131d4 into Automattic:master Nov 24, 2017
vkarpov15 added a commit that referenced this pull request Nov 25, 2017
@vkarpov15
BREAKING CHANGE: treat all non-whitelisted values as cast errors for …
bb41fd3 
…boolean

Fix #4245
Re: #5211, #5344
@ekulabuhov
Copy link
Contributor

ekulabuhov commented Nov 29, 2017 •
edited
Loading

Hi! I just used this feature and it works like a charm. The only problem is that I have 10 or so Booleans on a single Schema. Would it make sense to move this options to the Schema level? Or even higher? Seems like a reasonable default to enforce.

@ekulabuhov ekulabuhov mentioned this pull request Nov 29, 2017
Merged
@ancms2600
Copy link

@vkarpov15 why was this feature removed?
https://github.com/c0d0g3n/mongoose/blob/master/lib/schema/boolean.js#L62-L73
no longer in master
6227232#diff-17c0b6da40c0e37e45bc929cbd5b7056

i would like to disable the strictness. OR
i would like the string value 'on' to be in the list of truthy values, as this is the default value provided by browsers like Chrome for <input type="checkbox"/> elements that have no value= attribute set.

@vkarpov15
Copy link
Collaborator

@ancmikesmullin we removed this for 5.0 because converting any non falsy value to true was a bit too permissive. We'll open up an alternative way to support this behavior, but for now you can just do a custom setter set: v => v === 'on' ? true : v

@vkarpov15 vkarpov15 mentioned this pull request Jul 22, 2018
Closed
@DevBrent DevBrent mentioned this pull request Jul 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vkarpov15 vkarpov15 vkarpov15 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.13.5
Development

Successfully merging this pull request may close these issues.
4 participants
@c0d0g3n @ekulabuhov @ancms2600 @vkarpov15