Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #11215 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

[PATCH] Eager loading includes duplicate objects

Reported by: Catfish Assigned to: core
Priority: normal Milestone: 2.x
Component: ActiveRecord Version: edge
Severity: normal Keywords:
Cc:

Description

The new eager loading system has a slight oddity when including duplicate objects

If I do A.find(:all, :include => {:B => :C}), and multiple A's reference the same B, we get duplicate C's included in every B. For every A that references the same B, it adds another set of duplicate C objects to each B.

Clear as mud? Maybe the failing test case I've attached will make thing more obvious. Reader belongs_to :post, Post has_many :comments. If a post had comments [1,2], and two readers referenced that post, then : 

Reader.find(:all, :include => {:post => :comments})[0].post.comments # =>  [1,1,2,2]

This gets exponentially worse as you get more top-level objects referencing the same included object.

Attachments

eager_loading_duplicate_problems.diff (2.0 kB) - added by Catfish on 02/25/08 15:42:55.
Failing tests for duplicate eager loads
preloaded_duplicates.diff (3.7 kB) - added by Catfish on 02/28/08 11:02:57.
Patch+tests, supersedes older diff.

Change History

02/25/08 15:42:55 changed by Catfish

  • attachment eager_loading_duplicate_problems.diff added.

Failing tests for duplicate eager loads

02/25/08 15:43:45 changed by Catfish

Oh, I should point out this was introduced in r8672.

02/27/08 21:18:53 changed by Catfish

  • summary changed from [BUG] Eager loading includes duplicate objects to [PATCH] Eager loading includes duplicate objects.

02/28/08 11:01:17 changed by Catfish

Tracked down the problem - remove_duplicate_results! wasn't working as expected. When creating parent_records, it uses uniq! (which returns nil when there's no change), so deep duplicates weren't being found. That fixes the has_many case, but the belongs_to case doesn't go via find_with_associations, so it's not affected. Instead, preload_associations needs to run its arguments through uniq.

Patch + tests are attached, ignored the older diff.

(One thing that I'd like confirmation on is whether the "next unless descendant" is necessary. (In the old code, it was "next unless record.send(reflection.name)"). When would that ever be nil?)

02/28/08 11:02:57 changed by Catfish

  • attachment preloaded_duplicates.diff added.

Patch+tests, supersedes older diff.

02/28/08 20:42:05 changed by bitsweat

  • status changed from new to closed.
  • resolution set to fixed.

(In [8942]) Fix that batched :include would pull in duplicate records in some cases. Closes #11215 [Catfish]