Merge pull request #39057 from kamipo/deprecate_in_clause_length

Deprecate `in_clause_length` in `DatabaseLimits`

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Deprecate `in_clause_length` in `DatabaseLimits`.
+
+    *Ryuta Kamizono*
+
 *   Fix aggregate functions to return numeric value consistently even on custom attribute type.
 
     *Ryuta Kamizono*

activerecord/lib/active_record/connection_adapters/abstract/database_limits.rb

@@ -61,6 +61,7 @@ module ActiveRecord
       def in_clause_length
         nil
       end
+      deprecate :in_clause_length
 
       # Returns the maximum length of an SQL query.
       def sql_query_length

activerecord/lib/arel/visitors/to_sql.rb

@@ -513,64 +513,33 @@ module Arel # :nodoc: all
         end
 
         def visit_Arel_Nodes_In(o, collector)
-          unless Array === o.right
-            return collect_in_clause(o.left, o.right, collector)
-          end
+          attr, values = o.left, o.right
 
-          unless o.right.empty?
-            o.right.delete_if { |value| unboundable?(value) }
-          end
-
-          return collector << "1=0" if o.right.empty?
-
-          in_clause_length = @connection.in_clause_length
-
-          if !in_clause_length || o.right.length <= in_clause_length
-            collect_in_clause(o.left, o.right, collector)
-          else
-            collector << "("
-            o.right.each_slice(in_clause_length).each_with_index do |right, i|
-              collector << " OR " unless i == 0
-              collect_in_clause(o.left, right, collector)
+          if Array === values
+            unless values.empty?
+              values.delete_if { |value| unboundable?(value) }
             end
-            collector << ")"
-          end
-        end
 
-        def collect_in_clause(left, right, collector)
-          collector = visit left, collector
-          collector << " IN ("
-          visit(right, collector) << ")"
+            return collector << "1=0" if values.empty?
+          end
+
+          visit(attr, collector) << " IN ("
+          visit(values, collector) << ")"
         end
 
         def visit_Arel_Nodes_NotIn(o, collector)
-          unless Array === o.right
-            return collect_not_in_clause(o.left, o.right, collector)
-          end
+          attr, values = o.left, o.right
 
-          unless o.right.empty?
-            o.right.delete_if { |value| unboundable?(value) }
-          end
-
-          return collector << "1=1" if o.right.empty?
-
-          in_clause_length = @connection.in_clause_length
-
-          if !in_clause_length || o.right.length <= in_clause_length
-            collect_not_in_clause(o.left, o.right, collector)
-          else
-            o.right.each_slice(in_clause_length).each_with_index do |right, i|
-              collector << " AND " unless i == 0
-              collect_not_in_clause(o.left, right, collector)
+          if Array === values
+            unless values.empty?
+              values.delete_if { |value| unboundable?(value) }
             end
-            collector
-          end
-        end
 
-        def collect_not_in_clause(left, right, collector)
-          collector = visit left, collector
-          collector << " NOT IN ("
-          visit(right, collector) << ")"
+            return collector << "1=1" if values.empty?
+          end
+
+          visit(attr, collector) << " NOT IN ("
+          visit(values, collector) << ")"
         end
 
         def visit_Arel_Nodes_And(o, collector)

activerecord/test/cases/adapter_test.rb

@@ -410,6 +410,12 @@ module ActiveRecord
     def test_joins_per_query_is_deprecated
       assert_deprecated { @connection.joins_per_query }
     end
+
+    unless current_adapter?(:OracleAdapter)
+      def test_in_clause_length_is_deprecated
+        assert_deprecated { @connection.in_clause_length }
+      end
+    end
   end
 
   class AdapterForeignKeyTest < ActiveRecord::TestCase

activerecord/test/cases/arel/support/fake_record.rb

@@ -64,10 +64,6 @@ module FakeRecord
       comment
     end
 
-    def in_clause_length
-      3
-    end
-
     def schema_cache
       self
     end

activerecord/test/cases/arel/visitors/to_sql_test.rb

@@ -395,11 +395,6 @@ module Arel
           _(compile(node)).must_be_like %{
             "users"."id" IN (1, 2, 3)
           }
-
-          node = @attr.in [1, 2, 3, 4, 5]
-          _(compile(node)).must_be_like %{
-            ("users"."id" IN (1, 2, 3) OR "users"."id" IN (4, 5))
-          }
         end
 
         it "should return 1=0 when empty right which is always false" do
@@ -550,11 +545,6 @@ module Arel
           _(compile(node)).must_be_like %{
             "users"."id" NOT IN (1, 2, 3)
           }
-
-          node = @attr.not_in [1, 2, 3, 4, 5]
-          _(compile(node)).must_be_like %{
-            "users"."id" NOT IN (1, 2, 3) AND "users"."id" NOT IN (4, 5)
-          }
         end
 
         it "should return 1=1 when empty right which is always true" do

activerecord/test/cases/associations/eager_test.rb

@@ -222,61 +222,6 @@ class EagerAssociationTest < ActiveRecord::TestCase
     end
   end
 
-  def test_preloading_has_many_in_multiple_queries_with_more_ids_than_database_can_handle
-    assert_called(Comment.connection, :in_clause_length, returns: 5) do
-      posts = Post.all.merge!(includes: :comments).to_a
-      assert_equal 11, posts.size
-    end
-  end
-
-  def test_preloading_has_many_in_one_queries_when_database_has_no_limit_on_ids_it_can_handle
-    assert_called(Comment.connection, :in_clause_length, returns: nil) do
-      posts = Post.all.merge!(includes: :comments).to_a
-      assert_equal 11, posts.size
-    end
-  end
-
-  def test_preloading_habtm_in_multiple_queries_with_more_ids_than_database_can_handle
-    assert_called(Comment.connection, :in_clause_length, times: 2, returns: 5) do
-      posts = Post.all.merge!(includes: :categories).to_a
-      assert_equal 11, posts.size
-    end
-  end
-
-  def test_preloading_habtm_in_one_queries_when_database_has_no_limit_on_ids_it_can_handle
-    assert_called(Comment.connection, :in_clause_length, times: 2, returns: nil) do
-      posts = Post.all.merge!(includes: :categories).to_a
-      assert_equal 11, posts.size
-    end
-  end
-
-  def test_load_associated_records_in_one_query_when_adapter_has_no_limit
-    assert_not_called(Comment.connection, :in_clause_length) do
-      post = posts(:welcome)
-      assert_queries(2) do
-        Post.includes(:comments).where(id: post.id).to_a
-      end
-    end
-  end
-
-  def test_load_associated_records_in_several_queries_when_many_ids_passed
-    assert_called(Comment.connection, :in_clause_length, times: 2, returns: 1) do
-      post1, post2 = posts(:welcome), posts(:thinking)
-      assert_queries(2) do
-        Post.includes(:comments).where(id: [post1.id, post2.id]).to_a
-      end
-    end
-  end
-
-  def test_load_associated_records_in_one_query_when_a_few_ids_passed
-    assert_not_called(Comment.connection, :in_clause_length) do
-      post = posts(:welcome)
-      assert_queries(2) do
-        Post.includes(:comments).where(id: post.id).to_a
-      end
-    end
-  end
-
   def test_including_duplicate_objects_from_belongs_to
     popular_post = Post.create!(title: "foo", body: "I like cars!")
     comment = popular_post.comments.create!(body: "lol")

activerecord/test/cases/relation/where_test.rb

@@ -20,13 +20,6 @@ module ActiveRecord
   class WhereTest < ActiveRecord::TestCase
     fixtures :posts, :comments, :edges, :authors, :author_addresses, :binaries, :essays, :cars, :treasures, :price_estimates, :topics
 
-    def test_in_clause_is_correctly_sliced
-      assert_called(Author.connection, :in_clause_length, returns: 1) do
-        david = authors(:david)
-        assert_equal [david], Author.where(name: "David", id: [1, 2])
-      end
-    end
-
     def test_type_casting_nested_joins
       comment = comments(:eager_other_comment1)
       assert_equal [comment], Comment.joins(post: :author).where(authors: { id: "2-foo" })