Fix ActiveStorage has_many_attached when record is not persisted

[intrip]

Summary

This is a follow up of #42256.

Purging a not persisted record no longer raise an error forhas_many_attached.

Moves the purge and purge_later logic of ActiveStorage::Attached to Attached::Changes API.

[intrip]

@zzak @pixeltrix Could you please take a look and LMK your take on this 🙇 ?

[zzak]

@intrip I think of delete/purge as a change, so rather than creating a Base class and adding this change to every interface (even create was surprising) let's figure out how we can introduce this into "changes" api.

The fact that we have to define Attached::Changes::CreateOne.purge and Attached::Changes::CreateOne.purge_later feels like we're doing something wrong 🤔

[intrip]

@zzak thanks for the feedback!

I think of delete/purge as a change, so rather than creating a Base class and adding this change to every interface (even create was surprising) let's figure out how we can introduce this into "changes" api.

The fact that we have to define Attached::Changes::CreateOne.purge and Attached::Changes::CreateOne.purge_later feels like we're doing something wrong 🤔

Gotcha, let's reason together around it:

• purge/purge_later are actually parts of the attached public API (not the changes API)
  • the proposal of moving the purge methods implementations to the "changes" objects is because from the attached API we work with both changes and AR association in the same purge method; the idea is to let each object deal with his own data
• the upload method is also part of the changes API, isn't it the opposite of the purge method (hence they are related 🤷‍♂️ ) ?

Your proposal is to create two new classes Attached::Changes::PurgeOne/Attached::Changes::PurgeMany in order to deal with purges?

LMK your thoughts and please forgive me if I'm overlooking at something.

[zzak]

@intrip Thanks for thinking about this, and apologies for a half-baked response it was at the end of my day 😅

I think we're on the same page, we would have to add two new classes for the PurgeOne and PurgeMany changes.

Would you mind giving that some more thought and maybe trying it out?

[intrip]

I think we can remove this:

• This extension is no longer called from the Attached API
• I didn't see any documentation explicitly referring to #{name}_attachments.purge/purge_later
• No tests are using this extension explicitly

For the above reasons shouldn't be a breaking change and allows us to avoid a maintenance burden.

[zzak]

@intrip Are these public API? We could add a deprecation (in a follow-up PR)

[intrip]

Honestly I'm not sure (hence my comment above): Is public API a method which has public visibility but is not documented and doesn't have an explicitly # :nodoc: comment? 🤷‍♂️

[intrip]

@zzak I've followed your suggestion in #42383 (comment)

Could you please check again ?

[intrip]

@zzak thanks!
I've answered your questions and also noted something interesting in #42383 (comment). Could you please check again 🙇 ?

[zzak]

I think we should not remove the purge/purge_later version (in this PR).

Unless the parent class/module is already :nodoc: we can assume all methods inside are public API (unless method visibility is already private).

But perhaps @pixeltrix has some more ideas here, particularly w/r/t this thread: #42383 (comment)

Raising this in the comments since it's easy for code-threads to get lost 😭

[intrip]

@zzak I've postponed the removal of purge/purge_later as you suggested in #42383 (comment)
I've also followed your suggestion to fix the docs.

Please LMK if you have anything else to add 🙇

[zzak]

@intrip Nice work! Thanks for putting this together 🙏

[pixeltrix]

@intrip @zzak thanks for sorting this out 👍🏻

Just one thing before I merge - are the association extensions purge and purge_later in model.rb now redundant?

rails/activestorage/lib/active_storage/attached/model.rb

Lines 156 to 166 in 6d3acaf

	has_many :"#{name}_attachments", -> { where(name: name) }, as: :record, class_name: "ActiveStorage::Attachment", inverse_of: :record, dependent: :destroy, strict_loading: strict_loading do
	def purge
	each(&:purge)
	reset
	end
	def purge_later
	each(&:purge_later)
	reset
	end
	end

[intrip]

@pixeltrix thanks for checking!

Just one thing before I merge - are the association extensions purge and purge_later in model.rb now redundant?

Yes: If you check #42383 (comment) I've proposed to remove that but we weren't sure if we should do it in this PR or we should do it in two separate PRs with a deprecation warning + removal after a new release.

What's your take on this?

[pixeltrix]

What's your take on this?

Hmm, so those methods would only be called now if you call the association directly - not sure how much that code path would've been used but @zzak is right, we should deprecate them first.

[intrip]

@pixeltrix thanks for feedback.

I'll make a follow-up PR which introduces the deprecation warning.

[pixeltrix]

@intrip thanks! 👍🏻