Eager loading a nested association with conditions or order eager loads the wrong associated records

[lighthouse-import]

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/5623
Created by eirc - 2011-02-17 06:48:11 UTC

Eager loading a nested has_many association with conditions or order on the nested table loads only the first record.

class AuthorAddress < ActiveRecord::Base
  has_one :author
end

class Author < ActiveRecord::Base
  has_many :posts
  belongs_to :author_address
end

class Post < ActiveRecord::Base
  belongs_to :author
end

author_address = AuthorAddress.find(1, :include => {:author => :posts}, :order => "authors.id")

Using an order or conditions clause on a different table than the base one uses legacy SQL (LEFT OUTER JOIN) to load, but it incorrectly (eagerly) loads posts. Only one, of the many posts, is eagerly loaded and unfortunately the association thinks it is fully loaded.

Therefore doing something like this:

author_address.author.posts.length

returns a different answer than this:

author_address.author.posts.count

(which of course hits the database again to get the correct answer)

Any ideas on where we can look to solve this?

The above code works correctly in Rails 1.2.6!

Attached you'll find failing tests.

Thanks in advance for any help.

[lighthouse-import]

Imported from Lighthouse.
Comment by eirc - 2010-09-13 15:25:06 UTC

Diving in the code of ActiveRecord we traced the problem to ...
ActiveRecord::Associations::JoinDependency#construct_association and the following line:

return if record.instance_variable_defined?("@#{join.reflection.name}")

It appears that this line blocks loading nested has_many associations after the first record of the association has been loaded since the outer has_one association has already been loaded and the instance variable is set. So it loads only one record from the nested association, even tho there are more.

(It's not easy to explain the problem but that's why we attached simple failing tests. Unfortunately that's the best we can do, without help, since the logic is a bit complicated)

This line was committed in: http://github.com/rails/rails/commit/a445cdd8840 for ticket #904. (http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/904)

Jeremy since you signed-off that commit (if that commit is responsible for the bug) can you shed some light please?

[lighthouse-import]

Imported from Lighthouse.
Comment by eirc - 2010-10-20 14:30:44 UTC

Reverted tag changes from spam comment above.

[lighthouse-import]

Imported from Lighthouse.
Comment by Jeff Kreeftmeijer - 2010-11-01 17:07:18 UTC

Automatic cleanup of spam.

[lighthouse-import]

Imported from Lighthouse.
Comment by nicolas - 2011-01-20 16:57:32 UTC

Returning the already loaded outer has_one object will allow for its has_many associations to be added to the object.
I've add some success with

return record.instance_variable_get("@#{join.reflection.name}") if record.instance_variable_defined?("@#{join.reflection.name}")

[lighthouse-import]

Imported from Lighthouse.
Comment by Preston Marshall - 2011-01-21 19:08:12 UTC

I can confirm this issue, pretty big deal to be marked as low priority.

[lighthouse-import]

Imported from Lighthouse.
Comment by Fotos Georgiadis - 2011-01-21 22:29:48 UTC

Attached is a current patch against 2-3-stable activerecord which includes the failing tests introduced by eric and the solution proposed by nicolas. I have tested the patch using only sqlite3 (but the relevant code is database agnostic anyway).

Yes it is kinda strange that no-one else has seen this bug except us.

+1 to have this applied to 2-3-stable.

[lighthouse-import]

Imported from Lighthouse.
Comment by Preston Marshall - 2011-01-21 23:35:21 UTC

So you are only experiencing it with has_one? It is doing the same behavior in rails 3 with a has_many.

[lighthouse-import]

Imported from Lighthouse.
Comment by Fotos Georgiadis - 2011-01-21 23:59:37 UTC

A has_one nested with a has_many association triggered the bug for us. We've seen weird counts for eager loaded associations and we started investigating things with eric.

Other association combination might still trigger the bug..

We haven't tested it with rails 3 or edge, tho. The fix seems safe enough to be applied to 2-3-stable.

[lighthouse-import]

Imported from Lighthouse.
Comment by nicolas - 2011-01-24 14:35:44 UTC

a has_one nested with an has_many and an order or a condition triggers the bug for me.
I'm seeing that bug in rails 3.0.3 as well.

the way I understand it is that construct_association must return the association for the nested associations to be loaded.
In the case of has_many, the association is always returned so the nested associations can be loaded.
the nested are properly associated to their parent since JoinBase.instantiate(row) caches the records it has already seen from the previous rows.

Hope this help.

[lighthouse-import]

Imported from Lighthouse.
Comment by Guy Boertje - 2011-02-04 15:24:37 UTC

I am experiencing this bug as well in 2.2.2 and 2.3.5

Person has_one Wallet has_many Receipts

Receipt belongs_to Wallet belongs_to Person

Person.find :all, :include=>{:wallet=>receipts}, :conditions => "receipts.status IN ('unpaid','unconverted') AND receipts.ccy = 'EUR'"

Only returns the first record but it should return many

Please up the priority.

[lighthouse-import]

Imported from Lighthouse.
Comment by rob g - 2011-04-14 18:17:35 UTC

I just ran into this defect with active_record 3.0.5 as well. The change proposed by Nicolas and Fotos fixes the issue for me as well.

[lighthouse-import]

Imported from Lighthouse.
Comment by rob g - 2011-04-14 21:03:07 UTC

I created a patch against master with failing tests. I intended to also port the fix proposed by Nicolas and Fotos to master, but that code has changed so much on master that I'm not sure how to fix it yet.

[lighthouse-import]

Imported from Lighthouse.
Comment by nicolas - 2011-04-15 13:19:18 UTC

Hello rob g,

I took a quick look at the master, and here is what I believe could fix the issue.

diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb
index 504f252..26911c3 100644
--- a/activerecord/lib/active_record/associations/join_dependency.rb
+++ b/activerecord/lib/active_record/associations/join_dependency.rb
@@ -184,7 +184,7 @@ module ActiveRecord

     macro = join_part.reflection.macro
     if macro == :has_one

•      return if record.association_cache.key?(join_part.reflection.name)
  

•      return record.association(join_part.reflection.name).target if record.association_cache.key?(join_part.reflection.name)
     association = join_part.instantiate(row) unless row[join_part.aliased_primary_key].nil?
     set_target_and_inverse(join_part, association, record)
   else
  

the idea is for construct association to return already created 'has_one' association target, so that the children of that target can be loaded and associated with it.

Nicolas

[lighthouse-import]

Imported from Lighthouse.
Comment by nicolas - 2011-04-15 13:25:23 UTC

This is the properly formatted diff

diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb
index 504f252..26911c3 100644
--- a/activerecord/lib/active_record/associations/join_dependency.rb
+++ b/activerecord/lib/active_record/associations/join_dependency.rb
@@ -184,7 +184,7 @@ module ActiveRecord

         macro = join_part.reflection.macro
         if macro == :has_one
-          return if record.association_cache.key?(join_part.reflection.name)
+          return record.association(join_part.reflection.name).target if record.association_cache.key?(join_part.reflection.name)
           association = join_part.instantiate(row) unless row[join_part.aliased_primary_key].nil?
           set_target_and_inverse(join_part, association, record)
         else

[lighthouse-import]

Attachments saved to Gist: http://gist.github.com/971708

[achivetta]

I am experiencing this issue on Rails 3.0.7 and the above patch does not appear to have been applied to edge. Why is this issue closed?