From 9cc88043e70f927a3c8b151c862f6b3cb8b8a6f7 Mon Sep 17 00:00:00 2001 From: George Claghorn Date: Mon, 5 Mar 2018 17:01:31 -0500 Subject: Fix purging dependent blobs when attachments aren't loaded --- .../lib/active_storage/attached/macros.rb | 8 ++++-- activestorage/lib/active_storage/attached/one.rb | 32 ++++++++-------------- activestorage/test/models/attachments_test.rb | 18 ++++++++++-- activestorage/test/test_helper.rb | 2 ++ 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 -- cgit v1.2.3