Ticket Navigation

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

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)[0m   [0;1m

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

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}])

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/22/06 07:46:45 changed by anonymous

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

Download in other formats:

Rails Trac

Ticket Navigation

Ticket #3438 (new defect)

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

Description

Attachments

Change History

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Download in other formats: