Skip to content

Commit a8a6065

Browse files
committed
Fix ActiveStorage has_many_attached when record is not persisted
This is a follow up of rails#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.
1 parent 6d3acaf commit a8a6065

File tree

9 files changed

+149
-33
lines changed

9 files changed

+149
-33
lines changed

activestorage/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
* Allow to purge an attachment when record is not persisted for `has_many_attached`
2+
3+
*Jacopo Beschi*
4+
15
* Add `with_all_variant_records` method to eager load all variant records on an attachment at once.
26
`with_attached_image` scope now eager loads variant records if using variant tracking.
37

activestorage/lib/active_storage/attached.rb

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,6 @@ def initialize(name, record)
1616
def change
1717
record.attachment_changes[name]
1818
end
19-
20-
def reset_changes
21-
record.attachment_changes.delete(name)
22-
end
2319
end
2420
end
2521

activestorage/lib/active_storage/attached/changes.rb

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@ module Attached::Changes #:nodoc:
1111

1212
autoload :DeleteOne
1313
autoload :DeleteMany
14+
15+
autoload :PurgeOne
16+
autoload :PurgeMany
1417
end
1518
end
1619
end
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# frozen_string_literal: true
2+
3+
module ActiveStorage
4+
class Attached::Changes::PurgeMany #:nodoc:
5+
attr_reader :name, :record, :attachments
6+
7+
def initialize(name, record, attachments)
8+
@name, @record, @attachments = name, record, attachments
9+
end
10+
11+
def purge
12+
attachments.each(&:purge)
13+
reset
14+
end
15+
16+
def purge_later
17+
attachments.each(&:purge_later)
18+
reset
19+
end
20+
21+
private
22+
def reset
23+
record.attachment_changes.delete(name)
24+
record.public_send("#{name}_attachments").reset
25+
end
26+
end
27+
end
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# frozen_string_literal: true
2+
3+
module ActiveStorage
4+
class Attached::Changes::PurgeOne #:nodoc:
5+
attr_reader :name, :record, :attachment
6+
7+
def initialize(name, record, attachment)
8+
@name, @record, @attachment = name, record, attachment
9+
end
10+
11+
def purge
12+
attachment&.purge
13+
reset
14+
end
15+
16+
def purge_later
17+
attachment&.purge_later
18+
reset
19+
end
20+
21+
private
22+
def reset
23+
record.attachment_changes.delete(name)
24+
record.public_send("#{name}_attachment=", nil)
25+
end
26+
end
27+
end

activestorage/lib/active_storage/attached/many.rb

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,19 @@
33
module ActiveStorage
44
# Decorated proxy object representing of multiple attachments to a model.
55
class Attached::Many < Attached
6+
##
7+
# :method: purge
8+
#
9+
# Directly purges each associated attachment (i.e. destroys the blobs and
10+
# attachments and deletes the files on the service).
11+
delegate :purge, to: :purge_many
12+
13+
##
14+
# :method: purge_later
15+
#
16+
# Purges each associated attachment through the queuing system.
17+
delegate :purge_later, to: :purge_many
18+
619
delegate_missing_to :attachments
720

821
# Returns all the associated attachment records.
@@ -52,15 +65,9 @@ def detach
5265
attachments.delete_all if attached?
5366
end
5467

55-
##
56-
# :method: purge
57-
#
58-
# Directly purges each associated attachment (i.e. destroys the blobs and
59-
# attachments and deletes the files on the service).
60-
61-
##
62-
# :method: purge_later
63-
#
64-
# Purges each associated attachment through the queuing system.
68+
private
69+
def purge_many
70+
Attached::Changes::PurgeMany.new(name, record, attachments)
71+
end
6572
end
6673
end

activestorage/lib/active_storage/attached/one.rb

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,19 @@
33
module ActiveStorage
44
# Representation of a single attachment to a model.
55
class Attached::One < Attached
6+
##
7+
# :method: purge
8+
#
9+
# Directly purges the attachment (i.e. destroys the blob and
10+
# attachment and deletes the file on the service).
11+
delegate :purge, to: :purge_one
12+
13+
##
14+
# :method: purge_later
15+
#
16+
# Purges the attachment through the queuing system.
17+
delegate :purge_later, to: :purge_one
18+
619
delegate_missing_to :attachment, allow_nil: true
720

821
# Returns the associated attachment record.
@@ -62,28 +75,13 @@ def detach
6275
end
6376
end
6477

65-
# Directly purges the attachment (i.e. destroys the blob and
66-
# attachment and deletes the file on the service).
67-
def purge
68-
if attached?
69-
attachment.purge
70-
write_attachment nil
71-
reset_changes
72-
end
73-
end
74-
75-
# Purges the attachment through the queuing system.
76-
def purge_later
77-
if attached?
78-
attachment.purge_later
79-
write_attachment nil
80-
reset_changes
81-
end
82-
end
83-
8478
private
8579
def write_attachment(attachment)
8680
record.public_send("#{name}_attachment=", attachment)
8781
end
82+
83+
def purge_one
84+
Attached::Changes::PurgeOne.new(name, record, attachment)
85+
end
8886
end
8987
end

activestorage/test/models/attached/many_test.rb

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,33 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
483483
end
484484
end
485485

486+
test "purging when record is not persisted" do
487+
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
488+
user = User.new
489+
user.highlights.attach blobs
490+
assert user.highlights.attached?
491+
492+
attachments = user.highlights.attachments
493+
user.highlights.purge
494+
495+
assert_not user.highlights.attached?
496+
assert attachments.all?(&:destroyed?)
497+
blobs.each do |blob|
498+
assert_not ActiveStorage::Blob.exists?(blob.id)
499+
assert_not ActiveStorage::Blob.service.exist?(blob.key)
500+
end
501+
end
502+
end
503+
504+
test "purging delete changes when record is not persisted" do
505+
user = User.new
506+
user.highlights = []
507+
508+
user.highlights.purge
509+
510+
assert_nil user.attachment_changes["highlights"]
511+
end
512+
486513
test "purging later" do
487514
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
488515
@user.highlights.attach blobs
@@ -531,6 +558,24 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
531558
end
532559
end
533560

561+
test "purging attachment later when record is not persisted" do
562+
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
563+
user = User.new
564+
user.highlights.attach blobs
565+
assert user.highlights.attached?
566+
567+
perform_enqueued_jobs do
568+
user.highlights.purge_later
569+
end
570+
571+
assert_not user.highlights.attached?
572+
blobs.each do |blob|
573+
assert_not ActiveStorage::Blob.exists?(blob.id)
574+
assert_not ActiveStorage::Blob.service.exist?(blob.key)
575+
end
576+
end
577+
end
578+
534579
test "purging dependent attachment later on destroy" do
535580
[ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
536581
@user.highlights.attach blobs

activestorage/test/models/attached/one_test.rb

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,15 @@ def upload.open
497497
end
498498
end
499499

500+
test "purging delete changes when record is not persisted" do
501+
user = User.new
502+
user.avatar = nil
503+
504+
user.avatar.purge
505+
506+
assert_nil user.attachment_changes["avatar"]
507+
end
508+
500509
test "purging later" do
501510
create_blob(filename: "funky.jpg").tap do |blob|
502511
@user.avatar.attach blob

0 commit comments

Comments
 (0)