Merge pull request #12129 from Empact/deprecate-array-bang-delegation

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

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/test/cases/relation_test.rb

5 files changed, 265 insertions, 147 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 05fc079033..0cf50c512e 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*
+
 *   `CollectionAssociation#first`/`#last` (e.g. `has_many`) use a `LIMIT`ed
     query to fetch results rather than loading the entire collection.
 
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
diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb
new file mode 100644
index 0000000000..020fb24afa
--- /dev/null
+++ b/activerecord/test/cases/relation/mutation_test.rb
@@ -0,0 +1,148 @@
+require 'cases/helper'
+require 'models/post'
+
+module ActiveRecord
+  class RelationMutationTest < ActiveSupport::TestCase
+    class FakeKlass < Struct.new(:table_name, :name)
+      extend ActiveRecord::Delegation::DelegateCache
+      inherited self
+
+      def arel_table
+        Post.arel_table
+      end
+
+      def connection
+        Post.connection
+      end
+
+      def relation_delegate_class(klass)
+        self.class.relation_delegate_class(klass)
+      end
+    end
+
+    def relation
+      @relation ||= Relation.new FakeKlass.new('posts'), :b
+    end
+
+    (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order]).each do |method|
+      test "##{method}!" do
+        assert relation.public_send("#{method}!", :foo).equal?(relation)
+        assert_equal [:foo], relation.public_send("#{method}_values")
+      end
+    end
+
+    test '#order!' do
+      assert relation.order!('name ASC').equal?(relation)
+      assert_equal ['name ASC'], relation.order_values
+    end
+
+    test '#order! with symbol prepends the table name' do
+      assert relation.order!(:name).equal?(relation)
+      node = relation.order_values.first
+      assert node.ascending?
+      assert_equal :name, node.expr.name
+      assert_equal "posts", node.expr.relation.name
+    end
+
+    test '#order! on non-string does not attempt regexp match for references' do
+      obj = Object.new
+      obj.expects(:=~).never
+      assert relation.order!(obj)
+      assert_equal [obj], relation.order_values
+    end
+
+    test '#references!' do
+      assert relation.references!(:foo).equal?(relation)
+      assert relation.references_values.include?('foo')
+    end
+
+    test 'extending!' do
+      mod, mod2 = Module.new, Module.new
+
+      assert relation.extending!(mod).equal?(relation)
+      assert_equal [mod], relation.extending_values
+      assert relation.is_a?(mod)
+
+      relation.extending!(mod2)
+      assert_equal [mod, mod2], relation.extending_values
+    end
+
+    test 'extending! with empty args' do
+      relation.extending!
+      assert_equal [], relation.extending_values
+    end
+
+    (Relation::SINGLE_VALUE_METHODS - [:from, :lock, :reordering, :reverse_order, :create_with]).each do |method|
+      test "##{method}!" do
+        assert relation.public_send("#{method}!", :foo).equal?(relation)
+        assert_equal :foo, relation.public_send("#{method}_value")
+      end
+    end
+
+    test '#from!' do
+      assert relation.from!('foo').equal?(relation)
+      assert_equal ['foo', nil], relation.from_value
+    end
+
+    test '#lock!' do
+      assert relation.lock!('foo').equal?(relation)
+      assert_equal 'foo', relation.lock_value
+    end
+
+    test '#reorder!' do
+      relation = self.relation.order('foo')
+
+      assert relation.reorder!('bar').equal?(relation)
+      assert_equal ['bar'], relation.order_values
+      assert relation.reordering_value
+    end
+
+    test '#reorder! with symbol prepends the table name' do
+      assert relation.reorder!(:name).equal?(relation)
+      node = relation.order_values.first
+
+      assert node.ascending?
+      assert_equal :name, node.expr.name
+      assert_equal "posts", node.expr.relation.name
+    end
+
+    test 'reverse_order!' do
+      assert relation.reverse_order!.equal?(relation)
+      assert relation.reverse_order_value
+      relation.reverse_order!
+      assert !relation.reverse_order_value
+    end
+
+    test 'create_with!' do
+      assert relation.create_with!(foo: 'bar').equal?(relation)
+      assert_equal({foo: 'bar'}, relation.create_with_value)
+    end
+
+    test 'test_merge!' do
+      assert relation.merge!(where: :foo).equal?(relation)
+      assert_equal [:foo], relation.where_values
+    end
+
+    test 'merge with a proc' do
+      assert_equal [:foo], relation.merge(-> { where(:foo) }).where_values
+    end
+
+    test 'none!' do
+      assert relation.none!.equal?(relation)
+      assert_equal [NullRelation], relation.extending_values
+      assert relation.is_a?(NullRelation)
+    end
+
+    test 'distinct!' do
+      relation.distinct! :foo
+      assert_equal :foo, relation.distinct_value
+      assert_equal :foo, relation.uniq_value # deprecated access
+    end
+
+    test 'uniq! was replaced by distinct!' do
+      relation.uniq! :foo
+      assert_equal :foo, relation.distinct_value
+      assert_equal :foo, relation.uniq_value # deprecated access
+    end
+  end
+end
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index e77de19ac3..70d113fb39 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -223,148 +223,4 @@ module ActiveRecord
     end
 
   end
-
-  class RelationMutationTest < ActiveSupport::TestCase
-    class FakeKlass < Struct.new(:table_name, :name)
-      extend ActiveRecord::Delegation::DelegateCache
-      inherited self
-
-      def arel_table
-        Post.arel_table
-      end
-
-      def connection
-        Post.connection
-      end
-
-      def relation_delegate_class(klass)
-        self.class.relation_delegate_class(klass)
-      end
-    end
-
-    def relation
-      @relation ||= Relation.new FakeKlass.new('posts'), :b
-    end
-
-    (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order]).each do |method|
-      test "##{method}!" do
-        assert relation.public_send("#{method}!", :foo).equal?(relation)
-        assert_equal [:foo], relation.public_send("#{method}_values")
-      end
-    end
-
-    test "#order!" do
-      assert relation.order!('name ASC').equal?(relation)
-      assert_equal ['name ASC'], relation.order_values
-    end
-
-    test "#order! with symbol prepends the table name" do
-      assert relation.order!(:name).equal?(relation)
-      node = relation.order_values.first
-      assert node.ascending?
-      assert_equal :name, node.expr.name
-      assert_equal "posts", node.expr.relation.name
-    end
-
-    test "#order! on non-string does not attempt regexp match for references" do
-      obj = Object.new
-      obj.expects(:=~).never
-      assert relation.order!(obj)
-      assert_equal [obj], relation.order_values
-    end
-
-    test '#references!' do
-      assert relation.references!(:foo).equal?(relation)
-      assert relation.references_values.include?('foo')
-    end
-
-    test 'extending!' do
-      mod, mod2 = Module.new, Module.new
-
-      assert relation.extending!(mod).equal?(relation)
-      assert_equal [mod], relation.extending_values
-      assert relation.is_a?(mod)
-
-      relation.extending!(mod2)
-      assert_equal [mod, mod2], relation.extending_values
-    end
-
-    test 'extending! with empty args' do
-      relation.extending!
-      assert_equal [], relation.extending_values
-    end
-
-    (Relation::SINGLE_VALUE_METHODS - [:from, :lock, :reordering, :reverse_order, :create_with]).each do |method|
-      test "##{method}!" do
-        assert relation.public_send("#{method}!", :foo).equal?(relation)
-        assert_equal :foo, relation.public_send("#{method}_value")
-      end
-    end
-
-    test '#from!' do
-      assert relation.from!('foo').equal?(relation)
-      assert_equal ['foo', nil], relation.from_value
-    end
-
-    test '#lock!' do
-      assert relation.lock!('foo').equal?(relation)
-      assert_equal 'foo', relation.lock_value
-    end
-
-    test '#reorder!' do
-      relation = self.relation.order('foo')
-
-      assert relation.reorder!('bar').equal?(relation)
-      assert_equal ['bar'], relation.order_values
-      assert relation.reordering_value
-    end
-
-    test '#reorder! with symbol prepends the table name' do
-      assert relation.reorder!(:name).equal?(relation)
-      node = relation.order_values.first
-
-      assert node.ascending?
-      assert_equal :name, node.expr.name
-      assert_equal "posts", node.expr.relation.name
-    end
-
-    test 'reverse_order!' do
-      assert relation.reverse_order!.equal?(relation)
-      assert relation.reverse_order_value
-      relation.reverse_order!
-      assert !relation.reverse_order_value
-    end
-
-    test 'create_with!' do
-      assert relation.create_with!(foo: 'bar').equal?(relation)
-      assert_equal({foo: 'bar'}, relation.create_with_value)
-    end
-
-    def test_merge!
-      assert relation.merge!(where: :foo).equal?(relation)
-      assert_equal [:foo], relation.where_values
-    end
-
-    test 'merge with a proc' do
-      assert_equal [:foo], relation.merge(-> { where(:foo) }).where_values
-    end
-
-    test 'none!' do
-      assert relation.none!.equal?(relation)
-      assert_equal [NullRelation], relation.extending_values
-      assert relation.is_a?(NullRelation)
-    end
-
-    test "distinct!" do
-      relation.distinct! :foo
-      assert_equal :foo, relation.distinct_value
-      assert_equal :foo, relation.uniq_value # deprecated access
-    end
-
-    test "uniq! was replaced by distinct!" do
-      relation.uniq! :foo
-      assert_equal :foo, relation.distinct_value
-      assert_equal :foo, relation.uniq_value # deprecated access
-    end
-  end
 end