rails icon indicating copy to clipboard operation
rails copied to clipboard

`ActiveRecord::Associations::Preloader` ignores `available_records` when association has a scope

Open tbuehlmann opened this issue 1 year ago • 0 comments

When preloading a has_one association that has a scope and providing available_records for the association, the available_records are ignored.

Steps to reproduce

require 'bundler/inline'

gemfile(false) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem 'activerecord', ''
  gem 'sqlite3'
  gem 'pry'
  gem 'rspec'

require 'active_record'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts do |t|

  create_table :comments do |t|
    t.belongs_to :post, foreign_key: true

class Post < ActiveRecord::Base
  has_many :comments
  has_one :latest_comment, -> { order(created_at: :desc) }, class_name: 'Comment', foreign_key: :post_id

class Comment < ActiveRecord::Base
  belongs_to :post

def preload_latest_comments(posts)
  # these are the relevant comments, the latest per post
  comments = Comment.where(post: posts).group(:post_id).having('MAX(created_at)').to_a

  # using postgres:
  # comments = Comment.where(post: posts).order(created_at: :desc).select('DISTINCT ON (post_id) *').to_a

  # this "works", but it won't use `comments` to assign the latest comment per post,
  # it will fetch all (!) comments per post and assign
  preloader = ActiveRecord::Associations::Preloader.new(records: posts, associations: :latest_comment, available_records: comments)

RSpec.describe 'preloading using available_records' do
  it 'only uses a single sql query' do
    post_1 = Post.create!
    post_1_latest_comment = Comment.create!(post: post_1)

    post_2 = Post.create!
    Comment.create!(post: post_2)
    post_2_latest_comment = Comment.create!(post: post_2)

    post_3 = Post.create!
    Comment.create!(post: post_3)
    Comment.create!(post: post_3)
    post_3_latest_comment = Comment.create!(post: post_3)

    posts = [post_1, post_2, post_3]

    queries = []
    callback = ->(name, start, finish, message_id, values) { queries << values[:sql] }

    # memoize sql queries, should only be one
    ActiveSupport::Notifications.subscribed(callback, 'sql.active_record') do

      expect(post_1.latest_comment).to eq(post_1_latest_comment)
      expect(post_2.latest_comment).to eq(post_2_latest_comment)
      expect(post_3.latest_comment).to eq(post_3_latest_comment)

      # this should just be:
      #   SELECT "comments".* FROM "comments" WHERE "comments"."post_id" IN (?, ?, ?) GROUP BY "comments"."post_id" HAVING (MAX(created_at))
      #  but will be two:
      #    SELECT "comments".* FROM "comments" WHERE "comments"."post_id" IN (?, ?, ?) GROUP BY "comments"."post_id" HAVING (MAX(created_at))
      #    and
      #    SELECT "comments".* FROM "comments" WHERE "comments"."post_id" IN (?, ?, ?) ORDER BY "comments"."created_at" DESC
      expect(queries.join("\n")).to eq('SELECT "comments".* FROM "comments" WHERE "comments"."post_id" IN (?, ?, ?) GROUP BY "comments"."post_id" HAVING (MAX(created_at))')


Expected behavior

I expect available_records to be used and no additional sql query to be perfomed.

Actual behavior

available_records is ignored.

I did some research and this line is why an early return is triggered and the available_records aren't used: https://github.com/rails/rails/blob/v7.0.3.1/activerecord/lib/active_record/associations/preloader/association.rb#L204

reflection_scope.empty_scope? return false because of the has_one scope option (-> { order(created_at: :desc) }) so !reflection_scope.empty_scope? returns true and the method returns early.

The corresponding code comes from this commit. If I comment that one early return line, it works as expected.

Pingin you @jhawthorn, as you seem to be involved. :)

System configuration

Rails version:

Ruby version: 3.1.2

tbuehlmann avatar Aug 10 '22 13:08 tbuehlmann

Hey there @tbuehlmann, I think this isn't a proper bug, looking at preloader.rb file, the comments over the initializer specific dictates that optimization is only applied to single associations with no scopes. And if we think about the association with scope uses a query to go search in database, so a think this is working properly. Also, there's no way to know without querying the database that the available_records array contains the records that the scoped association would retrieve.

tonyaraujop avatar Aug 12 '22 19:08 tonyaraujop

Yep! That's correct. We can't use available records for a scoped association as there's no way to be sure that they match, so this is working as intended.

Please note also that all of ActiveRecord::Associations::Preloader including available_records is a private API (though admittedly one that people do use 😅) so is not guaranteed to exist or have any particular behaviour.

jhawthorn avatar Aug 12 '22 20:08 jhawthorn