Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nil and false as a :dependent option for has_one_attached and has_many_attached #44378

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fatkodima
Copy link
Member

@fatkodima fatkodima commented Feb 9, 2022

Closes #44362

Originally, dependent: false was supported. Now this can be also done as dependent: nil (as supported on has_one/has_many).

cc @rafaelfranca

@@ -1,3 +1,7 @@
* Support all `:dependent` options for `has_one_attached` and `has_many_attached`

*fatkodima*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @fatkodima thanks for working on this.

Removing the null column constraint or making the belong_to optional might not be desirable for many apps. If you are happy with the current behavior, you probably don't want to lose those data-integrity protections for a feature you won't use.

Could you elaborate on the motivation for this change? I understand how it could be useful in some scenario, but it certainly introduces a conflict. Maybe it should be an option, but then that would make things more complex too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! Yes, agreed.

Do you think these changes should be better done as a suggestion in the has_one(many)_attached method's docs to remove the NULL constraint on the active_storage_attachments table and making the record relation optional if using dependent: nil?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fatkodima Another problem I see is that there is a bit of lack of clarity in the API. Is not clear that :dependant will configure both the Active Storage attachment removal AND the active record association. You can find yourself with conventions such as :purge_later in ASt meaning :destroy in AR. What about in the opposite direction: what does nullify means here?

I am not sure this is the right approach. Could you expose why you think this is needed? Which need you are trying to solve in your app?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the linked issue #44362 (comment). The poster was trying to implement soft deletions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @fatkodima, sorry, somehow I had missed the linked issue 🙈.

Checking the implementation and the docs it says:

If the +:dependent+ option isn't set, the attachment will be purged (i.e. destroyed)
whenever the record is destroyed.

Checking the code, I think this is accurate: the default dependant:value :purge_later is the only one that triggers a purge of the attachment. Any other value, including nil or false, will skip the deletion.

With this behavior, I think supporting any other regular Active Record dependant: option can be a bit confusing. As mentioned, for example, :delete_all would result in the record being deleted, but won't purge the attachment. I think this is not very intuitive. I'd change the behavior and add explicit support for nil/false instead.

dependent: (:destroy if dependent)

So, when passing a false value, then it won't destroy neither the attachment or the associated record.

+1 to document the need to remove the FK constraint when passing a false value, and to make the association optional in that case.

making the record relation optional if using dependent: nil

How would you handle that automatically at declaration time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jorgemanrubia for your time and thoughtful review ❤️
I updated this PR with your suggestions, please, make another review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fatkodima! My only concern left is what to do with the validation we lose with that optional: true in the belongs_to association. I think with the default null constraint in place it should be good, but it's still something we are changing for most users. Any thoughts there @rafaelfranca?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgemanrubia Is there any chance we can reactivate this PR? 🙏🏻 It's really unfortunate that there is no proper support for soft-delete records in ActiveStorage.

@fatkodima fatkodima force-pushed the has_attached-dependent-option branch from 920ca19 to 4f8df37 Compare June 1, 2022 22:40
@fatkodima fatkodima changed the title Support all :dependent options for has_one_attached and has_many_attached Support nil and false as a :dependent option for has_one_attached and has_many_attached Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle dependent: nil on ActiveStorage attachments
3 participants