Fix oracle for Rails 7.1 (#4296)

Remove redundant monkey patches.
Add necessary patches.
Clarify comments so we know when we can remove them.

Author: akostadinov

Gemfile

@@ -111,7 +111,6 @@ gem 'svg-graph', require: false
 gem 'swagger-ui_rails', git: 'https://github.com/3scale/swagger-ui_rails.git', branch: 'dev'
 gem 'swagger-ui_rails2', git: 'https://github.com/3scale/swagger-ui_rails.git', branch: 'dev-2.1.3'
 gem 'thinking-sphinx', '~> 5.6.0'
-gem 'ts-datetime-delta', require: 'thinking_sphinx/deltas/datetime_delta'
 gem 'will_paginate', '~> 3.3'
 gem 'zip-zip', require: false
 

Gemfile.lock

@@ -938,8 +938,6 @@ GEM
     tilt (2.0.11)
     timeout (0.4.3)
     tomlrb (2.0.3)
-    ts-datetime-delta (2.0.2)
-      thinking-sphinx (>= 1.3.8)
     ttfunk (1.7.0)
     tty-color (0.6.0)
     tty-cursor (0.7.1)
@@ -1161,7 +1159,6 @@ DEPENDENCIES
   swagger-ui_rails!
   swagger-ui_rails2!
   thinking-sphinx (~> 5.6.0)
-  ts-datetime-delta
   uglifier
   unicorn
   unicorn-rails

config/initializers/oracle.rb

@@ -2,6 +2,14 @@
 
 ActiveSupport.on_load(:active_record) do
   if System::Database.oracle?
+    ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter.class_eval do
+      # https://github.com/rsim/oracle-enhanced/pull/2573
+      # Perhaps will end-up into adapter version 8.1.4+ but double check
+      def supports_fetch_first_n_rows_and_offset?
+        true
+      end
+    end
+
     require 'arel/visitors/oracle12_hack' || next # once done, we can skip setup
 
     # in 6.0.6 automatic detection of max identifier length was introduced
@@ -41,7 +49,15 @@ def close_and_clear_statements
       end
     end
 
+    # https://github.com/rsim/oracle-enhanced/pull/2485 enables callbacks
+    #   which are basically redundant for use case we don't have.
+    # Version 7.1 has only the `update` callbacks so the `create` ones
+    #   need to be uncommented by probably 7.2 upgrade.
+    # If you uncomment them and things don't fail, then we are good.
     ActiveRecord::Base.skip_callback(:update, :after, :enhanced_write_lobs)
+    # ActiveRecord::Base.skip_callback(:create, :after, :enhanced_write_lobs)
+    ActiveRecord::Base.skip_callback(:update, :before, :record_changed_lobs)
+    # ActiveRecord::Base.skip_callback(:create, :before, :record_lobs_for_create)
 
     # For more information see https://github.com/rsim/oracle-enhanced/pull/2483
     module OracleEnhancedSmartQuoting
@@ -159,6 +175,9 @@ def add_column(table_name, column_name, type, **options)
       # ar_object.with_lock doesn't work OOB on oracle, see https://github.com/rsim/oracle-enhanced/issues/2237
       # A workaround is to avoid using FETCH FIRST when reloading an object by primary key.
       # https://github.com/rails/rails/blob/v7.1.5.1/activerecord/lib/active_record/relation/finder_methods.rb#L506
+      # Might be fixed in 8.1.4+ with
+      #   https://github.com/rsim/oracle-enhanced/pull/2573 and
+      #   https://github.com/rsim/oracle-enhanced/pull/2693
       def find_one(id)
         if ActiveRecord::Base === id
           raise ArgumentError, <<-MSG.squish
@@ -184,153 +203,6 @@ def find_one(id)
       end
     end)
 
-    BabySqueel::Nodes::Attribute.prepend(Module.new do
-      # those relations are used in subqueries and oracle does not support ORDER in subqueries
-      private def sanitize_relation(rel)
-        super rel.unscope(:order)
-      end
-    end)
-
-    ThinkingSphinx::ActiveRecord::SQLSource.prepend(Module.new do
-      # If the Adapter is Oracle then we forcibly use ODBC client
-      def type
-        @type ||= case adapter
-                  when ThinkingSphinx::ActiveRecord::DatabaseAdapters::OracleAdapter
-                    'odbc'
-                  else
-                    super
-                  end
-      end
-    end)
-
-    ThinkingSphinx::ActiveRecord::DatabaseAdapters.module_eval do
-      class << self
-        prepend(Module.new do
-          # https://github.com/pat/thinking-sphinx/blob/v3.2.0/lib/thinking_sphinx/active_record/database_adapters.rb#L35-L45
-          # Adding a new DatabaseAdapters for ThinkingSphinx
-          # This adds support for Oracle. OracleAdapter is responsible of query generation for Sphinx
-          def adapter_for(model)
-            return default.new(model) if default
-
-            adapter = adapter_type_for(model)
-            klass = case adapter
-                    when :oracle
-                      ThinkingSphinx::ActiveRecord::DatabaseAdapters::OracleAdapter
-                    else
-                      super
-                    end
-            klass.new model
-          end
-
-          # https://github.com/pat/thinking-sphinx/blob/v3.2.0/lib/thinking_sphinx/active_record/database_adapters.rb#L21-L33
-          # This method is only needed for `adapter_for`
-          # Part of freedom patch for ThinkingSphinx handling with Oracle
-          def adapter_type_for(model)
-            class_name = model.connection.class.name
-            case class_name.split('::').last
-            when /oracle/i
-              :oracle
-            else
-              super
-            end
-          end
-        end
-        )
-      end
-    end
-
-    ThinkingSphinx::Deltas::DatetimeDelta.prepend(Module.new do
-      # https://github.com/pat/ts-datetime-delta/blob/v2.0.2/lib/thinking_sphinx/deltas/datetime_delta.rb#L167
-      # A SQL condition (as part of the WHERE clause) that limits the result set to
-      # just the delta data, or all data, depending on whether the toggled argument
-      # is true or not. For datetime deltas, the former value is a check on the
-      # delta column being within the threshold. In the latter's case, no condition
-      # is needed, so nil is returned.
-      def clause(*args)
-        model = (args.length >= 2 ? args[0] : nil)
-        is_delta = (args.length >= 2 ? args[1] : args[0]) || false
-
-        table_name = (model.nil? ? adapter.quoted_table_name : model.quoted_table_name)
-        column_name = (model.nil? ? adapter.quote(@column.to_s) : model.connection.quote_column_name(@column.to_s))
-
-        if is_delta
-          if adapter.class.name.downcase[/oracle/]
-            "EXTRACT( day from ((#{table_name}.#{column_name} - SYSDATE) * 60 * 60 * 24)) + #{@threshold} > 0"
-          else
-            super
-          end
-        else
-          nil
-        end
-      end
-    end)
-
-    ThinkingSphinx::ActiveRecord::SQLSource.prepend(Module.new do
-      # This autogenerate the odbc_dsn string based on database.yml
-      # For now it is only particular to our project, later we could extract it and make a PR to the upstream project
-      def set_database_settings(settings)
-        super
-        conf = System::Database.database_config.configuration_hash
-        if type == 'odbc'
-          @odbc_dsn ||= "DSN=oracle;Driver={Oracle-Driver};Dbq=#{conf[:host]}:#{conf[:port] || 1521}/#{conf[:database]};Uid=#{conf[:username]};Pwd=#{conf[:password]}"
-        end
-      end
-    end)
-
-    ActiveRecord::ConnectionAdapters::OracleEnhanced::SchemaStatements.module_eval do
-      def add_index(table_name, column_name, **options) #:nodoc:
-        # All this code is exactly the same as the original except the line of the ALTER TABLE, which adds an additional USING INDEX #{quote_column_name(index_name)}
-        # The reason of this is otherwise it picks the first index that finds that contains that column name, even if it is shared with other columns and it is not unique.
-        # upstreamed: https://github.com/rsim/oracle-enhanced/pull/2293
-        index_name, index_type, quoted_column_names, tablespace, index_options = add_index_options(table_name, column_name, **options)
-        quoted_table_name = quote_table_name(table_name)
-        quoted_column_name = quote_column_name(index_name)
-        execute "CREATE #{index_type} INDEX #{quoted_column_name} ON #{quoted_table_name} (#{quoted_column_names})#{tablespace} #{index_options}"
-        if index_type == 'UNIQUE' && quoted_column_names !~ /\(.*\)/
-          execute "ALTER TABLE #{quoted_table_name} ADD CONSTRAINT #{quoted_column_name} #{index_type} (#{quoted_column_names}) USING INDEX #{quoted_column_name}"
-        end
-      end
-
-      def distinct_relation_for_primary_key(relation) # :nodoc:
-        primary_key_columns = Array(relation.primary_key).map do |column|
-          visitor.compile(relation.table[column])
-        end
-
-        values = columns_for_distinct(
-          primary_key_columns,
-          relation.order_values
-        )
-
-        limited = relation.reselect(values).distinct!
-
-        # The original code in https://github.com/rails/rails/blob/v7.1.5.1/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L1404-L1406
-        # ----
-        # limited_ids = select_rows(limited.arel, "SQL").map do |results|
-        #   results.last(Array(relation.primary_key).length) # ignores order values for MySQL and PostgreSQL
-        # end
-        # ----
-        # The change is needed because in Oracle, because otherwise the resulting `limited_ids` array would be wrong. For example,
-        # for a ServiceContract model that has the primary key "id" and a query with ordering and filtering you may get:
-        # 1. `limited` variable can be something like: #<ActiveRecord::Relation [#<ServiceContract alias_0__: "live", id: 4, raw_rnum_: 1>]>
-        # 2. `select_rows` would then return [["live",4,1]]
-        # 3. `limited_ids` calculation will result in [[1]], which is an invalid IDs list (the expected is [[4]])
-        # The updated code doesn't make assumptions about how many columns are selected or their order, but fetches the values according
-        # to the primary key column names
-        limited_ids = select_all(limited.arel, "SQL").to_ary.map do |result|
-          result.values_at(*Array(relation.primary_key))
-        end
-
-        if limited_ids.empty?
-          relation.none!
-        else
-          relation.where!(**Array(relation.primary_key).zip(limited_ids.transpose).to_h)
-        end
-
-        relation.limit_value = relation.offset_value = nil
-        relation
-      end
-    end
-
     # see https://github.com/rsim/oracle-enhanced/issues/2276
     module OracleEnhancedAdapterSchemaIssue2276
       def column_definitions(table_name)

lib/arel/visitors/oracle12_hack.rb

@@ -3,69 +3,64 @@
 module Arel
   module Visitors
     Oracle12.class_eval do
-      # we need to strip ORDER from subqueries because Oracle does not support it
-      def strip_order_from_select(o)
-        case (node = o.right)
-        when Arel::Nodes::SelectStatement
-          node.orders = []
-        end
-      end
-
-      def visit_Arel_Nodes_In(o, collector)
-        strip_order_from_select(o)
-
-        super
-      end
-
-      def visit_Arel_Nodes_NotIn(o, collector)
-        strip_order_from_select(o)
-
-        super
-      end
-
       # Another wonderful piece.
       # Oracle can't compare CLOB columns with standard SQL operators for comparison.
       # We need to replace standard equality for text/binary columns to use DBMS_LOB.COMPARE function.
       # Fixes ORA-00932: inconsistent datatypes: expected - got CLOB
       # remove when https://github.com/rsim/oracle-enhanced/pull/2415 is merged
       def visit_Arel_Nodes_Equality(o, collector)
+        left = o.left
         right = o.right
 
-        return super if right.nil?
+        return super unless right && %i(text binary).include?(cached_column_for(left)&.type)
 
-        case (left = o.left)
-        when Arel::Attributes::Attribute
-          table = left.relation.table_name
-          schema_cache = @connection.schema_cache
+        comparator = Arel::Nodes::NamedFunction.new("DBMS_LOB.COMPARE", [left, right])
+        collector = visit comparator, collector
+        collector << " = 0"
+        collector
+      end
 
-          return super unless schema_cache.data_source_exists?(table)
+      # remove once fixed in https://github.com/rsim/oracle-enhanced/pull/2573 (8.1.4+)
+      # Note that I'm not fully happy with this as the other visitor is missing potential
+      # patches that we have in this visitor. But limit+lock should be rare and simpler.
+      # An example failing operation without this is ProxyRule#move_to_top
+      def visit_Arel_Nodes_SelectStatement(o, collector)
+        if o.limit && o.lock
+          @oracle11_visitor ||= Arel::Visitors::Oracle.new(@connection)
+          return @oracle11_visitor.accept(o.dup, collector)
+        end
+        super
+      end
 
-          column = schema_cache.columns_hash(table)[left.name.to_s]
+      # can remove both after upgrade to a version with (8.1.4+ perhaps)
+      # https://github.com/rsim/oracle-enhanced/pull/2654
+      def visit_Arel_Nodes_In(o, collector)
+        attr, values = o.left, o.right
+        return super unless values.is_a?(Array)
 
-          case column.type
-          when :text, :binary
-            # https://docs.oracle.com/cd/B19306_01/appdev.102/b14258/d_lob.htm#i1016668
-            # returns 0 when the comparison succeeds
-            comparator = Arel::Nodes::NamedFunction.new('DBMS_LOB.COMPARE', [left, right])
-            collector = visit comparator, collector
-            collector << ' = 0'
-            collector
-          else
-            super
-          end
-        else
-          super
+        in_clause_length = @connection.in_clause_length
+        return super if values.length <= in_clause_length
+
+        # Split into multiple IN nodes and combine with OR
+        in_nodes = values.each_slice(in_clause_length).map do |slice|
+          Arel::Nodes::In.new(attr, slice)
         end
+        or_node = in_nodes.reduce { |left, right| Arel::Nodes::Or.new(left, right) }
+        visit(Arel::Nodes::Grouping.new(or_node), collector)
       end
 
-      # remove when addressed: https://github.com/rsim/oracle-enhanced/pull/2247 - included in v7.1.0
-      def visit_Arel_Nodes_Matches o, collector
-        if !o.case_sensitive && o.left && o.right
-          o.left = Arel::Nodes::NamedFunction.new('UPPER', [o.left])
-          o.right = Arel::Nodes::NamedFunction.new('UPPER', [o.right])
-        end
+      def visit_Arel_Nodes_NotIn(o, collector)
+        attr, values = o.left, o.right
+        return super unless values.is_a?(Array)
 
-        super o, collector
+        in_clause_length = @connection.in_clause_length
+        return super if values.length <= in_clause_length
+
+        # Split into multiple NOT IN nodes and combine with AND
+        not_in_nodes = values.each_slice(in_clause_length).map do |slice|
+          Arel::Nodes::NotIn.new(attr, slice)
+        end
+        visit(Arel::Nodes::And.new(not_in_nodes), collector)
       end
     end
   end

test/unit/oracle_hacks_test.rb

@@ -1,8 +1,57 @@
+# frozen_string_literal: true
+
 require 'test_helper'
 
 class OracleHacksTest < ActiveSupport::TestCase
-  test "using #with_lock block" do
-    object = FactoryBot.create(:audit)
-    assert( object.with_lock { :ok } )
+  setup do
+    skip "Oracle-specific tests" unless System::Database.oracle?
+  end
+
+  class ArelTest < OracleHacksTest
+    test "using #with_lock block" do
+      object = FactoryBot.create(:audit)
+      assert( object.with_lock { :ok } )
+    end
+  end
+
+  class CollectionsTest < OracleHacksTest
+    setup do
+      messages = FactoryBot.create_list(:message, 1010, sender: master_account)
+      @ids = messages.map(&:id)
+    end
+
+    test "should allow more than 1000 items using Arel::Nodes::In" do
+      table = Message.arel_table
+      in_node = Arel::Nodes::In.new(table[:id], @ids)
+      query = table.where(in_node).project(Arel.star)
+
+      sql = query.to_sql
+      messages = Message.connection.select_all(sql).to_a
+
+      assert_equal @ids.size, messages.size, "Expected to retrieve all #{@ids.size} messages"
+
+      # SQL contains multiple IN clauses (split due to 1000 limit)
+      in_clause_count = sql.scan("IN (").size
+      assert in_clause_count > 1, "Expected multiple IN clauses due to Oracle's 1000 item limit, but found #{in_clause_count}"
+    end
+
+    test "should allow more than 1000 items using raw Arel::Nodes::NotIn" do
+      ids = @ids.dup
+      non_not_in = ids.pop
+
+      table = Message.arel_table
+      not_in_node = Arel::Nodes::NotIn.new(table[:id], ids)
+      query = table.where(not_in_node).project(Arel.star)
+
+      sql = query.to_sql
+      messages = Message.connection.select_all(sql).to_a
+
+      assert_equal 1, messages.size, "Expected to retrieve exactly 1 message"
+      assert_equal non_not_in, messages.first["id"], "Expected to retrieve the message with id #{non_not_in}"
+
+      # SQL contains multiple NOT IN clauses (split due to 1000 limit)
+      not_in_clause_count = sql.scan("NOT IN (").size
+      assert not_in_clause_count > 1, "Expected multiple NOT IN clauses due to Oracle's 1000 item limit, but found #{not_in_clause_count}"
+    end
   end
 end