Eager loading doesn't respect prior #includes call

[jrdioko]

Using #includes to eager load a model that has already been eager loaded in a previous step causes ActiveRecord to unnecessarily fetch the models again. This means that eager loading must be done at a single top-level place in the code, rather than being able to add it to all the specific lower-level places where it is required (just like .transaction calls can be added anywhere a transaction is required, and they will all join the parent transaction). See the example below:

Steps to reproduce

Consider a simple case with three levels of associations:

class Foo
  has_many :bars
end

class Bar
  belongs_to :foo
  has_one :baz
end

class Baz
  belongs_to :bar
end

This call illustrates the issue:

Foo.all.includes(bars: :baz).each do |foo|
  foo.bars.includes(:baz).each { |bar| do_something }
end

Expected behavior

The includes(bars: :baz) call should execute a database query to load bars and a query to load bazs. The includes(:baz) calls should do nothing, since bazs have already been eager loaded.

Actual behavior

Each includes(:baz) call (one per iteration in the loop) executes separate eager loading queries, despite the fact that bazs have already been eager loaded before entering the loop.

System configuration

Rails version: 4.2.7.1

Ruby version: 2.3.1

[mechanicles]

It happens in Rails 5 too. But I feel this is a correct flow. We don't get any clue about the outer includes while the inner includes is being performed. Both have different scopes/contexts I guess. Inner includes looks redundant to me. Why would anyone do that?

[jrdioko]

For the same reason that you'd wrap something in a transaction even though the parent call also happens to be in a transaction several layers up: keeping the call close to the code that must be transactional ensures that that code is always protected, regardless of how the calling code changes or the called code is used in different places in the future.

For the includes case, imagine a low-level helper method that eager loads an association and does something with it. It makes sense to put the includes call in that helper rather than repeated in many places that construct the initial relation and call the helper. But this behavior means that if any of those places also happen to eager load the same association (for some other purpose than what the helper is doing), duplicate queries will occur.

In general, it seems to be restrict code re-use in a complex application. High-level methods doing eager loading need to ensure that no lower-level methods ever repeat the same eager loading on the same relation object. And low-level methods adding eager loading to a passed relation object need to ensure that nowhere up the chain also added the same eager loading. Considering how transaction works, it surprised me that includes is not aware of when an association has already been included.

[stale]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

[jrdioko]

I just created a new test Rails app (on 5.1.1) and confirmed this issue still exists. With the setup mentioned above, running the call generates the following SQL:

  Foo Load (0.1ms)  SELECT "foos".* FROM "foos"
  Bar Load (0.2ms)  SELECT "bars".* FROM "bars" WHERE "bars"."foo_id" = 1
  Baz Load (0.2ms)  SELECT "bazs".* FROM "bazs" WHERE "bazs"."bar_id" = 1
  Bar Load (0.2ms)  SELECT "bars".* FROM "bars" WHERE "bars"."foo_id" = ?  [["foo_id", 1]]
  Baz Load (0.1ms)  SELECT "bazs".* FROM "bazs" WHERE "bazs"."bar_id" = 1

Notice that the bars and bazs are eager loaded twice.

[rafaelfranca]

Could you create a reproduction script with this template https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb?

[jrdioko]

Sure, how's this?

https://gist.github.com/jrdioko/5b371da56604647e011da9e58235286b

[matthewd]

This is known behaviour; includes always returns a new (unpopulated) Relation instance. Making it smarter would be nice, but it would be more feature request than bug.