Delete conflicting redirects

4 files changed, 127 insertions, 20 deletions

diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb
index e36ca988701..99812bcde53 100644
--- a/app/models/redirect_route.rb
+++ b/app/models/redirect_route.rb
@@ -7,4 +7,6 @@ class RedirectRoute < ActiveRecord::Base
     length: { within: 1..255 },
     presence: true,
     uniqueness: { case_sensitive: false }
+
+  scope :matching_path_and_descendants, -> (path) { where('redirect_routes.path = ? OR redirect_routes.path LIKE ?', path, "#{sanitize_sql_like(path)}/%") }
 end
diff --git a/app/models/route.rb b/app/models/route.rb
index df801714b5f..e0d85ff7db7 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -8,7 +8,8 @@ class Route < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false }
 
-  after_update :create_redirect_if_path_changed
+  after_save :delete_conflicting_redirects
+  after_update :create_redirect_for_old_path
   after_update :rename_direct_descendant_routes
 
   scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") }
@@ -34,13 +35,19 @@ class Route < ActiveRecord::Base
     end
   end
 
-  def create_redirect_if_path_changed
-    if path_changed?
-      create_redirect(path_was)
-    end
+  def delete_conflicting_redirects
+    conflicting_redirects.delete_all
+  end
+
+  def conflicting_redirects
+    RedirectRoute.matching_path_and_descendants(path)
+  end
+
+  def create_redirect_for_old_path
+    create_redirect(path_was) if path_changed?
   end
 
-  def create_redirect(old_path)
-    source.redirect_routes.create(path: old_path)
+  def create_redirect(path)
+    RedirectRoute.create(source: source, path: path)
   end
 end
diff --git a/spec/models/redirect_route_spec.rb b/spec/models/redirect_route_spec.rb
index fb72d87d94d..71827421dd7 100644
--- a/spec/models/redirect_route_spec.rb
+++ b/spec/models/redirect_route_spec.rb
@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 describe RedirectRoute, models: true do
-  let!(:group) { create(:group, path: 'git_lab', name: 'git_lab') }
+  let(:group) { create(:group) }
   let!(:redirect_route) { group.redirect_routes.create(path: 'gitlabb') }
 
   describe 'relationships' do
@@ -13,4 +13,15 @@ describe RedirectRoute, models: true do
     it { is_expected.to validate_presence_of(:path) }
     it { is_expected.to validate_uniqueness_of(:path) }
   end
+
+  describe '.matching_path_and_descendants' do
+    let!(:redirect2) { group.redirect_routes.create(path: 'gitlabb/test') }
+    let!(:redirect3) { group.redirect_routes.create(path: 'gitlabb/test/foo') }
+    let!(:redirect4) { group.redirect_routes.create(path: 'gitlabb/test/foo/bar') }
+    let!(:redirect5) { group.redirect_routes.create(path: 'gitlabb/test/baz') }
+
+    it 'returns correct routes' do
+      expect(RedirectRoute.matching_path_and_descendants('gitlabb/test')).to match_array([redirect2, redirect3, redirect4, redirect5])
+    end
+  end
 end
diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb
index c95043469a8..1aeddcef982 100644
--- a/spec/models/route_spec.rb
+++ b/spec/models/route_spec.rb
@@ -1,19 +1,43 @@
 require 'spec_helper'
 
 describe Route, models: true do
-  let!(:group) { create(:group, path: 'git_lab', name: 'git_lab') }
-  let!(:route) { group.route }
+  let(:group) { create(:group, path: 'git_lab', name: 'git_lab') }
+  let(:route) { group.route }
 
   describe 'relationships' do
     it { is_expected.to belong_to(:source) }
   end
 
   describe 'validations' do
+    before { route }
     it { is_expected.to validate_presence_of(:source) }
     it { is_expected.to validate_presence_of(:path) }
     it { is_expected.to validate_uniqueness_of(:path) }
   end
 
+  describe 'callbacks' do
+    context 'after update' do
+      it 'calls #create_redirect_for_old_path' do
+        expect(route).to receive(:create_redirect_for_old_path)
+        route.update_attributes(path: 'foo')
+      end
+
+      it 'calls #delete_conflicting_redirects' do
+        expect(route).to receive(:delete_conflicting_redirects)
+        route.update_attributes(path: 'foo')
+      end
+    end
+
+    context 'after create' do
+      it 'calls #delete_conflicting_redirects' do
+        route.destroy
+        new_route = Route.new(source: group, path: group.path)
+        expect(new_route).to receive(:delete_conflicting_redirects)
+        new_route.save!
+      end
+    end
+  end
+
   describe '.inside_path' do
     let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) }
     let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) }
@@ -50,7 +74,7 @@ describe Route, models: true do
       context 'when route name is set' do
         before { route.update_attributes(path: 'bar') }
 
-        it "updates children routes with new path" do
+        it 'updates children routes with new path' do
           expect(described_class.exists?(path: 'bar')).to be_truthy
           expect(described_class.exists?(path: 'bar/test')).to be_truthy
           expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy
@@ -69,10 +93,24 @@ describe Route, models: true do
           expect(route.update_attributes(path: 'bar')).to be_truthy
         end
       end
+
+      context 'when conflicting redirects exist' do
+        let!(:conflicting_redirect1) { route.create_redirect('bar/test') }
+        let!(:conflicting_redirect2) { route.create_redirect('bar/test/foo') }
+        let!(:conflicting_redirect3) { route.create_redirect('gitlab-org') }
+
+        it 'deletes the conflicting redirects' do
+          route.update_attributes(path: 'bar')
+
+          expect(RedirectRoute.exists?(path: 'bar/test')).to be_falsey
+          expect(RedirectRoute.exists?(path: 'bar/test/foo')).to be_falsey
+          expect(RedirectRoute.exists?(path: 'gitlab-org')).to be_truthy
+        end
+      end
     end
 
     context 'name update' do
-      it "updates children routes with new path" do
+      it 'updates children routes with new path' do
         route.update_attributes(name: 'bar')
 
         expect(described_class.exists?(name: 'bar')).to be_truthy
@@ -91,21 +129,70 @@ describe Route, models: true do
     end
   end
 
-  describe '#create_redirect_if_path_changed' do
+  describe '#create_redirect_for_old_path' do
     context 'if the path changed' do
       it 'creates a RedirectRoute for the old path' do
+        redirect_scope = route.source.redirect_routes.where(path: 'git_lab')
+        expect(redirect_scope.exists?).to be_falsey
         route.path = 'new-path'
         route.save!
-        redirect = route.source.redirect_routes.first
-        expect(redirect.path).to eq('git_lab')
+        expect(redirect_scope.exists?).to be_truthy
       end
     end
+  end
 
-    context 'if the path did not change' do
-      it 'does not create a RedirectRoute' do
-        route.updated_at = Time.zone.now.utc
-        route.save!
-        expect(route.source.redirect_routes).to be_empty
+  describe '#create_redirect' do
+    it 'creates a RedirectRoute with the same source' do
+      redirect_route = route.create_redirect('foo')
+      expect(redirect_route).to be_a(RedirectRoute)
+      expect(redirect_route).to be_persisted
+      expect(redirect_route.source).to eq(route.source)
+      expect(redirect_route.path).to eq('foo')
+    end
+  end
+
+  describe '#delete_conflicting_redirects' do
+    context 'when a redirect route with the same path exists' do
+      let!(:redirect1) { route.create_redirect(route.path) }
+
+      it 'deletes the redirect' do
+        route.delete_conflicting_redirects
+        expect(route.conflicting_redirects).to be_empty
+      end
+
+      context 'when redirect routes with paths descending from the route path exists' do
+        let!(:redirect2) { route.create_redirect("#{route.path}/foo") }
+        let!(:redirect3) { route.create_redirect("#{route.path}/foo/bar") }
+        let!(:redirect4) { route.create_redirect("#{route.path}/baz/quz") }
+        let!(:other_redirect) { route.create_redirect("other") }
+
+        it 'deletes all redirects with paths that descend from the route path' do
+          route.delete_conflicting_redirects
+          expect(route.conflicting_redirects).to be_empty
+        end
+      end
+    end
+  end
+
+  describe '#conflicting_redirects' do
+    context 'when a redirect route with the same path exists' do
+      let!(:redirect1) { route.create_redirect(route.path) }
+
+      it 'returns the redirect route' do
+        expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
+        expect(route.conflicting_redirects).to match_array([redirect1])
+      end
+
+      context 'when redirect routes with paths descending from the route path exists' do
+        let!(:redirect2) { route.create_redirect("#{route.path}/foo") }
+        let!(:redirect3) { route.create_redirect("#{route.path}/foo/bar") }
+        let!(:redirect4) { route.create_redirect("#{route.path}/baz/quz") }
+        let!(:other_redirect) { route.create_redirect("other") }
+
+        it 'returns the redirect routes' do
+          expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
+          expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3, redirect4])
+        end
       end
     end
   end