Merge branch 'fj-fix-snippet-import-when-fails' into 'master'

Fix "Invalid state of snippets after Project import"

See merge request gitlab-org/gitlab!33621

changelogs/unreleased/fj-fix-snippet-import-when-fails.yml

+---
+title: Remove non migrated snippets from failed imports
+merge_request: 33621
+author:
+type: fixed

lib/gitlab/import_export/importer.rb

@@ -24,6 +24,11 @@ module Gitlab
           raise Projects::ImportService::Error.new(shared.errors.to_sentence)
         end
       rescue => e
+        # If some exception was raised could mean that the SnippetsRepoRestorer
+        # was not called. This would leave us with snippets without a repository.
+        # This is a state we don't want them to be, so we better delete them.
+        remove_non_migrated_snippets
+
         raise Projects::ImportService::Error.new(e.message)
       ensure
         remove_base_tmp_dir
@@ -153,6 +158,14 @@ module Gitlab
       def remove_base_tmp_dir
         FileUtils.rm_rf(@shared.base_path)
       end
+
+      def remove_non_migrated_snippets
+        project
+          .snippets
+          .left_joins(:snippet_repository)
+          .where(snippet_repositories: { snippet_id: nil })
+          .delete_all
+      end
     end
   end
 end

lib/gitlab/import_export/snippet_repo_restorer.rb

@@ -5,6 +5,8 @@ module Gitlab
     class SnippetRepoRestorer < RepoRestorer
       attr_reader :snippet
 
+      SnippetRepositoryError = Class.new(StandardError)
+
       def initialize(snippet:, user:, shared:, path_to_bundle:)
         @snippet = snippet
         @user = user
@@ -35,6 +37,10 @@ module Gitlab
 
       def create_repository_from_db
         Gitlab::BackgroundMigration::BackfillSnippetRepositories.new.perform_by_ids([snippet.id])
+
+        unless snippet.reset.snippet_repository
+          raise SnippetRepositoryError, _("Error creating repository for snippet with id %{snippet_id}") % { snippet_id: snippet.id }
+        end
       end
     end
   end

lib/gitlab/import_export/snippets_repo_restorer.rb

@@ -10,13 +10,13 @@ module Gitlab
       end
 
       def restore
-        @project.snippets.find_each.all? do |snippet|
+        @project.snippets.find_each.map do |snippet|
           Gitlab::ImportExport::SnippetRepoRestorer.new(snippet: snippet,
                                                         user: @user,
                                                         shared: @shared,
                                                         path_to_bundle: snippet_repo_bundle_path(snippet))
                                                    .restore
-        end
+        end.all?(true)
       end
 
       private

locale/gitlab.pot

@@ -8,8 +8,6 @@ msgid ""
 msgstr ""
 "Project-Id-Version: gitlab 1.0.0\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2020-06-01 14:24-0400\n"
-"PO-Revision-Date: 2020-06-01 14:24-0400\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
 "Language: \n"
@@ -8736,6 +8734,9 @@ msgstr ""
 msgid "Error creating new iteration"
 msgstr ""
 
+msgid "Error creating repository for snippet with id %{snippet_id}"
+msgstr ""
+
 msgid "Error creating the snippet"
 msgstr ""
 

spec/lib/gitlab/import_export/importer_spec.rb

@@ -97,6 +97,49 @@ describe Gitlab::ImportExport::Importer do
       end
     end
 
+    context 'when import fails' do
+      let(:error_message) { 'foo' }
+
+      shared_examples 'removes any non migrated snippet' do
+        specify do
+          create_list(:project_snippet, 2, project: project)
+          snippet_with_repo = create(:project_snippet, :repository, project: project)
+
+          expect { importer.execute }.to change(Snippet, :count).by(-2).and(raise_error(Projects::ImportService::Error))
+
+          expect(snippet_with_repo.reload).to be_present
+        end
+      end
+
+      context 'when there is a graceful error' do
+        before do
+          allow_next_instance_of(Gitlab::ImportExport::AvatarRestorer) do |instance|
+            allow(instance).to receive(:avatar_export_file).and_raise(StandardError, error_message)
+          end
+        end
+
+        it 'raises and exception' do
+          expect { importer.execute }.to raise_error(Projects::ImportService::Error, error_message)
+        end
+
+        it_behaves_like 'removes any non migrated snippet'
+      end
+
+      context 'when an unexpected exception is raised' do
+        before do
+          allow_next_instance_of(Gitlab::ImportExport::AvatarRestorer) do |instance|
+            allow(instance).to receive(:restore).and_raise(StandardError, error_message)
+          end
+        end
+
+        it 'captures it and raises the Projects::ImportService::Error exception' do
+          expect { importer.execute }.to raise_error(Projects::ImportService::Error, error_message)
+        end
+
+        it_behaves_like 'removes any non migrated snippet'
+      end
+    end
+
     context 'when project successfully restored' do
       context "with a project in a user's namespace" do
         let!(:existing_project) { create(:project, namespace: user.namespace) }

spec/lib/gitlab/import_export/snippet_repo_restorer_spec.rb

@@ -34,6 +34,15 @@ describe Gitlab::ImportExport::SnippetRepoRestorer do
         expect(blob.data).to eq(snippet.content)
       end
     end
+
+    context 'when the repository creation fails' do
+      it 'returns false' do
+        allow_any_instance_of(Gitlab::BackgroundMigration::BackfillSnippetRepositories).to receive(:perform_by_ids).and_return(nil)
+
+        expect(restorer.restore).to be false
+        expect(shared.errors.first).to match(/Error creating repository for snippet/)
+      end
+    end
   end
 
   context 'when the snippet does not have a bundle file path' do

spec/lib/gitlab/import_export/snippets_repo_restorer_spec.rb

@@ -86,13 +86,14 @@ describe Gitlab::ImportExport::SnippetsRepoRestorer do
       it_behaves_like 'imports snippet repositories'
     end
 
-    context 'when one snippet cannot be saved' do
-      it 'returns false and do not process other snippets' do
+    context 'when any of the snippet repositories cannot be created' do
+      it 'continues processing other snippets and returns false' do
         allow(Gitlab::ImportExport::SnippetRepoRestorer).to receive(:new).with(hash_including(snippet: snippet1)).and_return(service)
         allow(service).to receive(:restore).and_return(false)
 
-        expect(Gitlab::ImportExport::SnippetRepoRestorer).not_to receive(:new).with(hash_including(snippet: snippet2))
-        expect(restorer.restore).to be_falsey
+        expect(Gitlab::ImportExport::SnippetRepoRestorer).to receive(:new).with(hash_including(snippet: snippet2)).and_call_original
+
+        expect(restorer.restore).to be false
       end
     end