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

Ticket #3438 (new defect)

Opened 3 years ago

Last modified 2 years ago

[PATCH] Eager loading doesn't respect :order of association

Reported by: jon@burningbush.us Assigned to: David
Priority: normal Milestone: 2.x
Component: ActiveRecord Version: edge
Severity: major Keywords: activerecord eager association order include
Cc: rpnielsen@gmail.com, sjmadsen, vzctl

Description

I've got a model that :has_many of something else, with a specific :order condition.

When I do instance.association, the :order is correct. However, when I do

list = Model.find(:all, :include => :association)

The :order clause is ignored for my association.

Attachments

uncompleted_fix_for_non_ordering_off_eager_loaded_associations.diff (0.6 kB) - added by JanPrill@blauton.de on 01/11/06 17:46:41.
eager_loading_order_fix.diff (1.2 kB) - added by coda.hale@gmail.com on 06/22/06 20:58:57.
A fix for this, no unit tests, relatively untested
eager_loading_order_patch_with_unit_tests.diff (7.4 kB) - added by coda.hale@gmail.com on 06/24/06 22:30:05.
FULL PATCH, INCLUDING UNIT TESTS
eager_loading_order_patch_sql_standard.diff (7.6 kB) - added by coda.hale@gmail.com on 07/02/06 22:11:24.
no longer assuming that ordering by primary key is reasonable
eager_loading_order_patch_1_1_stable.diff (7.6 kB) - added by oleg on 02/17/07 00:31:28.
Patch for 1-1-stable branch
eager_loading_order_patch.diff (7.8 kB) - added by josh on 04/16/07 18:28:16.
Patch against [6530].

Change History

01/11/06 17:46:11 changed by JanPrill@blauton.de

@jon:

while I was trying to reproduce your reported behaviour I was at least able to reproduce the error for :include statements on the associations itself

(reported underneath). For your specific misbehaviour you might have a look at http://dev.rubyonrails.org/browser/trunk/activerecord/test/associations_go_eager_test.rb . You may replicate your :order in the find method. At least this is what the tests are showing and what seems to result in successful ordering.

Anyway while I was trying to understand what is going on in associations.rb I've stumbled across the following problem:

Lets say I've got a model for a source of information (e. g. page 23 line 2) of a specific book (e.g. 'Agile Webdevelopment ...') written by two authors (DHH and Dave Thomas):

class Source < ActiveRecord::Base

  belongs_to :book, :include => :authors

end

--

class Book < ActiveRecord::Base

  has_and_belongs_to_many :authors, :order => 'lastname, firstname DESC'

end

--

class Author < ActiveRecord::Base

  has_and_belongs_to_many :books

end

then I would expect that the source.book association of a source would contains a collection of the eagerly loaded authors, right?

I've tried to display these after I've fetched Source.find(:all) with the following view:


<% for source in @sources -%>
  <p>
  <strong>source: <%= source.id %></strong><br/>
  <strong>book: <%= source.book.title %></strong><br/>
  <% for author in source.book.authors -%>
    <%= author.lastname %><br/>
  <% end -%>
  </p>
<% end -%>

the resulting SQL would be something along the development.log-lines of

Book Load Including Associations (0.000000)   

SELECT books.`created_at` AS t0_r6, books.`updated_at` AS t0_r7, books.`publisher_id` AS t0_r8, authors.`id` AS t1_r0, authors.`firstname` AS t1_r1, books.`id` AS t0_r0, authors.`lastname` AS t1_r2, books.`title` AS t0_r1, authors.`academic` AS t1_r3, books.`subtitle` AS t0_r2, authors.`created_at` AS t1_r4, books.`print_run` AS t0_r3, authors.`updated_at` AS t1_r5, books.`publishing_year` AS t0_r4, books.`isbn` AS t0_r5 
FROM books 
LEFT OUTER JOIN authors_books ON authors_books.book_id = books.id 
LEFT OUTER JOIN authors ON authors_books.author_id = authors.id 
WHERE (books.id = 3)

to my surprise there is no ORDER BY clause on this SELECT.

Investigating why this could be I've found the construct_finder_sql_with_included_associations(options, schema_abbreviations, reflections) in associations.rb. This method puts together the sql-statement and includes the line

sql << "ORDER BY #{options[:order]} " if options[:order]

options[:order] is nil in my example. IMHO it shouldn't. I've tried to put the :order parameter into the belongs to :book getting something like belongs_to :book, :include => :authors, :order => "authors.lastname, authors.firstname DESC" but again to no avail. options[:order] remains to be nil and therefore no ORDER BY clause joins the sql-statement.

The attached diff is definitly uncomplete, because it won't put together different ORDER BY clauses of many included associations. And this may nail the problem down. What if there are many included associations? How should the ORDER of ORDER BY attributes be? Anyway, IMHO, the current implementation seems to be never true for options[:order].

sql << "ORDER BY #{reflections.first.options[:order]} " if reflections.first.options[:order]

is a least capable of fetching an :order clause at all.

@core-team: Are we (the original reporter and I) are getting anything wrong here?

Regards

Jan Prill

01/11/06 17:46:41 changed by JanPrill@blauton.de

  • attachment uncompleted_fix_for_non_ordering_off_eager_loaded_associations.diff added.

06/22/06 20:58:57 changed by coda.hale@gmail.com

  • attachment eager_loading_order_fix.diff added.

A fix for this, no unit tests, relatively untested

06/22/06 21:15:55 changed by coda.hale@gmail.com

  • summary changed from Eager loading doesn't respect :order of association to [PATCH] Eager loading doesn't respect :order of association.

Sorry for the lack of unit tests--I really don't have the time today to get the AR tests running on my workstation. Hopefully someone will take pity on me and help me out on that front.

This patch changes the behavior of ActiveRecord::Associations::ClassMethods#construct_finder_sql_with_included_associations to add the various :order options of the model's associations, should they be mentioned in the :include option.

Given the above test case, Source.find(:all) would produce the following SQL:

SELECT books.`created_at` AS t0_r6, books.`updated_at` AS t0_r7, books.`publisher_id` AS t0_r8, authors.`id` AS t1_r0, authors.`firstname` AS t1_r1, books.`id` AS t0_r0, authors.`lastname` AS t1_r2, books.`title` AS t0_r1, authors.`academic` AS t1_r3, books.`subtitle` AS t0_r2, authors.`created_at` AS t1_r4, books.`print_run` AS t0_r3, authors.`updated_at` AS t1_r5, books.`publishing_year` AS t0_r4, books.`isbn` AS t0_r5 
FROM books 
LEFT OUTER JOIN authors_books ON authors_books.book_id = books.id 
LEFT OUTER JOIN authors ON authors_books.author_id = authors.id 
WHERE (books.id = 3) ORDER BY lastname, firstname DESC

If a Rails app has many models in it which have overlapping attributes (e.g., sort_order), this patch will cause eager loading to eat flaming death unless the :order options for those associations have explicit table names (e.g., authors.name ASC vs name ASC). Personally, I'm OK with that, but then again, I don't have to wade through defect tickets here. ;-)

Also, this patch doesn't play particularly well with nested includes. this_island_earth.tree.monkeys here won't be sorted by the appropriate order:

  this_island_earth = Island.find(:first, :include => [{ :tree => :monkeys}])

06/24/06 06:00:48 changed by coda.hale@gmail.com

  • version changed from 1.0.0 to 1.1.1.

06/24/06 18:51:18 changed by coda.hale@gmail.com

  • summary changed from [PATCH] Eager loading doesn't respect :order of association to Eager loading doesn't respect :order of association.

Finally got unit tests running, and this patch needs some serious work. I'm withdrawing it until it passes unit tests.

06/24/06 22:30:05 changed by coda.hale@gmail.com

  • attachment eager_loading_order_patch_with_unit_tests.diff added.

FULL PATCH, INCLUDING UNIT TESTS

06/24/06 22:33:09 changed by coda.hale@gmail.com

  • summary changed from Eager loading doesn't respect :order of association to [PATCH] Eager loading doesn't respect :order of association.

Okay, here's a full patch which actually uses JoinDependency instead of dicking around with hashes. It passes all existing tests, and I've added two more fairly comprehensive tests which make sure that all eagerly-loaded associations have the same order as non-eagerly-loaded ones.

07/01/06 23:16:12 changed by coda.hale@gmail.com

Okay, so I've realized that just posting an "oi, patch over here" message doesn't quite convince anyone to spend their time sorting through the whole thing themselves, so here's a run-down of the ticket and my patch, in the hopes of convincing rails-core that the bug is important and the patch is solid.

The problem:

Author.find(1).posts != Author.find(1, :include => [:posts]).posts

if Author has_many :posts, :order => anything

The :order option of an association isn't respected by eager loading, which forces developers to either repeat the :order option for each eagerly loaded find, or to actually sort things themselves. The workarounds are neither pretty, DRY, nor necessary. This is not a design decision nor a technical limitation; this is just functionality which never got added in.

The solution:

My patch adds 21 lines of code to ActiveRecord::Associations::ClassMethods#construct_finder_sql_with_included_associations which run through the various associations in the join dependency, extract all :order options, expand them to their full form (e.g., 'posts.published_at' instead of 'published_at', also minding table aliases), and append them to the SQL's ORDER BY clause. If there is no existing :order option in the find itself, the ORDER BY clause begins with the primary key of the model being searched, which is the default SQL order.

This patch passes all existing unit tests without modification, and I've added two units tests which have a combined total of 38 assertions which check that the eagerly-loaded and non-eagerly-loaded associations both have the exact same order. The code is written to standards, has comments to explain any tricky bits, and doesn't rely on high magic. It doesn't break any existing code, works with STI, doesn't overshadow the find method's :order options, and smells faintly of apple pie.

07/02/06 22:11:24 changed by coda.hale@gmail.com

  • attachment eager_loading_order_patch_sql_standard.diff added.

no longer assuming that ordering by primary key is reasonable

07/22/06 07:46:45 changed by anonymous

beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida beida

08/04/06 03:39:44 changed by anonymous

duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou duosou

08/28/06 13:18:57 changed by rpnielsen@gmail.com

  • cc set to rpnielsen@gmail.com.
  • version changed from 1.1.1 to edge.
  • severity changed from normal to major.

I just hit this today and it's exceedingly annoying. Please consider fixing this issue for 1.2.

11/15/06 19:32:01 changed by divoxx

  • keywords changed from activerecord eager association to activerecord eager association order.

02/17/07 00:31:28 changed by oleg

  • attachment eager_loading_order_patch_1_1_stable.diff added.

Patch for 1-1-stable branch

04/16/07 18:28:16 changed by josh

  • attachment eager_loading_order_patch.diff added.

Patch against [6530].

04/16/07 18:28:49 changed by josh

  • keywords changed from activerecord eager association order to activerecord eager association order unverified.
  • milestone set to 1.x.

04/17/07 23:39:38 changed by sjmadsen

  • cc changed from rpnielsen@gmail.com to rpnielsen@gmail.com, sjmadsen.

07/19/07 23:29:42 changed by lifofifo

  • keywords changed from activerecord eager association order unverified to activerecord eager association order.

08/01/07 12:05:18 changed by iktorn

  • keywords changed from activerecord eager association order to activerecord eager association order include.

10/18/07 05:57:37 changed by vzctl

  • cc changed from rpnielsen@gmail.com, sjmadsen to rpnielsen@gmail.com, sjmadsen, vzctl.