Skip to content

Commit

Permalink
touch parent model when an attachment is purged
Browse files Browse the repository at this point in the history
Currently `delete` is used on the `purge` and `purge_later` methods,
that prevent any callbacks to be triggered which causes the parent
model to not be updated when an attachment is purged. This behaviour
cause issues on some caching strategies as reported here: #39858

Changes:

* Add `record&.touch` on `attachment#purge`
* Add `record&.touch` on `attachment#purge_later`
* Remove extra blank line on attachment.rb
* Add tests which are failing before this change and pass after the change
  • Loading branch information
victorperez committed Jul 26, 2020
1 parent b949e69 commit 396b43a
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 7 deletions.
4 changes: 4 additions & 0 deletions activestorage/CHANGELOG.md
@@ -1,3 +1,7 @@
* Touch parent model when an attachment is purged.

*Víctor Pérez Rodríguez*

* Files can now be served by proxying them from the underlying storage service
instead of redirecting to a signed service URL. Use the
`rails_storage_proxy_path` and `_url` helpers to proxy an attached file:
Expand Down
11 changes: 8 additions & 3 deletions activestorage/app/models/active_storage/attachment.rb
Expand Up @@ -21,13 +21,19 @@ class ActiveStorage::Attachment < ActiveRecord::Base

# Synchronously deletes the attachment and {purges the blob}[rdoc-ref:ActiveStorage::Blob#purge].
def purge
delete
transaction do
delete
record&.touch
end
blob&.purge
end

# Deletes the attachment and {enqueues a background job}[rdoc-ref:ActiveStorage::Blob#purge_later] to purge the blob.
def purge_later
delete
transaction do
delete
record&.touch
end
blob&.purge_later
end

Expand All @@ -48,7 +54,6 @@ def purge_dependent_blob_later
blob&.purge_later if dependent == :purge_later
end


def dependent
record.attachment_reflections[name]&.options[:dependent]
end
Expand Down
1 change: 1 addition & 0 deletions activestorage/test/database/create_users_migration.rb
Expand Up @@ -5,6 +5,7 @@ def change
create_table :users do |t|
t.string :name
t.integer :group_id
t.timestamps
end
end
end
8 changes: 6 additions & 2 deletions activestorage/test/models/attached/many_test.rb
Expand Up @@ -445,7 +445,9 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
@user.highlights.attach blobs
assert @user.highlights.attached?

@user.highlights.purge
assert_changes -> { @user.updated_at } do
@user.highlights.purge
end
assert_not @user.highlights.attached?
assert_not ActiveStorage::Blob.exists?(blobs.first.id)
assert_not ActiveStorage::Blob.exists?(blobs.second.id)
Expand Down Expand Up @@ -487,7 +489,9 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
assert @user.highlights.attached?

perform_enqueued_jobs do
@user.highlights.purge_later
assert_changes -> { @user.updated_at } do
@user.highlights.purge_later
end
end

assert_not @user.highlights.attached?
Expand Down
8 changes: 6 additions & 2 deletions activestorage/test/models/attached/one_test.rb
Expand Up @@ -449,7 +449,9 @@ def upload.open
@user.avatar.attach blob
assert @user.avatar.attached?

@user.avatar.purge
assert_changes -> { @user.updated_at } do
@user.avatar.purge
end
assert_not @user.avatar.attached?
assert_not ActiveStorage::Blob.exists?(blob.id)
assert_not ActiveStorage::Blob.service.exist?(blob.key)
Expand Down Expand Up @@ -478,7 +480,9 @@ def upload.open
assert @user.avatar.attached?

perform_enqueued_jobs do
@user.avatar.purge_later
assert_changes -> { @user.updated_at } do
@user.avatar.purge_later
end
end

assert_not @user.avatar.attached?
Expand Down

0 comments on commit 396b43a

Please sign in to comment.