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
This commit is contained in:
Jacopo 2021-05-19 10:12:15 +02:00
parent 0f5700ac35
commit abeb3eed01
7 changed files with 55 additions and 3 deletions

@ -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`

@ -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 @@ def purge
def purge_later
transaction do
delete
record&.touch
record.touch if record&.persisted?
end
blob&.purge_later
end

@ -293,8 +293,9 @@ def delete
# 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

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

@ -61,6 +61,7 @@ def purge
if attached?
attachment.purge
write_attachment nil
reset_changes
end
end
@ -69,6 +70,7 @@ def purge_later
if attached?
attachment.purge_later
write_attachment nil
reset_changes
end
end

@ -481,6 +481,22 @@ def upload.open
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 @@ def upload.open
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

@ -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)