Merge pull request rails#33405 from georgeclaghorn/activestorage-refe…

…rential-integrity

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

activestorage/app/jobs/active_storage/purge_job.rb

@@ -2,7 +2,7 @@
 
 # Provides asynchronous purging of ActiveStorage::Blob records via ActiveStorage::Blob#purge_later.
 class ActiveStorage::PurgeJob < ActiveStorage::BaseJob
-  discard_on ActiveRecord::RecordNotFound
+  discard_on ActiveRecord::RecordNotFound, ActiveRecord::InvalidForeignKey
 
   def perform(blob)
     blob.purge

activestorage/app/models/active_storage/attachment.rb

@@ -19,14 +19,14 @@ class ActiveStorage::Attachment < ActiveRecord::Base
 
   # Synchronously deletes the attachment and {purges the blob}[rdoc-ref:ActiveStorage::Blob#purge].
   def purge
-    blob.purge
     delete
+    blob.purge
   end
 
   # Deletes the attachment and {enqueues a background job}[rdoc-ref:ActiveStorage::Blob#purge_later] to purge the blob.
   def purge_later
-    blob.purge_later
     delete
+    blob.purge_later
   end
 
   private

activestorage/app/models/active_storage/blob.rb

@@ -205,8 +205,8 @@ def delete
   # blobs. Note, though, that deleting the file off the service will initiate a HTTP connection to the service, which may
   # be slow or prevented, so you should not use this method inside a transaction or in callbacks. Use #purge_later instead.
   def purge
-    delete
     destroy
+    delete
   end
 
   # Enqueues an ActiveStorage::PurgeJob to call #purge. This is the recommended way to purge blobs from a transaction,

activestorage/db/migrate/20170806125915_create_active_storage_tables.rb

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

activestorage/test/jobs/purge_job_test.rb

@@ -24,4 +24,14 @@ class ActiveStorage::PurgeJobTest < ActiveJob::TestCase
       end
     end
   end
+
+  test "ignores attached blob" do
+    User.create! name: "DHH", avatar: @blob
+
+    perform_enqueued_jobs do
+      assert_nothing_raised do
+        ActiveStorage::PurgeJob.perform_later @blob
+      end
+    end
+  end
 end

activestorage/test/models/attached/many_test.rb

@@ -10,7 +10,7 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     @user = User.create!(name: "Josh")
   end
 
-  teardown { ActiveStorage::Blob.all.each(&:purge) }
+  teardown { ActiveStorage::Blob.all.each(&:delete) }
 
   test "attaching existing blobs to an existing record" do
     @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")

activestorage/test/models/attached/one_test.rb

@@ -10,7 +10,7 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     @user = User.create!(name: "Josh")
   end
 
-  teardown { ActiveStorage::Blob.all.each(&:purge) }
+  teardown { ActiveStorage::Blob.all.each(&:delete) }
 
   test "attaching an existing blob to an existing record" do
     @user.avatar.attach create_blob(filename: "funky.jpg")

activestorage/test/models/blob_test.rb

@@ -174,6 +174,14 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     assert_not ActiveStorage::Blob.service.exist?(variant.key)
   end
 
+  test "purge fails when attachments exist" do
+    create_blob.tap do |blob|
+      User.create! name: "DHH", avatar: blob
+      assert_raises(ActiveRecord::InvalidForeignKey) { blob.purge }
+      assert ActiveStorage::Blob.service.exist?(blob.key)
+    end
+  end
+
   private
     def expected_url_for(blob, disposition: :inline, filename: nil)
       filename ||= blob.filename