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

Add a foreign-key constraint to the active_storage_attachments table for blobs #33405

Merged
merged 5 commits into from Jul 20, 2018

Conversation

Projects
None yet
6 participants
@georgeclaghorn
Member

georgeclaghorn commented Jul 20, 2018

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.

@georgeclaghorn georgeclaghorn changed the title from Add a foreign-key constraint to the active_storage_attachments_table for blobs to Add a foreign-key constraint to the active_storage_attachments table for blobs Jul 20, 2018

@georgeclaghorn georgeclaghorn merged commit 205aa14 into rails:master Jul 20, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@georgeclaghorn georgeclaghorn deleted the georgeclaghorn:activestorage-referential-integrity branch Jul 20, 2018

@@ -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

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jul 22, 2018

Contributor

@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

@bogdanvlviv

bogdanvlviv Jul 22, 2018

Contributor

@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

This comment has been minimized.

@kaspth

kaspth Jul 22, 2018

Member

👍, or we can perhaps copy an upgrade migration on rails app:update.

@kaspth

kaspth Jul 22, 2018

Member

👍, or we can perhaps copy an upgrade migration on rails app:update.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jul 22, 2018

Member

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.

@georgeclaghorn

georgeclaghorn Jul 22, 2018

Member

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.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jul 22, 2018

Member

On second thought, adding a migration on update would be nice.

@georgeclaghorn

georgeclaghorn Jul 22, 2018

Member

On second thought, adding a migration on update would be nice.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jul 23, 2018

Add foreign key to active_storage_attachments for `blob_id` via new m…
…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

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jul 23, 2018

`rake app:update` should update active_storage
`rake app:update` should execute `rake active_storage:install`
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)

georgeclaghorn added a commit that referenced this pull request Aug 8, 2018

Merge pull request #33405 from georgeclaghorn/activestorage-referenti…
…al-integrity

Add a foreign-key constraint to the active_storage_attachments table for blobs
@BKSpurgeon

This comment has been minimized.

Show comment
Hide comment
@BKSpurgeon

BKSpurgeon Aug 31, 2018

Contributor

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

Contributor

BKSpurgeon commented Aug 31, 2018

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

@georgeclaghorn

This comment has been minimized.

Show comment
Hide comment
@georgeclaghorn

georgeclaghorn Aug 31, 2018

Member

This’ll be included in Rails 5.2.2 and newer.

Member

georgeclaghorn commented Aug 31, 2018

This’ll be included in Rails 5.2.2 and newer.

@chaadow

This comment has been minimized.

Show comment
Hide comment
@chaadow

chaadow Sep 15, 2018

@georgeclaghorn Hi could you add the activestorage label to this PR. As well as add the related issue to the 5.2.2 milestone

Just adding this constraint fixed the global issue when you update a record with failed validations.

  • It ends up with an attachment pointing with a blob_id that does not exist anymore (because of the automatic purge_job),
    -Hence making helpers like url_for fail with a undefined method #signed_id for nil:NilClass
  • And your model is also pointing to an attachment, so you can't properly check with your_model.image.attached? cause it will return true since the attachment record is not destroyed.

PS : When you try to migrate your database with invalid data (for existing apps), the migration will fail. It would tell you that the blob_id with (id)=(x) column on active_storage_attachments table does not exist on the active_storage_blobs table.
This means that the blob was destroyed, but the attachments referencing the particular blob were not.
The solution i found is simple, in rails console you run :

ActiveStorage::Attachment.where(blob_id: x).destroy_all

Then re run the migration again, then in my case it would tell me about another attachment pointing to another non existent blob.
So you repeat the operation (replacing x with the ID specified in the failed migration) until there are no more invalid data, and the migration passes.

==> And for the time being, this makes activestorage work "normally" ( at least, for PostgreSQL) until 5.2.2 release or 6.0.0 which will remove the automatic upload on assignment, and the asynchrnous blob purging resulting in invalid data

chaadow commented Sep 15, 2018

@georgeclaghorn Hi could you add the activestorage label to this PR. As well as add the related issue to the 5.2.2 milestone

Just adding this constraint fixed the global issue when you update a record with failed validations.

  • It ends up with an attachment pointing with a blob_id that does not exist anymore (because of the automatic purge_job),
    -Hence making helpers like url_for fail with a undefined method #signed_id for nil:NilClass
  • And your model is also pointing to an attachment, so you can't properly check with your_model.image.attached? cause it will return true since the attachment record is not destroyed.

PS : When you try to migrate your database with invalid data (for existing apps), the migration will fail. It would tell you that the blob_id with (id)=(x) column on active_storage_attachments table does not exist on the active_storage_blobs table.
This means that the blob was destroyed, but the attachments referencing the particular blob were not.
The solution i found is simple, in rails console you run :

ActiveStorage::Attachment.where(blob_id: x).destroy_all

Then re run the migration again, then in my case it would tell me about another attachment pointing to another non existent blob.
So you repeat the operation (replacing x with the ID specified in the failed migration) until there are no more invalid data, and the migration passes.

==> And for the time being, this makes activestorage work "normally" ( at least, for PostgreSQL) until 5.2.2 release or 6.0.0 which will remove the automatic upload on assignment, and the asynchrnous blob purging resulting in invalid data

@geoffharcourt

This comment has been minimized.

Show comment
Hide comment
@geoffharcourt

geoffharcourt Sep 15, 2018

Contributor

Hi @chaadow I think you meant to tag georgeclaghorn instead?

Contributor

geoffharcourt commented Sep 15, 2018

Hi @chaadow I think you meant to tag georgeclaghorn instead?

@chaadow

This comment has been minimized.

Show comment
Hide comment
@chaadow

chaadow commented Sep 15, 2018

@geoffharcourt Yes my bad!

@chaadow

This comment has been minimized.

Show comment
Hide comment
@chaadow

chaadow Sep 15, 2018

@georgeclaghorn My message was meant to be read by you 😅

chaadow commented Sep 15, 2018

@georgeclaghorn My message was meant to be read by you 😅

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 16, 2018

Add foreign key to active_storage_attachments for `blob_id` via new m…
…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

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 16, 2018

Add foreign key to active_storage_attachments for `blob_id` via new m…
…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

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 16, 2018

Raise `ActiveRecord::InvalidForeignKey` in `before_destroy` for a blo…
…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 implement 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

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 16, 2018

Raise `ActiveRecord::InvalidForeignKey` in `before_destroy` for a blo…
…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

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 16, 2018

Raise `ActiveRecord::InvalidForeignKey` in `before_destroy` for a blo…
…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

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 16, 2018

Change AR::Migration version from 5.2 to 6.0 in activestorage
Since we decided edit the existing migration
"create_active_storage_tables" in rails#33405 in order to
generate only one migration for a new app I think
we should consider merging this PR - rails#32042

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 16, 2018

Change AR::Migration version from 5.2 to 6.0 in activestorage
Since we decided edit the existing migration
"create_active_storage_tables" in rails#33405 in order to
generate only one migration for a new app I think
we should change version of `ActiveRecord::Migration`
to 6.0.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 20, 2018

Add foreign key to active_storage_attachments for `blob_id` via new m…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment