Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

This patch changes update_attribute implementatino so:

- it will only save the attribute it has been asked to save and not all dirty attributes

- it does not invoke callbacks

- it does change updated_at/on

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information...
commit 01629d180468049d17a8be6900e27a4f0d2b18c4 1 parent 690352d
@neerajdotname neerajdotname authored josevalim committed
View
17 activerecord/lib/active_record/persistence.rb
@@ -102,12 +102,19 @@ def becomes(klass)
became
end
- # Updates a single attribute and saves the record without going through the normal validation procedure.
- # This is especially useful for boolean flags on existing records. The regular +update_attribute+ method
- # in Base is replaced with this when the validations module is mixed in, which it is by default.
+ # Updates a single attribute and saves the record without going through the normal validation procedure
+ # or callbacks. This is especially useful for boolean flags on existing records.
def update_attribute(name, value)
send("#{name}=", value)
- save(:validate => false)
+ primary_key = self.class.primary_key
+ h = {name => value}
+ if should_record_update_timestamps
+ self.send(:record_update_timestamps)
+ current_time = current_time_from_proper_timezone
+ timestamp_attributes_for_update_in_model.each { |column| h.merge!(column => current_time) }
+ end
+ self.class.update_all(h, {primary_key => self[primary_key]}) == 1
+ @changed_attributes.delete(name.to_s)

Why the hell would update_attribute do this (i.e. hide itself from dirty tracking)? Isn't this a regression compared to AR 2.3.x (see http://github.com/rails/rails/blob/v2.3.8/activerecord/lib/active_record/base.rb#L2657)?

@neerajdotname Collaborator

after the operation is performed then :name attribute is no longer dirty. Hence it is being removed from the @changed_attributes. Yes it is a change from the way AR 2.3.x works.

i'm not sure i understand what your saying. it should be dirty during the after_update callback (which now is not longer called) like that behaved in 2.3.x. what exactly is the reasoning for not just sticking to the implementation from 2.3.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
# Updates all the attributes from the passed-in Hash and saves the record.
@@ -234,4 +241,4 @@ def attributes_from_column_definition
end
end
end
-end
+end
View
12 activerecord/lib/active_record/timestamp.rb
@@ -58,14 +58,22 @@ def create #:nodoc:
end
def update(*args) #:nodoc:
- if record_timestamps && (!partial_updates? || changed?)
+ record_update_timestamps
+ super
+ end
+
+ def record_update_timestamps
+ if should_record_update_timestamps
current_time = current_time_from_proper_timezone
timestamp_attributes_for_update_in_model.each { |column| write_attribute(column.to_s, current_time) }
end
+ end
- super
+ def should_record_update_timestamps
+ record_timestamps && (!partial_updates? || changed?)
end
+
def timestamp_attributes_for_update #:nodoc:
[:updated_at, :updated_on]
end
View
40 activerecord/test/cases/base_test.rb
@@ -893,6 +893,46 @@ def test_update_attribute
assert !Topic.find(1).approved?
end
+ def test_update_attribute_with_one_changed_and_one_updated
+ t = Topic.order('id').limit(1).first
+ title, author_name = t.title, t.author_name
+ t.author_name = 'John'
+ t.update_attribute(:title, 'super_title')
+ assert_equal 'John', t.author_name
+ assert_equal 'super_title', t.title
+ assert t.changed?, "topic should have changed"
+ assert t.author_name_changed?, "author_name should have changed"
+ assert !t.title_changed?, "title should not have changed"
+ assert_nil t.title_change, 'title change should be nil'
+ assert_equal ['author_name'], t.changed
+
+ t.reload
+ assert_equal 'David', t.author_name
+ assert_equal 'super_title', t.title
+ end
+
+ def test_update_attribute_with_one_updated
+ t = Topic.first
+ title = t.title
+ t.update_attribute(:title, 'super_title')
+ assert_equal 'super_title', t.title
+ assert !t.changed?, "topic should not have changed"
+ assert !t.title_changed?, "title should not have changed"
+ assert_nil t.title_change, 'title change should be nil'
+
+ t.reload
+ assert_equal 'super_title', t.title
+ end
+
+ def test_update_attribute_for_udpated_at_on
+ developer = Developer.find(1)
+ updated_at = developer.updated_at
+ developer.update_attribute(:salary, 80001)
+ assert_not_equal updated_at, developer.updated_at
+ developer.reload
+ assert_not_equal updated_at, developer.updated_at
+ end
+
def test_update_attributes
topic = Topic.find(1)
assert !topic.approved?
View
7 activerecord/test/cases/dirty_test.rb
@@ -475,10 +475,9 @@ def test_previous_changes
pirate = Pirate.find_by_catchphrase("Ahoy!")
pirate.update_attribute(:catchphrase, "Ninjas suck!")
- assert_equal 2, pirate.previous_changes.size
- assert_equal ["Ahoy!", "Ninjas suck!"], pirate.previous_changes['catchphrase']
- assert_not_nil pirate.previous_changes['updated_on'][0]
- assert_not_nil pirate.previous_changes['updated_on'][1]
+ assert_equal 0, pirate.previous_changes.size
+ assert_nil pirate.previous_changes['catchphrase']
+ assert_nil pirate.previous_changes['updated_on']
assert !pirate.previous_changes.key?('parrot_id')
assert !pirate.previous_changes.key?('created_on')
end
View
18 activerecord/test/cases/nested_attributes_test.rb
@@ -195,7 +195,7 @@ def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy
[1, '1', true, 'true'].each do |truth|
@pirate.reload.create_ship(:name => 'Mister Pablo')
assert_difference('Ship.count', -1) do
- @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => truth })
+ @pirate.update_attributes(:ship_attributes => { :id => @pirate.ship.id, :_destroy => truth })
end
end
end
@@ -203,7 +203,7 @@ def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy
def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy
[nil, '0', 0, 'false', false].each do |not_truth|
assert_no_difference('Ship.count') do
- @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => not_truth })
+ @pirate.update_attributes(:ship_attributes => { :id => @pirate.ship.id, :_destroy => not_truth })
end
end
end
@@ -212,7 +212,7 @@ def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false
Pirate.accepts_nested_attributes_for :ship, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? }
assert_no_difference('Ship.count') do
- @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => '1' })
+ @pirate.update_attributes(:ship_attributes => { :id => @pirate.ship.id, :_destroy => '1' })
end
Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
@@ -247,13 +247,13 @@ def test_should_automatically_enable_autosave_on_the_association
end
def test_should_accept_update_only_option
- @pirate.update_attribute(:update_only_ship_attributes, { :id => @pirate.ship.id, :name => 'Mayflower' })
+ @pirate.update_attributes(:update_only_ship_attributes => { :id => @pirate.ship.id, :name => 'Mayflower' })
end
def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true
@ship.delete
assert_difference('Ship.count', 1) do
- @pirate.reload.update_attribute(:update_only_ship_attributes, { :name => 'Mayflower' })
+ @pirate.reload.update_attributes(:update_only_ship_attributes => { :name => 'Mayflower' })
end
end
@@ -353,7 +353,7 @@ def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy
[1, '1', true, 'true'].each do |truth|
@ship.reload.create_pirate(:catchphrase => 'Arr')
assert_difference('Pirate.count', -1) do
- @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => truth })
+ @ship.update_attributes(:pirate_attributes => { :id => @ship.pirate.id, :_destroy => truth })
end
end
end
@@ -361,7 +361,7 @@ def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy
def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy
[nil, '0', 0, 'false', false].each do |not_truth|
assert_no_difference('Pirate.count') do
- @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => not_truth })
+ @ship.update_attributes(:pirate_attributes => { :id => @ship.pirate.id, :_destroy => not_truth })
end
end
end
@@ -370,7 +370,7 @@ def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false
Ship.accepts_nested_attributes_for :pirate, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? }
assert_no_difference('Pirate.count') do
- @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => '1' })
+ @ship.update_attributes(:pirate_attributes => { :id => @ship.pirate.id, :_destroy => '1' })
end
Ship.accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
@@ -398,7 +398,7 @@ def test_should_automatically_enable_autosave_on_the_association
def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true
@pirate.delete
assert_difference('Pirate.count', 1) do
- @ship.reload.update_attribute(:update_only_pirate_attributes, { :catchphrase => 'Arr' })
+ @ship.reload.update_attributes(:update_only_pirate_attributes => { :catchphrase => 'Arr' })
end
end

12 comments on commit 01629d1

@svenfuchs

Why the hell would update_attribute do this (i.e. hide itself from dirty tracking)? Isn't this a regression compared to AR 2.3.x (see http://github.com/rails/rails/blob/v2.3.8/activerecord/lib/active_record/base.rb#L2657)?

@csmuc

This change at such a late time (just before RC/release) looks scary to me

@vokle

hmm.... If this does go in there should be a huge note about it since this is a pretty core method. I imagine it could break a bunch of plugins because of the callbacks thing

@neerajdotname

after the operation is performed then :name attribute is no longer dirty. Hence it is being removed from the @changed_attributes. Yes it is a change from the way AR 2.3.x works.

@neerajdotname
Collaborator

@csmuc yes this is unfortunate that it was implemented so late in the game. Do notice that previously update_attribute used to save all the dirty attributes not just the attribute asked for. And that bug needed to be fixed.

@vokle: Doc has been updated http://github.com/lifo/docrails/blob/master/activerecord/lib/active_record/persistence.rb#L105

update_attribute was originally meant for sure shot way of updating a record. Assume that after sending out an email you want to mark the record as sent. @user.update_attribute(:email_sent, true). Previously this method used to skip validations but still used to call all the callbacks. Which means if one of the callbacks returns false then record will not be updated. And potentially user might be multiple emails. So it was necessary to ensure that callbacks are not invoked.

Previously one could do @user.update_attribute(:car, Car.new). That won't work anymore. Remember update_attribute skips all the validations and this way of assigning record is not recommended to beging with. With this commit one can't use update_attribute to assign a new record. Already I marked a few tickets as 'invalid' related to this change.

Overall the places where update_attribute could be used has been restricted. One should use update_attributes for anything more than just updating a column.

It is an unfortunate side effect that some plugins might break. But rails3 is a big change and it already changes the way plugins are written.

@fxn
Owner

This change is going to break existing code because update_attribute no longer invokes callbacks. For sure. And it is unfortunate because there's no way Rails can warn about it in 2.3 or anything. So those breakages won't be even apparent in general. That's not good.

The email example... well in that case you have a bug. update_attribute invokes callbacks, a callback halts save, no update is performed. That's fine! There's nothing to fix here, is the way it is supposed to be. On the other hand, when I write an after_save, I want it called whenever the model is saved. And update_attribute was following that expectation.

If you wanted a way to get directly to the database bypassing callbacks then a new method should have been implemented in my view.

@neerajdotname
Collaborator

I had a rather long chat with José Valim regarding this issue. Later Jeremy Kemper was also involved in the decision making process.

If you look at update_attribute and update_attributes the only difference was that update_attribute was not calling validation. Now we could create a new method (as suggested by Xavier) which would hit the database straight. Or we could shift update_attribute to do that and if you were using update_attribute for not invoking validation then change the code to have update_attributes(:validation => false).

Based on the number of tickets being generated in this area it seems it would have been better to deprecate update_attribute and create a new method. And then get rid of update_attribute in 3.1. That would have saved a lot of trouble.

@aaronchi

Why not just create a new method for updating without callbacks/validation and leave update_attribute as is?

@neerajdotname
Collaborator

update_attribute saves all the dirty attributes. leaving it as is would be a bug.

@neerajdotname
Collaborator

Also touch uses update_attribute . So when you meant to touch :email_sent_at it was saving all the dirty attributes.

@aaronchi

I'm saying, don't make it save all dirty attributes but still trigger callbacks/validations like before. Then, create a new method to update without callbacks/validations.

In that way, update_attribute and update_attributes work basically the same way and update_attribute is just a convenience method for updating only one attribute. Then, create a new method or add an option for updating attribute/attributes without triggering callbacks/validations

@michaeldv

@aaronchi instead of entirely new method we could have update_attribute(name, value, :validate => true/false)

@aaronchi

considering that save now has :validate => true/false it might be nice to add these to all save methods

:validate => true/false

:callbacks => true/false

:touch => true/false

@geoffgarside

I was bitten by #touch not triggering callbacks earlier today, as I only needed it to trigger callbacks in one case within my models I opted to override #touch such that it invoked save! after calling super which seems kind of bad as it now results in two DB hits, having a :callbacks => true/false would be nice.

@svenfuchs

i'm not sure i understand what your saying. it should be dirty during the after_update callback (which now is not longer called) like that behaved in 2.3.x. what exactly is the reasoning for not just sticking to the implementation from 2.3.x?

Please sign in to comment.
Something went wrong with that request. Please try again.