Merge pull request #10022 from wangjohn/find_on_nonexistent_id_raises_error

Throwing a RecordNotFound exception when a record is scanned using the inverse_of option

3 files changed, 55 insertions, 12 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 2385c90c1a..79e626f67e 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -79,8 +79,20 @@ module ActiveRecord
         if block_given?
           load_target.find(*args) { |*block_args| yield(*block_args) }
         else
-          if options[:finder_sql] || options[:inverse_of]
+          if options[:finder_sql]
             find_by_scan(*args)
+          elsif options[:inverse_of]
+            args = args.flatten
+            raise RecordNotFound, "Must specify an id to find" if args.blank?
+
+            result = find_by_scan(*args)
+
+            result_size = Array(result).size
+            if !result || result_size != args.size
+              scope.raise_record_not_found_exception!(args, result_size, args.size)
+            else
+              result
+            end
           else
             scope.find(*args)
           end
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index e2685d0478..a03d4bfeff 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -176,6 +176,28 @@ module ActiveRecord
       false
     end
 
+    # This method is called whenever no records are found with either a single
+    # id or multiple ids and raises a +ActiveRecord::RecordNotFound+ exception.
+    #
+    # The error message is different depending on whether a single id or
+    # multiple ids are provided. If multiple ids are provided, then the number
+    # of results obtained should be provided in the +result_size+ argument and
+    # the expected number of results should be provided in the +expected_size+
+    # argument.
+    def raise_record_not_found_exception!(ids, result_size, expected_size) #:nodoc:
+      conditions = arel.where_sql
+      conditions = " [#{conditions}]" if conditions
+
+      if Array(ids).size == 1
+        error = "Couldn't find #{@klass.name} with #{primary_key}=#{ids}#{conditions}"
+      else
+        error = "Couldn't find all #{@klass.name.pluralize} with IDs "
+        error << "(#{ids.join(", ")})#{conditions} (found #{result_size} results, but was looking for #{expected_size})"
+      end
+
+      raise RecordNotFound, error
+    end
+
     protected
 
     def find_with_associations
@@ -259,11 +281,7 @@ module ActiveRecord
       relation.bind_values += [[column, id]]
       record = relation.take
 
-      unless record
-        conditions = arel.where_sql
-        conditions = " [#{conditions}]" if conditions
-        raise RecordNotFound, "Couldn't find #{@klass.name} with #{primary_key}=#{id}#{conditions}"
-      end
+      raise_record_not_found_exception!(id, 0, 1) unless record
 
       record
     end
@@ -286,12 +304,7 @@ module ActiveRecord
       if result.size == expected_size
         result
       else
-        conditions = arel.where_sql
-        conditions = " [#{conditions}]" if conditions
-
-        error = "Couldn't find all #{@klass.name.pluralize} with IDs "
-        error << "(#{ids.join(", ")})#{conditions} (found #{result.size} results, but was looking for #{expected_size})"
-        raise RecordNotFound, error
+        raise_record_not_found_exception!(ids, result.size, expected_size)
       end
     end
 
diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb
index c8cad84013..89d2876c6c 100644
--- a/activerecord/test/cases/associations/inverse_associations_test.rb
+++ b/activerecord/test/cases/associations/inverse_associations_test.rb
@@ -303,6 +303,24 @@ class InverseHasManyTests < ActiveRecord::TestCase
     assert_equal man.name, man.interests.find(interest.id).man.name, "The name of the man should match after the child name is changed"
   end
 
+  def test_raise_record_not_found_error_when_invalid_ids_are_passed
+    man = Man.create!
+    interest = Interest.create!(man: man)
+
+    invalid_id = 2394823094892348920348523452345
+    assert_raise(ActiveRecord::RecordNotFound) { man.interests.find(invalid_id) }
+
+    invalid_ids = [8432342, 2390102913, 2453245234523452]
+    assert_raise(ActiveRecord::RecordNotFound) { man.interests.find(invalid_ids) }
+  end
+
+  def test_raise_record_not_found_error_when_no_ids_are_passed
+    man = Man.create!
+    interest = Interest.create!(man: man)
+
+    assert_raise(ActiveRecord::RecordNotFound) { man.interests.find() }
+  end
+
   def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error
     assert_raise(ActiveRecord::InverseOfAssociationNotFoundError) { Man.first.secret_interests }
   end