Remove permanent redirects

Removes permanent redirects, this means that redirects will only be
possible as long as the old route isn't taken by a new project/group.

app/models/redirect_route.rb

@@ -17,32 +17,4 @@ class RedirectRoute < ActiveRecord::Base
 
     where(wheres, path, "#{sanitize_sql_like(path)}/%")
   end
-
-  scope :permanent, -> do
-    if column_permanent_exists?
-      where(permanent: true)
-    else
-      none
-    end
-  end
-
-  scope :temporary, -> do
-    if column_permanent_exists?
-      where(permanent: [false, nil])
-    else
-      all
-    end
-  end
-
-  default_value_for :permanent, false
-
-  def permanent=(value)
-    if self.class.column_permanent_exists?
-      super
-    end
-  end
-
-  def self.column_permanent_exists?
-    ActiveRecord::Base.connection.column_exists?(:redirect_routes, :permanent)
-  end
 end

app/models/route.rb

@@ -10,8 +10,6 @@ class Route < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false }
 
-  validate :ensure_permanent_paths, if: :path_changed?
-
   before_validation :delete_conflicting_orphaned_routes
   after_create :delete_conflicting_redirects
   after_update :delete_conflicting_redirects, if: :path_changed?
@@ -45,7 +43,7 @@ class Route < ActiveRecord::Base
         # We are not calling route.delete_conflicting_redirects here, in hopes
         # of avoiding deadlocks. The parent (self, in this method) already
         # called it, which deletes conflicts for all descendants.
-        route.create_redirect(old_path, permanent: permanent_redirect?) if attributes[:path]
+        route.create_redirect(old_path) if attributes[:path]
       end
     end
   end
@@ -55,31 +53,17 @@ class Route < ActiveRecord::Base
   end
 
   def conflicting_redirects
-    RedirectRoute.temporary.matching_path_and_descendants(path)
+    RedirectRoute.matching_path_and_descendants(path)
   end
 
-  def create_redirect(path, permanent: false)
-    RedirectRoute.create(source: source, path: path, permanent: permanent)
+  def create_redirect(path)
+    RedirectRoute.create(source: source, path: path)
   end
 
   private
 
   def create_redirect_for_old_path
-    create_redirect(path_was, permanent: permanent_redirect?) if path_changed?
-  end
-
-  def permanent_redirect?
-    source_type != "Project"
-  end
-
-  def ensure_permanent_paths
-    return if path.nil?
-
-    errors.add(:path, "has been taken before") if conflicting_redirect_exists?
-  end
-
-  def conflicting_redirect_exists?
-    RedirectRoute.permanent.matching_path_and_descendants(path).exists?
+    create_redirect(path_was) if path_changed?
   end
 
   def delete_conflicting_orphaned_routes

changelogs/unreleased/bvl-no-permanent-redirect.yml

+---
+title: Don't create permanent redirect routes
+merge_request: 17521
+author:
+type: changed

lib/gitlab/checks/project_moved.rb

@@ -9,20 +9,16 @@ module Gitlab
         super(project, user, protocol)
       end
 
-      def message(rejected: false)
+      def message
         <<~MESSAGE
         Project '#{redirected_path}' was moved to '#{project.full_path}'.
 
         Please update your Git remote:
 
-          #{remote_url_message(rejected)}
+          git remote set-url origin #{url_to_repo}
         MESSAGE
       end
 
-      def permanent_redirect?
-        RedirectRoute.permanent.exists?(path: redirected_path)
-      end
-
       private
 
       attr_reader :redirected_path
@@ -30,18 +26,6 @@ module Gitlab
       def self.message_key(user_id, project_id)
         "#{REDIRECT_NAMESPACE}:#{user_id}:#{project_id}"
       end
-
-      def remote_url_message(rejected)
-        if rejected
-          "git remote set-url origin #{url_to_repo} and try again."
-        else
-          "git remote set-url origin #{url_to_repo}"
-        end
-      end
-
-      def url
-        protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo
-      end
     end
   end
 end

lib/gitlab/git_access.rb

@@ -53,7 +53,7 @@ module Gitlab
       ensure_project_on_push!(cmd, changes)
 
       check_project_accessibility!
-      check_project_moved!
+      add_project_moved_message!
       check_repository_existence!
 
       case cmd
@@ -125,16 +125,12 @@ module Gitlab
       end
     end
 
-    def check_project_moved!
+    def add_project_moved_message!
       return if redirected_path.nil?
 
       project_moved = Checks::ProjectMoved.new(project, user, protocol, redirected_path)
 
-      if project_moved.permanent_redirect?
-        project_moved.add_message
-      else
-        raise ProjectMovedError, project_moved.message(rejected: true)
-      end
+      project_moved.add_message
     end
 
     def check_command_disabled!(cmd)

spec/factories/redirect_routes.rb

@@ -2,14 +2,5 @@ FactoryBot.define do
   factory :redirect_route do
     sequence(:path) { |n| "redirect#{n}" }
     source factory: :group
-    permanent false
-
-    trait :permanent do
-      permanent true
-    end
-
-    trait :temporary do
-      permanent false
-    end
   end
 end

spec/lib/gitlab/checks/project_moved_spec.rb

@@ -44,44 +44,17 @@ describe Gitlab::Checks::ProjectMoved, :clean_gitlab_redis_shared_state do
   end
 
   describe '#message' do
-    context 'when the push is rejected' do
-      it 'returns a redirect message telling the user to try again' do
-        project_moved = described_class.new(project, user, 'http', 'foo/bar')
-        message = "Project 'foo/bar' was moved to '#{project.full_path}'." +
-          "\n\nPlease update your Git remote:" +
-          "\n\n  git remote set-url origin #{project.http_url_to_repo} and try again.\n"
+    it 'returns a redirect message' do
+      project_moved = described_class.new(project, user, 'http', 'foo/bar')
+      message = <<~MSG
+                Project 'foo/bar' was moved to '#{project.full_path}'.
 
-        expect(project_moved.message(rejected: true)).to eq(message)
-      end
-    end
+                Please update your Git remote:
 
-    context 'when the push is not rejected' do
-      it 'returns a redirect message' do
-        project_moved = described_class.new(project, user, 'http', 'foo/bar')
-        message = "Project 'foo/bar' was moved to '#{project.full_path}'." +
-          "\n\nPlease update your Git remote:" +
-          "\n\n  git remote set-url origin #{project.http_url_to_repo}\n"
+                  git remote set-url origin #{project.http_url_to_repo}
+                MSG
 
-        expect(project_moved.message).to eq(message)
-      end
-    end
-  end
-
-  describe '#permanent_redirect?' do
-    context 'with a permanent RedirectRoute' do
-      it 'returns true' do
-        project.route.create_redirect('foo/bar', permanent: true)
-        project_moved = described_class.new(project, user, 'http', 'foo/bar')
-        expect(project_moved.permanent_redirect?).to be_truthy
-      end
-    end
-
-    context 'without a permanent RedirectRoute' do
-      it 'returns false' do
-        project.route.create_redirect('foo/bar')
-        project_moved = described_class.new(project, user, 'http', 'foo/bar')
-        expect(project_moved.permanent_redirect?).to be_falsy
-      end
+      expect(project_moved.message).to eq(message)
     end
   end
 end

spec/lib/gitlab/git_access_spec.rb

@@ -240,14 +240,21 @@ describe Gitlab::GitAccess do
   end
 
   shared_examples 'check_project_moved' do
-    it 'enqueues a redirected message' do
+    it 'enqueues a redirected message for pushing' do
       push_access_check
 
       expect(Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id)).not_to be_nil
     end
+
+    it 'allows push and pull access' do
+      aggregate_failures do
+        expect { push_access_check }.not_to raise_error
+        expect { pull_access_check }.not_to raise_error
+      end
+    end
   end
 
-  describe '#check_project_moved!', :clean_gitlab_redis_shared_state do
+  describe '#add_project_moved_message!', :clean_gitlab_redis_shared_state do
     before do
       project.add_master(user)
     end
@@ -261,62 +268,18 @@ describe Gitlab::GitAccess do
       end
     end
 
-    context 'when a permanent redirect and ssh protocol' do
+    context 'with a redirect and ssh protocol' do
       let(:redirected_path) { 'some/other-path' }
 
-      before do
-        allow_any_instance_of(Gitlab::Checks::ProjectMoved).to receive(:permanent_redirect?).and_return(true)
-      end
-
-      it 'allows push and pull access' do
-        aggregate_failures do
-          expect { push_access_check }.not_to raise_error
-        end
-      end
-
       it_behaves_like 'check_project_moved'
     end
 
-    context 'with a permanent redirect and http protocol' do
+    context 'with a redirect and http protocol' do
       let(:redirected_path) { 'some/other-path' }
       let(:protocol) { 'http' }
 
-      before do
-        allow_any_instance_of(Gitlab::Checks::ProjectMoved).to receive(:permanent_redirect?).and_return(true)
-      end
-
-      it 'allows_push and pull access' do
-        aggregate_failures do
-          expect { push_access_check }.not_to raise_error
-        end
-      end
-
       it_behaves_like 'check_project_moved'
     end
-
-    context 'with a temporal redirect and ssh protocol' do
-      let(:redirected_path) { 'some/other-path' }
-
-      it 'blocks push and pull access' do
-        aggregate_failures do
-          expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /Project '#{redirected_path}' was moved to '#{project.full_path}'/)
-          expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.ssh_url_to_repo}/)
-
-          expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /Project '#{redirected_path}' was moved to '#{project.full_path}'/)
-          expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.ssh_url_to_repo}/)
-        end
-      end
-    end
-
-    context 'with a temporal redirect and http protocol' do
-      let(:redirected_path) { 'some/other-path' }
-      let(:protocol) { 'http' }
-
-      it 'does not allow to push and pull access' do
-        expect { push_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
-        expect { pull_access_check }.to raise_error(described_class::ProjectMovedError, /git remote set-url origin #{project.http_url_to_repo}/)
-      end
-    end
   end
 
   describe '#check_authentication_abilities!' do

spec/models/route_spec.rb

@@ -16,66 +16,6 @@ describe Route do
     it { is_expected.to validate_presence_of(:source) }
     it { is_expected.to validate_presence_of(:path) }
     it { is_expected.to validate_uniqueness_of(:path).case_insensitive }
-
-    describe '#ensure_permanent_paths' do
-      context 'when the route is not yet persisted' do
-        let(:new_route) { described_class.new(path: 'foo', source: build(:group)) }
-
-        context 'when permanent conflicting redirects exist' do
-          it 'is invalid' do
-            redirect = build(:redirect_route, :permanent, path: 'foo/bar/baz')
-            redirect.save!(validate: false)
-
-            expect(new_route.valid?).to be_falsey
-            expect(new_route.errors.first[1]).to eq('has been taken before')
-          end
-        end
-
-        context 'when no permanent conflicting redirects exist' do
-          it 'is valid' do
-            expect(new_route.valid?).to be_truthy
-          end
-        end
-      end
-
-      context 'when path has changed' do
-        before do
-          route.path = 'foo'
-        end
-
-        context 'when permanent conflicting redirects exist' do
-          it 'is invalid' do
-            redirect = build(:redirect_route, :permanent, path: 'foo/bar/baz')
-            redirect.save!(validate: false)
-
-            expect(route.valid?).to be_falsey
-            expect(route.errors.first[1]).to eq('has been taken before')
-          end
-        end
-
-        context 'when no permanent conflicting redirects exist' do
-          it 'is valid' do
-            expect(route.valid?).to be_truthy
-          end
-        end
-      end
-
-      context 'when path has not changed' do
-        context 'when permanent conflicting redirects exist' do
-          it 'is valid' do
-            redirect = build(:redirect_route, :permanent, path: 'git_lab/foo/bar')
-            redirect.save!(validate: false)
-
-            expect(route.valid?).to be_truthy
-          end
-        end
-        context 'when no permanent conflicting redirects exist' do
-          it 'is valid' do
-            expect(route.valid?).to be_truthy
-          end
-        end
-      end
-    end
   end
 
   describe 'callbacks' do
@@ -211,43 +151,31 @@ describe Route do
     end
 
     context 'when the source is a Project' do
-      it 'creates a temporal RedirectRoute' do
+      it 'creates a RedirectRoute' do
         project = create(:project)
         route = project.route
         redirect_route = route.create_redirect('foo')
-        expect(redirect_route.permanent?).to be_falsy
+        expect(redirect_route).not_to be_nil
       end
     end
 
     context 'when the source is not a project' do
-      it 'creates a permanent RedirectRoute' do
-        redirect_route = route.create_redirect('foo', permanent: true)
-        expect(redirect_route.permanent?).to be_truthy
+      it 'creates a RedirectRoute' do
+        redirect_route = route.create_redirect('foo')
+        expect(redirect_route).not_to be_nil
       end
     end
   end
 
   describe '#delete_conflicting_redirects' do
-    context 'with permanent redirect' do
-      it 'does not delete the redirect' do
-        route.create_redirect("#{route.path}/foo", permanent: true)
-
-        expect do
-          route.delete_conflicting_redirects
-        end.not_to change { RedirectRoute.count }
-      end
-    end
-
-    context 'with temporal redirect' do
-      let(:route) { create(:project).route }
+    let(:route) { create(:project).route }
 
-      it 'deletes the redirect' do
-        route.create_redirect("#{route.path}/foo")
+    it 'deletes the redirect' do
+      route.create_redirect("#{route.path}/foo")
 
-        expect do
-          route.delete_conflicting_redirects
-        end.to change { RedirectRoute.count }.by(-1)
-      end
+      expect do
+        route.delete_conflicting_redirects
+      end.to change { RedirectRoute.count }.by(-1)
     end
 
     context 'when a redirect route with the same path exists' do
@@ -289,31 +217,18 @@ describe Route do
   end
 
   describe '#conflicting_redirects' do
+    let(:route) { create(:project).route }
+
     it 'returns an ActiveRecord::Relation' do
       expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
     end
 
-    context 'with permanent redirects' do
-      it 'does not return anything' do
-        route.create_redirect("#{route.path}/foo", permanent: true)
-        route.create_redirect("#{route.path}/foo/bar", permanent: true)
-        route.create_redirect("#{route.path}/baz/quz", permanent: true)
+    it 'returns the redirect routes' do
+      redirect1 = route.create_redirect("#{route.path}/foo")
+      redirect2 = route.create_redirect("#{route.path}/foo/bar")
+      redirect3 = route.create_redirect("#{route.path}/baz/quz")
 
-        expect(route.conflicting_redirects).to be_empty
-      end
-    end
-
-    context 'with temporal redirects' do
-      let(:route) { create(:project).route }
-
-      it 'returns the redirect routes' do
-        route = create(:project).route
-        redirect1 = route.create_redirect("#{route.path}/foo")
-        redirect2 = route.create_redirect("#{route.path}/foo/bar")
-        redirect3 = route.create_redirect("#{route.path}/baz/quz")
-
-        expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3])
-      end
+      expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3])
     end
 
     context 'when a redirect route with the same path exists' do
@@ -348,44 +263,6 @@ describe Route do
     end
   end
 
-  describe "#conflicting_redirect_exists?" do
-    context 'when a conflicting redirect exists' do
-      let(:group1) { create(:group, path: 'foo') }
-      let(:group2) { create(:group, path: 'baz') }
-
-      it 'should not be saved' do
-        group1.path = 'bar'
-        group1.save
-
-        group2.path = 'foo'
-
-        expect(group2.save).to be_falsy
-      end
-
-      it 'should return an error on path' do
-        group1.path = 'bar'
-        group1.save
-
-        group2.path = 'foo'
-        group2.valid?
-        expect(group2.errors[:path]).to eq(['has been taken before'])
-      end
-    end
-
-    context 'when a conflicting redirect does not exist' do
-      let(:project1) { create(:project, path: 'foo') }
-      let(:project2) { create(:project, path: 'baz') }
-
-      it 'should be saved' do
-        project1.path = 'bar'
-        project1.save
-
-        project2.path = 'foo'
-        expect(project2.save).to be_truthy
-      end
-    end
-  end
-
   describe '#delete_conflicting_orphaned_routes' do
     context 'when there is a conflicting route' do
       let!(:conflicting_group) { create(:group, path: 'foo') }

spec/models/user_spec.rb

@@ -126,23 +126,6 @@ describe User do
         end
       end
 
-      context 'when the username was used by another user before' do
-        let(:username) { 'foo' }
-        let!(:other_user) { create(:user, username: username) }
-
-        before do
-          other_user.username = 'bar'
-          other_user.save!
-        end
-
-        it 'is invalid' do
-          user = build(:user, username: username)
-
-          expect(user).not_to be_valid
-          expect(user.errors.full_messages).to eq(['Username has been taken before'])
-        end
-      end
-
       context 'when the username is in use by another user' do
         let(:username) { 'foo' }
         let!(:other_user) { create(:user, username: username) }
@@ -2699,27 +2682,19 @@ describe User do
     end
   end
 
-  describe "#username_previously_taken?" do
-    let(:user1) { create(:user, username: 'foo') }
+  context 'changing a username' do
+    let(:user) { create(:user, username: 'foo') }
 
-    context 'when the username has been taken before' do
-      before do
-        user1.username = 'bar'
-        user1.save!
-      end
-
-      it 'should raise an ActiveRecord::RecordInvalid exception' do
-        user2 = build(:user, username: 'foo')
-        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Username has been taken before/)
-      end
+    it 'creates a redirect route' do
+      expect { user.update!(username: 'bar') }
+        .to change { RedirectRoute.where(path: 'foo').count }.by(1)
     end
 
-    context 'when the username has not been taken before' do
-      it 'should be valid' do
-        expect(RedirectRoute.count).to eq(0)
-        user2 = build(:user, username: 'baz')
-        expect(user2).to be_valid
-      end
+    it 'deletes the redirect when a user with the old username was created' do
+      user.update!(username: 'bar')
+
+      expect { create(:user, username: 'foo') }
+        .to change { RedirectRoute.where(path: 'foo').count }.by(-1)
     end
   end
 end

spec/requests/git_http_spec.rb

@@ -344,20 +344,11 @@ describe 'Git HTTP requests' do
         context 'and the user requests a redirected path' do
           let!(:redirect) { project.route.create_redirect('foo/bar') }
           let(:path) { "#{redirect.path}.git" }
-          let(:project_moved_message) do
-            <<-MSG.strip_heredoc
-              Project '#{redirect.path}' was moved to '#{project.full_path}'.
 
-              Please update your Git remote:
-
-                git remote set-url origin #{project.http_url_to_repo} and try again.
-            MSG
-          end
-
-          it 'downloads get status 404 with "project was moved" message' do
+          it 'downloads get status 200 for redirects' do
             clone_get(path, {})
-            expect(response).to have_gitlab_http_status(:not_found)
-            expect(response.body).to match(project_moved_message)
+
+            expect(response).to have_gitlab_http_status(:ok)
           end
         end
       end
@@ -559,20 +550,19 @@ describe 'Git HTTP requests' do
 
                     Please update your Git remote:
 
-                      git remote set-url origin #{project.http_url_to_repo} and try again.
+                      git remote set-url origin #{project.http_url_to_repo}.
                   MSG
                 end
 
-                it 'downloads get status 404 with "project was moved" message' do
+                it 'downloads get status 200' do
                   clone_get(path, env)
-                  expect(response).to have_gitlab_http_status(:not_found)
-                  expect(response.body).to match(project_moved_message)
+
+                  expect(response).to have_gitlab_http_status(:ok)
                 end
 
                 it 'uploads get status 404 with "project was moved" message' do
                   upload(path, env) do |response|
-                    expect(response).to have_gitlab_http_status(:not_found)
-                    expect(response.body).to match(project_moved_message)
+                    expect(response).to have_gitlab_http_status(:ok)
                   end
                 end
               end

spec/services/groups/transfer_service_spec.rb

@@ -222,8 +222,8 @@ describe Groups::TransferService, :postgresql do
           expect(new_parent_group.children.first).to eq(group)
         end
 
-        it 'should create a permanent redirect for the group' do
-          expect(group.redirect_routes.permanent.count).to eq(1)
+        it 'should create a redirect for the group' do
+          expect(group.redirect_routes.count).to eq(1)
         end
       end
 
@@ -243,10 +243,10 @@ describe Groups::TransferService, :postgresql do
           end
         end
 
-        it 'should create permanent redirects for the subgroups' do
-          expect(group.redirect_routes.permanent.count).to eq(1)
-          expect(subgroup1.redirect_routes.permanent.count).to eq(1)
-          expect(subgroup2.redirect_routes.permanent.count).to eq(1)
+        it 'should create redirects for the subgroups' do
+          expect(group.redirect_routes.count).to eq(1)
+          expect(subgroup1.redirect_routes.count).to eq(1)
+          expect(subgroup2.redirect_routes.count).to eq(1)
         end
 
         context 'when the new parent has a higher visibility than the children' do
@@ -287,9 +287,9 @@ describe Groups::TransferService, :postgresql do
         end
 
         it 'should create permanent redirects for the projects' do
-          expect(group.redirect_routes.permanent.count).to eq(1)
-          expect(project1.redirect_routes.permanent.count).to eq(1)
-          expect(project2.redirect_routes.permanent.count).to eq(1)
+          expect(group.redirect_routes.count).to eq(1)
+          expect(project1.redirect_routes.count).to eq(1)
+          expect(project2.redirect_routes.count).to eq(1)
         end
 
         context 'when the new parent has a higher visibility than the projects' do
@@ -338,12 +338,12 @@ describe Groups::TransferService, :postgresql do
           end
         end
 
-        it 'should create permanent redirect for the subgroups and projects' do
-          expect(group.redirect_routes.permanent.count).to eq(1)
-          expect(subgroup1.redirect_routes.permanent.count).to eq(1)
-          expect(subgroup2.redirect_routes.permanent.count).to eq(1)
-          expect(project1.redirect_routes.permanent.count).to eq(1)
-          expect(project2.redirect_routes.permanent.count).to eq(1)
+        it 'should create redirect for the subgroups and projects' do
+          expect(group.redirect_routes.count).to eq(1)
+          expect(subgroup1.redirect_routes.count).to eq(1)
+          expect(subgroup2.redirect_routes.count).to eq(1)
+          expect(project1.redirect_routes.count).to eq(1)
+          expect(project2.redirect_routes.count).to eq(1)
         end
       end
 
@@ -380,12 +380,12 @@ describe Groups::TransferService, :postgresql do
           end
         end
 
-        it 'should create permanent redirect for the subgroups and projects' do
-          expect(group.redirect_routes.permanent.count).to eq(1)
-          expect(project1.redirect_routes.permanent.count).to eq(1)
-          expect(subgroup1.redirect_routes.permanent.count).to eq(1)
-          expect(nested_subgroup.redirect_routes.permanent.count).to eq(1)
-          expect(nested_project.redirect_routes.permanent.count).to eq(1)
+        it 'should create redirect for the subgroups and projects' do
+          expect(group.redirect_routes.count).to eq(1)
+          expect(project1.redirect_routes.count).to eq(1)
+          expect(subgroup1.redirect_routes.count).to eq(1)
+          expect(nested_subgroup.redirect_routes.count).to eq(1)
+          expect(nested_project.redirect_routes.count).to eq(1)
         end
       end