Commit 60b14e52 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'jprovazn-remote-upload-destroy' into 'master'

Delete remote uploads

Closes #45425

See merge request gitlab-org/gitlab-ce!18698
parents bd0a12be 2060533f
......@@ -2,6 +2,7 @@ class Appearance < ActiveRecord::Base
include CacheMarkdownField
include AfterCommitQueue
include ObjectStorage::BackgroundMove
include WithUploads
cache_markdown_field :description
cache_markdown_field :new_project_guidelines
......@@ -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
......
# 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 dependent: :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).find_each do |upload|
upload.destroy
end
end
end
......@@ -10,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
......@@ -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'
......
......@@ -23,6 +23,7 @@ class Project < ActiveRecord::Base
include ::Gitlab::Utils::StrongMemoize
include ChronicDurationAttribute
include FastDestroyAll::Helpers
include WithUploads
extend Gitlab::ConfigHelper
......@@ -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) }
......
......@@ -17,6 +17,7 @@ class User < ActiveRecord::Base
include IgnorableColumn
include BulkMemberAccessLoad
include BlocksJsonSerialization
include WithUploads
DEFAULT_NOTIFICATION_LEVEL = :participating
......@@ -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'
......
---
title: Fix deletion of Object Store uploads
merge_request:
author:
type: fixed
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......@@ -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
......@@ -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') }
......@@ -2809,4 +2809,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
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