Commit 7da3b2cd authored by Jan Provaznik's avatar Jan Provaznik

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
parent 14507fd1
class Appearance < ActiveRecord::Base class Appearance < ActiveRecord::Base
include WithUploads
include CacheMarkdownField include CacheMarkdownField
include AfterCommitQueue include AfterCommitQueue
include ObjectStorage::BackgroundMove include ObjectStorage::BackgroundMove
...@@ -14,8 +15,6 @@ class Appearance < ActiveRecord::Base ...@@ -14,8 +15,6 @@ class Appearance < ActiveRecord::Base
mount_uploader :logo, AttachmentUploader mount_uploader :logo, AttachmentUploader
mount_uploader :header_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 CACHE_KEY = "current_appearance:#{Gitlab::VERSION}".freeze
after_commit :flush_redis_cache after_commit :flush_redis_cache
......
# 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
...@@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord' ...@@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord'
class Group < Namespace class Group < Namespace
include Gitlab::ConfigHelper include Gitlab::ConfigHelper
include WithUploads
include AfterCommitQueue include AfterCommitQueue
include AccessRequestable include AccessRequestable
include Avatarable include Avatarable
...@@ -30,8 +31,6 @@ class Group < Namespace ...@@ -30,8 +31,6 @@ class Group < Namespace
has_many :variables, class_name: 'Ci::GroupVariable' has_many :variables, class_name: 'Ci::GroupVariable'
has_many :custom_attributes, class_name: 'GroupCustomAttribute' has_many :custom_attributes, class_name: 'GroupCustomAttribute'
has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :boards has_many :boards
has_many :badges, class_name: 'GroupBadge' has_many :badges, class_name: 'GroupBadge'
......
...@@ -4,6 +4,7 @@ class Project < ActiveRecord::Base ...@@ -4,6 +4,7 @@ class Project < ActiveRecord::Base
include Gitlab::ConfigHelper include Gitlab::ConfigHelper
include Gitlab::ShellAdapter include Gitlab::ShellAdapter
include Gitlab::VisibilityLevel include Gitlab::VisibilityLevel
include WithUploads
include AccessRequestable include AccessRequestable
include Avatarable include Avatarable
include CacheMarkdownField include CacheMarkdownField
...@@ -301,8 +302,6 @@ class Project < ActiveRecord::Base ...@@ -301,8 +302,6 @@ class Project < ActiveRecord::Base
inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } } inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
validates :variables, variable_duplicates: { scope: :environment_scope } validates :variables, variable_duplicates: { scope: :environment_scope }
has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
# Scopes # Scopes
scope :pending_delete, -> { where(pending_delete: true) } scope :pending_delete, -> { where(pending_delete: true) }
scope :without_deleted, -> { where(pending_delete: false) } scope :without_deleted, -> { where(pending_delete: false) }
......
...@@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord' ...@@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord'
class User < ActiveRecord::Base class User < ActiveRecord::Base
extend Gitlab::ConfigHelper extend Gitlab::ConfigHelper
include WithUploads
include Gitlab::ConfigHelper include Gitlab::ConfigHelper
include Gitlab::SQL::Pattern include Gitlab::SQL::Pattern
include AfterCommitQueue include AfterCommitQueue
...@@ -137,7 +138,6 @@ class User < ActiveRecord::Base ...@@ -137,7 +138,6 @@ class User < ActiveRecord::Base
has_many :custom_attributes, class_name: 'UserCustomAttribute' has_many :custom_attributes, class_name: 'UserCustomAttribute'
has_many :callouts, class_name: 'UserCallout' has_many :callouts, class_name: 'UserCallout'
has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :term_agreements has_many :term_agreements
belongs_to :accepted_term, class_name: 'ApplicationSetting::Term' belongs_to :accepted_term, class_name: 'ApplicationSetting::Term'
......
---
title: Fix deletion of Object Store uploads
merge_request:
author:
type: fixed
...@@ -5,7 +5,7 @@ describe Appearance do ...@@ -5,7 +5,7 @@ describe Appearance do
it { is_expected.to be_valid } 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 describe '.current', :use_clean_rails_memory_store_caching do
let!(:appearance) { create(:appearance) } let!(:appearance) { create(:appearance) }
...@@ -41,4 +41,12 @@ describe Appearance do ...@@ -41,4 +41,12 @@ describe Appearance do
expect(new_row.valid?).to eq(false) expect(new_row.valid?).to eq(false)
end end
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 end
...@@ -15,7 +15,7 @@ describe Group do ...@@ -15,7 +15,7 @@ describe Group do
it { is_expected.to have_many(:notification_settings).dependent(:destroy) } 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(:labels).class_name('GroupLabel') }
it { is_expected.to have_many(:variables).class_name('Ci::GroupVariable') } 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_one(:chat_team) }
it { is_expected.to have_many(:custom_attributes).class_name('GroupCustomAttribute') } it { is_expected.to have_many(:custom_attributes).class_name('GroupCustomAttribute') }
it { is_expected.to have_many(:badges).class_name('GroupBadge') } it { is_expected.to have_many(:badges).class_name('GroupBadge') }
...@@ -691,4 +691,12 @@ describe Group do ...@@ -691,4 +691,12 @@ describe Group do
end end
end 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 end
...@@ -76,7 +76,7 @@ describe Project do ...@@ -76,7 +76,7 @@ describe Project do
it { is_expected.to have_many(:project_group_links) } 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(:notification_settings).dependent(:delete_all) }
it { is_expected.to have_many(:forks).through(:forked_project_links) } 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(:pipeline_schedules) }
it { is_expected.to have_many(:members_and_requesters) } it { is_expected.to have_many(:members_and_requesters) }
it { is_expected.to have_many(:clusters) } it { is_expected.to have_many(:clusters) }
...@@ -3739,4 +3739,12 @@ describe Project do ...@@ -3739,4 +3739,12 @@ describe Project do
it { is_expected.to be_nil } it { is_expected.to be_nil }
end end
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 end
...@@ -39,7 +39,7 @@ describe User do ...@@ -39,7 +39,7 @@ describe User do
it { is_expected.to have_many(:builds).dependent(:nullify) } it { is_expected.to have_many(:builds).dependent(:nullify) }
it { is_expected.to have_many(:pipelines).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(: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(:reported_abuse_reports).dependent(:destroy).class_name('AbuseReport') }
it { is_expected.to have_many(:custom_attributes).class_name('UserCustomAttribute') } it { is_expected.to have_many(:custom_attributes).class_name('UserCustomAttribute') }
...@@ -2769,4 +2769,12 @@ describe User do ...@@ -2769,4 +2769,12 @@ describe User do
expect { user.increment_failed_attempts! }.not_to change(user, :failed_attempts) expect { user.increment_failed_attempts! }.not_to change(user, :failed_attempts)
end end
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 end
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment