Merge pull request #28705 from kamipo/extract_construct_relation_for_exists

Extract `construct_relation_for_exists` in `FinderMethods`

2 files changed, 33 insertions, 17 deletions

diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 5d24f5f5ca..14c2ddd85b 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -312,16 +312,7 @@ module ActiveRecord
       relation = apply_join_dependency(self, construct_join_dependency(eager_loading: false))
       return false if ActiveRecord::NullRelation === relation
 
-      relation = relation.except(:select, :distinct).select(ONE_AS_ONE).limit(1)
-
-      case conditions
-      when Array, Hash
-        relation = relation.where(conditions)
-      else
-        unless conditions == :none
-          relation = relation.where(primary_key => conditions)
-        end
-      end
+      relation = construct_relation_for_exists(relation, conditions)
 
       connection.select_value(relation, "#{name} Exists", relation.bound_attributes) ? true : false
     rescue ::RangeError
@@ -391,6 +382,19 @@ module ActiveRecord
         end
       end
 
+      def construct_relation_for_exists(relation, conditions)
+        relation = relation.except(:select, :distinct)._select!(ONE_AS_ONE).limit!(1)
+
+        case conditions
+        when Array, Hash
+          relation.where!(conditions)
+        else
+          relation.where!(primary_key => conditions) unless conditions == :none
+        end
+
+        relation
+      end
+
       def construct_join_dependency(joins = [], eager_loading: true)
         including = eager_load_values + includes_values
         ActiveRecord::Associations::JoinDependency.new(@klass, including, joins, eager_loading: eager_loading)
@@ -401,8 +405,7 @@ module ActiveRecord
       end
 
       def apply_join_dependency(relation, join_dependency)
-        relation = relation.except(:includes, :eager_load, :preload)
-        relation = relation.joins join_dependency
+        relation = relation.except(:includes, :eager_load, :preload).joins!(join_dependency)
 
         if using_limitable_reflections?(join_dependency.reflections)
           relation
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 89d8a8bdca..4fa664697c 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -202,11 +202,24 @@ class FinderTest < ActiveRecord::TestCase
     assert_equal true, Topic.first.replies.exists?
   end
 
-  # ensures +exists?+ runs valid SQL by excluding order value
-  def test_exists_with_order
+  # Ensure +exists?+ runs without an error by excluding distinct value.
+  # See https://github.com/rails/rails/pull/26981.
+  def test_exists_with_order_and_distinct
     assert_equal true, Topic.order(:id).distinct.exists?
   end
 
+  def test_exists_with_joins
+    assert_equal true, Topic.joins(:replies).where(replies_topics: { approved: true }).order("replies_topics.created_at DESC").exists?
+  end
+
+  def test_exists_with_left_joins
+    assert_equal true, Topic.left_joins(:replies).where(replies_topics: { approved: true }).order("replies_topics.created_at DESC").exists?
+  end
+
+  def test_exists_with_eager_load
+    assert_equal true, Topic.eager_load(:replies).where(replies_topics: { approved: true }).order("replies_topics.created_at DESC").exists?
+  end
+
   def test_exists_with_includes_limit_and_empty_result
     assert_equal false, Topic.includes(:replies).limit(0).exists?
     assert_equal false, Topic.includes(:replies).limit(1).where("0 = 1").exists?
@@ -236,9 +249,9 @@ class FinderTest < ActiveRecord::TestCase
 
   def test_exists_with_aggregate_having_three_mappings_with_one_difference
     existing_address = customers(:david).address
-    assert_equal false, Customer.exists?(address:       Address.new(existing_address.street, existing_address.city, existing_address.country + "1"))
-    assert_equal false, Customer.exists?(address:       Address.new(existing_address.street, existing_address.city + "1", existing_address.country))
-    assert_equal false, Customer.exists?(address:       Address.new(existing_address.street + "1", existing_address.city, existing_address.country))
+    assert_equal false, Customer.exists?(address: Address.new(existing_address.street, existing_address.city, existing_address.country + "1"))
+    assert_equal false, Customer.exists?(address: Address.new(existing_address.street, existing_address.city + "1", existing_address.country))
+    assert_equal false, Customer.exists?(address: Address.new(existing_address.street + "1", existing_address.city, existing_address.country))
   end
 
   def test_exists_does_not_instantiate_records