From e17e2caba6f9c4d7014f13cfb3fb636454377ef3 Mon Sep 17 00:00:00 2001 From: Pedro Baracho Date: Wed, 29 May 2024 15:43:59 -0700 Subject: [PATCH 1/2] Revert "Use Arel instead of String for AR Enumerator conditionals" --- CHANGELOG.md | 3 -- lib/job-iteration/active_record_cursor.rb | 22 +++++++----- lib/job-iteration/active_record_enumerator.rb | 10 +++--- test/test_helper.rb | 34 ------------------- test/unit/active_record_enumerator_test.rb | 9 ----- 5 files changed, 19 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15738170..bed8f7d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,9 +17,6 @@ Nil when generating position for cursor based on `:id` column (Rails 7.1 and above, where composite primary models are now supported). This ensures we grab the value of the id column, rather than a potentially composite primary key value. -- [456](https://github.com/Shopify/job-iteration/pull/431) - Use Arel to generate SQL that's type compatible for the - cursor pagination conditionals in ActiveRecord cursor. Previously, the cursor would coerce numeric ids to a string value - (e.g.: `... AND id > '1'`) ## v1.4.1 (Sep 5, 2023) diff --git a/lib/job-iteration/active_record_cursor.rb b/lib/job-iteration/active_record_cursor.rb index d60c8976..10a8f4ef 100644 --- a/lib/job-iteration/active_record_cursor.rb +++ b/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 diff --git a/lib/job-iteration/active_record_enumerator.rb b/lib/job-iteration/active_record_enumerator.rb index f21fdd91..363a4ecf 100644 --- a/lib/job-iteration/active_record_enumerator.rb +++ b/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 diff --git a/test/test_helper.rb b/test/test_helper.rb index d57bdfa1..d9733382 100644 --- a/test/test_helper.rb +++ b/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) diff --git a/test/unit/active_record_enumerator_test.rb b/test/unit/active_record_enumerator_test.rb index f87b17b8..724bbaae 100644 --- a/test/unit/active_record_enumerator_test.rb +++ b/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) From 6aa24b22538c9e48923e2f296242fb092aebdfa9 Mon Sep 17 00:00:00 2001 From: Pedro Baracho Date: Wed, 29 May 2024 15:56:08 -0700 Subject: [PATCH 2/2] Update changelog --- CHANGELOG.md | 6 ++++++ Gemfile.lock | 2 +- lib/job-iteration/version.rb | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bed8f7d3..c800af4d 100644 --- a/CHANGELOG.md +++ b/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 @@ -17,6 +20,9 @@ Nil when generating position for cursor based on `:id` column (Rails 7.1 and above, where composite primary models are now supported). This ensures we grab the value of the id column, rather than a potentially composite primary key value. +- [456](https://github.com/Shopify/job-iteration/pull/431) - Use Arel to generate SQL that's type compatible for the + cursor pagination conditionals in ActiveRecord cursor. Previously, the cursor would coerce numeric ids to a string value + (e.g.: `... AND id > '1'`) ## v1.4.1 (Sep 5, 2023) diff --git a/Gemfile.lock b/Gemfile.lock index d8d996d0..427d5bf7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -7,7 +7,7 @@ GIT PATH remote: . specs: - job-iteration (1.5.0) + job-iteration (1.5.1) activejob (>= 5.2) GEM diff --git a/lib/job-iteration/version.rb b/lib/job-iteration/version.rb index 30bc6276..6b24e622 100644 --- a/lib/job-iteration/version.rb +++ b/lib/job-iteration/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module JobIteration - VERSION = "1.5.0" + VERSION = "1.5.1" end