Merge pull request #10561 from Empact/nix-throwresult

Rather than raising ThrowResult when construct_limited_ids_conditions comes up empty, set the relation to NullRelation and rely on its results.

6 files changed, 35 insertions, 23 deletions

diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb
index 017b8bace6..7e38719811 100644
--- a/activerecord/lib/active_record/errors.rb
+++ b/activerecord/lib/active_record/errors.rb
@@ -69,10 +69,6 @@ module ActiveRecord
     end
   end
 
-  # Raised when SQL statement is invalid and the application gets a blank result.
-  class ThrowResult < ActiveRecordError
-  end
-
   # Defunct wrapper class kept for compatibility.
   # +StatementInvalid+ wraps the original exception now.
   class WrappedDatabaseException < StatementInvalid
diff --git a/activerecord/lib/active_record/null_relation.rb b/activerecord/lib/active_record/null_relation.rb
index 711fc8b883..d166f0dd66 100644
--- a/activerecord/lib/active_record/null_relation.rb
+++ b/activerecord/lib/active_record/null_relation.rb
@@ -39,7 +39,7 @@ module ActiveRecord
     end
 
     def to_sql
-      @to_sql ||= ""
+      ""
     end
 
     def where_values_hash
@@ -55,7 +55,11 @@ module ActiveRecord
     end
 
     def calculate(_operation, _column_name, _options = {})
-      nil
+      if _operation == :count
+        0
+      else
+        nil
+      end
     end
 
     def exists?(_id = false)
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index e234e02032..ccb48247b7 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -106,8 +106,6 @@ module ActiveRecord
       else
         relation.calculate(operation, column_name, options)
       end
-    rescue ThrowResult
-      0
     end
 
     # Use <tt>pluck</tt> as a shortcut to select one or more attributes without
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index ba222aac93..f0edef58bf 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -160,8 +160,9 @@ module ActiveRecord
       conditions = conditions.id if Base === conditions
       return false if !conditions
 
-      join_dependency = construct_join_dependency
-      relation = construct_relation_for_association_find(join_dependency)
+      relation = construct_relation_for_association_find(construct_join_dependency)
+      return false if ActiveRecord::NullRelation === relation
+
       relation = relation.except(:select, :order).select("1 AS one").limit(1)
 
       case conditions
@@ -172,8 +173,6 @@ module ActiveRecord
       end
 
       connection.select_value(relation, "#{name} Exists", relation.bind_values)
-    rescue ThrowResult
-      false
     end
 
     # This method is called whenever no records are found with either a single
@@ -203,10 +202,12 @@ module ActiveRecord
     def find_with_associations
       join_dependency = construct_join_dependency
       relation = construct_relation_for_association_find(join_dependency)
-      rows = connection.select_all(relation, 'SQL', relation.bind_values.dup)
-      join_dependency.instantiate(rows)
-    rescue ThrowResult
-      []
+      if ActiveRecord::NullRelation === relation
+        []
+      else
+        rows = connection.select_all(relation, 'SQL', relation.bind_values.dup)
+        join_dependency.instantiate(rows)
+      end
     end
 
     def construct_join_dependency(joins = [])
@@ -230,21 +231,22 @@ module ActiveRecord
       if using_limitable_reflections?(join_dependency.reflections)
         relation
       else
-        relation.where!(construct_limited_ids_condition(relation)) if relation.limit_value
+        if relation.limit_value
+          limited_ids = limited_ids_for(relation)
+          limited_ids.empty? ? relation.none! : relation.where!(table[primary_key].in(limited_ids))
+        end
         relation.except(:limit, :offset)
       end
     end
 
-    def construct_limited_ids_condition(relation)
+    def limited_ids_for(relation)
       values = @klass.connection.columns_for_distinct(
         "#{quoted_table_name}.#{quoted_primary_key}", relation.order_values)
 
       relation = relation.except(:select).select(values).distinct!
 
       id_rows = @klass.connection.select_all(relation.arel, 'SQL', relation.bind_values)
-      ids_array = id_rows.map {|row| row[primary_key]}
-
-      ids_array.empty? ? raise(ThrowResult) : table[primary_key].in(ids_array)
+      id_rows.map {|row| row[primary_key]}
     end
 
     def find_with_ids(*ids)
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 095b78c6c8..c8ac77984f 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -6,6 +6,7 @@ require 'models/edge'
 require 'models/organization'
 require 'models/possession'
 require 'models/topic'
+require 'models/reply'
 require 'models/minivan'
 require 'models/speedometer'
 require 'models/ship_part'
@@ -472,6 +473,11 @@ class CalculationsTest < ActiveRecord::TestCase
     assert_equal [1,2,3,4], Topic.order(:id).pluck(:id)
   end
 
+  def test_pluck_without_column_names
+    assert_equal [[1, "Firm", 1, nil, "37signals", nil, 1, nil, ""]],
+      Company.order(:id).limit(1).pluck
+  end
+
   def test_pluck_type_cast
     topic = topics(:first)
     relation = Topic.where(:id => topic.id)
@@ -533,6 +539,11 @@ class CalculationsTest < ActiveRecord::TestCase
     assert_equal Company.all.map(&:id).sort, Company.ids.sort
   end
 
+  def test_pluck_with_includes_limit_and_empty_result
+    assert_equal [], Topic.includes(:replies).limit(0).pluck(:id)
+    assert_equal [], Topic.includes(:replies).limit(1).where('0 = 1').pluck(:id)
+  end
+
   def test_pluck_not_auto_table_name_prefix_if_column_included
     Company.create!(:name => "test", :contracts => [Contract.new(:developer_id => 7)])
     ids = Company.includes(:contracts).pluck(:developer_id)
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index b64ff13d29..e746ca2805 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -278,8 +278,9 @@ class RelationTest < ActiveRecord::TestCase
 
   def test_null_relation_calculations_methods
     assert_no_queries do
-      assert_equal 0,     Developer.none.count
-      assert_equal nil,   Developer.none.calculate(:average, 'salary')
+      assert_equal 0, Developer.none.count
+      assert_equal 0, Developer.none.calculate(:count, nil, {})
+      assert_equal nil, Developer.none.calculate(:average, 'salary')
     end
   end