Merge pull request #42383 from intrip/has-many-attached-attachment-not-persisted

Fix ActiveStorage has_many_attached when record is not persisted

activestorage/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Allow to purge an attachment when record is not persisted for `has_many_attached`
+
+    *Jacopo Beschi*
+
 *   Add `with_all_variant_records` method to eager load all variant records on an attachment at once.
     `with_attached_image` scope now eager loads variant records if using variant tracking.
 

activestorage/lib/active_storage/attached.rb

@@ -16,10 +16,6 @@ module ActiveStorage
       def change
         record.attachment_changes[name]
       end
-
-      def reset_changes
-        record.attachment_changes.delete(name)
-      end
   end
 end
 

activestorage/lib/active_storage/attached/changes.rb

@@ -11,6 +11,9 @@ module ActiveStorage
 
       autoload :DeleteOne
       autoload :DeleteMany
+
+      autoload :PurgeOne
+      autoload :PurgeMany
     end
   end
 end

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

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::PurgeMany #:nodoc:
+    attr_reader :name, :record, :attachments
+
+    def initialize(name, record, attachments)
+      @name, @record, @attachments = name, record, attachments
+    end
+
+    def purge
+      attachments.each(&:purge)
+      reset
+    end
+
+    def purge_later
+      attachments.each(&:purge_later)
+      reset
+    end
+
+    private
+      def reset
+        record.attachment_changes.delete(name)
+        record.public_send("#{name}_attachments").reset
+      end
+  end
+end

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

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::PurgeOne #:nodoc:
+    attr_reader :name, :record, :attachment
+
+    def initialize(name, record, attachment)
+      @name, @record, @attachment = name, record, attachment
+    end
+
+    def purge
+      attachment&.purge
+      reset
+    end
+
+    def purge_later
+      attachment&.purge_later
+      reset
+    end
+
+    private
+      def reset
+        record.attachment_changes.delete(name)
+        record.public_send("#{name}_attachment=", nil)
+      end
+  end
+end

activestorage/lib/active_storage/attached/many.rb

@@ -3,6 +3,19 @@
 module ActiveStorage
   # Decorated proxy object representing of multiple attachments to a model.
   class Attached::Many < Attached
+    ##
+    # :method: purge
+    #
+    # Directly purges each associated attachment (i.e. destroys the blobs and
+    # attachments and deletes the files on the service).
+    delegate :purge, to: :purge_many
+
+    ##
+    # :method: purge_later
+    #
+    # Purges each associated attachment through the queuing system.
+    delegate :purge_later, to: :purge_many
+
     delegate_missing_to :attachments
 
     # Returns all the associated attachment records.
@@ -52,15 +65,9 @@ module ActiveStorage
       attachments.delete_all if attached?
     end
 
-    ##
-    # :method: purge
-    #
-    # Directly purges each associated attachment (i.e. destroys the blobs and
-    # attachments and deletes the files on the service).
-
-    ##
-    # :method: purge_later
-    #
-    # Purges each associated attachment through the queuing system.
+    private
+      def purge_many
+        Attached::Changes::PurgeMany.new(name, record, attachments)
+      end
   end
 end

activestorage/lib/active_storage/attached/one.rb

@@ -3,6 +3,19 @@
 module ActiveStorage
   # Representation of a single attachment to a model.
   class Attached::One < Attached
+    ##
+    # :method: purge
+    #
+    # Directly purges the attachment (i.e. destroys the blob and
+    # attachment and deletes the file on the service).
+    delegate :purge, to: :purge_one
+
+    ##
+    # :method: purge_later
+    #
+    # Purges the attachment through the queuing system.
+    delegate :purge_later, to: :purge_one
+
     delegate_missing_to :attachment, allow_nil: true
 
     # Returns the associated attachment record.
@@ -62,28 +75,13 @@ module ActiveStorage
       end
     end
 
-    # Directly purges the attachment (i.e. destroys the blob and
-    # attachment and deletes the file on the service).
-    def purge
-      if attached?
-        attachment.purge
-        write_attachment nil
-        reset_changes
-      end
-    end
-
-    # Purges the attachment through the queuing system.
-    def purge_later
-      if attached?
-        attachment.purge_later
-        write_attachment nil
-        reset_changes
-      end
-    end
-
     private
       def write_attachment(attachment)
         record.public_send("#{name}_attachment=", attachment)
       end
+
+      def purge_one
+        Attached::Changes::PurgeOne.new(name, record, attachment)
+      end
   end
 end

activestorage/test/models/attached/many_test.rb

@@ -483,6 +483,33 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "purging when record is not persisted" do
+    [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
+      user = User.new
+      user.highlights.attach blobs
+      assert user.highlights.attached?
+
+      attachments = user.highlights.attachments
+      user.highlights.purge
+
+      assert_not user.highlights.attached?
+      assert attachments.all?(&:destroyed?)
+      blobs.each do |blob|
+        assert_not ActiveStorage::Blob.exists?(blob.id)
+        assert_not ActiveStorage::Blob.service.exist?(blob.key)
+      end
+    end
+  end
+
+  test "purging delete changes when record is not persisted" do
+    user = User.new
+    user.highlights = []
+
+    user.highlights.purge
+
+    assert_nil user.attachment_changes["highlights"]
+  end
+
   test "purging later" do
     [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
       @user.highlights.attach blobs
@@ -531,6 +558,24 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "purging attachment later when record is not persisted" do
+    [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
+      user = User.new
+      user.highlights.attach blobs
+      assert user.highlights.attached?
+
+      perform_enqueued_jobs do
+        user.highlights.purge_later
+      end
+
+      assert_not user.highlights.attached?
+      blobs.each do |blob|
+        assert_not ActiveStorage::Blob.exists?(blob.id)
+        assert_not ActiveStorage::Blob.service.exist?(blob.key)
+      end
+    end
+  end
+
   test "purging dependent attachment later on destroy" do
     [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
       @user.highlights.attach blobs

activestorage/test/models/attached/one_test.rb

@@ -497,6 +497,15 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "purging delete changes when record is not persisted" do
+    user = User.new
+    user.avatar = nil
+
+    user.avatar.purge
+
+    assert_nil user.attachment_changes["avatar"]
+  end
+
   test "purging later" do
     create_blob(filename: "funky.jpg").tap do |blob|
       @user.avatar.attach blob