Fix: ActiveRecord::Calculations#ids returns duplicate ids

[joshuay03]

Motivation / Background

This Pull Request has been created to address #46455.

Detail

This Pull Request changes ActiveRecord::Calculations#ids to only return the unique ids of the base model, ignoring the join on eager loaded, preloaded and included associations.

The cause for this was the join dependancy applied within #pluck when the relation contained preloads. #ids has been updated to query the relation's foreign key itself rather than relying on #pluck unless the records have already been loaded.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
• CI is passing.

[adrianna-chang-shopify]

👋 Hey Joshua, thanks for picking this up!

Relying on the base relation makes sense to me, but I think #ids should still rely on #pluck, instead of extracting the pluck logic to #perform_pluck and calling that directly from #ids. #pluck avoids making queries if the relation is already loaded:

def pluck(*column_names)
  if loaded? && all_attributes?(column_names)
    result = records.pluck(*column_names)
    if @async
      return Promise::Complete.new(result)
    else
      return result
    end
  end
...
end

So if we jump straight to the #perform_pluck logic you've extracted when #ids is called, we will always perform a query. What do you think about still using #pluck in #ids, but scoping to the base relation there, ie.

def ids
  klass.pluck(primary_key)
end

?

[joshuay03]

Thanks for the review! @adrianna-chang-shopify

So if we jump straight to the #perform_pluck logic you've extracted when #ids is called, we will always perform a query. What do you think about still using #pluck in #ids, but scoping to the base relation there, ie.

This makes way more sense. I'm still getting used to how AR base classes are structured so thanks for the suggestion which I've now actioned.

[adrianna-chang-shopify]

No problem! Test failure looks legit though, let me take a look 🤔

Ah, it's just the assertion that's wrong now that we're deduplicating ids, so 👍

[adrianna-chang-shopify]

Ah, yes scoping would be a problem 🤦‍♀️ Is there a reason not to do pluck(primary_key).uniq? That seems simplest..

[casperisfine]

Is there a reason not to do pluck(primary_key).uniq? That seems simplest..

That is doing the deduplication on the Ruby side, that isn't ideal. Something like SELECT DISTINCT <pk> might be a solution, but not 100% sure I haven't dug much in the original issue yet.

[adrianna-chang-shopify]

Ah, yeah that makes sense to me 👍 distinct.pluck(primary_key) gives us what we want, right? Although we're still constructing the join dependency unnecessarily at that point, so perhaps it makes more sense to inline the query inside #ids instead of relying on #pluck..?

[joshuay03]

so perhaps it makes more sense to inline the query inside #ids instead of relying on #pluck..?

I agree @adrianna-chang-shopify.
I've updated #ids to pluck if records have been preloaded, and select the foreign key otherwise, handling where contradictions and async just like #pluck and #pick. Changes and additional test coverage visible in these comparisons:

• https://github.com/rails/rails/compare/f962068a6eb37581682f94edf76b1dbaad773ea8..307816734ee0122380fdc8a74156c82872501193
• https://github.com/rails/rails/compare/de0c9f2bf8805f663f8a8e1f01863f29f308e111..8fd336927a5d7519adac4361c47aa09a295bfe3d

I'm happy to drop async_ids and move to a separate PR if required.

[byroot]

Nice work! Thank you.

[simi]

🤔 this adds async_ids as well, which is missing in commit message and changelog. Is that on purpose (to not promote async methods yet)?

[matthewd]

I'm not sure this is something we should / need to change. But if we're going to, I think we need to take some more inspiration from the existing pluck tests, as the new implementation is no longer covered by them:

  def test_ids_if_table_included
    c = Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)])
    assert_equal [c.id], Company.includes(:contracts).where("contracts.id" => c.contracts.first).ids
  end

[joshuay03]

I think we need to take some more inspiration from the existing pluck tests, as the new implementation is no longer covered by them.

@matthewd I'll get a separate PR up with improved test coverage.

Update: Got this PR up: #46697.