Hello all,
I still have a patch for #3438 (eager loading doesn't respect
association orders); it passes all unit tests, and has additional unit
tests with full coverage. The patch is attached.
== The Bug ==
Author.find(1).posts != Author.find(1, :include => [:posts]).posts
if Author has_many :posts, :order => anything.
This means that either one must avoid eager loading or constantly
specify an :order option (despite having already specified one in the
association options). Awkward.
http://dev.rubyonrails.org/ticket/3438
== The Solution ==
My patch adds ~20 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', minding table
aliases), and append them to the SQL's ORDER BY clause, if any exists
(creating one if not).
== The Catch ==
Developers who have previously relied on their database's default
order (if any exists) may be unpleasantly surprised to find that their
records are not coming back in the expected order. For example:
Author.find(:all) != Author.find(:all, :include => [:posts])
It has been suggested by some that these developers would do well to
be surprised, as not specifying an ORDER BY clause means that the
order of the records is not meaningful to you, and that relying upon
an implementation quirk is bad form. This "fire hot" school of
pedagogy is certainly effective, but may result in additional support
demands.
== The Good News ==
This patch passes all existing unit tests, and adds two additional
tests which assert the proper behavior over a wide variety of model
types, including STI, Polymorphic, Rich Joins, etc., and even cascaded
eager loading. 2 tests, 149 assertions.
== The Previously Addressed Issues ==
My original version of this patch assumed a default SQL sort order. It
no longer does so.
== The Begging ==
C'mon. Please. Dude. C'mon. Duuuude. Please. C'mon. Please? Awww....
dude. Dude? C'mon?
== The End ==
Since Trac is on sabbatical, I figured I'd toss up my patch here, in
the hopes that it will be applied. The last time I tried this I got a
lot of great feedback and even some publicity--thanks, Rodney!--but
everyone was at RailsConf, and I guess that was more fun than fixing
my pet peeves. ;-)
Thanks!
--
Coda Hale
http://blog.codahale.com
Index: activerecord/test/associations_cascaded_eager_loading_test.rb
===================================================================
--- activerecord/test/associations_cascaded_eager_loading_test.rb (revision 4527)
+++ activerecord/test/associations_cascaded_eager_loading_test.rb (working copy)
@@ -103,4 +103,16 @@
authors.first.posts.first.special_comments.first.post.very_special_comment
end
end
+
+ def test_association_orders_work_with_cascaded_eager_loading
+ [ { :posts => :comments }, { :posts => :categories },
+ { :posts => [:comments, :categories] },
+ { :comments => :post }, { :funky_comments => :post } ].each do |association|
+ regular_records = Author.find(:all).sort_by(&:id)
+ eager_records = Author.find(:all, :include => association).sort_by(&:id)
+ regular_records.size.times do |i|
+ assert_equal regular_records[i].send(association.keys.first).map(&:id), eager_records[i].send(association.keys.first).map(&:id), "Author.#{association.keys.first} has a different order when eagerly loaded via #{association.inspect}."
+ end
+ end
+ end
end
Index: activerecord/test/associations_go_eager_test.rb
===================================================================
--- activerecord/test/associations_go_eager_test.rb (revision 4527)
+++ activerecord/test/associations_go_eager_test.rb (working copy)
@@ -356,4 +356,31 @@
assert_equal 2, one.comments.size
assert_equal 2, one.categories.size
end
+
+ def test_association_orders_work_with_eager_loading
+ { Author => [ :posts, :posts_with_comments, :posts_with_categories, :posts_with_comments_and_categories,
+ :comments, :funky_comments, :special_posts, :hello_posts, :nonexistent_posts,
+ :posts_with_callbacks, :posts_with_proc_callbacks, :posts_with_multiple_callbacks,
+ :unchangable_posts, :author_favorites ],
+ Category => [ :posts, :special_posts, :hello_posts, :nonexistent_posts ],
+ DependentFirm => [ :companies ],
+ Firm => [ :clients, :clients_sorted_desc, :clients_of_firm, :dependent_clients_of_firm,
+ :exclusively_dependent_clients_of_firm ],
+ Person => [ :readers, :posts ],
+ Post => [ :comments, :special_comments, :categories, :special_categories, :readers, :people ]
+ }.each do |model, associations|
+ associations.each do |association|
+ regular_records = model.find(:all).sort_by(&:id)
+ eager_records = model.find(:all, :include => [association]).sort_by(&:id)
+ regular_records.size.times do |i|
+ assert_equal regular_records[i].send(association).map(&:id), eager_records[i].send(association).map(&:id), "#{model}.#{association} has a different order when eagerly loaded."
+ end
+
+ expected_order = model.find(:all, :order => "#{model.table_name}.#{model.primary_key}").map(&association).flatten.map(&:id)
+ actual_order = model.find(:all, :include => [association], :order => "#{model.table_name}.#{model.primary_key}").map(&association).flatten.map(&:id)
+ assert_equal expected_order, actual_order, "#{model}.#{association} has a different order when eagerly loaded given an :order option"
+ end
+ end
+ end
+
end
Index: activerecord/test/fixtures/post.rb
===================================================================
--- activerecord/test/fixtures/post.rb (revision 4527)
+++ activerecord/test/fixtures/post.rb (working copy)
@@ -15,10 +15,10 @@
has_one :very_special_comment
has_one :very_special_comment_with_post, :class_name => "VerySpecialComment", :include => :post
- has_many :special_comments
+ has_many :special_comments, :order => 'type'
- has_and_belongs_to_many :categories
- has_and_belongs_to_many :special_categories, :join_table => "categories_posts", :association_foreign_key => 'category_id'
+ has_and_belongs_to_many :categories, :order => 'name'
+ has_and_belongs_to_many :special_categories, :join_table => "categories_posts", :association_foreign_key => 'category_id', :order => 'name DESC, type ASC'
has_many :taggings, :as => :taggable
has_many :tags, :through => :taggings, :include => :tagging do
@@ -38,8 +38,8 @@
has_many :categorizations, :foreign_key => :category_id
has_many :authors, :through => :categorizations
- has_many :readers
- has_many :people, :through => :readers
+ has_many :readers, :order => 'readers.person_id'
+ has_many :people, :through => :readers, :order => 'first_name DESC, id ASC'
def self.what_are_you
'a post...'
Index: activerecord/test/fixtures/author.rb
===================================================================
--- activerecord/test/fixtures/author.rb (revision 4527)
+++ activerecord/test/fixtures/author.rb (working copy)
@@ -1,7 +1,7 @@
class Author < ActiveRecord::Base
- has_many :posts
- has_many :posts_with_comments, :include => :comments, :class_name => "Post"
- has_many :posts_with_categories, :include => :categories, :class_name => "Post"
+ has_many :posts, :order => 'title DESC'
+ has_many :posts_with_comments, :include => :comments, :class_name => "Post", :order => 'title DESC'
+ has_many :posts_with_categories, :include => :categories, :class_name => "Post", :order => 'title'
has_many :posts_with_comments_and_categories, :include => [ :comments, :categories ], :order => "posts.id", :class_name => "Post"
has_many :posts_with_extension, :class_name => "Post" do #, :extend => ProxyTestExtension
def testing_proxy_owner
@@ -14,8 +14,8 @@
proxy_target
end
end
- has_many :comments, :through => :posts
- has_many :funky_comments, :through => :posts, :source => :comments
+ has_many :comments, :through => :posts, :order => 'body DESC'
+ has_many :funky_comments, :through => :posts, :source => :comments, :order => 'body'
has_many :special_posts, :class_name => "Post"
has_many :hello_posts, :class_name => "Post", :conditions=>"\#{aliased_table_name}.body = 'hello'"
Index: activerecord/lib/active_record/associations.rb
===================================================================
--- activerecord/lib/active_record/associations.rb (revision 4527)
+++ activerecord/lib/active_record/associations.rb (working copy)
@@ -1148,7 +1148,33 @@
add_limited_ids_condition!(sql, options, join_dependency) if !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])
sql << "ORDER BY #{options[:order]} " if options[:order]
-
+
+ # Collect all eagerly-loaded assocations with any order options, and
+ # add in their table names unless it's already there.
+ associated_orders = join_dependency.join_associations.select do |assoc|
+ assoc.reflection.options[:order]
+ end.map do |assoc|
+ orders = assoc.reflection.options[:order].split(',').map(&:strip)
+ orders.map do |order|
+ if order =~ /\./
+ order
+ else
+ assoc.aliased_table_name + '.' + order
+ end
+ end
+ end
+
+ # Add the association orders to the SQL statement.
+ if associated_orders.any?
+ if options[:order]
+ sql << ", "
+ else
+ sql << "ORDER BY "
+ end
+ sql << associated_orders.flatten.join(', ')
+ end
+ sql << ' '
+
add_limit!(sql, options, scope) if using_limitable_reflections?(join_dependency.reflections)
return sanitize_sql(sql)
_______________________________________________
Rails-core mailing list
Rails-core@lists.rubyonrails.org
http://lists.rubyonrails.org/mailman/listinfo/rails-core
