assigning '0.0' to a nullable numeric column does not make it dirty #9042

Merged
merged 1 commit into from Mar 5, 2013

5 participants

@senny
Ruby on Rails member

This is a fix for the issue described in #9034

It treats '0.0' the same way as '0'. This makes sure that nullable numeric column don't get changed when you assign '0.0'.

If it's ok, I'll backport that change, since the behavior was reported from 3.2.x

@senny
Ruby on Rails member

@rafaelfranca could you review this one?

@dmathieu

The '0.0' seems to be only one possibility. If we specify an other value, it shouldn't be marked as changed either.

1.9.3p327 :002 > '1.1'.to_f
  => 1.1
@senny
Ruby on Rails member

@dmathieu I'm not sure I understand what you mean. Could you provide a complete example?

0 is a special case for nullable numeric columns because nil values will be stored aas NULL and not as 0.

@acapilleri

Sorry for the comments outside from this PR

@andrenpaes andrenpaes and 1 other commented on an outdated diff Jan 23, 2013
...verecord/lib/active_record/attribute_methods/dirty.rb
@@ -107,7 +107,11 @@ def changes_from_nil_to_empty_string?(column, old, value)
def changes_from_zero_to_string?(old, value)
# For columns with old 0 and value non-empty string
- old == 0 && value.is_a?(String) && value.present? && value != '0'
+ old == 0 && value.is_a?(String) && value.present? && non_zero?(value)
+ end
+
+ def non_zero?(value)
+ value != '0' && value != '0.0'

Wouldn't this break if value is equal to '0.00'?

Maybe this could work better:

numeric_value = Float(value) rescue nil
numeric_value && numeric_value != 0

I just submitted a dup patch, sorry (#9054). Mine doesn't break for '0.00' though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabianoalmeida fabianoalmeida commented on the diff Jan 23, 2013
activerecord/CHANGELOG.md
@@ -1,5 +1,15 @@
## Rails 4.0.0 (unreleased) ##
+* Assigning "0.0" to a nullable numeric column does not make it dirty.
+ Fix #9034
+
+ Example:
+
+ product = Product.create price: 0.0
+ product.price = '0.0'
+ product.changed? # => false (this used to return true)
+ product.changes # => {} (this used to return { price: [0.0, 0.0] })
+

@senny you missed your name here, no?

@senny
Ruby on Rails member
senny added a note Jan 24, 2013

Will fix that. thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Ruby on Rails member

Thanks for all the feedback. I updated the PR with the following changes:

  • test is now against a float column
  • code works with 0.0 and 0.00
  • rebased against master

@carlosantoniodasilva could you take a look?

@acapilleri

with the following test fails:

def test_integer_zero_to_string_zero_dot_zero_not_marked_as_changed_00
      pirate = Pirate.new
      pirate.parrot_id = 0
      pirate.catchphrase = 'arrr'
      assert pirate.save!

      assert !pirate.changed?

      pirate.parrot_id = '0.0001'
      assert pirate.changed?
  end
@senny
Ruby on Rails member

@acapilleri I think your test-case is not accurate.

When you assign 0.0001 to a integer column it will be converted to 0:

1.9.3-p194 :001 > '0.0001'.to_i
 => 0 

So the parrot_id did not actually change.

@acapilleri

with something like

def non_zero?(value)
  value !~ /^0+(.0+)?$/
end

not fails

@senny
Ruby on Rails member

@acapilleri I see your point but I think we need a different test-case. I'll revisit when I got a couple of minutes.

@acapilleri

the strange behaviour is when you try to test numericality:

def test_integer_zero_to_string_zero_dot_zero_not_marked_as_changed_00
      Pirate.validates :parrot_id, :numericality => { :only_integer => true }
      pirate = Pirate.new
      pirate.parrot_id = 0
      pirate.catchphrase = 'arrr'
      assert pirate.save!

      assert !pirate.changed?

      pirate.parrot_id = '0.0001'

      assert !pirate.valid?
      assert pirate.changed? #fails here
  end

parrotbecomes invalid but it not changed

@rafaelfranca
Ruby on Rails member

Yes. I think we have to take in consideration the @acapilleri case

@senny
Ruby on Rails member

@acapilleri I adjusted the Regexp as you described but I'm not sure it's actually solving the problem:

  def test_integer_zero_to_string_float_marked_as_changed
    Pirate.validates :parrot_id, :numericality => { :only_integer => true }
    pirate = Pirate.new
    pirate.catchphrase = "Yarrrr, me hearties"
    pirate.parrot_id = 0
    pirate.save!

    pirate.parrot_id = '0.00001'
    p pirate.valid? # => false
    p pirate.parrot_id # => 0
    p pirate.parrot_id_before_type_cast # => "0.00001"
    p pirate.changed? # => true
    p pirate.changes # => {"parrot_id"=>[0, 0]}
  end

The Numericality validator uses the value before the type cast. That's why it's marked as "invalid". On the other hand, the changes hash uses the value after the type cast and it does not really make sense.

@rafaelfranca This problem existed before my patch so I don't think we should mix them together. What is your opinion?

@mhuggins mhuggins and 1 other commented on an outdated diff Jan 25, 2013
...verecord/lib/active_record/attribute_methods/dirty.rb
@@ -107,7 +107,11 @@ def changes_from_nil_to_empty_string?(column, old, value)
def changes_from_zero_to_string?(old, value)
# For columns with old 0 and value non-empty string
- old == 0 && value.is_a?(String) && value.present? && value != '0'
+ old == 0 && value.is_a?(String) && value.present? && non_zero?(value)
+ end
+
+ def non_zero?(value)
+ value !~ /\A0+(.0+)?\z/

I think you're missing a backslash before the dot. i.e.:

value !~ /\A0+(\.0+)?\z/
@senny
Ruby on Rails member
senny added a note Jan 25, 2013

good catch, thanks. It's fixed now.

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

On the other hand, the changes hash uses the value after the type cast and it does not really make sense.

Yes but currently I don't see another simple solution, anyway it's better that we know that the object changes and it is invalid.

Also I think that you could simply use:

def changes_from_zero_to_string?(old, value)
  old == 0 && value.is_a?(String) && value.present? && value.to_i != 0 #or !value.to_i.zero?
end
@senny
Ruby on Rails member

The problem is, it's not really a solution. I think the adjusted Regexp makes sense but it won't solve the validation / type case / dirty problem.

  def test_validation_is_strange_anyway
    Pirate.validates :parrot_id, :numericality => { :only_integer => true }
    pirate = Pirate.new
    pirate.catchphrase = "Yarrrr, me hearties"
    pirate.parrot_id = '0'
    pirate.save

    pirate.parrot_id = '0.0000'
    p pirate.parrot_id # => 0
    p pirate.parrot_id_before_type_cast # => "0.0000"
    p pirate.valid? # => false
    p pirate.changes # => {}
  end

the integer validation will always fail if you assign such values.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 5, 2013
activerecord/test/cases/dirty_test.rb
@@ -243,6 +243,21 @@ def test_integer_zero_to_integer_zero_not_marked_as_changed
assert !pirate.changed?
end
+ def test_float_zero_to_string_zero_not_marked_as_changed
+ data = NumericData.new :temperature => 0.0
+ data.save!
+
+ assert_not data.changed?
+
+ data.temperature = '0'
+ assert_equal Hash.new, data.changes
+
+ data.temperature = '0.0'
+ assert_equal Hash.new, data.changes
+
+ data.temperature = '0.00'
+ assert_equal Hash.new, data.changes
@carlosantoniodasilva
Ruby on Rails member

Perhaps we can use assert_empty data.changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Ruby on Rails member

@carlosantoniodasilva rebased and assert_empty used.

@carlosantoniodasilva carlosantoniodasilva merged commit 09d1fb2 into rails:master Mar 5, 2013
@carlosantoniodasilva
Ruby on Rails member

Thanks!

@senny senny deleted the senny:9034_float_0_0_is_always_dirty branch Mar 5, 2013
@yahonda yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Mar 5, 2013
@yahonda yahonda Assigning "0.0" to a nullable numeric column does not make it dirty 20e86bc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment