From 0dc3b6535dfb8127f568bb4abc2fb09118ce4b96 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Fri, 1 Dec 2017 11:49:36 +0900
Subject: Maintain raising `RecordNotFound` for class level `update` and`
 destroy`

In 35836019, class level `update` and `destroy` suppressed
`RecordNotFound` to ensure returning affected objects. But
`RecordNotFound` is a common exception caught by a `rescue_from`
handler. So changing the behavior when a typical `params[:id]` is passed
has a compatibility problem. The previous behavior should not be
changed.

Fixes #31301.
---
 activerecord/lib/active_record/persistence.rb | 13 +++++++++----
 activerecord/test/cases/persistence_test.rb   | 14 ++++++++++++--
 2 files changed, 21 insertions(+), 6 deletions(-)

(limited to 'activerecord')

diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 4e1b05dbf6..17459f2505 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -99,7 +99,11 @@ 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| update(one_id, attributes[idx]) }.compact
+          id.map.with_index { |one_id, idx|
+            object = find_by(primary_key => one_id)
+            object.update(attributes[idx]) if object
+            object
+          }.compact
         elsif id == :all
           all.each { |record| record.update(attributes) }
         else
@@ -112,7 +116,6 @@ module ActiveRecord
           object.update(attributes)
           object
         end
-      rescue RecordNotFound
       end
 
       # Destroy an object (or multiple objects) that has the given id. The object is instantiated first,
@@ -136,11 +139,13 @@ module ActiveRecord
       #   Todo.destroy(todos)
       def destroy(id)
         if id.is_a?(Array)
-          id.map { |one_id| destroy(one_id) }.compact
+          id.map { |one_id|
+            object = find_by(primary_key => one_id)
+            object.destroy if object
+          }.compact
         else
           find(id).destroy
         end
-      rescue RecordNotFound
       end
 
       # Deletes the row with a primary key matching the +id+ argument, using a
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index f088c064f5..643032e7cf 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -473,10 +473,18 @@ class PersistenceTest < ActiveRecord::TestCase
     assert_raise(ActiveRecord::RecordNotFound) { Topic.find(topic.id) }
   end
 
-  def test_record_not_found_exception
+  def test_find_raises_record_not_found_exception
     assert_raise(ActiveRecord::RecordNotFound) { Topic.find(99999) }
   end
 
+  def test_update_raises_record_not_found_exception
+    assert_raise(ActiveRecord::RecordNotFound) { Topic.update(99999, approved: true) }
+  end
+
+  def test_destroy_raises_record_not_found_exception
+    assert_raise(ActiveRecord::RecordNotFound) { Topic.destroy(99999) }
+  end
+
   def test_update_all
     assert_equal Topic.count, Topic.update_all("content = 'bulk updated!'")
     assert_equal "bulk updated!", Topic.find(1).content
@@ -938,7 +946,9 @@ class PersistenceTest < ActiveRecord::TestCase
     should_not_be_destroyed_reply = Reply.create("title" => "hello", "content" => "world")
     Topic.find(1).replies << should_not_be_destroyed_reply
 
-    assert_nil Topic.where("1=0").scoping { Topic.destroy(1) }
+    assert_raise(ActiveRecord::RecordNotFound) do
+      Topic.where("1=0").scoping { Topic.destroy(1) }
+    end
 
     assert_nothing_raised { Topic.find(1) }
     assert_nothing_raised { Reply.find(should_not_be_destroyed_reply.id) }
-- 
cgit v1.2.3