Limit number of expressions in a list during a "homogenous in" operation (#2013)

* Limit number of expressions in a list during a "homogenous in" operation

- ActiveRecord version 6.1.0.alpha
- This fixes the following test failure:
  OracleEnhancedAdapter eager loading should load included association with more than 1000 records
- The solution was to implement visit_Arel_Nodes_HomogeneousIn
- See rails/rails@72fd0ba for the "homogenous in" explanation

* Fix typo

* Code formatting to placate Rubocop.

* Bug fix: String#<< is an assignment operator, not a simple concatenation

* Again, style changes to appease RuboCop.

* Create a new test to try >1000 items in non-homogenous lists.

* More style tips, sigh.

Co-authored-by: James Nakagawa <[email protected]>

Author: jamesnakagawa

lib/arel/visitors/oracle.rb

@@ -86,6 +86,48 @@ def visit_Arel_Nodes_Except(o, collector)
           collector << " )"
         end
 
+        ##
+        # To avoid ORA-01795: maximum number of expressions in a list is 1000
+        # tell ActiveRecord to limit us to 1000 ids at a time
+        def visit_Arel_Nodes_HomogeneousIn(o, collector)
+          in_clause_length = @connection.in_clause_length
+          values = o.casted_values.map { |v| @connection.quote(v) }
+          column_name = quote_table_name(o.table_name) + "." + quote_column_name(o.column_name)
+          operator =
+            if o.type == :in
+              "IN ("
+            else
+              "NOT IN ("
+            end
+
+          if !Array === values || values.length <= in_clause_length
+            collector << column_name
+            collector << operator
+
+            expr =
+              if values.empty?
+                @connection.quote(nil)
+              else
+                values.join(",")
+              end
+
+            collector << expr
+            collector << ")"
+          else
+            collector << "("
+            values.each_slice(in_clause_length).each_with_index do |valuez, i|
+              collector << " OR " unless i == 0
+              collector << column_name
+              collector << operator
+              collector << valuez.join(",")
+              collector << ")"
+            end
+            collector << ")"
+          end
+
+          collector
+        end
+
         def visit_Arel_Nodes_In(o, collector)
           attr, values = o.left, o.right
 

lib/arel/visitors/oracle12.rb

@@ -39,6 +39,48 @@ def visit_Arel_Nodes_Except(o, collector)
           collector << " )"
         end
 
+        ##
+        # To avoid ORA-01795: maximum number of expressions in a list is 1000
+        # tell ActiveRecord to limit us to 1000 ids at a time
+        def visit_Arel_Nodes_HomogeneousIn(o, collector)
+          in_clause_length = @connection.in_clause_length
+          values = o.casted_values.map { |v| @connection.quote(v) }
+          column_name = quote_table_name(o.table_name) + "." + quote_column_name(o.column_name)
+          operator =
+            if o.type == :in
+              "IN ("
+            else
+              "NOT IN ("
+            end
+
+          if !Array === values || values.length <= in_clause_length
+            collector << column_name
+            collector << operator
+
+            expr =
+              if values.empty?
+                @connection.quote(nil)
+              else
+                values.join(",")
+              end
+
+            collector << expr
+            collector << ")"
+          else
+            collector << "("
+            values.each_slice(in_clause_length).each_with_index do |valuez, i|
+              collector << " OR " unless i == 0
+              collector << column_name
+              collector << operator
+              collector << valuez.join(",")
+              collector << ")"
+            end
+            collector << ")"
+          end
+
+          collector
+        end
+
         def visit_Arel_Nodes_In(o, collector)
           attr, values = o.left, o.right
 

spec/active_record/connection_adapters/oracle_enhanced/connection_spec.rb

@@ -81,13 +81,13 @@
       expect(ActiveRecord::Base.connection).to be_active
     end
 
-    it "should swith to specified schema" do
+    it "should switch to specified schema" do
       ActiveRecord::Base.establish_connection(CONNECTION_WITH_SCHEMA_PARAMS)
       expect(ActiveRecord::Base.connection.current_schema).to eq(CONNECTION_WITH_SCHEMA_PARAMS[:schema].upcase)
       expect(ActiveRecord::Base.connection.current_user).to eq(CONNECTION_WITH_SCHEMA_PARAMS[:username].upcase)
     end
 
-    it "should swith to specified schema after reset" do
+    it "should switch to specified schema after reset" do
       ActiveRecord::Base.connection.reset!
       expect(ActiveRecord::Base.connection.current_schema).to eq(CONNECTION_WITH_SCHEMA_PARAMS[:schema].upcase)
     end

spec/active_record/connection_adapters/oracle_enhanced_adapter_spec.rb

@@ -193,6 +193,46 @@ class ::TestComment < ActiveRecord::Base
     end
   end
 
+  describe "lists" do
+    before(:all) do
+      schema_define do
+        create_table :test_posts do |t|
+          t.string :title
+        end
+      end
+      class ::TestPost < ActiveRecord::Base
+        has_many :test_comments
+      end
+      @ids = (1..1010).to_a
+      TestPost.transaction do
+        @ids.each do |id|
+          TestPost.create!(id: id, title: "Title #{id}")
+        end
+      end
+    end
+
+    after(:all) do
+      schema_define do
+        drop_table :test_posts
+      end
+      Object.send(:remove_const, "TestPost")
+      ActiveRecord::Base.clear_cache!
+    end
+
+    ##
+    # See this GitHub issue for an explanation of homogenous lists.
+    # https://github.com/rails/rails/commit/72fd0bae5948c1169411941aeea6fef4c58f34a9
+    it "should allow more than 1000 items in a list where the list is homogenous" do
+      posts = TestPost.where(id: @ids).to_a
+      expect(posts.size).to eq(@ids.size)
+    end
+
+    it "should allow more than 1000 items in a list where the list is non-homogenous" do
+      posts = TestPost.where(id: [*@ids, nil]).to_a
+      expect(posts.size).to eq(@ids.size)
+    end
+  end
+
   describe "with statement pool" do
     before(:all) do
       ActiveRecord::Base.establish_connection(CONNECTION_PARAMS.merge(statement_limit: 3))