diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index c6820cef8c24f..82f15846ee907 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix N+1 query when fetching preview images for non-image assets + + *Aaron Patterson & Justin Searls* + * Fix all Active Storage database related models to respect `ActiveRecord::Base.table_name_prefix` configuration. diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index 791a74f20ce4e..51a10218e8704 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -42,7 +42,10 @@ class ActiveStorage::Attachment < ActiveStorage::Record # Eager load all variant records on an attachment at once. # # User.first.avatars.with_all_variant_records - scope :with_all_variant_records, -> { includes(blob: { variant_records: { image_attachment: :blob } }) } + scope :with_all_variant_records, -> { includes(blob: { + variant_records: { image_attachment: :blob }, + preview_image_attachment: { blob: { variant_records: { image_attachment: :blob } } } + }) } # Synchronously deletes the attachment and {purges the blob}[rdoc-ref:ActiveStorage::Blob#purge]. def purge diff --git a/activestorage/lib/active_storage/attached/model.rb b/activestorage/lib/active_storage/attached/model.rb index 362140b0c3474..940a823774028 100644 --- a/activestorage/lib/active_storage/attached/model.rb +++ b/activestorage/lib/active_storage/attached/model.rb @@ -116,7 +116,10 @@ def #{name}=(attachable) scope :"with_attached_#{name}", -> { if ActiveStorage.track_variants - includes("#{name}_attachment": { blob: { variant_records: { image_attachment: :blob } } }) + includes("#{name}_attachment": { blob: { + variant_records: { image_attachment: :blob }, + preview_image_attachment: { blob: { variant_records: { image_attachment: :blob } } } + } }) else includes("#{name}_attachment": :blob) end @@ -203,7 +206,10 @@ def #{name}=(attachables) scope :"with_attached_#{name}", -> { if ActiveStorage.track_variants - includes("#{name}_attachments": { blob: { variant_records: { image_attachment: :blob } } }) + includes("#{name}_attachments": { blob: { + variant_records: { image_attachment: :blob }, + preview_image_attachment: { blob: { variant_records: { image_attachment: :blob } } } + } }) else includes("#{name}_attachments": :blob) end diff --git a/activestorage/test/models/variant_with_record_test.rb b/activestorage/test/models/variant_with_record_test.rb index 4b70a69edf385..de78cababfc65 100644 --- a/activestorage/test/models/variant_with_record_test.rb +++ b/activestorage/test/models/variant_with_record_test.rb @@ -105,12 +105,13 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase users.reset assert_no_difference -> { ActiveStorage::VariantRecord.count } do - assert_queries(6) do - # 6 queries: + assert_queries(7) do + # 7 queries: # users x 1 # attachment (cover photos) x 1 # blob for the cover photo x 1 # variant record x 1 + # preview_image_attachments for non-images # attachment x 1 # variant record x 1 users.with_attached_cover_photo.each do |u| @@ -123,6 +124,60 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase end end + def find_keys_for_representation(filename) + user = User.create!(name: "Justin") + + 10.times do + blob = directly_upload_file_blob(filename: filename) + user.highlights_with_variants.attach(blob) + end + + # Force the processing + user.highlights_with_variants.each do |highlight| + highlight.representation(:thumb).processed.key + end + + highlights = User.with_attached_highlights_with_variants.find_by(name: "Justin").highlights_with_variants.to_a + + assert_queries(0) do + highlights.each do |highlight| + highlight.representation(:thumb).key + end + end + end + + def test_with_attached_image_variant_no_n_plus_1 + find_keys_for_representation "racecar.jpg" + end + + def test_with_attached_video_variant_no_n_plus_1 + find_keys_for_representation "video.mp4" + end + + def test_no_n_plus_1_with_all_variant_records_on_attached_video + user = User.create!(name: "Justin") + + 10.times do + blob = directly_upload_file_blob(filename: "video.mp4") + user.highlights_with_variants.attach(blob) + end + + # Force the processing + user.highlights_with_variants.each do |highlight| + highlight.representation(:thumb).processed.key + end + + user.reload + + highlights = user.highlights_with_variants.with_all_variant_records.to_a + + assert_queries(0) do + highlights.each do |highlight| + highlight.representation(:thumb).key + end + end + end + test "eager loading has_many_attached records" do user = User.create!(name: "Josh") @@ -200,11 +255,12 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase user.reload assert_no_difference -> { ActiveStorage::VariantRecord.count } do - assert_queries(5) do - # 5 queries: + assert_queries(6) do + # 6 queries: # attachments (vlogs) x 1 # blobs for the vlogs x 1 # variant records for the blobs x 1 + # preview_image_attachments for non-images # attachments for the variant records x 1 # blobs for the attachments for the variant records x 1 user.vlogs.with_all_variant_records.each do |vlog| @@ -219,12 +275,13 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase user.reload assert_no_difference -> { ActiveStorage::VariantRecord.count } do - assert_queries(6) do + assert_queries(7) do # 6 queries: # user x 1 # attachments (vlogs) x 1 # blobs for the vlogs x 1 # variant records for the blobs x 1 + # preview_image_attachments for non-images # attachments for the variant records x 1 # blobs for the attachments for the variant records x 1 User.where(id: user.id).with_attached_vlogs.each do |u| @@ -244,11 +301,12 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase # More queries here because we are creating a different variant. # The second time we load this variant, we are back down to just 3 queries. - assert_queries(9, matcher: /SELECT/) do + assert_queries(10, matcher: /SELECT/) do # 9 queries: # attachments (vlogs) initial load x 1 # blob x 1 (gets both records) # variant record x 1 (gets both records) + # preview_image_attachments for non-images # 2x get blob, attachment, variant records again, this happens when loading the new blob inside `VariantWithRecord#key` user.vlogs.with_all_variant_records.each do |vlog| rep = vlog.representation(resize_to_limit: [200, 200]) @@ -260,7 +318,7 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase user.reload - assert_queries(5) do + assert_queries(6) do user.vlogs.with_all_variant_records.each do |vlog| rep = vlog.representation(resize_to_limit: [200, 200]) rep.processed diff --git a/railties/test/application/test_runner_test.rb b/railties/test/application/test_runner_test.rb index 6c3fcf3c0f2a2..c36a4cb147366 100644 --- a/railties/test/application/test_runner_test.rb +++ b/railties/test/application/test_runner_test.rb @@ -799,7 +799,7 @@ def test_verify_fail_fast assert_match %r{Interrupt}, @error_output assert_equal 1, matches[3].to_i - assert matches[1].to_i < 11 + assert_operator matches[1].to_i, :<, 11 end def test_run_in_parallel_with_processes