Merge pull request #483 from Shopify/revert-456-pb-use-arel-for-conditions

Revert "Use Arel instead of String for AR Enumerator conditionals"

Author: pedropb

CHANGELOG.md

@@ -1,6 +1,9 @@
 ### Main (unreleased)
 Nil
 
+## v1.5.1 (May 29,2024)
+- [483](https://github.com/Shopify/job-iteration/pull/483) Reverts [#456 Use Arel instead of String for AR Enumerator conditionals](https://github.com/Shopify/job-iteration/pull/456)
+
 ## v1.5.0 (May 29, 2024)
 ### Changes
 

Gemfile.lock

@@ -7,7 +7,7 @@ GIT
 PATH
   remote: .
   specs:
-    job-iteration (1.5.0)
+    job-iteration (1.5.1)
       activejob (>= 5.2)
 
 GEM

lib/job-iteration/active_record_cursor.rb

@@ -18,8 +18,12 @@ def initialize
       end
     end
 
-    def initialize(relation, columns, position = nil)
-      @columns = columns
+    def initialize(relation, columns = nil, position = nil)
+      @columns = if columns
+        Array(columns)
+      else
+        Array(relation.primary_key).map { |pk| "#{relation.table_name}.#{pk}" }
+      end
       self.position = Array.wrap(position)
       raise ArgumentError, "Must specify at least one column" if columns.empty?
       if relation.joins_values.present? && !@columns.all? { |column| column.to_s.include?(".") }
@@ -30,7 +34,7 @@ def initialize(relation, columns, position = nil)
         raise ConditionNotSupportedError
       end
 
-      @base_relation = relation.reorder(*@columns)
+      @base_relation = relation.reorder(@columns.join(","))
       @reached_end = false
     end
 
@@ -50,10 +54,12 @@ def position=(position)
 
     def update_from_record(record)
       self.position = @columns.map do |column|
-        if ActiveRecord.version >= Gem::Version.new("7.1.0.alpha") && column.name == "id"
+        method = column.to_s.split(".").last
+
+        if ActiveRecord.version >= Gem::Version.new("7.1.0.alpha") && method == "id"
           record.id_value
         else
-          record.send(column.name)
+          record.send(method.to_sym)
         end
       end
     end
@@ -83,14 +89,14 @@ def conditions
       i = @position.size - 1
       column = @columns[i]
       conditions = if @columns.size == @position.size
-        column.gt(@position[i])
+        "#{column} > ?"
       else
-        column.gteq(@position[i])
+        "#{column} >= ?"
       end
       while i > 0
         i -= 1
         column = @columns[i]
-        conditions = column.gt(@position[i]).or(column.eq(@position[i]).and(conditions))
+        conditions = "#{column} > ? OR (#{column} = ? AND (#{conditions}))"
       end
       ret = @position.reduce([conditions]) { |params, value| params << value << value }
       ret.pop

lib/job-iteration/active_record_enumerator.rb

@@ -11,9 +11,9 @@ def initialize(relation, columns: nil, batch_size: 100, cursor: nil)
       @relation = relation
       @batch_size = batch_size
       @columns = if columns
-        Array(columns).map { |col| relation.arel_table[col.to_sym] }
+        Array(columns)
       else
-        Array(relation.primary_key).map { |pk| relation.arel_table[pk.to_sym] }
+        Array(relation.primary_key).map { |pk| "#{relation.table_name}.#{pk}" }
       end
       @cursor = cursor
     end
@@ -45,7 +45,7 @@ def size
 
     def cursor_value(record)
       positions = @columns.map do |column|
-        attribute_name = column.name.to_sym
+        attribute_name = column.to_s.split(".").last
         column_value(record, attribute_name)
       end
       return positions.first if positions.size == 1
@@ -58,8 +58,8 @@ def finder_cursor
     end
 
     def column_value(record, attribute)
-      value = record.read_attribute(attribute)
-      case record.class.columns_hash.fetch(attribute.to_s).type
+      value = record.read_attribute(attribute.to_sym)
+      case record.class.columns_hash.fetch(attribute).type
       when :datetime
         value.strftime(SQL_DATETIME_WITH_NSEC)
       else

lib/job-iteration/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module JobIteration
-  VERSION = "1.5.0"
+  VERSION = "1.5.1"
 end

test/test_helper.rb

@@ -106,40 +106,6 @@ def assert_logged(message)
   end
 end
 
-module ActiveRecordHelpers
-  def assert_sql(*patterns_to_match, &block)
-    captured_queries = []
-    assert_nothing_raised do
-      ActiveSupport::Notifications.subscribed(
-        ->(_name, _start_time, _end_time, _subscriber_id, payload) { captured_queries << payload[:sql] },
-        "sql.active_record",
-        &block
-      )
-    end
-
-    failed_patterns = []
-    patterns_to_match.each do |pattern|
-      failed_check = captured_queries.none? do |sql|
-        case pattern
-        when Regexp
-          sql.match?(pattern)
-        when String
-          sql == pattern
-        else
-          raise ArgumentError, "#assert_sql encountered an unknown matcher #{pattern.inspect}"
-        end
-      end
-      failed_patterns << pattern if failed_check
-    end
-    queries = captured_queries.empty? ? "" : "\nQueries:\n  #{captured_queries.join("\n  ")}"
-    assert_predicate(
-      failed_patterns,
-      :empty?,
-      "Query pattern(s) #{failed_patterns.map(&:inspect).join(", ")} not found.#{queries}",
-    )
-  end
-end
-
 JobIteration.logger = Logger.new(IO::NULL)
 ActiveJob::Base.logger = Logger.new(IO::NULL)
 

test/unit/active_record_enumerator_test.rb

@@ -4,8 +4,6 @@
 
 module JobIteration
   class ActiveRecordEnumeratorTest < IterationUnitTest
-    include ActiveRecordHelpers
-
     SQL_TIME_FORMAT = "%Y-%m-%d %H:%M:%S.%N"
     test "#records yields every record with their cursor position" do
       enum = build_enumerator.records
@@ -135,13 +133,6 @@ class ActiveRecordEnumeratorTest < IterationUnitTest
       end
     end
 
-    test "enumerator paginates using integer conditionals for primary key when no columns are defined" do
-      enum = build_enumerator(relation: Product.all, batch_size: 1).records
-      assert_sql(/`products`\.`id` > 1/) do
-        enum.take(2)
-      end
-    end
-
     private
 
     def build_enumerator(relation: Product.all, batch_size: 2, columns: nil, cursor: nil)