Skip to content

Commit

Permalink
Fix ActiveStorage has_one_attached when blob or record are not persisted
Browse files Browse the repository at this point in the history
- Purging a not persisted blob no longer raise a `FrozenError`.
- Purging a not persisted record no longer raise an `ActiveRecord::ActiveRecordError`.

Fixes #37069
  • Loading branch information
intrip committed May 31, 2021
1 parent 0f5700a commit abeb3ee
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 3 deletions.
4 changes: 4 additions & 0 deletions 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`
Expand Down
4 changes: 2 additions & 2 deletions activestorage/app/models/active_storage/attachment.rb
Expand Up @@ -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
Expand All @@ -32,7 +32,7 @@ def purge
def purge_later
transaction do
delete
record&.touch
record.touch if record&.persisted?
end
blob&.purge_later
end
Expand Down
3 changes: 2 additions & 1 deletion activestorage/app/models/active_storage/blob.rb
Expand Up @@ -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

Expand Down
4 changes: 4 additions & 0 deletions activestorage/lib/active_storage/attached.rb
Expand Up @@ -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

Expand Down
2 changes: 2 additions & 0 deletions activestorage/lib/active_storage/attached/one.rb
Expand Up @@ -61,6 +61,7 @@ def purge
if attached?
attachment.purge
write_attachment nil
reset_changes
end
end

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

Expand Down
34 changes: 34 additions & 0 deletions activestorage/test/models/attached/one_test.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions activestorage/test/models/blob_test.rb
Expand Up @@ -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)
Expand Down

0 comments on commit abeb3ee

Please sign in to comment.