Deprecate update_attributes and update_attributes!

elebow · jeremy · commit 5645149d3a27 · 2018-02-17T13:58:54.000-08:00
Closes #31998
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,4 +1,7 @@
 ## Rails 6.0.0.alpha (Unreleased) ##
+*   Deprecate `update_attributes`/`!` in favor of `update`/`!`.
+
+    *Eddie Lebow*
 
 *   Add ActiveRecord::Base.create_or_find_by/! to deal with the SELECT/INSERT race condition in
     ActiveRecord::Base.find_or_create_by/! by leaning on unique constraints in the database.
diff --git a/activerecord/lib/active_record/internal_metadata.rb b/activerecord/lib/active_record/internal_metadata.rb
@@ -17,7 +17,7 @@ def table_name
       end
 
       def []=(key, value)
-        find_or_initialize_by(key: key).update_attributes!(value: value)
+        find_or_initialize_by(key: key).update!(value: value)
       end
 
       def [](key)
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
@@ -418,6 +418,7 @@ def update(attributes)
     end
 
     alias update_attributes update
+    deprecate :update_attributes
 
     # Updates its receiver just like #update but calls #save! instead
     # of +save+, so an exception is raised if the record is invalid and saving will fail.
@@ -431,6 +432,7 @@ def update!(attributes)
     end
 
     alias update_attributes! update!
+    deprecate :update_attributes!
 
     # Equivalent to <code>update_columns(name => value)</code>.
     def update_column(name, value)
diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb
@@ -20,7 +20,7 @@ def test_update_prepared_statement
         b = Book.create(name: "my \x00 book")
         b.reload
         assert_equal "my \x00 book", b.name
-        b.update_attributes(name: "my other \x00 book")
+        b.update(name: "my other \x00 book")
         b.reload
         assert_equal "my other \x00 book", b.name
       end
diff --git a/activerecord/test/cases/adapters/mysql2/transaction_test.rb b/activerecord/test/cases/adapters/mysql2/transaction_test.rb
@@ -46,15 +46,15 @@ class Sample < ActiveRecord::Base
           Sample.transaction do
             s1.lock!
             barrier.wait
-            s2.update_attributes value: 1
+            s2.update value: 1
           end
         end
 
         begin
           Sample.transaction do
             s2.lock!
             barrier.wait
-            s1.update_attributes value: 2
+            s1.update value: 2
           end
         ensure
           thread.join
diff --git a/activerecord/test/cases/adapters/postgresql/transaction_test.rb b/activerecord/test/cases/adapters/postgresql/transaction_test.rb
@@ -76,15 +76,15 @@ class Sample < ActiveRecord::Base
             Sample.transaction do
               s1.lock!
               barrier.wait
-              s2.update_attributes value: 1
+              s2.update value: 1
             end
           end
 
           begin
             Sample.transaction do
               s2.lock!
               barrier.wait
-              s1.update_attributes value: 2
+              s1.update value: 2
             end
           ensure
             thread.join
diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -662,7 +662,7 @@ def test_symbol_join_table
     assert_includes developer.sym_special_projects, sp
   end
 
-  def test_update_attributes_after_push_without_duplicate_join_table_rows
+  def test_update_columns_after_push_without_duplicate_join_table_rows
     developer = Developer.new("name" => "Kano")
     project = SpecialProject.create("name" => "Special Project")
     assert developer.save
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1211,20 +1211,20 @@ def test_custom_named_counter_cache
     end
   end
 
-  def test_calling_update_attributes_on_id_changes_the_counter_cache
+  def test_calling_update_on_id_changes_the_counter_cache
     topic = Topic.order("id ASC").first
     original_count = topic.replies.to_a.size
     assert_equal original_count, topic.replies_count
 
     first_reply = topic.replies.first
-    first_reply.update_attributes(parent_id: nil)
+    first_reply.update(parent_id: nil)
     assert_equal original_count - 1, topic.reload.replies_count
 
-    first_reply.update_attributes(parent_id: topic.id)
+    first_reply.update(parent_id: topic.id)
     assert_equal original_count, topic.reload.replies_count
   end
 
-  def test_calling_update_attributes_changing_ids_doesnt_change_counter_cache
+  def test_calling_update_changing_ids_doesnt_change_counter_cache
     topic1 = Topic.find(1)
     topic2 = Topic.find(3)
     original_count1 = topic1.replies.to_a.size
@@ -1233,11 +1233,11 @@ def test_calling_update_attributes_changing_ids_doesnt_change_counter_cache
     reply1 = topic1.replies.first
     reply2 = topic2.replies.first
 
-    reply1.update_attributes(parent_id: topic2.id)
+    reply1.update(parent_id: topic2.id)
     assert_equal original_count1 - 1, topic1.reload.replies_count
     assert_equal original_count2 + 1, topic2.reload.replies_count
 
-    reply2.update_attributes(parent_id: topic1.id)
+    reply2.update(parent_id: topic1.id)
     assert_equal original_count1, topic1.reload.replies_count
     assert_equal original_count2, topic2.reload.replies_count
   end
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
@@ -50,7 +50,7 @@ def self.name; "Reference"; end
     }
 
     u = person.create!(first_name: "cool")
-    u.update_attributes!(first_name: "nah") # still valid because validation only applies on 'create'
+    u.update!(first_name: "nah") # still valid because validation only applies on 'create'
     assert_predicate reference.create!(person: u), :persisted?
   end
 
diff --git a/activerecord/test/cases/batches_test.rb b/activerecord/test/cases/batches_test.rb
@@ -508,7 +508,7 @@ def test_in_batches_relations_with_condition_should_not_overlap_with_each_other
   def test_in_batches_relations_update_all_should_not_affect_matching_records_in_other_batches
     Post.update_all(author_id: 0)
     person = Post.last
-    person.update_attributes(author_id: 1)
+    person.update(author_id: 1)
 
     Post.in_batches(of: 2) do |batch|
       batch.where("author_id >= 1").update_all("author_id = author_id + 1")
diff --git a/activerecord/test/cases/json_shared_test_cases.rb b/activerecord/test/cases/json_shared_test_cases.rb
@@ -101,7 +101,7 @@ def test_select_nil_json_after_update
     x = klass.where(payload: nil).first
     assert_nil(x)
 
-    json.update_attributes(payload: nil)
+    json.update(payload: nil)
     x = klass.where(payload: nil).first
     assert_equal(json.reload, x)
   end
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
@@ -525,7 +525,7 @@ def self.name; "Topic"; end
   def test_update_does_not_run_sql_if_record_has_not_changed
     topic = Topic.create(title: "Another New Topic")
     assert_queries(0) { assert topic.update(title: "Another New Topic") }
-    assert_queries(0) { assert topic.update_attributes(title: "Another New Topic") }
+    assert_queries(0) { assert topic.update(title: "Another New Topic") }
   end
 
   def test_delete
@@ -934,42 +934,33 @@ def test_update
     topic.reload
     assert_not_predicate topic, :approved?
     assert_equal "The First Topic", topic.title
-  end
-
-  def test_update_attributes
-    topic = Topic.find(1)
-    assert_not_predicate topic, :approved?
-    assert_equal "The First Topic", topic.title
-
-    topic.update_attributes("approved" => true, "title" => "The First Topic Updated")
-    topic.reload
-    assert_predicate topic, :approved?
-    assert_equal "The First Topic Updated", topic.title
-
-    topic.update_attributes(approved: false, title: "The First Topic")
-    topic.reload
-    assert_not_predicate topic, :approved?
-    assert_equal "The First Topic", topic.title
 
     error = assert_raise(ActiveRecord::RecordNotUnique, ActiveRecord::StatementInvalid) do
-      topic.update_attributes(id: 3, title: "Hm is it possible?")
+      topic.update(id: 3, title: "Hm is it possible?")
     end
     assert_not_nil error.cause
     assert_not_equal "Hm is it possible?", Topic.find(3).title
 
-    topic.update_attributes(id: 1234)
+    topic.update(id: 1234)
     assert_nothing_raised { topic.reload }
     assert_equal topic.title, Topic.find(1234).title
   end
 
-  def test_update_attributes_parameters
+  def test_update_attributes
+    topic = Topic.find(1)
+    assert_deprecated do
+      topic.update_attributes("title" => "The First Topic Updated")
+    end
+  end
+
+  def test_update_parameters
     topic = Topic.find(1)
     assert_nothing_raised do
-      topic.update_attributes({})
+      topic.update({})
     end
 
     assert_raises(ArgumentError) do
-      topic.update_attributes(nil)
+      topic.update(nil)
     end
   end
 
@@ -995,24 +986,10 @@ def test_update!
   end
 
   def test_update_attributes!
-    Reply.validates_presence_of(:title)
     reply = Reply.find(2)
-    assert_equal "The Second Topic of the day", reply.title
-    assert_equal "Have a nice day", reply.content
-
-    reply.update_attributes!("title" => "The Second Topic of the day updated", "content" => "Have a nice evening")
-    reply.reload
-    assert_equal "The Second Topic of the day updated", reply.title
-    assert_equal "Have a nice evening", reply.content
-
-    reply.update_attributes!(title: "The Second Topic of the day", content: "Have a nice day")
-    reply.reload
-    assert_equal "The Second Topic of the day", reply.title
-    assert_equal "Have a nice day", reply.content
-
-    assert_raise(ActiveRecord::RecordInvalid) { reply.update_attributes!(title: nil, content: "Have a nice evening") }
-  ensure
-    Reply.clear_validators!
+    assert_deprecated do
+      reply.update_attributes!("title" => "The Second Topic of the day updated")
+    end
   end
 
   def test_destroyed_returns_boolean
diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb
@@ -366,7 +366,7 @@ def test_query_cache_doesnt_leak_cached_results_of_rolled_back_queries
     post = Post.first
 
     Post.transaction do
-      post.update_attributes(title: "rollback")
+      post.update(title: "rollback")
       assert_equal 1, Post.where(title: "rollback").to_a.count
       raise ActiveRecord::Rollback
     end
@@ -379,7 +379,7 @@ def test_query_cache_doesnt_leak_cached_results_of_rolled_back_queries
 
     begin
       Post.transaction do
-        post.update_attributes(title: "rollback")
+        post.update(title: "rollback")
         assert_equal 1, Post.where(title: "rollback").to_a.count
         raise "broken"
       end
diff --git a/activerecord/test/cases/validations/length_validation_test.rb b/activerecord/test/cases/validations/length_validation_test.rb
@@ -56,7 +56,7 @@ def test_validates_size_of_respects_records_marked_for_destruction
     assert owner.save
 
     pet_count = Pet.count
-    assert_not owner.update_attributes pets_attributes: [ { _destroy: 1, id: pet.id } ]
+    assert_not owner.update pets_attributes: [ { _destroy: 1, id: pet.id } ]
     assert_not_predicate owner, :valid?
     assert_predicate owner.errors[:pets], :any?
     assert_equal pet_count, Pet.count
diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb
@@ -62,7 +62,7 @@ class TopicWithAfterCreate < Topic
   after_create :set_author
 
   def set_author
-    update_attributes!(author_name: "#{title} #{id}")
+    update!(author_name: "#{title} #{id}")
   end
 end
 
diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb
@@ -76,7 +76,7 @@ class CommentThatAutomaticallyAltersPostBody < Comment
   belongs_to :post, class_name: "PostThatLoadsCommentsInAnAfterSaveHook", foreign_key: :post_id
 
   after_save do |comment|
-    comment.post.update_attributes(body: "Automatically altered")
+    comment.post.update(body: "Automatically altered")
   end
 end
 
@@ -87,6 +87,6 @@ class CommentWithDefaultScopeReferencesAssociation < Comment
 
 class CommentWithAfterCreateUpdate < Comment
   after_create do
-    update_attributes(body: "bar")
+    update(body: "bar")
   end
 end
diff --git a/guides/source/active_record_callbacks.md b/guides/source/active_record_callbacks.md
@@ -264,7 +264,7 @@ The whole callback chain is wrapped in a transaction. If any callback raises an
 throw :abort
 ```
 
-WARNING. Any exception that is not `ActiveRecord::Rollback` or `ActiveRecord::RecordInvalid` will be re-raised by Rails after the callback chain is halted. Raising an exception other than `ActiveRecord::Rollback` or `ActiveRecord::RecordInvalid` may break code that does not expect methods like `save` and `update_attributes` (which normally try to return `true` or `false`) to raise an exception.
+WARNING. Any exception that is not `ActiveRecord::Rollback` or `ActiveRecord::RecordInvalid` will be re-raised by Rails after the callback chain is halted. Raising an exception other than `ActiveRecord::Rollback` or `ActiveRecord::RecordInvalid` may break code that does not expect methods like `save` and `update` (which normally try to return `true` or `false`) to raise an exception.
 
 Relational Callbacks
 --------------------

Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ def self.name; "Reference"; end`
`50`	`50`	`}`
`51`	`51`
`52`	`52`	`u = person.create!(first_name: "cool")`
`53`		`- u.update_attributes!(first_name: "nah") # still valid because validation only applies on 'create'`
	`53`	`+ u.update!(first_name: "nah") # still valid because validation only applies on 'create'`
`54`	`54`	`assert_predicate reference.create!(person: u), :persisted?`
`55`	`55`	`end`
`56`	`56`