Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes _field_changed? #7585

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

bkuo commented Sep 10, 2012

When a non-empty string that casts to zero is assigned to a number column already containing zero, then _field_changed? should not become true.

@rafaelfranca rafaelfranca commented on an outdated diff Sep 10, 2012

activerecord/test/cases/dirty_test.rb
+
+ assert !pirate.changed?
+
+ pirate.parrot_id = 'arrr' # Casts to 0
+ assert !pirate.changed?
+
+ pirate.parrot_id = '123arrr' # Casts to 123
+ assert pirate.changed?
+
+ pirate.parrot_id = 'arrr123' # Casts to 0
+ assert !pirate.changed?
+
+ pirate.parrot_id = '' # Casts to nil
+ assert pirate.changed?
+ end
+
@rafaelfranca

rafaelfranca Sep 10, 2012

Owner

Remove this extra blank line

@rafaelfranca rafaelfranca commented on an outdated diff Sep 10, 2012

...verecord/lib/active_record/attribute_methods/dirty.rb
@@ -75,29 +75,13 @@ def update(*)
def _field_changed?(attr, old, value)
if column = column_for_attribute(attr)
- if column.number? && (changes_from_nil_to_empty_string?(column, old, value) ||
- changes_from_zero_to_string?(old, value))
- value = nil
- else
- value = column.type_cast(value)
- end
+ # We consider the field changed if the new value after type-casting is different from the old value
@rafaelfranca

rafaelfranca Sep 10, 2012

Owner

Use the proper indentation.

@rafaelfranca rafaelfranca commented on the diff Sep 10, 2012

activerecord/test/cases/dirty_test.rb
@@ -225,6 +225,27 @@ def test_integer_zero_to_integer_zero_not_marked_as_changed
assert !pirate.changed?
end
+ def test_integer_zero_to_string_non_zero_not_marked_as_changed
+ pirate = Pirate.new
+ pirate.parrot_id = 0
+ pirate.catchphrase = 'arrr' # Casts to 0
+ assert pirate.save!
+
+ assert !pirate.changed?
+
+ pirate.parrot_id = 'arrr' # Casts to 0
+ assert !pirate.changed?
+
+ pirate.parrot_id = '123arrr' # Casts to 123
@rafaelfranca

rafaelfranca Sep 10, 2012

Owner

I don't think that this case should return true for changed?

@rafaelfranca

rafaelfranca Sep 10, 2012

Owner

After think better about this, I saw that is fine that it returns true but I think that now the value would be 123 and not nil as it was before the change.

@bkuo

bkuo Sep 10, 2012

Not nil, but 0. You only get nil if you assign a nil or a ''. This test case is meant to test the changed?-ness of assigning various strings to a column originally containing 0

@rafaelfranca

rafaelfranca Sep 10, 2012

Owner

You are right. These change seems fine.

bkuo commented Sep 10, 2012

extra blank line removed and indentation fixed

Owner

rafaelfranca commented Sep 10, 2012

@tenderlove I would love hear your thoughts about this pull request. I'm not confident to change it because I may be missing something.

@bkuo bkuo commented on the diff Sep 10, 2012

activerecord/test/cases/nested_attributes_test.rb
assert interest.save
- assert !man.update_attributes({interests_attributes: { id: interest.id, zine_id: 'foo' }})
@bkuo

bkuo Sep 10, 2012

One thing I wanted to point out and get some confirmation on:

I reversed this assertion as I think it was incorrect. 'foo' casts to 0 which is a perfectly valid :zine_id absent other validations suggesting that it is not, so update_attributes should work just fine.

@rafaelfranca

rafaelfranca Sep 10, 2012

Owner

Now that you said I think the assertion was right since 'foo' is not a number either if it casts to 0

@bkuo

bkuo Sep 11, 2012

Sorry, I didn't quite understand what you meant here. Did you mean you think the previous was correct and my change is wrong or that the previous was wrong and my change is correct?

Thanks

@rafaelfranca

rafaelfranca Sep 18, 2012

Owner

I think the previous was correct.

@bkuo

bkuo Sep 18, 2012

Are you suggesting that it was broken in 3.2.0->3.2.6 also then? My test passes in those, the previous would have broken them.

Logically, what would be the reasoning that update_attribute should fail here? 0 could be a perfectly valid zine_id. If not then it is the user's responsibility to put a validation on it.

bkuo commented Sep 18, 2012

rebased

A field should be considered changed only if the fully type-casted ne…
…w value is different than the old value.

Specifically, when a number field contains zero,  and a string is assigned that also casts to zero,  then _field_changed? should be false.
Member

steveklabnik commented Nov 28, 2012

@rafaelfranca @bkuo what's the current status on this?

bkuo commented Dec 9, 2012

Though I don't agree with it, I can see the reasoning for its current behavior, and it is enough of an edge case that I really don't want to push it. Closing..

@bkuo bkuo closed this Dec 9, 2012

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