Merge branch 'fix/gb/recover-from-renaming-project-with-container-images' into 'master'

Recover from renaming project that has container images

Closes #23019

See merge request !12840

7 files changed, 117 insertions, 38 deletions

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 87a69e8e6f9..c769693255c 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -50,10 +50,13 @@ class ProjectsController < Projects::ApplicationController
     respond_to do |format|
       if result[:status] == :success
         flash[:notice] = _("Project '%{project_name}' was successfully updated.") % { project_name: @project.name }
+
         format.html do
           redirect_to(edit_project_path(@project))
         end
       else
+        flash[:alert] = result[:message]
+
         format.html { render 'edit' }
       end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index c0a1aadc76d..0b357d5d003 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -486,7 +486,9 @@ class Project < ActiveRecord::Base
   end
 
   def has_container_registry_tags?
-    container_repositories.to_a.any?(&:has_tags?) ||
+    return @images if defined?(@images)
+
+    @images = container_repositories.to_a.any?(&:has_tags?) ||
       has_root_container_repository_tags?
   end
 
@@ -977,8 +979,6 @@ class Project < ActiveRecord::Base
 
     Rails.logger.error "Attempting to rename #{old_path_with_namespace} -> #{new_path_with_namespace}"
 
-    expire_caches_before_rename(old_path_with_namespace)
-
     if has_container_registry_tags?
       Rails.logger.error "Project #{old_path_with_namespace} cannot be renamed because container registry tags are present!"
 
@@ -986,6 +986,8 @@ class Project < ActiveRecord::Base
       raise StandardError.new('Project cannot be renamed, because images are present in its container registry')
     end
 
+    expire_caches_before_rename(old_path_with_namespace)
+
     if gitlab_shell.mv_repository(repository_storage_path, old_path_with_namespace, new_path_with_namespace)
       # If repository moved successfully we need to send update instructions to users.
       # However we cannot allow rollback since we moved repository
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index 55d9cb13ae4..30ca95eef7a 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -1,22 +1,16 @@
 module Projects
   class UpdateService < BaseService
     def execute
-      # check that user is allowed to set specified visibility_level
-      new_visibility = params[:visibility_level]
-
-      if new_visibility && new_visibility.to_i != project.visibility_level
-        unless can?(current_user, :change_visibility_level, project) &&
-            Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
-
-          deny_visibility_level(project, new_visibility)
-          return error('Visibility level unallowed')
-        end
+      unless visibility_level_allowed?
+        return error('New visibility level not allowed!')
       end
 
-      new_branch = params[:default_branch]
+      if project.has_container_registry_tags?
+        return error('Cannot rename project because it contains container registry tags!')
+      end
 
-      if project.repository.exists? && new_branch && new_branch != project.default_branch
-        project.change_head(new_branch)
+      if changing_default_branch?
+        project.change_head(params[:default_branch])
       end
 
       if project.update_attributes(params.except(:default_branch))
@@ -28,8 +22,33 @@ module Projects
 
         success
       else
-        error('Project could not be updated')
+        error('Project could not be updated!')
       end
     end
+
+    private
+
+    def visibility_level_allowed?
+      # check that user is allowed to set specified visibility_level
+      new_visibility = params[:visibility_level]
+
+      if new_visibility && new_visibility.to_i != project.visibility_level
+        unless can?(current_user, :change_visibility_level, project) &&
+            Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
+
+          deny_visibility_level(project, new_visibility)
+          return false
+        end
+      end
+
+      true
+    end
+
+    def changing_default_branch?
+      new_branch = params[:default_branch]
+
+      project.repository.exists? &&
+        new_branch && new_branch != project.default_branch
+    end
   end
 end
diff --git a/changelogs/unreleased/fix-gb-recover-from-renaming-project-with-container-images.yml b/changelogs/unreleased/fix-gb-recover-from-renaming-project-with-container-images.yml
new file mode 100644
index 00000000000..7adc53eb8fa
--- /dev/null
+++ b/changelogs/unreleased/fix-gb-recover-from-renaming-project-with-container-images.yml
@@ -0,0 +1,4 @@
+---
+title: Recover from renaming project that has container images
+merge_request: 12840
+author:
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index f96fe7ad5cb..192cca45d99 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -211,24 +211,43 @@ describe ProjectsController do
 
     let(:admin) { create(:admin) }
     let(:project) { create(:project, :repository) }
-    let(:new_path) { 'renamed_path' }
-    let(:project_params) { { path: new_path } }
 
     before do
       sign_in(admin)
     end
 
-    it "sets the repository to the right path after a rename" do
-      controller.instance_variable_set(:@project, project)
+    context 'when only renaming a project path' do
+      it "sets the repository to the right path after a rename" do
+        expect { update_project path: 'renamed_path' }
+          .to change { project.reload.path }
 
-      put :update,
-          namespace_id: project.namespace,
-          id: project.id,
-          project: project_params
+        expect(project.path).to include 'renamed_path'
+        expect(assigns(:repository).path).to include project.path
+        expect(response).to have_http_status(302)
+      end
+    end
 
-      expect(project.repository.path).to include(new_path)
-      expect(assigns(:repository).path).to eq(project.repository.path)
-      expect(response).to have_http_status(302)
+    context 'when project has container repositories with tags' do
+      before do
+        stub_container_registry_config(enabled: true)
+        stub_container_registry_tags(repository: /image/, tags: %w[rc1])
+        create(:container_repository, project: project, name: :image)
+      end
+
+      it 'does not allow to rename the project' do
+        expect { update_project path: 'renamed_path' }
+          .not_to change { project.reload.path }
+
+        expect(controller).to set_flash[:alert].to(/container registry tags/)
+        expect(response).to have_http_status(200)
+      end
+    end
+
+    def update_project(**parameters)
+      put :update,
+          namespace_id: project.namespace.path,
+          id: project.path,
+          project: parameters
     end
   end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index c4bc129dd37..e636250c37d 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1236,7 +1236,7 @@ describe Project, models: true do
 
       subject { project.rename_repo }
 
-      it { expect{subject}.to raise_error(Exception) }
+      it { expect{subject}.to raise_error(StandardError) }
     end
   end
 
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index 05b18fef061..fd4011ad606 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -1,11 +1,14 @@
 require 'spec_helper'
 
-describe Projects::UpdateService, services: true do
+describe Projects::UpdateService, '#execute', :services do
   let(:user) { create(:user) }
   let(:admin) { create(:admin) }
-  let(:project) { create(:empty_project, creator_id: user.id, namespace: user.namespace) }
 
-  describe 'update_by_user' do
+  let(:project) do
+    create(:empty_project, creator: user, namespace: user.namespace)
+  end
+
+  context 'when changing visibility level' do
     context 'when visibility_level is INTERNAL' do
       it 'updates the project to internal' do
         result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
@@ -40,7 +43,7 @@ describe Projects::UpdateService, services: true do
         it 'does not update the project to public' do
           result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
 
-          expect(result).to eq({ status: :error, message: 'Visibility level unallowed' })
+          expect(result).to eq({ status: :error, message: 'New visibility level not allowed!' })
           expect(project).to be_private
         end
 
@@ -55,12 +58,13 @@ describe Projects::UpdateService, services: true do
     end
   end
 
-  describe 'visibility_level' do
+  describe 'when updating project that has forks' do
     let(:project) { create(:empty_project, :internal) }
     let(:forked_project) { create(:forked_project_with_submodules, :internal) }
 
     before do
-      forked_project.build_forked_project_link(forked_to_project_id: forked_project.id, forked_from_project_id: project.id)
+      forked_project.build_forked_project_link(forked_to_project_id: forked_project.id,
+                                               forked_from_project_id: project.id)
       forked_project.save
     end
 
@@ -89,10 +93,38 @@ describe Projects::UpdateService, services: true do
     end
   end
 
-  it 'returns an error result when record cannot be updated' do
-    result = update_project(project, admin, { name: 'foo&bar' })
+  context 'when updating a default branch' do
+    let(:project) { create(:project, :repository) }
+
+    it 'changes a default branch' do
+      update_project(project, admin, default_branch: 'feature')
+
+      expect(Project.find(project.id).default_branch).to eq 'feature'
+    end
+  end
 
-    expect(result).to eq({ status: :error, message: 'Project could not be updated' })
+  context 'when renaming project that contains container images' do
+    before do
+      stub_container_registry_config(enabled: true)
+      stub_container_registry_tags(repository: /image/, tags: %w[rc1])
+      create(:container_repository, project: project, name: :image)
+    end
+
+    it 'does not allow to rename the project' do
+      result = update_project(project, admin, path: 'renamed')
+
+      expect(result).to include(status: :error)
+      expect(result[:message]).to match(/contains container registry tags/)
+    end
+  end
+
+  context 'when passing invalid parameters' do
+    it 'returns an error result when record cannot be updated' do
+      result = update_project(project, admin, { name: 'foo&bar' })
+
+      expect(result).to eq({ status: :error,
+                             message: 'Project could not be updated!' })
+    end
   end
 
   def update_project(project, user, opts)