Skip to content

Better detection of standard preload #160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Better detection of standard preload #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/active_record/virtual_attributes/virtual_fields.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,12 @@ def call

# convert the includes with virtual attributes to includes with proper associations
records_by_assoc = records.group_by { |rec| assoc_cache[rec.class] }
# if these are the same includes, then do the preloader work
return super if records_by_assoc.size == 1 && records_by_assoc.keys.first == associations
# If the association were already translated, then short circuit / do the standard preloader work.
# When replace_virtual_fields removes the outer array, match that too.
if records_by_assoc.size == 1 &&
(associations == records_by_assoc.keys.first || associations == [records_by_assoc.keys.first])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it sometimes wrapped in an array and not other times?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would flatten or similar help keep it consistent for comparison?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The associaions will probably always be an array. Even a hash becomes a single element array with a hash in it.

fix_association optimizes the array right out of there.
So we end up comparing :symbol with [:symbol], they are different and we get one extra set of lambdas (and one extra group_by) - all with the same class.

This is an optimization only, Without this change, all works fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, would Array.wrap(...) normalize to an array make this less complex or introduce a possible performance hit for the case when it's already an array?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had thought I could avoid this via:

- assoc_cache = Hash.new { |h, klass| h[klass] = klass.replace_virtual_fields(associations) }
+ assoc_cache = Hash.new { |h, klass| h[klass] = Array.wrap(klass.replace_virtual_fields(associations)) }

Since we have Array.wrap on that field later in the block, but ended up with an infinite loop.

So if we do this, I'll need to do the reverse:

records_by_assoc.keys.first == [associations] || records_by_assoc.keys.first == associations

Still tempting

return super
end

# for each of the associations, run a preloader
records_by_assoc.each do |klass_associations, klass_records|
Expand Down
48 changes: 38 additions & 10 deletions spec/virtual_includes_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,6 @@
expect(Author.includes(:books => :author_name).references(:books)).to preload_values(:first_book_author_name, author_name)
expect(Author.includes(:books => [:author_name]).references(:books)).to preload_values(:first_book_author_name, author_name)
expect(Author.includes(:books => {:author_name => {}}).references(:books)).to preload_values(:first_book_author_name, author_name)
end

it "uses preloaded fields" do
expect(Author.includes(:books => :author_name).references(:books)).to preload_values(:first_book_author_name, author_name)
expect(Author.includes(:books => [:author_name]).references(:books)).to preload_values(:first_book_author_name, author_name)
expect(Author.includes(:books => {:author_name => {}}).references(:books)).to preload_values(:first_book_author_name, author_name)
inc = Author.virtual_includes(:first_book_author_name)
expect(Author.includes(inc).references(:books)).to preload_values(:first_book_author_name, author_name)
end
Expand Down Expand Up @@ -308,10 +302,6 @@
it "uses included fields" do
expect(preloaded(Author.all.to_a, :books => :author_name)).to preload_values(:first_book_author_name, author_name)
end

it "uses preloaded fields" do
expect(preloaded(Author.all.to_a, :books => :author_name)).to preload_values(:first_book_author_name, author_name)
end
end

context "preloads virtual_reflection with includes" do
Expand Down Expand Up @@ -386,11 +376,49 @@
end
end

describe ".eager_load" do
it "preloads standard associations (:books)" do
expect(Author.eager_load(:books)).to preload_values(:first_book_name, book_name)
expect(Author.eager_load([:books])).to preload_values(:first_book_name, book_name)
expect(Author.eager_load([[:books]])).to preload_values(:first_book_name, book_name)
expect(Author.eager_load(:books => {})).to preload_values(:first_book_name, book_name)
end

it "preloads associations (:uses => :books)" do
expect(Author.eager_load(:first_book_name)).to preload_values(:first_book_name, book_name)
expect(Author.eager_load([:first_book_name])).to preload_values(:first_book_name, book_name)
expect(Author.eager_load([[:first_book_name]])).to preload_values(:first_book_name, book_name)
expect(Author.eager_load(:first_book_name => {})).to preload_values(:first_book_name, book_name)
end
end

describe ".preload" do
it "preloads standard associations (:books)" do
expect(Author.preload(:books)).to preload_values(:first_book_name, book_name)
expect(Author.preload([:books])).to preload_values(:first_book_name, book_name)
expect(Author.preload([[:books]])).to preload_values(:first_book_name, book_name)
expect(Author.preload(:books => {})).to preload_values(:first_book_name, book_name)
end

it "preloads associations (:uses => :books)" do
expect(Author.preload(:first_book_name)).to preload_values(:first_book_name, book_name)
expect(Author.preload([:first_book_name])).to preload_values(:first_book_name, book_name)
expect(Author.preload([[:first_book_name]])).to preload_values(:first_book_name, book_name)
expect(Author.preload(:first_book_name => {})).to preload_values(:first_book_name, book_name)
end
end

context "preloads virtual_reflection with preloader" do
it "preloads virtual_reflection (:uses => :books)" do
expect(preloaded(Author.all.to_a, :named_books)).to preload_values(:named_books, named_books)
end

it "preloads virtual_reflection ([:books])" do
expect(preloaded(Author.all.to_a, :books)).to preload_values(:named_books, named_books)
expect(preloaded(Author.all.to_a, [:books])).to preload_values(:named_books, named_books)
expect(preloaded(Author.all.to_a, :books => {})).to preload_values(:named_books, named_books)
end

it "preloads virtual_reflection (:uses => {:books => :author_name})" do
expect(preloaded(Author.all.to_a, :books_with_authors)).to preload_values(:books_with_authors, named_books)
end
Expand Down