Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix find_in_batches against string IDs when start option is not specified #8073

Merged
merged 1 commit into from

2 participants

@alexisbernard

Hi,

Here is a fix for find_in_batches when Model#id is a string. In fact when calling find_in_batches (or find_each) without the start option it generates a wrong SQL statement:

Subscriber.find_each { |subscriber| ...  }
# Generates SQL:
# SELECT  "subscribers".* FROM "subscribers"  WHERE ("subscribers"."nick" >= 0) ...

That statement is invalid with strict databases such as PostgreSQL, because it tries to compare a string against an integer.

With more tolerant databases such as SQLite it's passing, but in fact it skips IDs starting with a lower character than '0'.

Once accepted and merged, I plan to backport that fix in pull request #7987 for rails 3.2.

cc @spastorino

@carlosantoniodasilva carlosantoniodasilva commented on the diff
activerecord/test/cases/batches_test.rb
@@ -136,4 +136,13 @@ def test_find_in_batches_should_use_any_column_as_primary_key
assert_equal nick_order_subscribers[1..-1].map(&:id), subscribers.map(&:id)
end
+
+ def test_find_in_batches_should_use_any_column_as_primary_key_when_start_is_not_specified
+ Subscriber.count('nick') # preheat arel's table cache
+ assert_queries(Subscriber.count + 1) do
+ Subscriber.find_each(:batch_size => 1) do |subscriber|
+ assert_kind_of Subscriber, subscriber
+ end
+ end
+ end

Wrong indent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva

It needs a changelog entry as well. thanks.

@alexisbernard

Thanks for pointing those details.

activerecord/test/cases/batches_test.rb
@@ -136,4 +136,13 @@ def test_find_in_batches_should_use_any_column_as_primary_key
assert_equal nick_order_subscribers[1..-1].map(&:id), subscribers.map(&:id)
end
+
+ def test_find_in_batches_should_use_any_column_as_primary_key_when_start_is_not_specified

This line is still bad indented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva

This will need a rebase since github says it cannot be automatically merged anymore, and please squash your commits. Thanks!

@alexisbernard

Sorry for the poor indentation. I just rebased master into that branch and squashed my commits :)

@carlosantoniodasilva carlosantoniodasilva merged commit 92bae49 into from
@alexisbernard

Awesome, thanks for the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 31, 2012
This page is out of date. Refresh to see the latest.
View
4 activerecord/CHANGELOG.md
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* Fix `find_in_batches` crashing when IDs are strings and start option is not specified.
+
+ *Alexis Bernard*
+
* `AR::Base#attributes_before_type_cast` now returns unserialized values for serialized attributes.
*Nikita Afanasenko*
View
4 activerecord/lib/active_record/relation/batches.rb
@@ -62,11 +62,11 @@ def find_in_batches(options = {})
ActiveRecord::Base.logger.warn("Scoped order and limit are ignored, it's forced to be batch order and batch size")
end
- start = options.delete(:start) || 0
+ start = options.delete(:start)
batch_size = options.delete(:batch_size) || 1000
relation = relation.reorder(batch_order).limit(batch_size)
- records = relation.where(table[primary_key].gteq(start)).to_a
+ records = start ? relation.where(table[primary_key].gteq(start)).to_a : relation.to_a
while records.any?
records_size = records.size
View
9 activerecord/test/cases/batches_test.rb
@@ -136,4 +136,13 @@ def test_find_in_batches_should_use_any_column_as_primary_key
assert_equal nick_order_subscribers[1..-1].map(&:id), subscribers.map(&:id)
end
+
+ def test_find_in_batches_should_use_any_column_as_primary_key_when_start_is_not_specified
+ Subscriber.count('nick') # preheat arel's table cache
+ assert_queries(Subscriber.count + 1) do
+ Subscriber.find_each(:batch_size => 1) do |subscriber|
+ assert_kind_of Subscriber, subscriber
+ end
+ end
+ end

Wrong indent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
Something went wrong with that request. Please try again.