Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Removed #update_attribute method. New #update_column method.

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information...
commit 45c233ef819dc7b67e259dd73f24721fec28b8c8 1 parent eb8beb3
@smartinez87 smartinez87 authored spastorino committed
View
3  activerecord/lib/active_record/associations/has_one_association.rb
@@ -34,7 +34,8 @@ def delete(method = options[:dependent])
when :destroy
target.destroy
when :nullify
- target.update_attribute(reflection.foreign_key, nil)
+ target.send("#{reflection.foreign_key}=", nil)
+ target.save(:validations => false)
end
end
end
View
18 activerecord/lib/active_record/persistence.rb
@@ -104,19 +104,17 @@ def becomes(klass)
became
end
- # Updates a single attribute and saves the record.
- # This is especially useful for boolean flags on existing records. Also note that
+ # Updates a single attribute of an object, without calling save.
#
# * Validation is skipped.
- # * Callbacks are invoked.
- # * updated_at/updated_on column is updated if that column is available.
- # * Updates all the attributes that are dirty in this object.
+ # * Callbacks are skipped.
+ # * updated_at/updated_on column is not updated in any case.
#
- def update_attribute(name, value)
+ def update_column(name, value)
name = name.to_s
raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
send("#{name}=", value)
- save(:validate => false)
+ self.class.update_all({ name => value }, self.class.primary_key => id)
end
# Updates the attributes of the model from the passed-in hash and saves the
@@ -156,7 +154,7 @@ def increment(attribute, by = 1)
# Saving is not subjected to validation checks. Returns +true+ if the
# record could be saved.
def increment!(attribute, by = 1)
- increment(attribute, by).update_attribute(attribute, self[attribute])
+ increment(attribute, by).update_column(attribute, self[attribute])
end
# Initializes +attribute+ to zero if +nil+ and subtracts the value passed as +by+ (default is 1).
@@ -173,7 +171,7 @@ def decrement(attribute, by = 1)
# Saving is not subjected to validation checks. Returns +true+ if the
# record could be saved.
def decrement!(attribute, by = 1)
- decrement(attribute, by).update_attribute(attribute, self[attribute])
+ decrement(attribute, by).update_column(attribute, self[attribute])
end
# Assigns to +attribute+ the boolean opposite of <tt>attribute?</tt>. So
@@ -190,7 +188,7 @@ def toggle(attribute)
# Saving is not subjected to validation checks. Returns +true+ if the
# record could be saved.
def toggle!(attribute)
- toggle(attribute).update_attribute(attribute, self[attribute])
+ toggle(attribute).update_column(attribute, self[attribute])
end
# Reloads the attributes of this object from the database.
View
2  activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -604,7 +604,7 @@ def test_update_attributes_after_push_without_duplicate_join_table_rows
project = SpecialProject.create("name" => "Special Project")
assert developer.save
developer.projects << project
- developer.update_attribute("name", "Bruza")
+ developer.update_column("name", "Bruza")
assert_equal 1, Developer.connection.select_value(<<-end_sql).to_i
SELECT count(*) FROM developers_projects
WHERE project_id = #{project.id}
View
6 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -639,7 +639,7 @@ def test_deleting_updates_counter_cache_without_dependent_option
def test_deleting_updates_counter_cache_with_dependent_delete_all
post = posts(:welcome)
- post.update_attribute(:taggings_with_delete_all_count, post.taggings_count)
+ post.update_column(:taggings_with_delete_all_count, post.taggings_count)
assert_difference "post.reload.taggings_with_delete_all_count", -1 do
post.taggings_with_delete_all.delete(post.taggings_with_delete_all.first)
@@ -648,7 +648,7 @@ def test_deleting_updates_counter_cache_with_dependent_delete_all
def test_deleting_updates_counter_cache_with_dependent_destroy
post = posts(:welcome)
- post.update_attribute(:taggings_with_destroy_count, post.taggings_count)
+ post.update_column(:taggings_with_destroy_count, post.taggings_count)
assert_difference "post.reload.taggings_with_destroy_count", -1 do
post.taggings_with_destroy.delete(post.taggings_with_destroy.first)
@@ -787,7 +787,7 @@ def test_delete_all_association_with_primary_key_deletes_correct_records
firm = Firm.find(:first)
# break the vanilla firm_id foreign key
assert_equal 2, firm.clients.count
- firm.clients.first.update_attribute(:firm_id, nil)
+ firm.clients.first.update_column(:firm_id, nil)
assert_equal 1, firm.clients(true).count
assert_equal 1, firm.clients_using_primary_key_with_delete_all.count
old_record = firm.clients_using_primary_key_with_delete_all.first
View
4 activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -286,7 +286,7 @@ def test_update_counter_caches_on_delete
def test_update_counter_caches_on_delete_with_dependent_destroy
post = posts(:welcome)
tag = post.tags.create!(:name => 'doomed')
- post.update_attribute(:tags_with_destroy_count, post.tags.count)
+ post.update_column(:tags_with_destroy_count, post.tags.count)
assert_difference ['post.reload.taggings_count', 'post.reload.tags_with_destroy_count'], -1 do
posts(:welcome).tags_with_destroy.delete(tag)
@@ -296,7 +296,7 @@ def test_update_counter_caches_on_delete_with_dependent_destroy
def test_update_counter_caches_on_delete_with_dependent_nullify
post = posts(:welcome)
tag = post.tags.create!(:name => 'doomed')
- post.update_attribute(:tags_with_nullify_count, post.tags.count)
+ post.update_column(:tags_with_nullify_count, post.tags.count)
assert_no_difference 'post.reload.taggings_count' do
assert_difference 'post.reload.tags_with_nullify_count', -1 do
View
4 activerecord/test/cases/associations/has_one_through_associations_test.rb
@@ -90,12 +90,12 @@ def test_has_one_through_eager_loading_through_polymorphic
def test_has_one_through_with_conditions_eager_loading
# conditions on the through table
assert_equal clubs(:moustache_club), Member.find(@member.id, :include => :favourite_club).favourite_club
- memberships(:membership_of_favourite_club).update_attribute(:favourite, false)
+ memberships(:membership_of_favourite_club).update_column(:favourite, false)
assert_equal nil, Member.find(@member.id, :include => :favourite_club).reload.favourite_club
# conditions on the source table
assert_equal clubs(:moustache_club), Member.find(@member.id, :include => :hairy_club).hairy_club
- clubs(:moustache_club).update_attribute(:name, "Association of Clean-Shaven Persons")
+ clubs(:moustache_club).update_column(:name, "Association of Clean-Shaven Persons")
assert_equal nil, Member.find(@member.id, :include => :hairy_club).reload.hairy_club
end
View
12 activerecord/test/cases/associations/join_model_test.rb
@@ -161,7 +161,7 @@ def test_create_polymorphic_has_one_with_scope
def test_delete_polymorphic_has_many_with_delete_all
assert_equal 1, posts(:welcome).taggings.count
- posts(:welcome).taggings.first.update_attribute :taggable_type, 'PostWithHasManyDeleteAll'
+ posts(:welcome).taggings.first.update_column :taggable_type, 'PostWithHasManyDeleteAll'
post = find_post_with_dependency(1, :has_many, :taggings, :delete_all)
old_count = Tagging.count
@@ -172,7 +172,7 @@ def test_delete_polymorphic_has_many_with_delete_all
def test_delete_polymorphic_has_many_with_destroy
assert_equal 1, posts(:welcome).taggings.count
- posts(:welcome).taggings.first.update_attribute :taggable_type, 'PostWithHasManyDestroy'
+ posts(:welcome).taggings.first.update_column :taggable_type, 'PostWithHasManyDestroy'
post = find_post_with_dependency(1, :has_many, :taggings, :destroy)
old_count = Tagging.count
@@ -183,7 +183,7 @@ def test_delete_polymorphic_has_many_with_destroy
def test_delete_polymorphic_has_many_with_nullify
assert_equal 1, posts(:welcome).taggings.count
- posts(:welcome).taggings.first.update_attribute :taggable_type, 'PostWithHasManyNullify'
+ posts(:welcome).taggings.first.update_column :taggable_type, 'PostWithHasManyNullify'
post = find_post_with_dependency(1, :has_many, :taggings, :nullify)
old_count = Tagging.count
@@ -194,7 +194,7 @@ def test_delete_polymorphic_has_many_with_nullify
def test_delete_polymorphic_has_one_with_destroy
assert posts(:welcome).tagging
- posts(:welcome).tagging.update_attribute :taggable_type, 'PostWithHasOneDestroy'
+ posts(:welcome).tagging.update_column :taggable_type, 'PostWithHasOneDestroy'
post = find_post_with_dependency(1, :has_one, :tagging, :destroy)
old_count = Tagging.count
@@ -205,7 +205,7 @@ def test_delete_polymorphic_has_one_with_destroy
def test_delete_polymorphic_has_one_with_nullify
assert posts(:welcome).tagging
- posts(:welcome).tagging.update_attribute :taggable_type, 'PostWithHasOneNullify'
+ posts(:welcome).tagging.update_column :taggable_type, 'PostWithHasOneNullify'
post = find_post_with_dependency(1, :has_one, :tagging, :nullify)
old_count = Tagging.count
@@ -707,7 +707,7 @@ def test_has_many_through_goes_through_all_sti_classes
# create dynamic Post models to allow different dependency options
def find_post_with_dependency(post_id, association, association_name, dependency)
class_name = "PostWith#{association.to_s.classify}#{dependency.to_s.classify}"
- Post.find(post_id).update_attribute :type, class_name
+ Post.find(post_id).update_column :type, class_name
klass = Object.const_set(class_name, Class.new(ActiveRecord::Base))
klass.set_table_name 'posts'
klass.send(association, association_name, :as => :taggable, :dependent => dependency)
View
4 activerecord/test/cases/associations_test.rb
@@ -66,7 +66,7 @@ def test_loading_the_association_target_should_load_most_recent_attributes_for_c
ship = Ship.create!(:name => "The good ship Dollypop")
part = ship.parts.create!(:name => "Mast")
part.mark_for_destruction
- ShipPart.find(part.id).update_attribute(:name, 'Deck')
+ ShipPart.find(part.id).update_column(:name, 'Deck')
ship.parts.send(:load_target)
assert_equal 'Deck', ship.parts[0].name
end
@@ -170,7 +170,7 @@ def test_save_on_parent_does_not_load_target
david = developers(:david)
assert !david.projects.loaded?
- david.update_attribute(:created_at, Time.now)
+ david.update_column(:created_at, Time.now)
assert !david.projects.loaded?
end
View
2  activerecord/test/cases/base_test.rb
@@ -484,7 +484,7 @@ def test_non_valid_identifier_column_name
weird.reload
assert_equal 'value', weird.send('a$b')
- weird.update_attribute('a$b', 'value2')
+ weird.update_column('a$b', 'value2')
weird.reload
assert_equal 'value2', weird.send('a$b')
end
View
4 activerecord/test/cases/calculations_test.rb
@@ -311,8 +311,8 @@ def test_should_count_scoped_select
def test_should_count_scoped_select_with_options
Account.update_all("credit_limit = NULL")
- Account.last.update_attribute('credit_limit', 49)
- Account.first.update_attribute('credit_limit', 51)
+ Account.last.update_column('credit_limit', 49)
+ Account.first.update_column('credit_limit', 51)
assert_equal 1, Account.scoped(:select => "credit_limit").count(:conditions => ['credit_limit >= 50'])
end
View
5 activerecord/test/cases/dirty_test.rb
@@ -413,7 +413,7 @@ def test_save_should_not_save_serialized_attribute_with_partial_updates_if_not_p
with_partial_updates(Topic) do
Topic.create!(:author_name => 'Bill', :content => {:a => "a"})
topic = Topic.select('id, author_name').first
- topic.update_attribute :author_name, 'John'
+ topic.update_column :author_name, 'John'
topic = Topic.first
assert_not_nil topic.content
end
@@ -487,7 +487,8 @@ def test_previous_changes
assert !pirate.previous_changes.key?('created_on')
pirate = Pirate.find_by_catchphrase("Ahoy!")
- pirate.update_attribute(:catchphrase, "Ninjas suck!")
+ pirate.catchphrase = "Ninjas suck!"
+ pirate.save(:validations => false)
assert_equal 2, pirate.previous_changes.size
assert_equal ["Ahoy!", "Ninjas suck!"], pirate.previous_changes['catchphrase']
View
88 activerecord/test/cases/persistence_test.rb
@@ -327,66 +327,62 @@ def test_destroy_record_with_associations
assert_raise(ActiveSupport::FrozenObjectError) { client.name = "something else" }
end
- def test_update_attribute
- assert !Topic.find(1).approved?
- Topic.find(1).update_attribute("approved", true)
- assert Topic.find(1).approved?
+ def test_update_column
+ topic = Topic.find(1)
+ topic.update_column("approved", true)
+ assert topic.approved?
+ topic.reload
+ assert topic.approved?
- Topic.find(1).update_attribute(:approved, false)
- assert !Topic.find(1).approved?
+ topic.update_column(:approved, false)
+ assert !topic.approved?
+ topic.reload
+ assert !topic.approved?
end
- def test_update_attribute_for_readonly_attribute
+ def test_update_column_with_model_having_primary_key_other_than_id
minivan = Minivan.find('m1')
- assert_raises(ActiveRecord::ActiveRecordError) { minivan.update_attribute(:color, 'black') }
- end
-
- # This test is correct, but it is hard to fix it since
- # update_attribute trigger simply call save! that triggers
- # all callbacks.
- # 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'
+ new_name = 'sebavan'
- t.reload
- assert_equal 'super_title', t.title
+ minivan.update_column(:name, new_name)
+ assert_equal new_name, minivan.name
end
- def test_update_attribute_for_updated_at_on
+ def test_update_column_for_readonly_attribute
+ minivan = Minivan.find('m1')
+ prev_color = minivan.color
+ assert_raises(ActiveRecord::ActiveRecordError) { minivan.update_column(:color, 'black') }
+ assert_equal prev_color, minivan.color
+ end
+
+ def test_update_column_should_not_modify_updated_at
developer = Developer.find(1)
prev_month = Time.now.prev_month
- developer.update_attribute(:updated_at, prev_month)
+ developer.update_column(:updated_at, prev_month)
assert_equal prev_month, developer.updated_at
- developer.update_attribute(:salary, 80001)
- assert_not_equal prev_month, developer.updated_at
+ developer.update_column(:salary, 80001)
+ assert_equal prev_month, developer.updated_at
developer.reload
- assert_not_equal prev_month, developer.updated_at
+ assert_equal prev_month, developer.updated_at
+ end
+
+ def test_update_column_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_column(: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 have changed"
+
+ t.reload
+ assert_equal author_name, t.author_name
+ assert_equal 'super_title', t.title
end
def test_update_attributes
View
6 activerecord/test/cases/timestamp_test.rb
@@ -113,7 +113,8 @@ def test_touching_a_record_with_a_belongs_to_that_uses_a_counter_cache_should_up
pet = Pet.first
owner = pet.owner
- owner.update_attribute(:happy_at, 3.days.ago)
+ owner.happy_at = 3.days.ago
+ owner.save
previously_owner_updated_at = owner.updated_at
pet.name = "I'm a parrot"
@@ -131,8 +132,9 @@ def test_touching_a_record_touches_parent_record_and_grandparent_record
toy = Toy.first
pet = toy.pet
owner = pet.owner
+ time = 3.days.ago
- owner.update_attribute(:updated_at, (time = 3.days.ago))
+ owner.update_column(:updated_at, time)
toy.touch
owner.reload

21 comments on commit 45c233e

@alindeman

What's the rationale behind this one? Is there a ticket/discussion?

@sikachu
Collaborator

Not even a deprecation warning? I smell a lot of code break here ...

@alindeman

Yah, that's what I was thinking :-/ How about a deprecation warning for 3.1, and removal sometime after that?

@sikachu
Collaborator

Just saw that the committer has another commit for deprecation warning. Can someone please merge into master?

smartinez87@310f617

@smartinez87

Deprecation warning will be merged into 3-0-stable anytime soon

@alindeman

What's the rationale?

@wallace

I'd love to hear the rationale as well.

@josevalim
Owner

Actually, I don't even agree that this should be deprecated. What is the gain in deprecating this? This deprecation will only cause pain and people to rewrite their current code. After all, update_attribute executes callbacks which is something desired in some scenarios (for instance, it updates the updated_at column). Besides, documentation and guides needs to be updated.

Finally, the current implementation is not complete. In the 3.0.rc times, we tried to change update_attribute to behave like update_column above and we had several bug reports. Most of them were fixed, you can see the final implementation (before the revert) in this commit:

https://github.com/rails/rails/commit/e4943e93

That said, we need to consider the following:

1) Should update_column change the value for that attribute in the current object? Notice that this implementation is changing the value of the current object, marking the current object as dirty, even though the attribute set was persisted to the database.

@user = User.first
@user.update_column(:age, 23)
@user.dirty? # => true

Even worse, the current code does not consider the attribute change done by the setter method. Let's suppose I have the following:

class User; def age=(value); write_attribute(:age, value * 2); end; end

@user = User.first
@user.update_column(:age, 23)
@user.age # => 46
@user.reload.age #=> 23

Or we call the setter method and use its value or we don't call it all.

2) Should update_column update timestamps like updated_at?

3) update_column must raise errors if called on new records

Questions made, my considerations are: 1) update_column should not call the setter method, although update the current object (this could be done directly in the @attributes hash). 2) update_column should not update timestamps (because it doesn't execute callbacks). If you want 1) and 2), you should use update_attribute, which is exactly another reason for keeping update_attribute intact.

@josevalim
Owner

@smartinez87, strong -1 for deprecating update_attribute in 3-0-stable. We did a lot of those mistakes when moving from Rails 2.3.x to Rails 3.0 and people just got upset even though it was a major release.

@vijaydev
Collaborator

Agree with @josevalim that this should not be deprecated in 3-0-stable.

Like to know the rationale too..

@josevalim
Owner

Rationale: update_attribute has a gotcha today that it actually updates all dirty attributes, not just the current one. This is actually very hard to fix and is a known bug since early versions. Also, update_attribute executes callbacks (although it doesn't run validations). update_column is an alternative that does not have the mentioned bug and is faster because it does not execute callbacks.

@josevalim
Owner

Two more notes:

1) Our recommendation in guides must be to always use update_attribute instead of update_column (unless you really know what you are doing).

2) If I am not wrong, ActiveRecord::Base.update_all returns an integer with the number of update rows in the database. This means update_column will probably return 1, which is a very poor choice. It should return a boolean.

@mtodd

I can see the benefit of having update_column in addition to update_attribute.

update_column should definitely update the @attributes hash directly.

@alindeman

I'm a tad confused now; is update_attribute going away or not?

@spastorino
Owner

We agreed on bringing back because this method have been there for a long time.

@alindeman

Awesome. Then I agree with mtodd's thought on having both update_column and update_attribute.

@mtodd

Thanks @spastorino.

Can you add a link to the ticket associated with the update_attribute bug that caused this? I'd like to take a look through but I haven't seen it on Lighthouse.

@spastorino
Owner

@mtodd there's no issue, just some complains asking why update_attribute doesn't call validations and stuff like that.

@dcrec1

+1 to keep update_attribute intact

@smartinez87

it has already been agreed on keeping #update_attribute intact and adding the new #update_column method.

@laserlemon

Really glad update_attribute is here to stay! I love the idea of update_column, as long as all it does to the object is update the attributes hash. I also don't think update_column should respect read-only attributes. My 2¢.

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