Merge branch 'deprecate-calculations-with-block'

Follow up of the discussion from the original merge commit:
https://github.com/rails/rails/commit/f9cb645dfcb5cc89f59d2f8b58a019486c828c73#commitcomment-1414561

We want to avoid people's mistakes with methods like count and sum when
called with a block, that can easily lead to code performing poorly and
that could be way better written with a db query.

Please check the discussion there for more background.
Closes #8268

6 files changed, 25 insertions, 49 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index f166d73801..e3ed3780db 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Deprecate calling `Relation#sum` with a block. To perform a calculation over
+    the array result of the relation, use `to_a.sum(&block)`.
+
+    *Carlos Antonio da Silva*
+
 *   Fix postgresql adapter to handle BC timestamps correctly
 
         HistoryEvent.create!(:name => "something", :occured_at => Date.new(0) - 5.years)
@@ -736,13 +741,6 @@
 
     *Marc-André Lafortune*
 
-*   Allow blocks for `count` with `ActiveRecord::Relation`, to work similar as
-    `Array#count`:
-
-        Person.where("age > 26").count { |person| person.gender == 'female' }
-
-    *Chris Finne & Carlos Antonio da Silva*
-
 *   Added support to `CollectionAssociation#delete` for passing `fixnum`
     or `string` values as record ids. This finds the records responding
     to the `id` and executes delete on them.
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 651b17920c..d8b6d7a86b 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -234,7 +234,7 @@ module ActiveRecord
     #   others.size                       |   X   |    X     |    X
     #   others.length                     |   X   |    X     |    X
     #   others.count                      |   X   |    X     |    X
-    #   others.sum(args*,&block)          |   X   |    X     |    X
+    #   others.sum(*args)                 |   X   |    X     |    X
     #   others.empty?                     |   X   |    X     |    X
     #   others.clear                      |   X   |    X     |    X
     #   others.delete(other,other,...)    |   X   |    X     |    X
diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 862ff201de..1548e68cea 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -161,15 +161,6 @@ module ActiveRecord
         end
       end
 
-      # Calculate sum using SQL, not Enumerable.
-      def sum(*args)
-        if block_given?
-          scope.sum(*args) { |*block_args| yield(*block_args) }
-        else
-          scope.sum(*args)
-        end
-      end
-
       # Count all records using SQL. If the +:counter_sql+ or +:finder_sql+ option is set for the
       # association, it will be used for the query. Otherwise, construct options and pass them with
       # scope to the target class's +count+.
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 741f94f777..72b035a023 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -1,3 +1,5 @@
+require 'active_support/deprecation'
+
 module ActiveRecord
   module Calculations
     # Count the records.
@@ -13,16 +15,9 @@ module ActiveRecord
     #
     #   Person.count(:age, distinct: true)
     #   # => counts the number of different age values
-    #
-    #   Person.where("age > 26").count { |person| person.gender == 'female' }
-    #   # => queries people where "age > 26" then count the loaded results filtering by gender
     def count(column_name = nil, options = {})
-      if block_given?
-        self.to_a.count { |item| yield item }
-      else
-        column_name, options = nil, column_name if column_name.is_a?(Hash)
-        calculate(:count, column_name, options)
-      end
+      column_name, options = nil, column_name if column_name.is_a?(Hash)
+      calculate(:count, column_name, options)
     end
 
     # Calculates the average value on a given column. Returns +nil+ if there's
@@ -56,13 +51,13 @@ module ActiveRecord
     # +calculate+ for examples with options.
     #
     #   Person.sum('age') # => 4562
-    #   # => returns the total sum of all people's age
-    #
-    #   Person.where('age > 100').sum { |person| person.age - 100 }
-    #   # queries people where "age > 100" then perform a sum calculation with the block returns
     def sum(*args)
       if block_given?
-        self.to_a.sum(*args) { |item| yield item }
+        ActiveSupport::Deprecation.warn(
+          "Calling #sum with a block is deprecated and will be removed in Rails 4.1. " \
+          "If you want to perform sum calculation over the array of elements, use `to_a.sum(&block)`."
+        )
+        self.to_a.sum(*args) {|*block_args| yield(*block_args)}
       else
         calculate(:sum, *args)
       end
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 01afa087be..6cdc166533 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1658,6 +1658,12 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_deprecated { klass.has_many :foo, :counter_sql => 'lol' }
   end
 
+  test "sum calculation with block for array compatibility is deprecated" do
+    assert_deprecated do
+      posts(:welcome).comments.sum { |c| c.id }
+    end
+  end
+
   test "has many associations on new records use null relations" do
     post = Post.new
 
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 65d28ea028..5cb7eabf0e 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -383,30 +383,16 @@ class CalculationsTest < ActiveRecord::TestCase
         Company.where(:type => "Firm").from('companies').count(:type)
   end
 
-  def test_count_with_block_acts_as_array
-    accounts = Account.where('id > 0')
-    assert_equal Account.count, accounts.count { true }
-    assert_equal 0, accounts.count { false }
-    assert_equal Account.where('credit_limit > 50').size, accounts.count { |account| account.credit_limit > 50 }
-    assert_equal Account.count, Account.count { true }
-    assert_equal 0, Account.count { false }
-  end
-
-  def test_sum_with_block_acts_as_array
-    accounts = Account.where('id > 0')
-    assert_equal Account.sum(:credit_limit), accounts.sum { |account| account.credit_limit }
-    assert_equal Account.sum(:credit_limit) + Account.count, accounts.sum{ |account| account.credit_limit + 1 }
-    assert_equal 0, accounts.sum { |account| 0 }
-  end
-
   def test_sum_with_from_option
     assert_equal Account.sum(:credit_limit), Account.from('accounts').sum(:credit_limit)
     assert_equal Account.where("credit_limit > 50").sum(:credit_limit),
         Account.where("credit_limit > 50").from('accounts').sum(:credit_limit)
   end
 
-  def test_sum_array_compatibility
-    assert_equal Account.sum(:credit_limit), Account.sum(&:credit_limit)
+  def test_sum_array_compatibility_deprecation
+    assert_deprecated do
+      assert_equal Account.sum(:credit_limit), Account.sum(&:credit_limit)
+    end
   end
 
   def test_average_with_from_option