Possible issue with ActiveRecord::Dirty

rubyonrails-core
1

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.

2

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

3

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: