Fix ActiveStorage has_many_attached when record is not persisted

This is a follow up of #42256.

Purging a not persisted record no longer raise an error for
`has_many_attached`.

Moves the `purge` and `purge_later` logic of `ActiveStorage::Attached`
to `Attached::Changes` API.
This commit is contained in:
Jacopo 2021-06-03 15:09:31 +02:00
parent 6d3acaf293
commit a8a60652a3
9 changed files with 149 additions and 33 deletions

View File

@ -1,3 +1,7 @@
* Allow to purge an attachment when record is not persisted for `has_many_attached`
*Jacopo Beschi*
* Add `with_all_variant_records` method to eager load all variant records on an attachment at once.
`with_attached_image` scope now eager loads variant records if using variant tracking.

View File

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

View File

@ -11,6 +11,9 @@ module ActiveStorage
autoload :DeleteOne
autoload :DeleteMany
autoload :PurgeOne
autoload :PurgeMany
end
end
end

View File

@ -0,0 +1,27 @@
# frozen_string_literal: true
module ActiveStorage
class Attached::Changes::PurgeMany #:nodoc:
attr_reader :name, :record, :attachments
def initialize(name, record, attachments)
@name, @record, @attachments = name, record, attachments
end
def purge
attachments.each(&:purge)
reset
end
def purge_later
attachments.each(&:purge_later)
reset
end
private
def reset
record.attachment_changes.delete(name)
record.public_send("#{name}_attachments").reset
end
end
end

View File

@ -0,0 +1,27 @@
# frozen_string_literal: true
module ActiveStorage
class Attached::Changes::PurgeOne #:nodoc:
attr_reader :name, :record, :attachment
def initialize(name, record, attachment)
@name, @record, @attachment = name, record, attachment
end
def purge
attachment&.purge
reset
end
def purge_later
attachment&.purge_later
reset
end
private
def reset
record.attachment_changes.delete(name)
record.public_send("#{name}_attachment=", nil)
end
end
end

View File

@ -3,6 +3,19 @@
module ActiveStorage
# Decorated proxy object representing of multiple attachments to a model.
class Attached::Many < Attached
##
# :method: purge
#
# Directly purges each associated attachment (i.e. destroys the blobs and
# attachments and deletes the files on the service).
delegate :purge, to: :purge_many
##
# :method: purge_later
#
# Purges each associated attachment through the queuing system.
delegate :purge_later, to: :purge_many
delegate_missing_to :attachments
# Returns all the associated attachment records.
@ -52,15 +65,9 @@ module ActiveStorage
attachments.delete_all if attached?
end
##
# :method: purge
#
# Directly purges each associated attachment (i.e. destroys the blobs and
# attachments and deletes the files on the service).
##
# :method: purge_later
#
# Purges each associated attachment through the queuing system.
private
def purge_many
Attached::Changes::PurgeMany.new(name, record, attachments)
end
end
end

View File

@ -3,6 +3,19 @@
module ActiveStorage
# Representation of a single attachment to a model.
class Attached::One < Attached
##
# :method: purge
#
# Directly purges the attachment (i.e. destroys the blob and
# attachment and deletes the file on the service).
delegate :purge, to: :purge_one
##
# :method: purge_later
#
# Purges the attachment through the queuing system.
delegate :purge_later, to: :purge_one
delegate_missing_to :attachment, allow_nil: true
# Returns the associated attachment record.
@ -62,28 +75,13 @@ module ActiveStorage
end
end
# Directly purges the attachment (i.e. destroys the blob and
# attachment and deletes the file on the service).
def purge
if attached?
attachment.purge
write_attachment nil
reset_changes
end
end
# Purges the attachment through the queuing system.
def purge_later
if attached?
attachment.purge_later
write_attachment nil
reset_changes
end
end
private
def write_attachment(attachment)
record.public_send("#{name}_attachment=", attachment)
end
def purge_one
Attached::Changes::PurgeOne.new(name, record, attachment)
end
end
end

View File

@ -483,6 +483,33 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
end
end
test "purging when record is not persisted" do
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
user = User.new
user.highlights.attach blobs
assert user.highlights.attached?
attachments = user.highlights.attachments
user.highlights.purge
assert_not user.highlights.attached?
assert attachments.all?(&:destroyed?)
blobs.each do |blob|
assert_not ActiveStorage::Blob.exists?(blob.id)
assert_not ActiveStorage::Blob.service.exist?(blob.key)
end
end
end
test "purging delete changes when record is not persisted" do
user = User.new
user.highlights = []
user.highlights.purge
assert_nil user.attachment_changes["highlights"]
end
test "purging later" do
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
@user.highlights.attach blobs
@ -531,6 +558,24 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
end
end
test "purging attachment later when record is not persisted" do
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
user = User.new
user.highlights.attach blobs
assert user.highlights.attached?
perform_enqueued_jobs do
user.highlights.purge_later
end
assert_not user.highlights.attached?
blobs.each do |blob|
assert_not ActiveStorage::Blob.exists?(blob.id)
assert_not ActiveStorage::Blob.service.exist?(blob.key)
end
end
end
test "purging dependent attachment later on destroy" do
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
@user.highlights.attach blobs

View File

@ -497,6 +497,15 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
end
end
test "purging delete changes when record is not persisted" do
user = User.new
user.avatar = nil
user.avatar.purge
assert_nil user.attachment_changes["avatar"]
end
test "purging later" do
create_blob(filename: "funky.jpg").tap do |blob|
@user.avatar.attach blob