From 7da3b2cdd09078984416aa03da108ad0a4a4e477 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Wed, 2 May 2018 18:21:42 +0200
Subject: [PATCH 1/5] Delete remote uploads

ObjectStore uploader requires presence of associated `uploads` record
when deleting the upload file (through the carrierwave's after_commit
hook) because we keep info whether file is LOCAL or REMOTE in `upload`
object.

For this reason we can not destroy uploads as "dependent: :destroy" hook
because these would be deleted too soon. Instead we rely on
carrierwave's hook to destroy `uploads` in after_commit hook.

But in before_destroy hook we still have to delete not-mounted uploads
(which don't use carrierwave's destroy hook). This has to be done in
before_Destroy instead of after_commit because `FileUpload` requires
existence of model's object on destroy action.

This is not ideal state of things, in a next step we should investigate
how to unify model dependencies so we can use same workflow for all
uploads.

Related to #45425
---
 app/models/appearance.rb                      |  3 +-
 app/models/concerns/with_uploads.rb           | 37 +++++++++++++++++++
 app/models/group.rb                           |  3 +-
 app/models/project.rb                         |  3 +-
 app/models/user.rb                            |  2 +-
 .../jprovazn-remote-upload-destroy.yml        |  5 +++
 spec/models/appearance_spec.rb                | 10 ++++-
 spec/models/group_spec.rb                     | 10 ++++-
 spec/models/project_spec.rb                   | 10 ++++-
 spec/models/user_spec.rb                      | 10 ++++-
 .../models/with_uploads_shared_examples.rb    | 23 ++++++++++++
 11 files changed, 105 insertions(+), 11 deletions(-)
 create mode 100644 app/models/concerns/with_uploads.rb
 create mode 100644 changelogs/unreleased/jprovazn-remote-upload-destroy.yml
 create mode 100644 spec/support/shared_examples/models/with_uploads_shared_examples.rb

diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index fb66dd0b766..f3cfc8ccd1e 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -1,4 +1,5 @@
 class Appearance < ActiveRecord::Base
+  include WithUploads
   include CacheMarkdownField
   include AfterCommitQueue
   include ObjectStorage::BackgroundMove
@@ -14,8 +15,6 @@ class Appearance < ActiveRecord::Base
   mount_uploader :logo,         AttachmentUploader
   mount_uploader :header_logo,  AttachmentUploader
 
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   CACHE_KEY = "current_appearance:#{Gitlab::VERSION}".freeze
 
   after_commit :flush_redis_cache
diff --git a/app/models/concerns/with_uploads.rb b/app/models/concerns/with_uploads.rb
new file mode 100644
index 00000000000..101d09f161d
--- /dev/null
+++ b/app/models/concerns/with_uploads.rb
@@ -0,0 +1,37 @@
+# Mounted uploaders are destroyed by carrierwave's after_commit
+# hook. This hook fetches upload location (local vs remote) from
+# Upload model. So it's neccessary to make sure that during that
+# after_commit hook model's associated uploads are not deleted yet.
+# IOW we can not use denepdent => :destroy :
+# has_many :uploads, as: :model, dependent: :destroy
+#
+# And because not-mounted uploads require presence of upload's
+# object model when destroying them (FileUploader's `build_upload` method
+# references `model` on delete), we can not use after_commit hook for these
+# uploads.
+#
+# Instead FileUploads are destroyed in before_destroy hook and remaining uploads
+# are destroyed by the carrierwave's after_commit hook.
+
+module WithUploads
+  extend ActiveSupport::Concern
+
+  # Currently there is no simple way how to select only not-mounted
+  # uploads, it should be all FileUploaders so we select them by
+  # `uploader` class
+  FILE_UPLOADERS = %w(PersonalFileUploader NamespaceFileUploader FileUploader).freeze
+
+  included do
+    has_many :uploads, as: :model
+
+    before_destroy :destroy_file_uploads
+  end
+
+  # mounted uploads are deleted in carrierwave's after_commit hook,
+  # but FileUploaders which are not mounted must be deleted explicitly and
+  # it can not be done in after_commit because FileUploader requires loads
+  # associated model on destroy (which is already deleted in after_commit)
+  def destroy_file_uploads
+    self.uploads.where(uploader: FILE_UPLOADERS).destroy_all
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index cefca316399..107711e3cc5 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord'
 
 class Group < Namespace
   include Gitlab::ConfigHelper
+  include WithUploads
   include AfterCommitQueue
   include AccessRequestable
   include Avatarable
@@ -30,8 +31,6 @@ class Group < Namespace
   has_many :variables, class_name: 'Ci::GroupVariable'
   has_many :custom_attributes, class_name: 'GroupCustomAttribute'
 
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   has_many :boards
   has_many :badges, class_name: 'GroupBadge'
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 534a0e630af..0b0d653c4af 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -4,6 +4,7 @@ class Project < ActiveRecord::Base
   include Gitlab::ConfigHelper
   include Gitlab::ShellAdapter
   include Gitlab::VisibilityLevel
+  include WithUploads
   include AccessRequestable
   include Avatarable
   include CacheMarkdownField
@@ -301,8 +302,6 @@ class Project < ActiveRecord::Base
     inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
   validates :variables, variable_duplicates: { scope: :environment_scope }
 
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   # Scopes
   scope :pending_delete, -> { where(pending_delete: true) }
   scope :without_deleted, -> { where(pending_delete: false) }
diff --git a/app/models/user.rb b/app/models/user.rb
index 173ab38e20c..082ec76e86a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord'
 class User < ActiveRecord::Base
   extend Gitlab::ConfigHelper
 
+  include WithUploads
   include Gitlab::ConfigHelper
   include Gitlab::SQL::Pattern
   include AfterCommitQueue
@@ -137,7 +138,6 @@ class User < ActiveRecord::Base
 
   has_many :custom_attributes, class_name: 'UserCustomAttribute'
   has_many :callouts, class_name: 'UserCallout'
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :term_agreements
   belongs_to :accepted_term, class_name: 'ApplicationSetting::Term'
 
diff --git a/changelogs/unreleased/jprovazn-remote-upload-destroy.yml b/changelogs/unreleased/jprovazn-remote-upload-destroy.yml
new file mode 100644
index 00000000000..22e55920fa3
--- /dev/null
+++ b/changelogs/unreleased/jprovazn-remote-upload-destroy.yml
@@ -0,0 +1,5 @@
+---
+title: Fix deletion of Object Store uploads
+merge_request:
+author:
+type: fixed
diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb
index 56b5d616284..5489c17bd82 100644
--- a/spec/models/appearance_spec.rb
+++ b/spec/models/appearance_spec.rb
@@ -5,7 +5,7 @@ describe Appearance do
 
   it { is_expected.to be_valid }
 
-  it { is_expected.to have_many(:uploads).dependent(:destroy) }
+  it { is_expected.to have_many(:uploads) }
 
   describe '.current', :use_clean_rails_memory_store_caching do
     let!(:appearance) { create(:appearance) }
@@ -41,4 +41,12 @@ describe Appearance do
       expect(new_row.valid?).to eq(false)
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', false do
+      let(:model_object) { create(:appearance, :with_logo) }
+      let(:upload_attribute) { :logo }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 0907d28d33b..f83b52e8975 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -15,7 +15,7 @@ describe Group do
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
     it { is_expected.to have_many(:labels).class_name('GroupLabel') }
     it { is_expected.to have_many(:variables).class_name('Ci::GroupVariable') }
-    it { is_expected.to have_many(:uploads).dependent(:destroy) }
+    it { is_expected.to have_many(:uploads) }
     it { is_expected.to have_one(:chat_team) }
     it { is_expected.to have_many(:custom_attributes).class_name('GroupCustomAttribute') }
     it { is_expected.to have_many(:badges).class_name('GroupBadge') }
@@ -691,4 +691,12 @@ describe Group do
       end
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', true do
+      let(:model_object) { create(:group, :with_avatar) }
+      let(:upload_attribute) { :avatar }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 5b452f17979..39625b559eb 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -76,7 +76,7 @@ describe Project do
     it { is_expected.to have_many(:project_group_links) }
     it { is_expected.to have_many(:notification_settings).dependent(:delete_all) }
     it { is_expected.to have_many(:forks).through(:forked_project_links) }
-    it { is_expected.to have_many(:uploads).dependent(:destroy) }
+    it { is_expected.to have_many(:uploads) }
     it { is_expected.to have_many(:pipeline_schedules) }
     it { is_expected.to have_many(:members_and_requesters) }
     it { is_expected.to have_many(:clusters) }
@@ -3739,4 +3739,12 @@ describe Project do
       it { is_expected.to be_nil }
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', true do
+      let(:model_object) { create(:project, :with_avatar) }
+      let(:upload_attribute) { :avatar }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bb5308221f0..9dff6173f94 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -39,7 +39,7 @@ describe User do
     it { is_expected.to have_many(:builds).dependent(:nullify) }
     it { is_expected.to have_many(:pipelines).dependent(:nullify) }
     it { is_expected.to have_many(:chat_names).dependent(:destroy) }
-    it { is_expected.to have_many(:uploads).dependent(:destroy) }
+    it { is_expected.to have_many(:uploads) }
     it { is_expected.to have_many(:reported_abuse_reports).dependent(:destroy).class_name('AbuseReport') }
     it { is_expected.to have_many(:custom_attributes).class_name('UserCustomAttribute') }
 
@@ -2769,4 +2769,12 @@ describe User do
       expect { user.increment_failed_attempts! }.not_to change(user, :failed_attempts)
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', false do
+      let(:model_object) { create(:user, :with_avatar) }
+      let(:upload_attribute) { :avatar }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/support/shared_examples/models/with_uploads_shared_examples.rb b/spec/support/shared_examples/models/with_uploads_shared_examples.rb
new file mode 100644
index 00000000000..47ad0c6345d
--- /dev/null
+++ b/spec/support/shared_examples/models/with_uploads_shared_examples.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+shared_examples_for 'model with mounted uploader' do |supports_fileuploads|
+  describe '.destroy' do
+    before do
+      stub_uploads_object_storage(uploader_class)
+
+      model_object.public_send(upload_attribute).migrate!(ObjectStorage::Store::REMOTE)
+    end
+
+    it 'deletes remote uploads' do
+      expect_any_instance_of(CarrierWave::Storage::Fog::File).to receive(:delete).and_call_original
+
+      expect { model_object.destroy }.to change { Upload.count }.by(-1)
+    end
+
+    it 'deletes any FileUploader uploads which are not mounted', skip: !supports_fileuploads do
+      create(:upload, uploader: FileUploader, model: model_object)
+
+      expect { model_object.destroy }.to change { Upload.count }.by(-2)
+    end
+  end
+end

From c81a37c1d3f864cf0a00386dab29da78f222e3a5 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Fri, 4 May 2018 19:52:43 +0200
Subject: [PATCH 2/5] Use find_in_batches instead of destroy_all

destroy_all loads all records at once
---
 app/models/concerns/with_uploads.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/models/concerns/with_uploads.rb b/app/models/concerns/with_uploads.rb
index 101d09f161d..3d99889c131 100644
--- a/app/models/concerns/with_uploads.rb
+++ b/app/models/concerns/with_uploads.rb
@@ -32,6 +32,8 @@ module WithUploads
   # it can not be done in after_commit because FileUploader requires loads
   # associated model on destroy (which is already deleted in after_commit)
   def destroy_file_uploads
-    self.uploads.where(uploader: FILE_UPLOADERS).destroy_all
+    self.uploads.where(uploader: FILE_UPLOADERS).find_each do |upload|
+      upload.destroy
+    end
   end
 end

From 32e22468300e3a52c82a855a01fc3983473107e0 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Mon, 7 May 2018 15:49:32 +0200
Subject: [PATCH 3/5] Changed order of include

---
 app/models/appearance.rb | 2 +-
 app/models/group.rb      | 2 +-
 app/models/project.rb    | 2 +-
 app/models/user.rb       | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index f3cfc8ccd1e..f8713138a93 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -1,8 +1,8 @@
 class Appearance < ActiveRecord::Base
-  include WithUploads
   include CacheMarkdownField
   include AfterCommitQueue
   include ObjectStorage::BackgroundMove
+  include WithUploads
 
   cache_markdown_field :description
   cache_markdown_field :new_project_guidelines
diff --git a/app/models/group.rb b/app/models/group.rb
index 107711e3cc5..8fb77a7869d 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -2,7 +2,6 @@ require 'carrierwave/orm/activerecord'
 
 class Group < Namespace
   include Gitlab::ConfigHelper
-  include WithUploads
   include AfterCommitQueue
   include AccessRequestable
   include Avatarable
@@ -11,6 +10,7 @@ class Group < Namespace
   include LoadedInGroupList
   include GroupDescendant
   include TokenAuthenticatable
+  include WithUploads
 
   has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   alias_method :members, :group_members
diff --git a/app/models/project.rb b/app/models/project.rb
index 0b0d653c4af..0975e64e995 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -4,7 +4,6 @@ class Project < ActiveRecord::Base
   include Gitlab::ConfigHelper
   include Gitlab::ShellAdapter
   include Gitlab::VisibilityLevel
-  include WithUploads
   include AccessRequestable
   include Avatarable
   include CacheMarkdownField
@@ -24,6 +23,7 @@ class Project < ActiveRecord::Base
   include ::Gitlab::Utils::StrongMemoize
   include ChronicDurationAttribute
   include FastDestroyAll::Helpers
+  include WithUploads
 
   extend Gitlab::ConfigHelper
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 082ec76e86a..b90f5471071 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -3,7 +3,6 @@ require 'carrierwave/orm/activerecord'
 class User < ActiveRecord::Base
   extend Gitlab::ConfigHelper
 
-  include WithUploads
   include Gitlab::ConfigHelper
   include Gitlab::SQL::Pattern
   include AfterCommitQueue
@@ -18,6 +17,7 @@ class User < ActiveRecord::Base
   include IgnorableColumn
   include BulkMemberAccessLoad
   include BlocksJsonSerialization
+  include WithUploads
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 

From 8c0166f2b96e233cbf8ec84e4314094521dc0316 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Wed, 9 May 2018 21:43:23 +0200
Subject: [PATCH 4/5] Fixed typo

---
 app/models/concerns/with_uploads.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/concerns/with_uploads.rb b/app/models/concerns/with_uploads.rb
index 3d99889c131..e7cfffb775b 100644
--- a/app/models/concerns/with_uploads.rb
+++ b/app/models/concerns/with_uploads.rb
@@ -2,7 +2,7 @@
 # hook. This hook fetches upload location (local vs remote) from
 # Upload model. So it's neccessary to make sure that during that
 # after_commit hook model's associated uploads are not deleted yet.
-# IOW we can not use denepdent => :destroy :
+# IOW we can not use dependent: :destroy :
 # has_many :uploads, as: :model, dependent: :destroy
 #
 # And because not-mounted uploads require presence of upload's

From 2060533f91b5527b568321f63a1aa7f4ef0081d5 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Fri, 11 May 2018 14:45:18 +0200
Subject: [PATCH 5/5] Whitelisted query limits for group destroy API

---
 lib/api/groups.rb    | 1 +
 lib/api/v3/groups.rb | 1 +
 2 files changed, 2 insertions(+)

diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 92e3d5cc10a..0d125cd7831 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -165,6 +165,7 @@ module API
         group = find_group!(params[:id])
         authorize! :admin_group, group
 
+        Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/46285')
         destroy_conditionally!(group) do |group|
           ::Groups::DestroyService.new(group, current_user).execute
         end
diff --git a/lib/api/v3/groups.rb b/lib/api/v3/groups.rb
index 2c52d21fa1c..3844fd4810d 100644
--- a/lib/api/v3/groups.rb
+++ b/lib/api/v3/groups.rb
@@ -131,6 +131,7 @@ module API
         delete ":id" do
           group = find_group!(params[:id])
           authorize! :admin_group, group
+          Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/46285')
           present ::Groups::DestroyService.new(group, current_user).execute, with: Entities::GroupDetail, current_user: current_user
         end