Add a foreign-key constraint to the active_storage_attachments table for blobs #33405
Conversation
georgeclaghorn
changed the title
| @@ -20,6 +20,7 @@ def change | |||
| t.datetime :created_at, null: false | |||
|
|
|||
| t.index [ :record_type, :record_id, :name, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true | |||
| t.foreign_key :active_storage_blobs, column: :blob_id | |||
There was a problem hiding this comment.
@georgeclaghorn I'm wondering how users that migrate from 5.2 to 6.0 will know about this change. Maybe we should create a new migration for that or/and describe what users should do in order to get this change in http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-5-2-to-rails-6-0
There was a problem hiding this comment.
👍, or we can perhaps copy an upgrade migration on rails app:update.
There was a problem hiding this comment.
I was planning to mention it in the upgrade guide. It’s not a required change—Active Storage works fine without the foreign key—so I don’t think we need to do anything fancy.
There was a problem hiding this comment.
On second thought, adding a migration on update would be nice.
…al-integrity Add a foreign-key constraint to the active_storage_attachments table for blobs
|
hi all thank you for your PR and contribution. was facing the same problem. my question: which version of rails will the above PR be incorporated in? chrs |
|
This’ll be included in Rails 5.2.2 and newer. |
|
@georgeclaghorn Hi could you add the Just adding this constraint fixed the global issue when you update a record with failed validations.
PS : When you try to migrate your database with invalid data (for existing apps), the migration will fail. It would tell you that the ActiveStorage::Attachment.where(blob_id: x).destroy_allThen re run the migration again, then in my case it would tell me about another attachment pointing to another non existent blob. ==> And for the time being, this makes activestorage work "normally" ( at least, for PostgreSQL) until |
|
Hi @chaadow I think you meant to tag georgeclaghorn instead? |
|
@geoffharcourt Yes my bad! |
|
@georgeclaghorn My message was meant to be read by you 😅 |
…b if attachments exist The issue rails#32584 was fixed in rails#33405 by adding foreign key constraint to the `active_storage_attachments` table for blobs. This commit implements fix on app-level in order to ensure that users can't delete a blob with attachments even if they don't have the foreign key constraint. See a related discussion in the Campfire: https://3.basecamp.com/3076981/buckets/24956/chats/12416418@1236718899 Note that, we should backport it to `5-2-stable` too. Related to rails#33405
…igration We need this in order to be able to add this migration for users that use ActiveStorage during update their apps from Rails 5.2 to Rails 6.0. Related to rails#33405 `rake app:update` should update active_storage `rake app:update` should execute `rake active_storage:update` if it is used in the app that is being updated. It will add new active_storage's migrations to users' apps during update Rails. Context rails#33405 (comment) Also, see a related discussion in the Campfire: https://3.basecamp.com/3076981/buckets/24956/chats/12416418@1236713081
|
thanks for this change. I wonder if there is an option, on Rails 7, to no add the FK to the migration? |
Enforce that purging a blob may not leave attachments behind. Closes #32584.
Note that this PR only adds the constraint in new apps. We’ll leave it up to maintainers of existing apps to add it if they wish.