Fix purging dependent blobs when attachments aren't loaded

4 files changed, 35 insertions, 25 deletions

diff --git a/activestorage/lib/active_storage/attached/macros.rb b/activestorage/lib/active_storage/attached/macros.rb
index a87f02efe1..1bda86369e 100644
--- a/activestorage/lib/active_storage/attached/macros.rb
+++ b/activestorage/lib/active_storage/attached/macros.rb
@@ -38,13 +38,15 @@ module ActiveStorage
         end
       CODE
 
-      has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record, inverse_of: :record, dependent: :delete
+      has_one :"#{name}_attachment", -> { where(name: name) }, class_name: "ActiveStorage::Attachment", as: :record, inverse_of: :record, dependent: false
       has_one :"#{name}_blob", through: :"#{name}_attachment", class_name: "ActiveStorage::Blob", source: :blob
 
       scope :"with_attached_#{name}", -> { includes("#{name}_attachment": :blob) }
 
       if dependent == :purge_later
         after_destroy_commit { public_send(name).purge_later }
+      else
+        before_destroy { public_send(name).detach }
       end
     end
 
@@ -83,13 +85,15 @@ module ActiveStorage
         end
       CODE
 
-      has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment", inverse_of: :record, dependent: :delete_all
+      has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment", inverse_of: :record, dependent: false
       has_many :"#{name}_blobs", through: :"#{name}_attachments", class_name: "ActiveStorage::Blob", source: :blob
 
       scope :"with_attached_#{name}", -> { includes("#{name}_attachments": :blob) }
 
       if dependent == :purge_later
         after_destroy_commit { public_send(name).purge_later }
+      else
+        before_destroy { public_send(name).detach }
       end
     end
   end
diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb
index 008f5a796b..a582ed0137 100644
--- a/activestorage/lib/active_storage/attached/one.rb
+++ b/activestorage/lib/active_storage/attached/one.rb
@@ -20,10 +20,16 @@ module ActiveStorage
     #   person.avatar.attach(io: File.open("/path/to/face.jpg"), filename: "face.jpg", content_type: "image/jpg")
     #   person.avatar.attach(avatar_blob) # ActiveStorage::Blob object
     def attach(attachable)
-      if attached? && dependent == :purge_later
-        replace attachable
-      else
-        write_attachment build_attachment_from(attachable)
+      blob_was = blob if attached?
+      blob = create_blob_from(attachable)
+
+      unless blob == blob_was
+        transaction do
+          detach
+          write_attachment build_attachment(blob: blob)
+        end
+
+        blob_was.purge_later if blob_was && dependent == :purge_later
       end
     end
 
@@ -63,23 +69,7 @@ module ActiveStorage
     end
 
     private
-      def replace(attachable)
-        blob_was = blob
-        blob = create_blob_from(attachable)
-
-        unless blob == blob_was
-          transaction do
-            detach
-            write_attachment build_attachment(blob: blob)
-          end
-
-          blob_was.purge_later
-        end
-      end
-
-      def build_attachment_from(attachable)
-        build_attachment blob: create_blob_from(attachable)
-      end
+      delegate :transaction, to: :record
 
       def build_attachment(blob:)
         ActiveStorage::Attachment.new(record: record, name: name, blob: blob)
diff --git a/activestorage/test/models/attachments_test.rb b/activestorage/test/models/attachments_test.rb
index a5fcad1bab..36e76f2845 100644
--- a/activestorage/test/models/attachments_test.rb
+++ b/activestorage/test/models/attachments_test.rb
@@ -217,13 +217,20 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     avatar_key = @user.avatar.key
 
     perform_enqueued_jobs do
-      @user.destroy
+      @user.reload.destroy
 
       assert_nil ActiveStorage::Blob.find_by(key: avatar_key)
       assert_not ActiveStorage::Blob.service.exist?(avatar_key)
     end
   end
 
+  test "delete attachment for independent blob when record is destroyed" do
+    @user.cover_photo.attach create_blob(filename: "funky.jpg")
+
+    @user.destroy
+    assert_not ActiveStorage::Attachment.exists?(record: @user, name: "cover_photo")
+  end
+
   test "find with attached blob" do
     records = %w[alice bob].map do |name|
       User.create!(name: name).tap do |user|
@@ -395,7 +402,7 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     highlight_keys = @user.highlights.collect(&:key)
 
     perform_enqueued_jobs do
-      @user.destroy
+      @user.reload.destroy
 
       assert_nil ActiveStorage::Blob.find_by(key: highlight_keys.first)
       assert_not ActiveStorage::Blob.service.exist?(highlight_keys.first)
@@ -404,4 +411,11 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
       assert_not ActiveStorage::Blob.service.exist?(highlight_keys.second)
     end
   end
+
+  test "delete attachments for independent blobs when the record is destroyed" do
+    @user.vlogs.attach create_blob(filename: "funky.mp4"), create_blob(filename: "wonky.mp4")
+
+    @user.destroy
+    assert_not ActiveStorage::Attachment.exists?(record: @user, name: "vlogs")
+  end
 end
diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb
index 9b5cd41335..2a8e153303 100644
--- a/activestorage/test/test_helper.rb
+++ b/activestorage/test/test_helper.rb
@@ -66,5 +66,7 @@ ActiveRecord::Base.send :include, GlobalID::Identification
 class User < ActiveRecord::Base
   has_one_attached :avatar
   has_one_attached :cover_photo, dependent: false
+
   has_many_attached :highlights
+  has_many_attached :vlogs, dependent: false
 end