Fix analyzing new blobs from uploaded files on attach

3 files changed, 120 insertions, 18 deletions

diff --git a/activestorage/lib/active_storage/attached/model.rb b/activestorage/lib/active_storage/attached/model.rb
index 287c7a9253..1cdaac2e32 100644
--- a/activestorage/lib/active_storage/attached/model.rb
+++ b/activestorage/lib/active_storage/attached/model.rb
@@ -53,6 +53,8 @@ module ActiveStorage
 
         after_save { attachment_changes[name.to_s]&.save }
 
+        after_commit(on: %i[ create update ]) { attachment_changes.delete(name.to_s).try(:upload) }
+
         ActiveRecord::Reflection.add_attachment_reflection(
           self,
           name,
@@ -117,6 +119,8 @@ module ActiveStorage
 
         after_save { attachment_changes[name.to_s]&.save }
 
+        after_commit(on: %i[ create update ]) { attachment_changes.delete(name.to_s).try(:upload) }
+
         ActiveRecord::Reflection.add_attachment_reflection(
           self,
           name,
@@ -125,26 +129,8 @@ module ActiveStorage
       end
     end
 
-    def committed!(*) #:nodoc:
-      unless destroyed?
-        upload_attachment_changes
-        clear_attachment_changes
-      end
-
-      super
-    end
-
     def attachment_changes #:nodoc:
       @attachment_changes ||= {}
     end
-
-    private
-      def upload_attachment_changes
-        attachment_changes.each_value { |change| change.try(:upload) }
-      end
-
-      def clear_attachment_changes
-        @attachment_changes = {}
-      end
   end
 end
diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb
index bc44e9da68..c4510a9e84 100644
--- a/activestorage/test/models/attached/many_test.rb
+++ b/activestorage/test/models/attached/many_test.rb
@@ -156,6 +156,46 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "analyzing a new blob from an uploaded file after attaching it to an existing record" do
+    perform_enqueued_jobs do
+      @user.highlights.attach fixture_file_upload("racecar.jpg")
+    end
+
+    assert @user.highlights.reload.first.analyzed?
+    assert_equal 4104, @user.highlights.first.metadata[:width]
+    assert_equal 2736, @user.highlights.first.metadata[:height]
+  end
+
+  test "analyzing a new blob from an uploaded file after attaching it to an existing record via update" do
+    perform_enqueued_jobs do
+      @user.update! highlights: [ fixture_file_upload("racecar.jpg") ]
+    end
+
+    assert @user.highlights.reload.first.analyzed?
+    assert_equal 4104, @user.highlights.first.metadata[:width]
+    assert_equal 2736, @user.highlights.first.metadata[:height]
+  end
+
+  test "analyzing a directly-uploaded blob after attaching it to an existing record" do
+    perform_enqueued_jobs do
+      @user.highlights.attach directly_upload_file_blob(filename: "racecar.jpg")
+    end
+
+    assert @user.highlights.reload.first.analyzed?
+    assert_equal 4104, @user.highlights.first.metadata[:width]
+    assert_equal 2736, @user.highlights.first.metadata[:height]
+  end
+
+  test "analyzing a directly-uploaded blob after attaching it to an existing record via update" do
+    perform_enqueued_jobs do
+      @user.update! highlights: [ directly_upload_file_blob(filename: "racecar.jpg") ]
+    end
+
+    assert @user.highlights.reload.first.analyzed?
+    assert_equal 4104, @user.highlights.first.metadata[:width]
+    assert_equal 2736, @user.highlights.first.metadata[:height]
+  end
+
   test "attaching existing blobs to a new record" do
     User.new(name: "Jason").tap do |user|
       user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")
@@ -257,6 +297,24 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     assert_equal "Could not find or build blob: expected attachable, got :foo", error.message
   end
 
+  test "analyzing a new blob from an uploaded file after attaching it to a new record" do
+    perform_enqueued_jobs do
+      user = User.create!(name: "Jason", highlights: [ fixture_file_upload("racecar.jpg") ])
+      assert user.highlights.reload.first.analyzed?
+      assert_equal 4104, user.highlights.first.metadata[:width]
+      assert_equal 2736, user.highlights.first.metadata[:height]
+    end
+  end
+
+  test "analyzing a directly-uploaded blob after attaching it to a new record" do
+    perform_enqueued_jobs do
+      user = User.create!(name: "Jason", highlights: [ directly_upload_file_blob(filename: "racecar.jpg") ])
+      assert user.highlights.reload.first.analyzed?
+      assert_equal 4104, user.highlights.first.metadata[:width]
+      assert_equal 2736, user.highlights.first.metadata[:height]
+    end
+  end
+
   test "purging" do
     [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
       @user.highlights.attach blobs
diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb
index 8654ecffef..716190bc53 100644
--- a/activestorage/test/models/attached/one_test.rb
+++ b/activestorage/test/models/attached/one_test.rb
@@ -181,6 +181,46 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "analyzing a new blob from an uploaded file after attaching it to an existing record" do
+    perform_enqueued_jobs do
+      @user.avatar.attach fixture_file_upload("racecar.jpg")
+    end
+
+    assert @user.avatar.reload.analyzed?
+    assert_equal 4104, @user.avatar.metadata[:width]
+    assert_equal 2736, @user.avatar.metadata[:height]
+  end
+
+  test "analyzing a new blob from an uploaded file after attaching it to an existing record via update" do
+    perform_enqueued_jobs do
+      @user.update! avatar: fixture_file_upload("racecar.jpg")
+    end
+
+    assert @user.avatar.reload.analyzed?
+    assert_equal 4104, @user.avatar.metadata[:width]
+    assert_equal 2736, @user.avatar.metadata[:height]
+  end
+
+  test "analyzing a directly-uploaded blob after attaching it to an existing record" do
+    perform_enqueued_jobs do
+      @user.avatar.attach directly_upload_file_blob(filename: "racecar.jpg")
+    end
+
+    assert @user.avatar.reload.analyzed?
+    assert_equal 4104, @user.avatar.metadata[:width]
+    assert_equal 2736, @user.avatar.metadata[:height]
+  end
+
+  test "analyzing a directly-uploaded blob after attaching it to an existing record via updates" do
+    perform_enqueued_jobs do
+      @user.update! avatar: directly_upload_file_blob(filename: "racecar.jpg")
+    end
+
+    assert @user.avatar.reload.analyzed?
+    assert_equal 4104, @user.avatar.metadata[:width]
+    assert_equal 2736, @user.avatar.metadata[:height]
+  end
+
   test "attaching an existing blob to a new record" do
     User.new(name: "Jason").tap do |user|
       user.avatar.attach create_blob(filename: "funky.jpg")
@@ -259,6 +299,24 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     assert_equal "Could not find or build blob: expected attachable, got :foo", error.message
   end
 
+  test "analyzing a new blob from an uploaded file after attaching it to a new record" do
+    perform_enqueued_jobs do
+      user = User.create!(name: "Jason", avatar: fixture_file_upload("racecar.jpg"))
+      assert user.avatar.reload.analyzed?
+      assert_equal 4104, user.avatar.metadata[:width]
+      assert_equal 2736, user.avatar.metadata[:height]
+    end
+  end
+
+  test "analyzing a directly-uploaded blob after attaching it to a new record" do
+    perform_enqueued_jobs do
+      user = User.create!(name: "Jason", avatar: directly_upload_file_blob(filename: "racecar.jpg"))
+      assert user.avatar.reload.analyzed?
+      assert_equal 4104, user.avatar.metadata[:width]
+      assert_equal 2736, user.avatar.metadata[:height]
+    end
+  end
+
   test "purging" do
     create_blob(filename: "funky.jpg").tap do |blob|
       @user.avatar.attach blob