Allow ActiveRecord's find to take a Range

This line doesn't look right at all:

ids.inject([]) {|m, o| m << (o.kind_of?(Range) ? o.to_a : o)}.flatten.compact.uniq

And why special-case Ranges? Why not accept anything that has a to_a method?

We could almost do

ids = ids.map(&:to_a).flatten.compact.uniq

The problem is that under 1.9, Fixnum doesn't have a to_a. So the code is more about special-casing Fixnum. Not clear to me that picking up everything that has to_a is the right answer - we need a specific thing, an array of integers, no?

@Mislav - Thanks for the feedback. I guess it would be better to handle anything that has a to_a:

ids.inject([]) {|m,o| m << (o.respond_to? :to_a ? o.to_a : o)}.flatten.compact.uniq

Other than the special case for Ranges what else doesn't look right about the line?

in 1.9 we still get;

Array(2)    # => [2]
Array(4..6) # => [4, 5, 6]

ids.map{|id| Array(id)}.flatten.compact.uniq

@MatthewRudy - that's much nicer. Thanks!

I've attached an updated patch with Matthew's changes.

This seems like a bad idea - what about people who are using non-numeric primary keys (GUIDs, for instance)?

I'm also not clear on what the use case for this is - is it really that much more effort to to_a the range yourself?

@Matt Jones - Thanks for the feedback.

This seems like a bad idea - what about people who are using non-numeric primary keys (GUIDs, for instance)?

You're correct, this is probably not very useful for people using non-numeric primary keys. I guess my only answer is that they shouldn't use Ranges in their finds. Adding this won't change SOP for those using non-numeric primary keys and will only be a benefit to those who stick with the Rails conventions.

I'm also not clear on what the use case for this is - is it really that much more effort to to_a the range yourself?

That's pretty much what I do now >> Person.find(1, (4..7).to_a) and I thought that if I'm a using a Range in my find then someone else may find it useful also. But the case may be that this does not belong in Core and is only a very specific use case.

+1 verified

I have attached a patch for Rails 3 as well.

Uses Array() now.

Need to use Array.wrap() since Array() complains about destructuring strings on 1.8.8. (require 'active_support/core_ext/array/wrap)

Also, is flatten needed? That could be

ids.sum { |id| Array.wrap(id) }.uniq

Updated the 2.3x patch with Jeremy's recommendations.

Mike, tests fail on this one, you will need to change it like this:

ids = ids.map {|id| Array.wrap(id)}.flatten.compact.uniq

Reading back up, this means find(1, 5..10000) will generate a query with 9996 ids. This should use normal range query mapping instead.