Permalink
Browse files

Validates_numericality_of is skipped when changing 0 to to non-empty …

…string

This happens when A has_many many B and A accepts_nested_attributes B that has a numeric colum
with initial 0 value. So a.update_attributes({:b_attributes => { :id => b.id, :numeric => 'foo' }})
passes the validation test but, the value of :numeric doesn't change.
his commit forces that the update fails with the above conditions.

Fixes #6393
Fixes #2331
  • Loading branch information...
1 parent 69f19b2 commit b9ec47da00e7f5e159ff130441585c3b42e41849 @acapilleri acapilleri committed with rafaelfranca Jun 8, 2012
@@ -77,11 +77,8 @@ def update(*)
def _field_changed?(attr, old, value)
if column = column_for_attribute(attr)
- if column.number? && column.null && (old.nil? || old == 0) && value.blank?
- # For nullable numeric 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)
+ if numeric_changes_from_nil_to_empty_string?(column, old, value) ||
+ numeric_changes_from_zero_to_string?(column, old, value)
value = nil
else
value = column.type_cast(value)
@@ -90,6 +87,19 @@ def _field_changed?(attr, old, value)
old != value
end
+
+ def numeric_changes_from_nil_to_empty_string?(column, old, value)
+ # For nullable numeric 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)
+ column.number? && column.null && (old.nil? || old == 0) && value.blank?
+ end
+
+ def numeric_changes_from_zero_to_string?(column, old, value)
+ # For numeric columns with old 0 and value non-empty string
+ column.number? && old == 0 && value != '0' && !value.blank? && !old.nil?
+ end
end
end
end
@@ -781,6 +781,16 @@ def test_can_use_symbols_as_object_identifier
assert_nothing_raised(NoMethodError) { @pirate.save! }
end
+ def test_numeric_colum_changes_from_zero_to_no_empty_string
+ Man.accepts_nested_attributes_for(:interests)
+ Interest.validates_numericality_of(:zine_id)
+ man = Man.create(:name => 'John')
+ interest = man.interests.create(:topic=>'bar',:zine_id => 0)
+ assert interest.save
+
+ assert !man.update_attributes({:interests_attributes => { :id => interest.id, :zine_id => 'foo' }})
+ end
+
private
def association_setter

12 comments on commit b9ec47d

@yahonda
Contributor

This test got failed with Oracle adapter. Oracle database handles equally NULL and empty string ( I know it's not an standard but it is what it is.)

@acapilleri
Contributor

@yahonda before this pull request, what was the behaviour?

@rafaelfranca
Member

I think was the same, because the code that check change from nil to empty string is the same, they only was not tested.

@yahonda do you know how to solve this?

@yahonda
Contributor

Thanks for your comments.

This behavior has not been changed, not just tested.
I can say this test contribute a lot to find this.

As of right now, I have other issue #6108 due to this behavior, handling '' as NULL or not.

This is a just a idea to implement null_as_empty_string? method
at ActiveRecord::ConnectionAdapters::AbstractAdapter which returns false as a default.
Then implementing this method to the Oracle enhanced adapter which returns true.

I'm still get confused what is the expected behavior.
Oracle handles '' as NULL. Number 0 does not equal neither NULL nor ''..

(Update: I was wrong with this comment. Oracle enhanced adapter already implements '' and NULL handling.)

@yahonda
Contributor
@acapilleri
Contributor

👍

@acapilleri
Contributor

@carlosantoniodasilva this pull request could be pushed on the 3-2-stable ?

@rafaelfranca
Member

@acapilleri I thought I applied it.

@acapilleri
Contributor

@rafaelfranca great! can you apply also this c3a5be0 small change to your commit ?
thanks

@rafaelfranca
Member

Ok. I'm running the tests and will push soon.

@rafaelfranca
Member
@acapilleri
Contributor

@rafaelfranca thanks!

Please sign in to comment.