From 695ec1fef0b7fdb3125a3e2e4364a7f449265c1b Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Fri, 1 Dec 2017 18:21:21 +0900
Subject: [PATCH] Class level `update` and `destroy` checks all the records
 exist before making changes (#31306)

It makes more sense than ignoring invalid IDs.
---
 activerecord/lib/active_record/persistence.rb | 13 ++----
 activerecord/test/cases/persistence_test.rb   | 41 +++++++++++++++++--
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 17459f2505..a13b0d0181 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -99,11 +99,9 @@ module ActiveRecord
       # for updating all records in a single query.
       def update(id = :all, attributes)
         if id.is_a?(Array)
-          id.map.with_index { |one_id, idx|
-            object = find_by(primary_key => one_id)
-            object.update(attributes[idx]) if object
-            object
-          }.compact
+          id.map { |one_id| find(one_id) }.each_with_index { |object, idx|
+            object.update(attributes[idx])
+          }
         elsif id == :all
           all.each { |record| record.update(attributes) }
         else
@@ -139,10 +137,7 @@ module ActiveRecord
       #   Todo.destroy(todos)
       def destroy(id)
         if id.is_a?(Array)
-          id.map { |one_id|
-            object = find_by(primary_key => one_id)
-            object.destroy if object
-          }.compact
+          find(id).each(&:destroy)
         else
           find(id).destroy
         end
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index 643032e7cf..4aea275d51 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -70,7 +70,7 @@ class PersistenceTest < ActiveRecord::TestCase
   end
 
   def test_update_many
-    topic_data = { 1 => { "content" => "1 updated" }, 2 => { "content" => "2 updated" }, nil => {} }
+    topic_data = { 1 => { "content" => "1 updated" }, 2 => { "content" => "2 updated" } }
     updated = Topic.update(topic_data.keys, topic_data.values)
 
     assert_equal [1, 2], updated.map(&:id)
@@ -78,10 +78,33 @@ class PersistenceTest < ActiveRecord::TestCase
     assert_equal "2 updated", Topic.find(2).content
   end
 
+  def test_update_many_with_duplicated_ids
+    updated = Topic.update([1, 1, 2], [
+      { "content" => "1 duplicated" }, { "content" => "1 updated" }, { "content" => "2 updated" }
+    ])
+
+    assert_equal [1, 1, 2], updated.map(&:id)
+    assert_equal "1 updated", Topic.find(1).content
+    assert_equal "2 updated", Topic.find(2).content
+  end
+
+  def test_update_many_with_invalid_id
+    topic_data = { 1 => { "content" => "1 updated" }, 2 => { "content" => "2 updated" }, 99999 => {} }
+
+    assert_raise(ActiveRecord::RecordNotFound) do
+      Topic.where("1=0").scoping { Topic.update(topic_data.keys, topic_data.values) }
+    end
+
+    assert_not_equal "1 updated", Topic.find(1).content
+    assert_not_equal "2 updated", Topic.find(2).content
+  end
+
   def test_class_level_update_is_affected_by_scoping
     topic_data = { 1 => { "content" => "1 updated" }, 2 => { "content" => "2 updated" } }
 
-    assert_equal [], Topic.where("1=0").scoping { Topic.update(topic_data.keys, topic_data.values) }
+    assert_raise(ActiveRecord::RecordNotFound) do
+      Topic.where("1=0").scoping { Topic.update(topic_data.keys, topic_data.values) }
+    end
 
     assert_not_equal "1 updated", Topic.find(1).content
     assert_not_equal "2 updated", Topic.find(2).content
@@ -175,15 +198,25 @@ class PersistenceTest < ActiveRecord::TestCase
   end
 
   def test_destroy_many
-    clients = Client.all.merge!(order: "id").find([2, 3])
+    clients = Client.find([2, 3])
 
     assert_difference("Client.count", -2) do
-      destroyed = Client.destroy([2, 3, nil]).sort_by(&:id)
+      destroyed = Client.destroy([2, 3])
       assert_equal clients, destroyed
       assert destroyed.all?(&:frozen?), "destroyed clients should be frozen"
     end
   end
 
+  def test_destroy_many_with_invalid_id
+    clients = Client.find([2, 3])
+
+    assert_raise(ActiveRecord::RecordNotFound) do
+      Client.destroy([2, 3, 99999])
+    end
+
+    assert_equal clients, Client.find([2, 3])
+  end
+
   def test_becomes
     assert_kind_of Reply, topics(:first).becomes(Reply)
     assert_equal "The First Topic", topics(:first).becomes(Reply).title