Merge pull request rails#50082 from chaadow/discard_unrepresentable_blobs_while_preprocessing

[ActiveStorage] Discard unrepresentable blobs while preprocessing

Author: jonathanhefner

activestorage/app/jobs/active_storage/transform_job.rb

@@ -3,7 +3,7 @@
 class ActiveStorage::TransformJob < ActiveStorage::BaseJob
   queue_as { ActiveStorage.queues[:transform] }
 
-  discard_on ActiveRecord::RecordNotFound
+  discard_on ActiveRecord::RecordNotFound, ActiveStorage::UnrepresentableError
   retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer
 
   def perform(blob, transformations)

activestorage/app/models/active_storage/blob/representable.rb

@@ -99,7 +99,7 @@ def representable?
   end
 
   def preprocessed(transformations) # :nodoc:
-    ActiveStorage::TransformJob.perform_later(self, transformations)
+    ActiveStorage::TransformJob.perform_later(self, transformations) if representable?
   end
 
   private

activestorage/test/jobs/transform_job_test.rb

@@ -44,4 +44,15 @@ class ActiveStorage::TransformJobTest < ActiveJob::TestCase
       ActiveStorage.track_variants = @was_tracking
     end
   end
+
+  test "ignores unrepresentable blob" do
+    unrepresentable_blob = create_blob(content_type: "text/plain")
+    transformations = { resize_to_limit: [100, 100] }
+
+    perform_enqueued_jobs do
+      assert_nothing_raised do
+        ActiveStorage::TransformJob.perform_later unrepresentable_blob, transformations
+      end
+    end
+  end
 end

activestorage/test/models/attached/many_test.rb

@@ -917,7 +917,7 @@ def self.name; superclass.name; end
   end
 
   test "transforms variants later" do
-    blob = create_blob(filename: "funky.jpg")
+    blob = create_file_blob(filename: "racecar.jpg")
 
     assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [1, 1]] do
       @user.highlights_with_preprocessed.attach blob
@@ -926,10 +926,10 @@ def self.name; superclass.name; end
 
   test "transforms variants later conditionally via proc" do
     assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
-      @user.highlights_with_conditional_preprocessed.attach create_blob(filename: "funky.jpg")
+      @user.highlights_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
     end
 
-    blob = create_blob(filename: "funky.jpg")
+    blob = create_file_blob(filename: "racecar.jpg")
     @user.update(name: "transform via proc")
 
     assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [2, 2]] do
@@ -939,17 +939,25 @@ def self.name; superclass.name; end
 
   test "transforms variants later conditionally via method" do
     assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
-      @user.highlights_with_conditional_preprocessed.attach create_blob(filename: "funky.jpg")
+      @user.highlights_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
     end
 
-    blob = create_blob(filename: "funky.jpg")
+    blob = create_file_blob(filename: "racecar.jpg")
     @user.update(name: "transform via method")
 
     assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [3, 3]] do
       @user.highlights_with_conditional_preprocessed.attach blob
     end
   end
 
+  test "avoids enqueuing transform later job when blob is not representable" do
+    unrepresentable_blob = create_blob(filename: "hello.txt")
+
+    assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
+      @user.highlights_with_preprocessed.attach unrepresentable_blob
+    end
+  end
+
   test "successfully attaches new blobs and destroys attachments marked for destruction via nested attributes" do
     town_blob = create_blob(filename: "town.jpg")
     @user.highlights.attach(town_blob)

activestorage/test/models/attached/one_test.rb

@@ -859,7 +859,7 @@ def self.name; superclass.name; end
   end
 
   test "transforms variants later" do
-    blob = create_blob(filename: "funky.jpg")
+    blob = create_file_blob(filename: "racecar.jpg")
 
     assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [1, 1]] do
       @user.avatar_with_preprocessed.attach blob
@@ -868,10 +868,10 @@ def self.name; superclass.name; end
 
   test "transforms variants later conditionally via proc" do
     assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
-      @user.avatar_with_conditional_preprocessed.attach create_blob(filename: "funky.jpg")
+      @user.avatar_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
     end
 
-    blob = create_blob(filename: "funky.jpg")
+    blob = create_file_blob(filename: "racecar.jpg")
     @user.update(name: "transform via proc")
 
     assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [2, 2]] do
@@ -881,14 +881,22 @@ def self.name; superclass.name; end
 
   test "transforms variants later conditionally via method" do
     assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
-      @user.avatar_with_conditional_preprocessed.attach create_blob(filename: "funky.jpg")
+      @user.avatar_with_conditional_preprocessed.attach create_file_blob(filename: "racecar.jpg")
     end
 
-    blob = create_blob(filename: "funky.jpg")
+    blob = create_file_blob(filename: "racecar.jpg")
     @user.update(name: "transform via method")
 
     assert_enqueued_with job: ActiveStorage::TransformJob, args: [blob, resize_to_limit: [3, 3]] do
       @user.avatar_with_conditional_preprocessed.attach blob
     end
   end
+
+  test "avoids enqueuing transform later job when blob is not representable" do
+    unrepresentable_blob = create_blob(filename: "hello.txt")
+
+    assert_no_enqueued_jobs only: ActiveStorage::TransformJob do
+      @user.avatar_with_preprocessed.attach unrepresentable_blob
+    end
+  end
 end