Fix ActiveRecord::FinderMethods.find when passing multiple ids and primary key is not selected

[fatkodima]

Fixes #45694.

I think it is easier and better to just raise than to introduce an implicit behavior by adding a primary key to the selected values.

[xjunior]

The behavior in the bug report is what I would expect, not an exception. Selecting attributes without the primary key is a valid scenario.

Also, checking if id is "truthy" doesn't work when the primary key is something else.

[fatkodima]

Selecting attributes without the primary key is a valid scenario.

Sure, but you should not call find on it. You can get the relation from somewhere else and calling find on it will get empty results and you will be very surprised (not to mention it should take some time when this surprise will show itself), like the OP.

Also, checking if id is "truthy" doesn't work when the primary key is something else.

No, because the primary can not be nil or false.

[xjunior]

No, because the primary can not be nil or false.

I mean if the model in question doesn't use id as the primary key.

[fatkodima]

If you mean id as a column name, then calling #id method actually returns the record's primary key - https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/PrimaryKey.html#method-i-id

[xjunior]

If you mean id as a column name, then calling #id method actually returns the record's primary key - https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/PrimaryKey.html#method-i-id

that I didn't know, good info!

[simi]

Why is ordering done in Ruby world? Can't we just rewrite the logic to not use in_order_of on Array, but on Relation (https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-in_order_of) instead? That way there is no need for primary_key selection and we can guarantee the order.

[fatkodima]

We can't use it, at least currently, because, probably, not every dbms supports such ordering (but I was unable to find any by checking right now all the most popular). But at least not all activerecord adapters support it -

rails/activerecord/lib/active_record/relation/query_methods.rb

Line 523 in 408d061

.order!(connection.field_ordered_value(arel_column, values))

We can move field_ordered_value to the AbstractAdapter, because all the SQL dbms have basically the same syntax. We need to merge #45670 first, I think.

[simi]

We can move field_ordered_value to the AbstractAdapter, because all the SQL dbms have basically the same syntax. We need to merge #45670 first, I think.

Ok, let's focus on that one for now. 👀

[fatkodima]

Another reason to order in memory: find queries are very popular and the generated SQL queries are nice, clean and simple, and should perform faster too. So if to order in database, this will change. If we find 10 records, that query will became monstrous already.

[simi]

Another reason to order in memory: find queries are very popular and the generated SQL queries are nice, clean and simple, and should perform faster too. So if to order in database, this will change. If we find 10 records, that query will became monstrous already.

I'm not sure that's the main purpose of AR to create nice clean and simple queries. Check the common query generated by eager_load. AFAIK AR is in here so you don't need to care about SQL queries at all if possible.

[fschwahn]

I did a simple benchmark, and sorting in DB (using in_order_of) is noticeably slower:

require "benchmark/ips"
id_list = 100.times.map { User.create!.id }.shuffle

Benchmark.ips do |x|
  x.report("sort in ruby") { User.find(id_list) }
  x.report("sort in DB") { User.in_order_of(:id, id_list).to_a }

  x.compare!
end

Warming up --------------------------------------
        sort in ruby    80.000  i/100ms
          sort in DB    50.000  i/100ms
Calculating -------------------------------------
        sort in ruby    795.172  (± 5.2%) i/s -      4.000k in   5.044808s
          sort in DB    512.828  (± 1.8%) i/s -      2.600k in   5.071509s

Comparison:
        sort in ruby:      795.2 i/s
          sort in DB:      512.8 i/s - 1.55x  (± 0.00) slower

The effect increases when id_list becomes larger.

[casperisfine]

Ok, I looked at the issue a bit, and I don't think either of the proposed resolutions so far are really satisfactory.

Raising an explicit error while better than returning an empty array seems like giving up to me.

Ordering in DB is mentioned before may have very detrimental performance effects.

I'm wondering wether implicitly selecting the primary key when passing a list to find() wouldn't be better.

[casperisfine]

That said, it's an orthogonal concern, but I think we should probably raise an error if in_order_of returns less elements than result.

[fatkodima]

@byroot Thank you for the review!

I'm wondering wether implicitly selecting the primary key when passing a list to find() wouldn't be better.

Updated with this approach.

[byroot]

thanks!

[byroot]

Hum, actually, this mutates the current instance, I don't think we should do that. Can you do a quick followup that doesn't do this?

I think you can just do it in find_some_ordered, at the same point where where(primary_key => ids).

Sorry, I should have been more careful.