diff options
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 |