From abeb3eed01f2fc669d4772707761b5c3c054ae73 Mon Sep 17 00:00:00 2001 From: Jacopo Date: Wed, 19 May 2021 10:12:15 +0200 Subject: [PATCH] Fix ActiveStorage has_one_attached when blob or record are not persisted - Purging a not persisted blob no longer raise a `FrozenError`. - Purging a not persisted record no longer raise an `ActiveRecord::ActiveRecordError`. Fixes #37069 --- activestorage/CHANGELOG.md | 4 +++ .../app/models/active_storage/attachment.rb | 4 +-- .../app/models/active_storage/blob.rb | 3 +- activestorage/lib/active_storage/attached.rb | 4 +++ .../lib/active_storage/attached/one.rb | 2 ++ .../test/models/attached/one_test.rb | 34 +++++++++++++++++++ activestorage/test/models/blob_test.rb | 7 ++++ 7 files changed, 55 insertions(+), 3 deletions(-) diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 5705649c35..5710898d8a 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,7 @@ +* Allow to purge an attachment when record is not persisted for `has_one_attached` + + *Jacopo Beschi* + * Add a load hook called `active_storage_variant_record` (providing `ActiveStorage::VariantRecord`) to allow for overriding aspects of the `ActiveStorage::VariantRecord` class. This makes `ActiveStorage::VariantRecord` consistent with `ActiveStorage::Blob` and `ActiveStorage::Attachment` diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index 4bde8d9bce..b2488c5d2e 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -23,7 +23,7 @@ class ActiveStorage::Attachment < ActiveStorage::Record def purge transaction do delete - record&.touch + record.touch if record&.persisted? end blob&.purge end @@ -32,7 +32,7 @@ class ActiveStorage::Attachment < ActiveStorage::Record def purge_later transaction do delete - record&.touch + record.touch if record&.persisted? end blob&.purge_later end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 761408e571..a570157670 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -293,8 +293,9 @@ class ActiveStorage::Blob < ActiveStorage::Record # blobs. Note, though, that deleting the file off the service will initiate an HTTP connection to the service, which may # be slow or prevented, so you should not use this method inside a transaction or in callbacks. Use #purge_later instead. def purge + previously_persisted = persisted? destroy - delete + delete if previously_persisted rescue ActiveRecord::InvalidForeignKey end diff --git a/activestorage/lib/active_storage/attached.rb b/activestorage/lib/active_storage/attached.rb index b540f85fbe..b72b584652 100644 --- a/activestorage/lib/active_storage/attached.rb +++ b/activestorage/lib/active_storage/attached.rb @@ -16,6 +16,10 @@ module ActiveStorage def change record.attachment_changes[name] end + + def reset_changes + record.attachment_changes.delete(name) + end end end diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb index 73527b08d2..ab5a671fe5 100644 --- a/activestorage/lib/active_storage/attached/one.rb +++ b/activestorage/lib/active_storage/attached/one.rb @@ -61,6 +61,7 @@ module ActiveStorage if attached? attachment.purge write_attachment nil + reset_changes end end @@ -69,6 +70,7 @@ module ActiveStorage if attached? attachment.purge_later write_attachment nil + reset_changes end end diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb index 6c5f885bb5..cfa4d2a5cb 100644 --- a/activestorage/test/models/attached/one_test.rb +++ b/activestorage/test/models/attached/one_test.rb @@ -481,6 +481,22 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase end end + test "purging when record is not persisted" do + create_blob(filename: "funky.jpg").tap do |blob| + user = User.new + user.avatar.attach blob + assert user.avatar.attached? + + attachment = user.avatar.attachment + user.avatar.purge + + assert_not user.avatar.attached? + assert attachment.destroyed? + assert_not ActiveStorage::Blob.exists?(blob.id) + assert_not ActiveStorage::Blob.service.exist?(blob.key) + end + end + test "purging later" do create_blob(filename: "funky.jpg").tap do |blob| @user.avatar.attach blob @@ -517,6 +533,24 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase end end + test "purging an attachment later when record is not persisted" do + create_blob(filename: "funky.jpg").tap do |blob| + user = User.new + user.avatar.attach blob + assert user.avatar.attached? + + attachment = user.avatar.attachment + perform_enqueued_jobs do + user.avatar.purge_later + end + + assert_not user.avatar.attached? + assert attachment.destroyed? + assert_not ActiveStorage::Blob.exists?(blob.id) + assert_not ActiveStorage::Blob.service.exist?(blob.key) + end + end + test "purging dependent attachment later on destroy" do create_blob(filename: "funky.jpg").tap do |blob| @user.avatar.attach blob diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 54588fc27a..21d43b7c0c 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -245,6 +245,13 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end end + test "purge doesn't raise when blob is not persisted" do + build_blob_after_unfurling.tap do |blob| + assert_nothing_raised { blob.purge } + assert blob.destroyed? + end + end + test "uses service from blob when provided" do with_service("mirror") do blob = create_blob(filename: "funky.jpg", service_name: :local)