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

ActiveStorage foreign key enforcement and purge delete order #32584

Closed
mauricew opened this issue Apr 15, 2018 · 8 comments
Closed

ActiveStorage foreign key enforcement and purge delete order #32584

mauricew opened this issue Apr 15, 2018 · 8 comments

Comments

@mauricew
Copy link

This issue is twofold:

  1. The attachments table doesn't create a foreign key association in the database by default
  2. 1 can't be done because of the order things happen in #purge

Problem information

So, to ensure maximum enforcement in the database, I would want to the attachments table to have an foreign key to the blobs table. I can easily do this by specifying it in the migration:

t.references :blob,     foreign_key: { to_table: :active_storage_blobs }, null: false

This works fine when initially adding an attachment.
But when I would delete it, the blob gets deleted first, then the attachment, creating a temporary case in which the FK wouldn't be used

> some_model.image.purge
# ActiveStorage deletes the file, then immediately afterwards...
  ActiveStorage::Blob Destroy (1.3ms)  DELETE FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = $1  [["id", 1]]                                                                                                               
   (0.2ms)  ROLLBACK
ActiveRecord::InvalidForeignKey: PG::ForeignKeyViolation: ERROR:  update or delete on table "active_storage_blobs" violates foreign key constraint "fk_rails_c3b3935057" on table "active_storage_attachments"                            
DETAIL:  Key (id)=(1) is still referenced from table "active_storage_attachments".
: DELETE FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = $1 

Expected behavior

The purge process should complete successfully. However, I see that the process should go a bit differently than it currently does:

  1. The attachment gets deleted. At this point, there is no association to whatever model you were originally looking at.
  2. ActiveStorage deletes the actual file from storage.
  3. After confirming the file's deletion, the blob gets deleted.

If it follows these steps, then the foreign key association is ok. Having the FK be a part of the default migration is another story, which I'm strongly suggesting with this.

Actual behavior

Blob gets deleted first, then attachment. Foreign key is not possible with this arrangement, and if something goes wrong this can lead to database orphans. Also it appears to do both deletes in separate transactions in a situation where this is successful.

System configuration

Rails version:
5.2.0
Ruby version:
2.4.4

Further information can be provided if necessary.

@georgeclaghorn
Copy link
Contributor

+1 for both flipping the order of those operations and adding a foreign key.

@juliusdelta
Copy link
Contributor

juliusdelta commented Apr 20, 2018

@georgeclaghorn Could I take a crack at this? unless @mauricew wants to

@georgeclaghorn
Copy link
Contributor

Please do! Note that we want to avoid making service calls or enqueuing background jobs inside a database transaction. I think that’s the trickiest part of this.

@juliusdelta
Copy link
Contributor

I'll probably open 2 PRs, one to handle the issue of ordering the blob and image in the delete. The second to handle the foreign key by default. @claudiob Any thoughts on the foreign key feature discussed?

@claudiob
Copy link
Member

@juliusdelta Sounds like a plan.
FYI there are a couple of open issues regarding purge: #32449 and #31985
I'm curious to see if your PR would fix those too.

@georgeclaghorn
Copy link
Contributor

I don’t think the changes proposed here would resolve either of those issues, but they’re nonetheless worthwhile.

@juliusdelta
Copy link
Contributor

juliusdelta commented Apr 22, 2018

@claudiob I’ll see what the work/issues are like and respond on those issues accordingly tomorrow.

@juliusdelta
Copy link
Contributor

Yea I'll see if I can wrap that stuff up in this patch @claudiob

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this issue Sep 16, 2018
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants