Possible issue with ActiveRecord::Dirty

[Ben Symonds]

I've been playing around with Dirty (change tracking) recently, and have
a question about the field_changed? method:

def field_changed?(attr, old, value)
if column = column_for_attribute(attr)
if column.type == :integer && column.null && (old.nil? || old == 0)
# For nullable integer columns, NULL gets stored in database for
blank (i.e. '') values.
# Hence we don't record it as a change if the value changes from
nil to ''.
# If an old value of 0 is set to '' we want this to get changed
to nil as otherwise it'll
# be typecast back to 0 (''.to_i => 0)
value = nil if value.blank?
else
value = column.type_cast(value)
end
end

old != value
end

I understand the reasoning behind the if...else statement, but what
about the situation where you're changing a nullable integer column from
0 to '0'? In this case it will go into the if block, leave 'value'
unchanged, and end up returning true - this seems incorrect to me!

If the value.blank? test was moved to the end of the long if statement,
the else block would be processed instead, so the value typecasted to 0,
and return false, which is what I would expect (and (obviously) is what
happens with non-nullable integer columns).

Am I missing something, or should I go ahead and write the patch?

Thanks all.

[Frederick Cheung]

On 3 Dec 2008, at 12:08, Ben Symonds wrote:

>
> I've been playing around with Dirty (change tracking) recently, and
> have
> a question about the field_changed? method:

I've been looking at this with ben and it does seem a bit messed up

With a table like so:

create_table :products do |t|
t.integer :quantity, :null => false
t.integer :product_id, :null => true
t.timestamps
end

p = Product.create :quantity => 0, :product_id => 0
=> #<Product id: 2, quantity: 0, product_id: 0, created_at:
"2008-12-04 10:34:04", updated_at: "2008-12-04 10:34:04">
>> p.changed? #=> false
>> p.quantity = '0'
>> p.changed? #=> false
>> p.product_id = '0'
>> p.changed? #=> true

It's not going to happen that often since this only happens when the
existing value is 0, and only affects integer columns that can be
null. Still, doesn't seem right to me. WHat ben has suggested
certainly seems to do the right thing

Fred

[Ben Symonds]

Well since noone objected, I went ahead and created a patch. See:

http://rails.lighthouseapp.com/projects/8994/tickets/1530

Any comments appreciated!

[Ben Symonds]

Well since noone objected, I went ahead and created a patch. See:

http://rails.lighthouseapp.com/projects/8994/tickets/1530

Any comments appreciated!


Frederick Cheung wrote: