Merge pull request #12590 from laurocaetano/whitelist-to-delegate-array-methods

Create a whitelist of methods to be delegated to Array.

4 files changed, 105 insertions, 104 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 26f5aa2c73..4845150a24 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,18 @@
+*   Create a whitelist of delegable methods to `Array`.
+
+    Currently `Relation` directly delegates methods to `Array`. With this change,
+    only the methods present in this whitelist will be delegated.
+
+    The whitelist contains:
+
+        #&, #+, #[], #all?, #collect, #detect, #each, #each_cons, #each_with_index,
+        #flat_map, #group_by, #include?, #length, #map, #none?, :one?, #reverse, #sample,
+        #second, #sort, #sort_by, #to_ary, #to_set, #to_xml, #to_yaml
+
+    To use any other method, instead first call `#to_a` on the association.
+
+    *Lauro Caetano*
+
 *   Use the right column to type cast grouped calculations with custom expressions.
 
     Fixes #13230.
@@ -449,12 +464,6 @@
 
     *Paul Nikitochkin*
 
-*   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 1e15bddcdf..603e5a9df5 100644
--- a/activerecord/lib/active_record/relation/delegation.rb
+++ b/activerecord/lib/active_record/relation/delegation.rb
@@ -36,7 +36,11 @@ module ActiveRecord
     # may vary depending on the klass of a relation, so we create a subclass of Relation
     # for each different klass, and the delegations are compiled into that subclass only.
 
-    delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :to => :to_a
+    delegate :&, :+, :[], :all?, :collect, :detect, :each, :each_cons,
+             :each_with_index, :flat_map, :group_by, :include?, :length,
+             :map, :none?, :one?, :reverse, :sample, :second, :sort, :sort_by,
+             :to_ary, :to_set, :to_xml, :to_yaml, :to => :to_a
+
     delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key,
              :connection, :columns_hash, :to => :klass
 
@@ -64,7 +68,7 @@ module ActiveRecord
               RUBY
             else
               define_method method do |*args, &block|
-                scoping { @klass.send(method, *args, &block) }
+                scoping { @klass.public_send(method, *args, &block) }
               end
             end
           end
@@ -83,13 +87,10 @@ module ActiveRecord
       def method_missing(method, *args, &block)
         if @klass.respond_to?(method)
           self.class.delegate_to_scoped_klass(method)
-          scoping { @klass.send(method, *args, &block) }
-        elsif array_delegable?(method)
-          self.class.delegate method, :to => :to_a
-          to_a.send(method, *args, &block)
+          scoping { @klass.public_send(method, *args, &block) }
         elsif arel.respond_to?(method)
           self.class.delegate method, :to => :arel
-          arel.send(method, *args, &block)
+          arel.public_send(method, *args, &block)
         else
           super
         end
@@ -109,30 +110,17 @@ module ActiveRecord
     end
 
     def respond_to?(method, include_private = false)
-      super || array_delegable?(method) ||
-        @klass.respond_to?(method, include_private) ||
+      super || @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_delegable?(method)
-        to_a.send(method, *args, &block)
+        scoping { @klass.public_send(method, *args, &block) }
       elsif arel.respond_to?(method)
-        arel.send(method, *args, &block)
+        arel.public_send(method, *args, &block)
       else
         super
       end
diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb
index 13677797cf..b56dd5e5db 100644
--- a/activerecord/test/cases/relation/delegation_test.rb
+++ b/activerecord/test/cases/relation/delegation_test.rb
@@ -6,22 +6,21 @@ module ActiveRecord
   class DelegationTest < ActiveRecord::TestCase
     fixtures :posts
 
-    def assert_responds(target, method)
-      assert target.respond_to?(method)
-      assert_nothing_raised do
-        method_arity = target.to_a.method(method).arity
+    def call_method(target, method)
+      method_arity = target.to_a.method(method).arity
 
-        if method_arity.zero?
-          target.send(method)
-        elsif method_arity < 0
-          if method == :shuffle!
-            target.send(method)
-          else
-            target.send(method, 1)
-          end
+      if method_arity.zero?
+        target.public_send(method)
+      elsif method_arity < 0
+        if method == :shuffle!
+          target.public_send(method)
         else
-          raise NotImplementedError
+          target.public_send(method, 1)
         end
+       elsif method_arity == 1
+        target.public_send(method, 1)
+      else
+        raise NotImplementedError
       end
     end
   end
@@ -31,31 +30,20 @@ module ActiveRecord
       Post.first.comments
     end
 
-    [:map, :collect].each do |method|
-      test "##{method} is delegated" do
-        assert_responds(target, method)
-        assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
-      end
-
-      test "##{method}! is not delegated" do
-        assert_deprecated do
-          assert_responds(target, "#{method}!")
-        end
+    [:&, :+, :[], :all?, :collect, :detect, :each, :each_cons,
+     :each_with_index, :flat_map, :group_by, :include?, :length,
+     :map, :none?, :one?, :reverse, :sample, :second, :sort, :sort_by,
+     :to_ary, :to_set, :to_xml, :to_yaml].each do |method|
+      test "association delegates #{method} to Array" do
+        assert_respond_to target, method
       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)
+     :shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
+     :keep_if, :pop, :shift, :delete_at, :compact].each do |method|
+      test "#{method} is not delegated to Array" do
+        assert_raises(NoMethodError) { call_method(target, method) }
       end
     end
   end
@@ -64,36 +52,23 @@ module ActiveRecord
     fixtures :comments
 
     def target
-      Comment.where(body: 'Normal type')
+      Comment.all
     end
 
-    [:map, :collect].each do |method|
-      test "##{method} is delegated" do
-        assert_responds(target, method)
-        assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
-      end
-
-      test "##{method}! is not delegated" do
-        assert_deprecated do
-          assert_responds(target, "#{method}!")
-        end
+    [:&, :+, :[], :all?, :collect, :detect, :each, :each_cons,
+     :each_with_index, :flat_map, :group_by, :include?, :length,
+     :map, :none?, :one?, :reverse, :sample, :second, :sort, :sort_by,
+     :to_ary, :to_set, :to_xml, :to_yaml].each do |method|
+      test "relation delegates #{method} to Array" do
+        assert_respond_to target, method
       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} triggers an immutable error" do
-        assert_raises ActiveRecord::ImmutableRelation do
-          assert_responds(target, method)
-        end
+     :shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
+     :keep_if, :pop, :shift, :delete_at, :compact].each do |method|
+      test "#{method} is not delegated to Array" do
+        assert_raises(NoMethodError) { call_method(target, method) }
       end
     end
   end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 09724d9884..2ccf4c7578 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1506,26 +1506,55 @@ class RelationTest < ActiveRecord::TestCase
     assert !Post.all.respond_to?(:by_lifo)
   end
 
-  class OMGTopic < ActiveRecord::Base
-    self.table_name = 'topics'
+  test "merge collapses wheres from the LHS only" do
+    left  = Post.where(title: "omg").where(comments_count: 1)
+    right = Post.where(title: "wtf").where(title: "bbq")
 
-    def self.__omg__
-      "omgtopic"
-    end
+    expected = [left.where_values[1]] + right.where_values
+    merged   = left.merge(right)
+
+    assert_equal expected, merged.where_values
+    assert !merged.to_sql.include?("omg")
+    assert merged.to_sql.include?("wtf")
+    assert merged.to_sql.include?("bbq")
   end
 
-  test "delegations do not clash across classes" do
-    begin
-      class ::Array
-        def __omg__
-          "array"
-        end
-      end
+  def test_merging_removes_rhs_bind_parameters
+    left  = Post.where(id: Arel::Nodes::BindParam.new('?'))
+    column = Post.columns_hash['id']
+    left.bind_values += [[column, 20]]
+    right   = Post.where(id: 10)
 
-      assert_equal "array",    Topic.all.__omg__
-      assert_equal "omgtopic", OMGTopic.all.__omg__
-    ensure
-      Array.send(:remove_method, :__omg__)
-    end
+    merged = left.merge(right)
+    assert_equal [], merged.bind_values
+  end
+
+  def test_merging_keeps_lhs_bind_parameters
+    column = Post.columns_hash['id']
+    binds = [[column, 20]]
+
+    right  = Post.where(id: Arel::Nodes::BindParam.new('?'))
+    right.bind_values += binds
+    left   = Post.where(id: 10)
+
+    merged = left.merge(right)
+    assert_equal binds, merged.bind_values
+  end
+
+  def test_merging_reorders_bind_params
+    post         = Post.first
+    id_column    = Post.columns_hash['id']
+    title_column = Post.columns_hash['title']
+
+    bv = Post.connection.substitute_at id_column, 0
+
+    right  = Post.where(id: bv)
+    right.bind_values += [[id_column, post.id]]
+
+    left   = Post.where(title: bv)
+    left.bind_values += [[title_column, post.title]]
+
+    merged = left.merge(right)
+    assert_equal post, merged.first
   end
 end