aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorYasyf Mohamedali <yasyfm@gmail.com>2015-03-02 17:18:35 -0500
committerYasyf Mohamedali <yasyfm@gmail.com>2015-03-02 17:18:35 -0500
commit524238139025ccddfa886bcfd7a1a6434954e305 (patch)
tree9de6a3a023bc0975725ebb9e080d029f3400614d
parent90387e3e6ddc87029ba1e862f87e4f27623bf360 (diff)
downloadrails-524238139025ccddfa886bcfd7a1a6434954e305.tar.gz
rails-524238139025ccddfa886bcfd7a1a6434954e305.tar.bz2
rails-524238139025ccddfa886bcfd7a1a6434954e305.zip
Honour the order of the joining model in a `has_many :through`
association when eager loading. Previously, eager loading a `has_many :through` association with no defined order would return the records in the natural order of the database. Now, these records will be returned in the order that the joining record is returned, in case there is a defined order there.
-rw-r--r--activerecord/CHANGELOG.md15
-rw-r--r--activerecord/lib/active_record/associations/preloader/association.rb2
-rw-r--r--activerecord/lib/active_record/associations/preloader/through_association.rb23
-rw-r--r--activerecord/test/cases/associations/eager_test.rb32
-rw-r--r--activerecord/test/models/tag.rb6
5 files changed, 69 insertions, 9 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index b5dd19988d..e63f86d45e 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,18 @@
+* Honour the order of the joining model in a `has_many :through` association when eager loading.
+
+ Example:
+
+ The below will now follow the order of `by_lines` when eager loading `authors`.
+
+ class Article < ActiveRecord::Base
+ has_many :by_lines, -> { order(:position) }
+ has_many :authors, through: :by_lines
+ end
+
+ Fixes #17864.
+
+ *Yasyf Mohamedali*, *Joel Turkel*
+
* Dont enroll records in the transaction if they dont have commit callbacks.
That was causing a memory grow problem when creating a lot of records inside a transaction.
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index 1dc8bff193..91de52e7e7 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -86,7 +86,7 @@ module ActiveRecord
if owner_keys.any?
# Some databases impose a limit on the number of ids in a list (in Oracle it's 1000)
# Make several smaller queries if necessary or make one query if the adapter supports it
- sliced = owner_keys.each_slice(klass.connection.in_clause_length || owner_keys.size)
+ sliced = owner_keys.each_slice(klass.connection.in_clause_length || owner_keys.size)
records = load_slices sliced
records.each do |record, owner_key|
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index 56aa23b173..5adb0aac65 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -37,12 +37,7 @@ module ActiveRecord
}
end
- record_offset = {}
- @preloaded_records.each_with_index do |record,i|
- record_offset[record] = i
- end
-
- through_records.each_with_object({}) { |(lhs,center),records_by_owner|
+ through_records.each_with_object({}) do |(lhs,center), records_by_owner|
pl_to_middle = center.group_by { |record| middle_to_pl[record] }
records_by_owner[lhs] = pl_to_middle.flat_map do |pl, middles|
@@ -52,13 +47,25 @@ module ActiveRecord
association.reader
}.compact
- rhs_records.sort_by { |rhs| record_offset[rhs] }
+ # Respect the order on `reflection_scope` if it exists, else use the natural order.
+ if reflection_scope.values[:order].present?
+ @id_map ||= id_to_index_map @preloaded_records
+ rhs_records.sort_by { |rhs| @id_map[rhs] }
+ else
+ rhs_records
+ end
end
- }
+ end
end
private
+ def id_to_index_map(ids)
+ id_map = {}
+ ids.each_with_index { |id, index| id_map[id] = index }
+ id_map
+ end
+
def reset_association(owners, association_name)
should_reset = (through_scope != through_reflection.klass.unscoped) ||
(reflection.options[:source_type] && through_reflection.collection?)
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 7d8b933992..79e660c7e5 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -743,6 +743,38 @@ class EagerAssociationTest < ActiveRecord::TestCase
}
end
+ def test_eager_has_many_through_with_order
+ tag = OrderedTag.create(name: 'Foo')
+ post1 = Post.create!(title: 'Beaches', body: "I like beaches!")
+ post2 = Post.create!(title: 'Pools', body: "I like pools!")
+
+ Tagging.create!(taggable_type: 'Post', taggable_id: post1.id, tag: tag)
+ Tagging.create!(taggable_type: 'Post', taggable_id: post2.id, tag: tag)
+
+ tag_with_includes = OrderedTag.includes(:tagged_posts).find(tag.id)
+ assert_equal(tag_with_includes.taggings.map(&:taggable).map(&:title), tag_with_includes.tagged_posts.map(&:title))
+ end
+
+ def test_eager_has_many_through_multiple_with_order
+ tag1 = OrderedTag.create!(name: 'Bar')
+ tag2 = OrderedTag.create!(name: 'Foo')
+
+ post1 = Post.create!(title: 'Beaches', body: "I like beaches!")
+ post2 = Post.create!(title: 'Pools', body: "I like pools!")
+
+ Tagging.create!(taggable: post1, tag: tag1)
+ Tagging.create!(taggable: post2, tag: tag1)
+ Tagging.create!(taggable: post2, tag: tag2)
+ Tagging.create!(taggable: post1, tag: tag2)
+
+ tags_with_includes = OrderedTag.where(id: [tag1, tag2].map(&:id)).includes(:tagged_posts).order(:id).to_a
+ tag1_with_includes = tags_with_includes.first
+ tag2_with_includes = tags_with_includes.last
+
+ assert_equal([post2, post1].map(&:title), tag1_with_includes.tagged_posts.map(&:title))
+ assert_equal([post1, post2].map(&:title), tag2_with_includes.tagged_posts.map(&:title))
+ end
+
def test_eager_with_default_scope
developer = EagerDeveloperWithDefaultScope.where(:name => 'David').first
projects = Project.order(:id).to_a
diff --git a/activerecord/test/models/tag.rb b/activerecord/test/models/tag.rb
index 80d4725f7e..b48b9a2155 100644
--- a/activerecord/test/models/tag.rb
+++ b/activerecord/test/models/tag.rb
@@ -5,3 +5,9 @@ class Tag < ActiveRecord::Base
has_many :tagged_posts, :through => :taggings, :source => 'taggable', :source_type => 'Post'
end
+
+class OrderedTag < Tag
+ self.table_name = "tags"
+
+ has_many :taggings, -> { order('taggings.id DESC') }, foreign_key: 'tag_id'
+end