Deprecate the delegation of Array bang methods in ActiveRecord::Delegation

The primary means of returning results for Array bang methods is to modify
the array in-place. When you call these methods on a relation, that
array is created, modified, and then thrown away. Only the secondary
return value is exposed to the caller.

Removing this delegation is a straight-forward way to reduce user error
by forcing callers to first explicitly call #to_a in order to expose
the array to be acted on by the bang method.

3 files changed, 117 insertions, 3 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index c7928e503b..94d9f0ce60 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Deprecate the delegation of Array bang methods for associations.
+    To use them, instead first call #to_a on the association to access the
+    array to be acted on.
+
+    *Ben Woosley*
+
 *   Fix PredicateBuilder so polymorhic association keys in `where` clause can
     also accept not only `ActiveRecord::Base` direct descendances (decorated
     models, for example).
diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb
index e28938f9d0..1e15bddcdf 100644
--- a/activerecord/lib/active_record/relation/delegation.rb
+++ b/activerecord/lib/active_record/relation/delegation.rb
@@ -1,4 +1,5 @@
 require 'active_support/concern'
+require 'active_support/deprecation'
 
 module ActiveRecord
   module Delegation # :nodoc:
@@ -83,7 +84,7 @@ module ActiveRecord
         if @klass.respond_to?(method)
           self.class.delegate_to_scoped_klass(method)
           scoping { @klass.send(method, *args, &block) }
-        elsif Array.method_defined?(method)
+        elsif array_delegable?(method)
           self.class.delegate method, :to => :to_a
           to_a.send(method, *args, &block)
         elsif arel.respond_to?(method)
@@ -108,17 +109,27 @@ module ActiveRecord
     end
 
     def respond_to?(method, include_private = false)
-      super || Array.method_defined?(method) ||
+      super || array_delegable?(method) ||
         @klass.respond_to?(method, include_private) ||
         arel.respond_to?(method, include_private)
     end
 
     protected
 
+    def array_delegable?(method)
+      defined = Array.method_defined?(method)
+      if defined && method.to_s.ends_with?('!')
+        ActiveSupport::Deprecation.warn(
+          "Association will no longer delegate #{method} to #to_a as of Rails 4.2. You instead must first call #to_a on the association to expose the array to be acted on."
+        )
+      end
+      defined
+    end
+
     def method_missing(method, *args, &block)
       if @klass.respond_to?(method)
         scoping { @klass.send(method, *args, &block) }
-      elsif Array.method_defined?(method)
+      elsif array_delegable?(method)
         to_a.send(method, *args, &block)
       elsif arel.respond_to?(method)
         arel.send(method, *args, &block)
diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb
new file mode 100644
index 0000000000..71ade0bcc2
--- /dev/null
+++ b/activerecord/test/cases/relation/delegation_test.rb
@@ -0,0 +1,97 @@
+require 'cases/helper'
+require 'models/post'
+require 'models/comment'
+
+module ActiveRecord
+  class DelegationTest < ActiveRecord::TestCase
+    fixtures :posts
+
+    def assert_responds(target, method)
+      assert target.respond_to?(method)
+      assert_nothing_raised do
+        case target.to_a.method(method).arity
+        when 0
+          target.send(method)
+        when -1
+          if method == :shuffle!
+            target.send(method)
+          else
+            target.send(method, 1)
+          end
+        else
+          raise NotImplementedError
+        end
+      end
+    end
+  end
+
+  class DelegationAssociationTest < DelegationTest
+    def target
+      Post.first.comments
+    end
+
+    [:map, :collect].each do |method|
+      test "##{method} is delgated" do
+        assert_responds(target, method)
+        assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
+      end
+
+      test "##{method}! is not delgated" do
+        assert_deprecated do
+          assert_responds(target, "#{method}!")
+        end
+      end
+    end
+
+    [:compact!, :flatten!, :reject!, :reverse!, :rotate!,
+      :shuffle!, :slice!, :sort!, :sort_by!].each do |method|
+      test "##{method} delegation is deprecated" do
+        assert_deprecated do
+          assert_responds(target, method)
+        end
+      end
+    end
+
+    [:select!, :uniq!].each do |method|
+      test "##{method} is implemented" do
+        assert_responds(target, method)
+      end
+    end
+  end
+
+  class DelegationRelationTest < DelegationTest
+    def target
+      Comment.where.not(body: nil)
+    end
+
+    [:map, :collect].each do |method|
+      test "##{method} is delgated" do
+        assert_responds(target, method)
+        assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
+      end
+
+      test "##{method}! is not delgated" do
+        assert_deprecated do
+          assert_responds(target, "#{method}!")
+        end
+      end
+    end
+
+    [:compact!, :flatten!, :reject!, :reverse!, :rotate!,
+      :shuffle!, :slice!, :sort!, :sort_by!].each do |method|
+      test "##{method} delegation is deprecated" do
+        assert_deprecated do
+          assert_responds(target, method)
+        end
+      end
+    end
+
+    [:select!, :uniq!].each do |method|
+      test "##{method} is triggers an immutable error" do
+        assert_raises ActiveRecord::ImmutableRelation do
+          assert_responds(target, method)
+        end
+      end
+    end
+  end
+end