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

Create a whitelist of methods to be delegated to Array.

2 files changed, 79 insertions, 75 deletions

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