This project is archived and is in readonly mode.

#5623 new
eirc

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

Reported by eirc | September 13th, 2010 @ 01:41 PM

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.

Comments and changes to this ticket

  • eirc

    eirc September 13th, 2010 @ 04:25 PM

    • Assigned user set to “Jeremy Kemper”
    • Tag changed from “active_record, eager, eagerloading, eager_loading, has_many, has_many_association, nested” to “2.3-stable, 2.3.9, active_record, eager, eagerloading, eager_loading, has_many, has_many_association, nested”

    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?

  • eirc

    eirc October 20th, 2010 @ 03:30 PM

    • Tag changed from “2, active_record, eager, eagerloading, eager_loading, has_many, has_many_association, nested” to “has_many has_many_association, 2.3-stable, 2.3.9, active_record, eager, eagerloading, eager_loading, nested”

    Reverted tag changes from spam comment above.

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer November 1st, 2010 @ 05:07 PM

    • Tag cleared.
    • Importance changed from “” to “Low”

    Automatic cleanup of spam.

  • nicolas

    nicolas January 20th, 2011 @ 04:57 PM

    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}")
  • Preston Marshall

    Preston Marshall January 21st, 2011 @ 07:08 PM

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

  • Preston Marshall

    Preston Marshall January 21st, 2011 @ 07:10 PM

    • Tag set to “bug activerecord includes”
  • Fotos Georgiadis

    Fotos Georgiadis January 21st, 2011 @ 10:29 PM

    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.

  • Preston Marshall

    Preston Marshall January 21st, 2011 @ 11:35 PM

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

  • Fotos Georgiadis

    Fotos Georgiadis January 21st, 2011 @ 11:59 PM

    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.

  • nicolas

    nicolas January 24th, 2011 @ 02:35 PM

    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.

  • Guy Boertje

    Guy Boertje February 4th, 2011 @ 03:24 PM

    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.

  • bingbing

    bingbing March 26th, 2011 @ 02:13 AM

  • rob g

    rob g April 14th, 2011 @ 07:17 PM

    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.

  • rob g

    rob g April 14th, 2011 @ 10:03 PM

    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.

  • nicolas

    nicolas April 15th, 2011 @ 02:19 PM

    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

  • nicolas

    nicolas April 15th, 2011 @ 02:25 PM

    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
  • laptopbatteries

    laptopbatteries April 16th, 2011 @ 03:00 AM

    rolex watches are very common in our Audemars Piguet Replicas lifes, there are quite several well known wrist watches brands, the majority Gucci Replicas of them are Swiss bands, Panerai Replicas ,and it is Omega Replicas unlikely, unless the replica watches 's owner is filthy rich and equally careless replica breitling with his, Even the highest quality, Some replica omega are believed to be to acquire luxury wrist that are Replica Concord founded of gold or platinum or other high priced materials. placing on these wrist fake rolex certainly will make us stand out from other Swiss Replica Watch people.Does everyone can afford these genuine, replica tag heuer Taking your or replica watch ? When should expensive replica watch uk , before you take your precious?

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages