Merge pull request rails#50758 from rails/fix-video-preview-nplus1

Eagerly load preview images (N+1)

Author: tenderlove

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.
 

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

activestorage/lib/active_storage/attached/model.rb

@@ -126,7 +126,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
@@ -223,7 +226,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

activestorage/test/models/variant_with_record_test.rb

@@ -105,11 +105,12 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase
     users.reset
 
     assert_no_difference -> { ActiveStorage::VariantRecord.count } do
-      assert_queries_count(5) do
-        # 5 queries:
+      assert_queries_count(6) do
+        # 6 queries:
         # 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|
@@ -122,6 +123,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_count(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_count(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")
 
@@ -199,11 +254,12 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase
     user.reload
 
     assert_no_difference -> { ActiveStorage::VariantRecord.count } do
-      assert_queries_count(5) do
-        # 5 queries:
+      assert_queries_count(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|
@@ -218,12 +274,13 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase
     user.reload
 
     assert_no_difference -> { ActiveStorage::VariantRecord.count } do
-      assert_queries_count(6) do
-        # 6 queries:
+      assert_queries_count(7) do
+        # 7 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|
@@ -243,11 +300,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_match(/SELECT/i, count: 9) do
+      assert_queries_match(/SELECT/i, count: 10) 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])
@@ -259,7 +317,7 @@ class ActiveStorage::VariantWithRecordTest < ActiveSupport::TestCase
 
       user.reload
 
-      assert_queries_count(5) do
+      assert_queries_count(6) do
         user.vlogs.with_all_variant_records.each do |vlog|
           rep = vlog.representation(resize_to_limit: [200, 200])
           rep.processed

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