Fix #41661 attaching multiple times within transaction

Co-authored-by: Abhay Nikam <nikam.abhay1@gmail.com>
Co-authored-by: Bruno Vezoli <brunvezoli@hotmail.com>
Co-authored-by: Juan Eduardo Roig <jeroig@gmail.com>

activestorage/CHANGELOG.md

@@ -1,3 +1,31 @@
+*   Fixes multiple `attach` calls within transaction not uploading files correctly.
+
+    In the following example, the code failed to upload all but the last file to the configured service.
+    ```ruby
+      ActiveRecord::Base.transaction do
+        user.attachments.attach({
+          content_type: "text/plain",
+          filename: "dummy.txt",
+          io: ::StringIO.new("dummy"),
+        })
+        user.attachments.attach({
+          content_type: "text/plain",
+          filename: "dummy2.txt",
+          io: ::StringIO.new("dummy2"),
+        })
+      end
+
+      assert_equal 2, user.attachments.count
+      assert user.attachments.first.service.exist?(user.attachments.first.key)  # Fails
+    ```
+
+    This was addressed by keeping track of the subchanges pending upload, and uploading them
+    once the transaction is committed.
+
+    Fixes #41661
+
+    *Santiago Bartesaghi*, *Bruno Vezoli*, *Juan Roig*, *Abhay Nikam*
+
 *   Raise an exception if `config.active_storage.service` is not set.
 
     If Active Storage is configured and `config.active_storage.service` is not

activestorage/lib/active_storage/attached/changes/create_many.rb

@@ -2,11 +2,12 @@
 
 module ActiveStorage
   class Attached::Changes::CreateMany # :nodoc:
-    attr_reader :name, :record, :attachables
+    attr_reader :name, :record, :attachables, :pending_uploads
 
-    def initialize(name, record, attachables)
+    def initialize(name, record, attachables, pending_uploads: [])
       @name, @record, @attachables = name, record, Array(attachables)
       blobs.each(&:identify_without_saving)
+      @pending_uploads = Array(pending_uploads) + subchanges_without_blobs
       attachments
     end
 
@@ -19,7 +20,7 @@ module ActiveStorage
     end
 
     def upload
-      subchanges.each(&:upload)
+      pending_uploads.each(&:upload)
     end
 
     def save
@@ -36,6 +37,10 @@ module ActiveStorage
         ActiveStorage::Attached::Changes::CreateOneOfMany.new(name, record, attachable)
       end
 
+      def subchanges_without_blobs
+        subchanges.reject { |subchange| subchange.attachable.is_a?(ActiveStorage::Blob) }
+      end
+
       def assign_associated_attachments
         record.public_send("#{name}_attachments=", persisted_or_new_attachments)
       end

activestorage/lib/active_storage/attached/model.rb

@@ -138,13 +138,14 @@ module ActiveStorage
 
           def #{name}=(attachables)
             attachables = Array(attachables).compact_blank
+            pending_uploads = attachment_changes["#{name}"].try(:pending_uploads)
 
             if ActiveStorage.replace_on_assign_to_many
               attachment_changes["#{name}"] =
                 if attachables.none?
                   ActiveStorage::Attached::Changes::DeleteMany.new("#{name}", self)
                 else
-                  ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables)
+                  ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables, pending_uploads: pending_uploads)
                 end
             else
               ActiveSupport::Deprecation.warn \
@@ -155,7 +156,7 @@ module ActiveStorage
 
               if attachables.any?
                 attachment_changes["#{name}"] =
-                  ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, #{name}.blobs + attachables)
+                  ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, #{name}.blobs + attachables, pending_uploads: pending_uploads)
               end
             end
           end

activestorage/test/models/attached/many_test.rb

@@ -132,6 +132,93 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     assert ActiveStorage::Blob.service.exist?(@user.highlights.second.key)
   end
 
+  test "attaching new blobs within a transaction uploads all the files" do
+    @user.highlights.attach fixture_file_upload("image.gif")
+
+    ActiveRecord::Base.transaction do
+      @user.highlights.attach fixture_file_upload("racecar.jpg")
+      @user.highlights.attach fixture_file_upload("video.mp4")
+    end
+
+    assert_equal "image.gif", @user.highlights.first.filename.to_s
+    assert_equal "racecar.jpg", @user.highlights.second.filename.to_s
+    assert_equal "video.mp4", @user.highlights.third.filename.to_s
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.first.key)
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.second.key)
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.third.key)
+  end
+
+  test "attaching many new blobs within a transaction uploads all the files" do
+    ActiveRecord::Base.transaction do
+      @user.highlights.attach [fixture_file_upload("image.gif"), fixture_file_upload("racecar.jpg")]
+      @user.highlights.attach fixture_file_upload("video.mp4")
+    end
+
+    assert_equal "image.gif", @user.highlights.first.filename.to_s
+    assert_equal "racecar.jpg", @user.highlights.second.filename.to_s
+    assert_equal "video.mp4", @user.highlights.third.filename.to_s
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.first.key)
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.second.key)
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.third.key)
+  end
+
+  test "attaching many new blobs within a transaction on a dirty record uploads all the files" do
+    @user.name = "Tina"
+
+    ActiveRecord::Base.transaction do
+      @user.highlights.attach fixture_file_upload("image.gif")
+      @user.highlights.attach fixture_file_upload("racecar.jpg")
+    end
+
+    @user.highlights.attach fixture_file_upload("video.mp4")
+    @user.save
+
+    assert_equal "image.gif", @user.highlights.first.filename.to_s
+    assert_equal "racecar.jpg", @user.highlights.second.filename.to_s
+    assert_equal "video.mp4", @user.highlights.third.filename.to_s
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.first.key)
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.second.key)
+    assert ActiveStorage::Blob.service.exist?(@user.highlights.third.key)
+  end
+
+  test "attaching new blobs within a transaction create the exact amount of records" do
+    assert_difference -> { ActiveStorage::Blob.count }, +2 do
+      ActiveRecord::Base.transaction do
+        @user.highlights.attach fixture_file_upload("racecar.jpg")
+        @user.highlights.attach fixture_file_upload("video.mp4")
+      end
+    end
+
+    assert_equal 2, @user.highlights.count
+  end
+
+  test "attaching new blobs within a transaction with append_on_assign config uploads all the files" do
+    append_on_assign do
+      ActiveRecord::Base.transaction do
+        @user.highlights.attach fixture_file_upload("racecar.jpg")
+        @user.highlights.attach fixture_file_upload("video.mp4")
+      end
+
+      assert_equal "racecar.jpg", @user.highlights.first.filename.to_s
+      assert_equal "video.mp4", @user.highlights.second.filename.to_s
+      assert ActiveStorage::Blob.service.exist?(@user.highlights.first.key)
+      assert ActiveStorage::Blob.service.exist?(@user.highlights.second.key)
+    end
+  end
+
+  test "attaching new blobs within a transaction with append_on_assign config create the exact amount of records" do
+    append_on_assign do
+      assert_difference -> { ActiveStorage::Blob.count }, +2 do
+        ActiveRecord::Base.transaction do
+          @user.highlights.attach fixture_file_upload("racecar.jpg")
+          @user.highlights.attach fixture_file_upload("video.mp4")
+        end
+      end
+
+      assert_equal 2, @user.highlights.count
+    end
+  end
+
   test "attaching existing blobs to an existing record one at a time" do
     @user.highlights.attach create_blob(filename: "funky.jpg")
     @user.highlights.attach create_blob(filename: "town.jpg")