Fix undefined method `to_i' introduced since 3.2.8 #8718

Merged
merged 1 commit into from Jan 3, 2013

Projects

None yet

8 participants

@jstirk
jstirk commented Jan 3, 2013

This commit fixes a bug introduced in 96a13fc which breaks behaviour of
integer fields in 3.2.8.

In 3.2.8, setting the value of an integer field to a non-integer (eg.
Array, Hash, etc.) would use the first value in the array :

#3.2.8
p = Post.new
p.category_id = [ 1, 2 ]
p.category_id # => 1
p.category_id = { 1 => 2 }
p.category_id # => 1

In 3.2.9 and above, this will raise a NoMethodError :

#3.2.9
p = Post.new
p.category_id = [ 1, 2 ]

NoMethodError: undefined method `to_i' for [1, 2]:Array

Whilst at first blush this appear to be sensible, it combines in bad
ways with scoping.

For example, it is common to use scopes to control access to data :

@collection = Posts.where(:category_id => [ 1,2 ])
@new_post = @collection.new

In 3.2.8, this would work as expected, creating a new Post object
(albeit with @new_post.category_id = 1). However, in 3.2.9 this will
cause the NoMethodError to be raised as above.

It is difficult to avoid triggering this error without descoping before
calling .new, breaking any apps running on 3.2.8 that rely on this
behaviour.

This patch deviates from 3.2.8 in that it does not retain the somewhat
spurious behaviour of setting the attribute to the first element or key.
Instead, it explicitly sets these invalid values to nil :

p = Post.new
p.category_id = [ 1, 2 ]
p.category_id # => nil

This also fixes the situation where a scope using an array will
"pollute" any newly instantiated records.

@new_post = @collection.new
@new_post.category_id # => nil

Finally, 3.2.8 exhibited a behaviour where setting an object to an
integer field caused it to be coerced to "1". This has not been
retained, as it is spurious and surprising in the same way that setting
Arrays and Hashes was :

c = Category.find(6)
p = Post.new

#3.2.8
p.category_id = c
p.category_id # => 1

# This patch
p.category_id = c
p.category_id # => nil
@jstirk
jstirk commented Jan 3, 2013

I didn't mention in the PR, but in my case this bug is preventing us from upgrading 3.2.8 apps to 3.2.10 for today's security fixes. I'm sure others are going to run into similar issues with many strange failures in their test suites from the changes introduced in 3.2.9.

@carlosantoniodasilva

Just as a side note, I'd rather have your last example working properly, ie setting the attribute to an AR object should just work.

/cc @rafaelfranca

@jstirk
jstirk commented Jan 3, 2013

I agree that AR case should just work (I had a failing spec in my app from exactly that case!), though in terms of intention it's a bit weird to set a field like that.

I think the easiest implementation here would be for the fallback case use .to_i if value responds to it, otherwise .id. Best case it works, worst case they get undefined method `id' (which at least makes sense) or the id/object_id deprecation warning depending on version of ruby.

Happy to make the change if you think it's worthwhile.

@cfabianski
Contributor

This PR fixes a bug where we are trying to build an object through an association as well

scope = Post.first.comments.where(:id => [1, 2, 3, 4]).scoped
scope.new.post
@imderek
imderek commented Jan 3, 2013

What @cfabianski said.

@jstin
jstin commented Jan 3, 2013

Just ran into this issue as well. I'd suggest a more robust approach however, since something like MyModel.new(:integer_field => SomeOtherModel) will still crash. So will anything that is not a Hash or Array.

assert_nil column.type_cast((1..2)) # test Range will fail

Probably need to rescue out or check if the value responds to to_i.

EDIT: Ah I see @jstirk already addressed this concern above.

@rafaelfranca
Member

First, a little more history for this pull request.

The previous behavior (at 3.2.8) was call to_i in every single object that value is, if it raise an exception, any exception, check if the object is truthy, if it is so return 1, if not return 0. (See thiagopradi@652107e)

This has nothing to do with the first element of a array or everything else. It just return 1 or 0.

That said I think the best approach is check if the value responds to to_i. The problem with this approach is if we return nil if the object doesn't respond to we will change the attribute value to nil when it should not change the value at all.

@jstirk
jstirk commented Jan 3, 2013

Updated to handle the AR object situation that @carlosantoniodasilva identified - now uses to_i then id, finally setting nil if nothing else can be done. This also cleans up @jstin's concern about Hash and Array being hardcoded.

@rafaelfranca my thoughts are that setting nil on an invalid argument is clearer and safer than leaving the value in its original state. Firstly, it's more likely to be caught by a validation or :null => false constraint. Setting nil also makes it clear that the assignment was at least attempted by AR, rather than leaving you wondering if something intercepted the call before it got there.

@rafaelfranca
Member

We don't need to check id. It should behave like any other object that doesn't respond to to_i.

No it is not clearer or safer, it is error prone. It should not change the original value at all. You are not assigning a valid value, so it should not change the object. I would expect to raise an exception instead of silently changing my value to nil. If I don't have validation (what is expected in an optional value) I will loose information without being warned of.

@rafaelfranca
Member

Between I agree the current implementation is wrong, but I would fix the scoped builder, instead of the value type casting.

@rafaelfranca
Member

Looking at the code I saw that decimal type casting already change the value if the object is not valid. So I guess it is fine. I would only remove the id check

@jstirk
jstirk commented Jan 3, 2013

@rafaelfranca OK, thanks for your feedback. I've updated the PR to remove the id check as you recommended, rebased and squashed.

I was thinking about your comment about fixing the scope builder. From what I can see it looks possible to do things this way, but I think we would need to have a ActiveRecord::* exception raised rather than NoMethodException on calling to_i. We could then have populate_with_current_scope_attributes silently ignore just that exception when it tries to set an attribute.

The big benefit of that approach is that post.category_id = Category.find(1) would fail with an exception, but Post.where(:category_id => [ 1, 2 ]).new would work correctly.

What do you think? I'm happy to prepare a separate PR for that if you think it's worthwhile, as it's a bit wider issue than this and probably needs a fair bit of discussion on how it works across all the types.

@rafaelfranca
Member

After thinking a lot about this I think your current implementation is the right.

p.category_id = [ 1, 2 ]

NoMethodError: undefined method `to_i' for [1, 2]:Array

I would not expect an exception to be raised at this point, a assignment should not be a dangerous operation like a save! or create!.

@jstirk
jstirk commented Jan 3, 2013

OK, understood. Let me know if you need me to do anything else to get this PR approved and merged.

@rafaelfranca
Member

Seems good. We need two things, first apply the same fix on master.

After we need a CHANGELOG entry here so I can merge it

Jason Stirk Fix undefined method `to_i' introduced since 3.2.8
This commit fixes a bug introduced in 96a13fc which breaks behaviour of
integer fields in 3.2.8.

In 3.2.8, setting the value of an integer field to a non-integer (eg.
Array, Hash, etc.) would default to 1 (true) :

    # 3.2.8
    p = Post.new
    p.category_id = [ 1, 2 ]
    p.category_id # => 1
    p.category_id = { 3 => 4 }
    p.category_id # => 1

In 3.2.9 and above, this will raise a NoMethodError :

    # 3.2.9
    p = Post.new
    p.category_id = [ 1, 2 ]

    NoMethodError: undefined method `to_i' for [1, 2]:Array

Whilst at first blush this appear to be sensible, it combines in bad
ways with scoping.

For example, it is common to use scopes to control access to data :

    @collection = Posts.where(:category_id => [ 1, 2 ])
    @new_post = @collection.new

In 3.2.8, this would work as expected, creating a new Post object
(albeit with @new_post.category_id = 1). However, in 3.2.9 this will
cause the NoMethodError to be raised as above.

It is difficult to avoid triggering this error without descoping before
calling .new, breaking any apps running on 3.2.8 that rely on this
behaviour.

This patch deviates from 3.2.8 in that it does not retain the somewhat
spurious behaviour of setting the attribute to 1. Instead, it explicitly
sets these invalid values to nil :

    p = Post.new
    p.category_id = [ 1, 2 ]
    p.category_id # => nil

This also fixes the situation where a scope using an array will
"pollute" any newly instantiated records.

    @new_post = @collection.new
    @new_post.category_id # => nil

Finally, 3.2.8 exhibited a behaviour where setting an object to an
integer field caused it to be coerced to "1". This has not been
retained, as it is spurious and surprising in the same way that setting
Arrays and Heshes was :

    c = Category.find(6)
    p = Post.new

    # 3.2.8
    p.category_id = c
    p.category_id # => 1

    # This patch
    p.category_id = c
    p.category_id # => nil

This commit includes explicit test cases that expose the original issue
with calling new on a scope that uses an Array. As this is a common
situation, an explicit test case is the best way to prevent regressions
in the future.

It also updates and separates existing tests to be explicit about the
situation that is being tested (eg. AR objects vs. other objects vs.
non-integers)
e842dbb
@rafaelfranca rafaelfranca merged commit 9a44cd1 into rails:3-2-stable Jan 3, 2013
@rafaelfranca
Member

Thank you so much.

@jstirk jstirk deleted the jstirk:column_writer_to_i_errors branch Jan 9, 2013
@johnkelly

Will this be released in 3.2.12? I can't upgrade to the latest security releases until this is fixed as I have several tests failing with this error.

@carlosantoniodasilva

@johnkelly probably yes, but there's not release date yet. Meanwhile, you should focus on the security releases, either by upgrading and monkey patching this issue, or by applying the available workarounds.

@rafaelfranca
Member

Or you can point to 3-2-stable branch

@descentintomael

Wasn't it a good thing that it raised a no method error? I would rather my code fail in a noisy fashion than have it quietly replace the value. Nil is not always a safe replacement, just the same as the previous value of 1 wasn't a safe value.

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