Add/update services to delete snippets repositories

When deleting a snippet repository, we need to consider
three scenarios:
- Project deletions
- User deletions
- Direct snippet deletions

We need to update `Projects::DestroyService` and
`Users::DestroyServices` to handle this new type of
repository.
parent 681b47de
......@@ -47,7 +47,7 @@ module Projects
private
def trash_repositories!
def trash_project_repositories!
unless remove_repository(project.repository)
raise_error(s_('DeleteProject|Failed to remove project repository. Please try again or contact administrator.'))
end
......@@ -57,6 +57,18 @@ module Projects
end
end
def trash_relation_repositories!
unless remove_snippets
raise_error(s_('DeleteProject|Failed to remove project snippets. Please try again or contact administrator.'))
end
end
def remove_snippets
response = Snippets::BulkDestroyService.new(current_user, project.snippets).execute
response.success?
end
def remove_repository(repository)
return true unless repository
......@@ -95,7 +107,8 @@ module Projects
Project.transaction do
log_destroy_event
trash_repositories!
trash_relation_repositories!
trash_project_repositories!
# Rails attempts to load all related records into memory before
# destroying: https://github.com/rails/rails/issues/22510
......@@ -103,7 +116,7 @@ module Projects
#
# Exclude container repositories because its before_destroy would be
# called multiple times, and it doesn't destroy any database records.
project.destroy_dependent_associations_in_batches(exclude: [:container_repositories])
project.destroy_dependent_associations_in_batches(exclude: [:container_repositories, :snippets])
project.destroy!
end
end
......
......@@ -7,8 +7,8 @@ class Repositories::BaseService < BaseService
attr_reader :repository
delegate :project, :disk_path, :full_path, to: :repository
delegate :repository_storage, to: :project
delegate :container, :disk_path, :full_path, to: :repository
delegate :repository_storage, to: :container
def initialize(repository)
@repository = repository
......@@ -31,7 +31,7 @@ class Repositories::BaseService < BaseService
# gitlab/cookies.git -> gitlab/cookies+119+deleted.git
#
def removal_path
"#{disk_path}+#{project.id}#{DELETED_FLAG}"
"#{disk_path}+#{container.id}#{DELETED_FLAG}"
end
# If we get a Gitaly error, the repository may be corrupted. We can
......@@ -40,7 +40,7 @@ class Repositories::BaseService < BaseService
def ignore_git_errors(&block)
yield
rescue Gitlab::Git::CommandError => e
Gitlab::GitLogger.warn(class: self.class.name, project_id: project.id, disk_path: disk_path, message: e.to_s)
Gitlab::GitLogger.warn(class: self.class.name, container_id: container.id, disk_path: disk_path, message: e.to_s)
end
def move_error(path)
......
......@@ -14,11 +14,11 @@ class Repositories::DestroyService < Repositories::BaseService
log_info(%Q{Repository "#{disk_path}" moved to "#{removal_path}" for repository "#{full_path}"})
current_repository = repository
project.run_after_commit do
container.run_after_commit do
Repositories::ShellDestroyService.new(current_repository).execute
end
log_info("Project \"#{project.full_path}\" was removed")
log_info("Repository \"#{full_path}\" was removed")
success
else
......
# frozen_string_literal: true
module Snippets
class BulkDestroyService
include Gitlab::Allowable
attr_reader :current_user, :snippets
DeleteRepositoryError = Class.new(StandardError)
SnippetAccessError = Class.new(StandardError)
def initialize(user, snippets)
@current_user = user
@snippets = snippets
end
def execute
return ServiceResponse.success(message: 'No snippets found.') if snippets.empty?
user_can_delete_snippets!
attempt_delete_repositories!
snippets.destroy_all # rubocop: disable DestroyAll
ServiceResponse.success(message: 'Snippets were deleted.')
rescue SnippetAccessError
service_response_error("You don't have access to delete these snippets.", 403)
rescue DeleteRepositoryError
attempt_rollback_repositories
service_response_error('Failed to delete snippet repositories.', 400)
rescue
# In case the delete operation fails
attempt_rollback_repositories
service_response_error('Failed to remove snippets.', 400)
end
private
def user_can_delete_snippets!
allowed = DeclarativePolicy.user_scope do
snippets.find_each.all? { |snippet| user_can_delete_snippet?(snippet) }
end
raise SnippetAccessError unless allowed
end
def user_can_delete_snippet?(snippet)
can?(current_user, :admin_snippet, snippet)
end
def attempt_delete_repositories!
snippets.each do |snippet|
result = Repositories::DestroyService.new(snippet.repository).execute
raise DeleteRepositoryError if result[:status] == :error
end
end
def attempt_rollback_repositories
snippets.each do |snippet|
result = Repositories::DestroyRollbackService.new(snippet.repository).execute
log_rollback_error(snippet) if result[:status] == :error
end
end
def log_rollback_error(snippet)
Gitlab::AppLogger.error("Repository #{snippet.full_path} in path #{snippet.disk_path} could not be rolled back")
end
def service_response_error(message, http_status)
ServiceResponse.error(message: message, http_status: http_status)
end
end
end
......@@ -4,12 +4,13 @@ module Snippets
class DestroyService
include Gitlab::Allowable
attr_reader :current_user, :project
attr_reader :current_user, :snippet
DestroyError = Class.new(StandardError)
def initialize(user, snippet)
@current_user = user
@snippet = snippet
@project = snippet&.project
end
def execute
......@@ -24,16 +25,29 @@ module Snippets
)
end
if snippet.destroy
ServiceResponse.success(message: 'Snippet was deleted.')
else
service_response_error('Failed to remove snippet.', 400)
end
attempt_destroy!
ServiceResponse.success(message: 'Snippet was deleted.')
rescue DestroyError
service_response_error('Failed to remove snippet repository.', 400)
rescue
attempt_rollback_repository
service_response_error('Failed to remove snippet.', 400)
end
private
attr_reader :snippet
def attempt_destroy!
result = Repositories::DestroyService.new(snippet.repository).execute
raise DestroyError if result[:status] == :error
snippet.destroy!
end
def attempt_rollback_repository
Repositories::DestroyRollbackService.new(snippet.repository).execute
end
def user_can_delete_snippet?
can?(current_user, :admin_snippet, snippet)
......
......@@ -56,10 +56,13 @@ module Users
MigrateToGhostUserService.new(user).execute unless options[:hard_delete]
response = Snippets::BulkDestroyService.new(current_user, user.snippets).execute
raise DestroyError, response.message if response.error?
# Rails attempts to load all related records into memory before
# destroying: https://github.com/rails/rails/issues/22510
# This ensures we delete records in batches.
user.destroy_dependent_associations_in_batches
user.destroy_dependent_associations_in_batches(exclude: [:snippets])
# Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
user_data = user.destroy
......
---
title: Add/update services to delete snippets repositories
merge_request: 22672
author:
type: added
......@@ -50,7 +50,7 @@ module EE
# Git data (e.g. a list of branch names).
flush_caches(project)
trash_repositories!
trash_project_repositories!
log_info("Project \"#{project.name}\" was removed")
end
......
......@@ -6252,6 +6252,9 @@ msgstr ""
msgid "DeleteProject|Failed to remove project repository. Please try again or contact administrator."
msgstr ""
msgid "DeleteProject|Failed to remove project snippets. Please try again or contact administrator."
msgstr ""
msgid "DeleteProject|Failed to remove some tags in project container registry. Please try again or contact administrator."
msgstr ""
......
......@@ -27,6 +27,8 @@ FactoryBot.define do
TestEnv.copy_repo(snippet,
bare_repo: TestEnv.factory_repo_path_bare,
refs: TestEnv::BRANCH_SHA)
snippet.track_snippet_repository
end
end
......
......@@ -20,7 +20,7 @@ describe 'User views merged merge request from deleted fork' do
fork_owner = source_project.namespace.owners.first
# Place the source_project in the weird in between state
source_project.update_attribute(:pending_delete, true)
Projects::DestroyService.new(source_project, fork_owner, {}).__send__(:trash_repositories!)
Projects::DestroyService.new(source_project, fork_owner, {}).__send__(:trash_project_repositories!)
end
it 'correctly shows the merge request' do
......
......@@ -536,7 +536,7 @@ describe Snippet do
end
describe '#track_snippet_repository' do
let(:snippet) { create(:snippet, :repository) }
let(:snippet) { create(:snippet) }
context 'when a snippet repository entry does not exist' do
it 'creates a new entry' do
......@@ -554,7 +554,8 @@ describe Snippet do
end
context 'when a tracking entry exists' do
let!(:snippet_repository) { create(:snippet_repository, snippet: snippet) }
let!(:snippet) { create(:snippet, :repository) }
let(:snippet_repository) { snippet.snippet_repository }
let!(:shard) { create(:shard, name: 'foo') }
it 'does not create a new entry in the database' do
......@@ -592,7 +593,7 @@ describe Snippet do
end
context 'when repository exists' do
let(:snippet) { create(:snippet, :repository) }
let!(:snippet) { create(:snippet, :repository) }
it 'does not try to create repository' do
expect(snippet.repository).not_to receive(:after_create)
......
......@@ -124,7 +124,7 @@ describe Projects::DestroyService do
allow(project.repository).to receive(:before_delete).and_raise(::Gitlab::Git::CommandError)
allow(Gitlab::GitLogger).to receive(:warn).with(
class: Repositories::DestroyService.name,
project_id: project.id,
container_id: project.id,
disk_path: project.disk_path,
message: 'Gitlab::Git::CommandError').and_call_original
end
......@@ -338,6 +338,39 @@ describe Projects::DestroyService do
end
end
context 'snippets' do
let!(:snippet1) { create(:project_snippet, project: project, author: user) }
let!(:snippet2) { create(:project_snippet, project: project, author: user) }
it 'does not include snippets when deleting in batches' do
expect(project).to receive(:destroy_dependent_associations_in_batches).with({ exclude: [:container_repositories, :snippets] })
destroy_project(project, user)
end
it 'calls the bulk snippet destroy service' do
expect(project.snippets.count).to eq 2
expect(Snippets::BulkDestroyService).to receive(:new)
.with(user, project.snippets).and_call_original
expect do
destroy_project(project, user)
end.to change(Snippet, :count).by(-2)
end
context 'when an error is raised deleting snippets' do
it 'does not delete project' do
allow_next_instance_of(Snippets::BulkDestroyService) do |instance|
allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'foo'))
end
expect(destroy_project(project, user)).to be_falsey
expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
end
end
end
def destroy_project(project, user, params = {})
described_class.new(project, user, params).public_send(async ? :async_execute : :execute)
end
......
# frozen_string_literal: true
require 'spec_helper'
describe Snippets::BulkDestroyService do
let_it_be(:project) { create(:project) }
let(:user) { create(:user) }
let!(:personal_snippet) { create(:personal_snippet, :repository, author: user) }
let!(:project_snippet) { create(:project_snippet, :repository, project: project, author: user) }
let(:snippets) { user.snippets }
let(:gitlab_shell) { Gitlab::Shell.new }
let(:service_user) { user }
before do
project.add_developer(user)
end
subject { described_class.new(service_user, snippets) }
describe '#execute' do
it 'deletes the snippets in bulk' do
response = nil
expect(Repositories::ShellDestroyService).to receive(:new).with(personal_snippet.repository).and_call_original
expect(Repositories::ShellDestroyService).to receive(:new).with(project_snippet.repository).and_call_original
aggregate_failures do
expect do
response = subject.execute
end.to change(Snippet, :count).by(-2)
expect(response).to be_success
expect(repository_exists?(personal_snippet)).to be_falsey
expect(repository_exists?(project_snippet)).to be_falsey
end
end
context 'when snippets is empty' do
let(:snippets) { Snippet.none }
it 'returns a ServiceResponse success response' do
response = subject.execute
expect(response).to be_success
expect(response.message).to eq 'No snippets found.'
end
end
shared_examples 'error is raised' do
it 'returns error' do
response = subject.execute
aggregate_failures do
expect(response).to be_error
expect(response.message).to eq error_message
end
end
it 'no record is deleted' do
expect do
subject.execute
end.not_to change(Snippet, :count)
end
end
context 'when user does not have access to remove the snippet' do
let(:service_user) { create(:user) }
it_behaves_like 'error is raised' do
let(:error_message) { "You don't have access to delete these snippets." }
end
end
context 'when an error is raised deleting the repository' do
before do
allow_next_instance_of(Repositories::DestroyService) do |instance|
allow(instance).to receive(:execute).and_return({ status: :error })
end
end
it_behaves_like 'error is raised' do
let(:error_message) { 'Failed to delete snippet repositories.' }
end
it 'tries to rollback the repository' do
expect(subject).to receive(:attempt_rollback_repositories)
subject.execute
end
end
context 'when an error is raised deleting the records' do
before do
allow(snippets).to receive(:destroy_all).and_raise(ActiveRecord::ActiveRecordError)
end
it_behaves_like 'error is raised' do
let(:error_message) { 'Failed to remove snippets.' }
end
it 'tries to rollback the repository' do
expect(subject).to receive(:attempt_rollback_repositories)
subject.execute
end
end
context 'when snippet does not have a repository attached' do
let!(:snippet_without_repo) { create(:personal_snippet, author: user) }
it 'does not schedule anything for the snippet without repository and return success' do
response = nil
expect(Repositories::ShellDestroyService).to receive(:new).with(personal_snippet.repository).and_call_original
expect(Repositories::ShellDestroyService).to receive(:new).with(project_snippet.repository).and_call_original
expect do
response = subject.execute
end.to change(Snippet, :count).by(-3)
expect(response).to be_success
end
end
end
describe '#attempt_rollback_repositories' do
before do
Repositories::DestroyService.new(personal_snippet.repository).execute
end
it 'rollbacks the repository' do
error_msg = personal_snippet.disk_path + "+#{personal_snippet.id}+deleted.git"
expect(repository_exists?(personal_snippet, error_msg)).to be_truthy
subject.__send__(:attempt_rollback_repositories)
aggregate_failures do
expect(repository_exists?(personal_snippet, error_msg)).to be_falsey
expect(repository_exists?(personal_snippet)).to be_truthy
end
end
context 'when an error is raised' do
before do
allow_next_instance_of(Repositories::DestroyRollbackService) do |instance|
allow(instance).to receive(:execute).and_return({ status: :error })
end
end
it 'logs the error' do
expect(Gitlab::AppLogger).to receive(:error).with(/\ARepository .* in path .* could not be rolled back\z/).twice
subject.__send__(:attempt_rollback_repositories)
end
end
end
def repository_exists?(snippet, path = snippet.disk_path + ".git")
gitlab_shell.repository_exists?(snippet.snippet_repository.shard_name, path)
end
end
......@@ -18,7 +18,7 @@ describe Snippets::CreateService do
let(:extra_opts) { {} }
let(:creator) { admin }
subject { Snippets::CreateService.new(project, creator, opts).execute }
subject { described_class.new(project, creator, opts).execute }
let(:snippet) { subject.payload[:snippet] }
......
......@@ -8,7 +8,7 @@ describe Snippets::DestroyService do
let_it_be(:other_user) { create(:user) }
describe '#execute' do
subject { Snippets::DestroyService.new(user, snippet).execute }
subject { described_class.new(user, snippet).execute }
context 'when snippet is nil' do
let(:snippet) { nil }
......@@ -30,7 +30,7 @@ describe Snippets::DestroyService do
shared_examples 'an unsuccessful destroy' do
it 'does not delete the snippet' do
expect { subject }.to change { Snippet.count }.by(0)
expect { subject }.not_to change { Snippet.count }
end
it 'returns ServiceResponse error' do
......@@ -38,8 +38,63 @@ describe Snippets::DestroyService do
end
end
shared_examples 'deletes the snippet repository' do
it 'removes the snippet repository' do
expect(snippet.repository.exists?).to be_truthy
expect(GitlabShellWorker).to receive(:perform_in)
expect_next_instance_of(Repositories::DestroyService) do |instance|
expect(instance).to receive(:execute).and_call_original
end
expect(subject).to be_success
end
context 'when the repository deletion service raises an error' do
before do
allow_next_instance_of(Repositories::DestroyService) do |instance|
allow(instance).to receive(:execute).and_return({ status: :error })
end
end
it_behaves_like 'an unsuccessful destroy'
it 'does not try to rollback repository' do
expect(Repositories::DestroyRollbackService).not_to receive(:new)
subject
end
end
context 'when a destroy error is raised' do
before do
allow(snippet).to receive(:destroy!).and_raise(ActiveRecord::ActiveRecordError)
end
it_behaves_like 'an unsuccessful destroy'
it 'attempts to rollback the repository' do
expect(Repositories::DestroyRollbackService).to receive(:new).and_call_original
subject
end
end
context 'when repository is nil' do
it 'does not schedule anything and return success' do
allow(snippet).to receive(:repository).and_return(nil)
expect(GitlabShellWorker).not_to receive(:perform_in)
expect_next_instance_of(Repositories::DestroyService) do |instance|
expect(instance).to receive(:execute).and_call_original
end
expect(subject).to be_success
end
end
end
context 'when ProjectSnippet' do
let!(:snippet) { create(:project_snippet, project: project, author: author) }
let!(:snippet) { create(:project_snippet, :repository, project: project, author: author) }
context 'when user is able to admin_project_snippet' do
let(:author) { user }
......@@ -49,6 +104,7 @@ describe Snippets::DestroyService do
end
it_behaves_like 'a successful destroy'
it_behaves_like 'deletes the snippet repository'
end
context 'when user is not able to admin_project_snippet' do
......@@ -59,12 +115,13 @@ describe Snippets::DestroyService do
end
context 'when PersonalSnippet' do
let!(:snippet) { create(:personal_snippet, author: author) }
let!(:snippet) { create(:personal_snippet, :repository, author: author) }
context 'when user is able to admin_personal_snippet' do
let(:author) { user }
it_behaves_like 'a successful destroy'
it_behaves_like 'deletes the snippet repository'
end
context 'when user is not able to admin_personal_snippet' do
......@@ -73,5 +130,21 @@ describe Snippets::DestroyService do
it_behaves_like 'an unsuccessful destroy'
end
end
context 'when the repository does not exists' do
let(:snippet) { create(:personal_snippet, author: user) }
it 'does not schedule anything and return success' do
expect(snippet.repository).not_to be_nil
expect(snippet.repository.exists?).to be_falsey
expect(GitlabShellWorker).not_to receive(:perform_in)
expect_next_instance_of(Repositories::DestroyService) do |instance|
expect(instance).to receive(:execute).and_call_original
end
expect(subject).to be_success
end
end
end
end
......@@ -18,7 +18,7 @@ describe Snippets::UpdateService do
let(:updater) { user }
subject do
Snippets::UpdateService.new(
described_class.new(
project,
updater,
options
......
......@@ -26,6 +26,12 @@ describe Users::DestroyService do
service.execute(user)
end
it 'does not include snippets when deleting in batches' do
expect(user).to receive(:destroy_dependent_associations_in_batches).with({ exclude: [:snippets] })
service.execute(user)
end
it 'will delete the project' do
expect_next_instance_of(Projects::DestroyService) do |destroy_service|
expect(destroy_service).to receive(:execute).once.and_return(true)
......@@ -33,6 +39,54 @@ describe Users::DestroyService do
service.execute(user)
end
it 'calls the bulk snippet destroy service for the user personal snippets' do
repo1 = create(:personal_snippet, :repository, author: user).snippet_repository
repo2 = create(:project_snippet, :repository, author: user).snippet_repository
repo3 = create(:project_snippet, :repository, project: project, author: user).snippet_repository
aggregate_failures do
expect(gitlab_shell.repository_exists?(repo1.shard_name, repo1.disk_path + '.git')).to be_truthy
expect(gitlab_shell.repository_exists?(repo2.shard_name, repo2.disk_path + '.git')).to be_truthy
expect(gitlab_shell.repository_exists?(repo3.shard_name, repo3.disk_path + '.git')).to be_truthy
end
# Call made when destroying user personal projects
expect(Snippets::BulkDestroyService).to receive(:new)
.with(admin, project.snippets).and_call_original
# Call to remove user personal snippets and for
# project snippets where projects are not user personal
# ones
expect(Snippets::BulkDestroyService).to receive(:new)
.with(admin, user.snippets).and_call_original
service.execute(user)
aggregate_failures do
expect(gitlab_shell.repository_exists?(repo1.shard_name, repo1.disk_path + '.git')).to be_falsey
expect(gitlab_shell.repository_exists?(repo2.shard_name, repo2.disk_path + '.git')).to be_falsey
expect(gitlab_shell.repository_exists?(repo3.shard_name, repo3.disk_path + '.git')).to be_falsey
end
end
context 'when an error is raised deleting snippets' do
it 'does not delete user' do
snippet = create(:personal_snippet, :repository, author: user)
bulk_service = double
allow(Snippets::BulkDestroyService).to receive(:new).and_call_original
allow(Snippets::BulkDestroyService).to receive(:new).with(admin, user.snippets).and_return(bulk_service)
allow(bulk_service).to receive(:execute).and_return(ServiceResponse.error(message: 'foo'))
aggregate_failures do
expect { service.execute(user) }
.to raise_error(Users::DestroyService::DestroyError, 'foo' )
expect(snippet.reload).not_to be_nil
expect(gitlab_shell.repository_exists?(snippet.repository_storage, snippet.disk_path + '.git')).to be_truthy
end
end
end
end
context 'projects in pending_delete' do
......
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