Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ensure_inclusion_of(...).in_array(...) can't cope with 0 in array #179

Closed
dolzenko opened this Issue Oct 25, 2012 · 45 comments

Comments

Projects
None yet

Assuming you have something like

validates :state_id, inclusion: { in: [0,1,2] }

spec setup as

it { should ensure_inclusion_of(:state_id).in_array([0,1,2]) }

will fail. This won't happen if array doesn't contain 0 element

Possibly related. Tested with shoulda-matchers 1.4.1

Code

validates :can_view_private_data, :inclusion => {:in => [true, false]}

Test

it { should ensure_inclusion_of(:can_view_private_data).in_array([true,false]) }

Error

Failure/Error: it { should ensure_inclusion_of(:can_view_private_data).in_array([true,false]) }
   [true, false] doesn't match array in validation

+1

Owner

drapergeek commented Nov 2, 2012

Maybe I'm missing a set up step here as I can't replicate either of these failures. I created a repo to test this, please ensure that I'm not missing something. Post of these tests pass.

https://github.com/drapergeek/shoulda-spec-test/blob/master/app/models/post.rb

https://github.com/drapergeek/shoulda-spec-test/blob/master/spec/models/post_spec.rb

Also please ensure you're using shoulda-matchers 1.4.1.

For the [true, false] issue I have found the cause.

if the field type is string then [true, false] works. However if the field type is specified as boolean in the migration file then the error occurs. However the other, originally reported, issue is not affected by this so I will create a new ticket for this specifically.

jayeff commented Nov 6, 2012

I can duplicate the behavior from @rurounijones. ensure_inclusion_of(:attr).in_array([true,false]) does not work with booleans.

Update: Please ignore the bit below. Don't use a presence validator for boolean values because when attr == false the model will no longer be valid.

But on the second look something like validates :can_view_private_data, :inclusion => {:in => [true, false]} makes no sense anyway. "Every" value ('hi', 0, 1) gets automatically coerced into a truthy/falsey value and the model is valid. The exception is nil. If the field must be true or false use a presence validator instead.

I believe the reason it was written that way is that the developers did not want to rely on coercion and wanted to explicit about what they allowed. If they persist about it I will tell them to test it using basic rspec stuff, without using the shoulda-matchers matchers.

jayeff commented Nov 8, 2012

@rurounijones Please ignore my last answer. Presence validation for boolean does not work as a false value will make the model invalid. I tested my change with update_attribute (without the s) thinking this validates before saving which it doesn't - my bad! (It's time that update_attribute is kicked out)

Will probably take a closer look on this issue this weekend.

riyad commented Nov 13, 2012

I just stumbled over the same issue as described in @rurounijones' first comment. Updating from 1.3.0 to 1.4.1 consistently produces this error. Going back to 1.3.0 everything works flawlessly again. Interestingly, if I update to 1.4.0 it produces a different error. o.O

Failure/Error: it { should ensure_inclusion_of(:closed).in_array([true, false]) }
NoMethodError:
  undefined method `next' for true:TrueClass

FYI: this is on boolean columns

@riyad riyad referenced this issue in gitlabhq/gitlabhq Nov 13, 2012

Merged

Update Gems #1981

same problem, downgraded to 1.3.0, following thread
anyway, thanks for this amazing gem =)

jayeff commented Nov 15, 2012

@drapergeek: I uploaded a rails repository which showcases both problems. Check out:
https://github.com/jayeff/shoulda-inclusion/blob/master/app/models/user.rb and
https://github.com/jayeff/shoulda-inclusion/blob/master/spec/models/user_spec.rb

Furthermore I started to dig into this issue. Both this cases trip over the disallows_value_outside_of_array?. This methods checks the array of possible values against an arbitrary string. In case of attr being a boolean or integer this string is automatically coerced into false or 0 respectively (which are allowed values for in: [true, false] or in: [0, 5]).

Here my thinking for a possible fix so far...

Integer: It's simple to introduce an ARBITRARY_OUTSIDE_INTEGER value but it's difficult to determine the type of the attribute. (subject.class.columns.find { |c| c.name == @attribute.to_s }).type feels a bit too hacky for detecting the correct type (I suspect there are easier ways to get the type?!).

Boolean: There is no real outside value for a boolean. The Rails-API suggests to use inclusion validator for booleans (http://api.rubyonrails.org/classes/ActiveModel/Validations/HelperMethods.html#method-i-validates_presence_of) so ensure_inclusion_of should probably be ok with boolean values. As validates_inclusion_of :field_name, :in => [true, false] is the suggested workaround for a presence validator for booleans maybe it would be ok to use nil as the "arbitrary" outside boolean value?

sewid commented Feb 20, 2013

same problem here - if if's a boolean field, I get the error:
[true, false] doesn't match array in validation

oli-g commented Feb 27, 2013

I have the same issue: my spec fails if the "inclusion" array contains a 0. Any plan to solve this?
Thank you.

larimer commented Mar 1, 2013

Ditto... Spec fails if the inclusion array includes 0.

in 1.4.2 the issue with booleans is still present. I dug into the code yesteday and found that the ultimate reason is that in Rails (3.2 at least) when you assign a value to a boolean field, i.e. a string, the string is automatically casted to a boolean value most of the times to false.

This makes disallow_value_outside_of_array? return false at this line:

if allows_all_values_in_array? && allows_blank_value? && allows_nil_value? && disallows_value_outside_of_array?

I'm not sure how to fix this, something should be done at least to warn that boolean cannot be checked for inclusion.

jayeff commented Mar 14, 2013

@freegenie don't get your hopes up. see my comment above from about 4 months ago where I explain exactly this - so far nothing happened.

mxie added a commit that referenced this issue Mar 26, 2013

Fixes #179: Allow attributes to accept non-string values
* Specifically for `ensure_inclusion_of` matcher to allow 0 for `in_array`

mxie added a commit that referenced this issue Mar 27, 2013

Fixes #179: Check that attributes are set properly when testing for m…
…atches

* AllowValueMatcher's `matches?` should check that the attribute was actually set to the value passed to it

@mxie mxie closed this in 73723cd Mar 28, 2013

Member

mxie commented Apr 1, 2013

Reopening since #273 was causing some other issues and is now reverted.

@mxie mxie reopened this Apr 1, 2013

panupan added a commit to panupan/shoulda-matchers that referenced this issue Apr 10, 2013

Fixes #179: Check that attributes are set properly when testing for m…
…atches

* AllowValueMatcher's `matches?` should check that the attribute was actually set to the value passed to it

marxarelli added a commit to nanowrimo/shoulda-matchers that referenced this issue Apr 23, 2013

Fixes #179: Check that attributes are set properly when testing for m…
…atches

* AllowValueMatcher's `matches?` should check that the attribute was actually set to the value passed to it

phaza pushed a commit to phaza/shoulda-matchers that referenced this issue May 3, 2013

Fixes #179: Check that attributes are set properly when testing for m…
…atches

* AllowValueMatcher's `matches?` should check that the attribute was actually set to the value passed to it

jfedgar commented May 18, 2013

I confirm the exact same thing when checking for [true, false] values when the field type is set to boolean in the database. 1.3.0 does not work for me on this either.

ensure_inclusion_of in_array([true, false]) failing for me on boolean fields with shoulda-matchers-2.1.0.

zduci commented Jun 8, 2013

This still fails in shoulda-matchers-2.1.0: ensure_inclusion_of in_array([true, false])

This still fails for me :/

marnen commented Jul 12, 2013

...and for me.

and me =(

and me :(

Contributor

pjg commented Jul 17, 2013

and me :/

davich commented Jul 18, 2013

And me

Still failing for me with 2.2.0

rctneil commented Aug 14, 2013

same here. Any chance this can get fixed?

Owner

drapergeek commented Aug 15, 2013

#337 should fix this.

@drapergeek drapergeek closed this Aug 15, 2013

Contributor

pjg commented Aug 15, 2013

@drapergeek I've bundled the latest version (87b80e2) and it still fails. But I'm testing it on Rails 4.0 so that might have something to do with it (i.e. there are other failures in different places because of Rails 4 incompatibilities).

Owner

drapergeek commented Aug 15, 2013

@pjg That is odd. I don't know about Rails 4 as I didn't test there. Can you post a gist to the scenario you used for testing?

Contributor

pjg commented Aug 15, 2013

In the model:

validates :admin, inclusion: { in: [false, true] }

in the spec:

it { should ensure_inclusion_of(:admin).in_array([false, true]) }

Fails with:

 Failure/Error: it { should ensure_inclusion_of(:admin).in_array([false, true]) }
   [false, true] doesn't match array in validation
 # ./spec/models/user_spec.rb:58:in `block (2 levels) in <top (required)>'
Owner

drapergeek commented Aug 15, 2013

Sorry, I should have been more specific when closing this issue. The boolean issue has not been fixed, only the 0 in array problem.

For the boolean issue I would recommend using allow_value. I'm not sure it makes sense to use the ensure_inclusion matcher there.

Contributor

pjg commented Aug 15, 2013

ensure_inclusion is one line/one spec. allow_value is at least a couple of lines/specs for different types...

Owner

drapergeek commented Aug 15, 2013

Why not check for not allowing a null value instead? If your column is a boolean those are the only three allowed values.

Contributor

pjg commented Aug 15, 2013

Sure, that's an option. But it's still three lines instead of one:

should allow_value(true)
should allow_value(false)
should_not allow_value(nil)
Owner

drapergeek commented Aug 15, 2013

Feel free to open another issue and reference this one.

I'm not sure I agree that this is the best idea but I can also see your point, would love to gather other opinions.

carpodaster referenced this issue in Absolventa/ereignishorizont Sep 30, 2013

Same issue here with [true, false], and same opinion that this should be accomplished with one line of code...

Ditto on the [true, false] issue

Contributor

adarsh commented Jan 26, 2014

@drapergeek I'm seeing the zero-array issue in 2.5.0:

ShippingCharges.all # => [0, 8, 14]

#spec
it { should ensure_inclusion_of(:shipping_fee).in_array(ShippingCharges.all) }

#model
validates :shipping_fee, inclusion: { in: ShippingCharges.all }

# => [0, 8, 14] doesn't match array in validation
Owner

drapergeek commented Jan 26, 2014

@adarsh I can't seem to recreate this, I tried a spec in the suite with no luck. Also created a new app using suspenders and made what I imagined was a similar setup.

Pushed a gist: https://gist.github.com/anonymous/8628107

Can't seem to get a failure. What difference am I missing to see a failure? (this was on 2.5.0)

ivanvc pushed a commit to ivanvc/shoulda-matchers that referenced this issue Apr 18, 2014

estiens commented Apr 25, 2014

Still getting this failure :(

Collaborator

mcmire commented Apr 26, 2014

@estiens Can you provide a failing test please?

estiens commented Apr 26, 2014

rails rescue app with silly booleans

#schema 
t.boolean  "recommended"

#model 
validates_inclusion_of :recommended, :in => [true, false, 0, 1, '0', '1'], :allow_blank => true

#spec
it { should ensure_inclusion_of(:recommended).in_array([true, false, 0, 1, '0', '1']) }

#failure
#doesn't match array in validation
Collaborator

mcmire commented Apr 26, 2014

Please file a separate issue for this. The original problem this issue is describing has to do with ensure_inclusion_of not accepting 0 in an array, which we fixed already.

With regards to the [true, false] issue:

As of shoulda-matchers v2.7.0+, you can change the code:

it { is_expected.to ensure_inclusion_of(:foo).in_array [true, false] }

To:

it { is_expected.to validate_inclusion_of(:foo).in_array [true, false] }

And this test now passes. However (!!!) it will also print the following warning message:


Warning from shoulda-matchers:

You are using validate_inclusion_of to assert that a boolean column
allows boolean values and disallows non-boolean ones. Be aware that it
is not possible to fully test this, as boolean columns will
automatically convert non-boolean values to boolean ones. Hence, you
should consider removing this test.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment