Merge branch 'mk-fix-case-insensitive-redirect-matching' into 'master'

Fix conflicting redirect search

See merge request !13357

app/models/redirect_route.rb

@@ -8,5 +8,13 @@ class RedirectRoute < ActiveRecord::Base
     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)}/%") }
+  scope :matching_path_and_descendants, -> (path) do
+    wheres = if Gitlab::Database.postgresql?
+               'LOWER(redirect_routes.path) = LOWER(?) OR LOWER(redirect_routes.path) LIKE LOWER(?)'
+             else
+               'redirect_routes.path = ? OR redirect_routes.path LIKE ?'
+             end
+             
+    where(wheres, path, "#{sanitize_sql_like(path)}/%")
+  end
 end

changelogs/unreleased/mk-fix-case-insensitive-redirect-matching.yml

+---
+title: Fix destroy of case-insensitive conflicting redirects
+merge_request: 13357
+author:

spec/models/redirect_route_spec.rb

@@ -20,8 +20,16 @@ describe RedirectRoute do
     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(described_class.matching_path_and_descendants('gitlabb/test')).to match_array([redirect2, redirect3, redirect4, redirect5])
+    context 'when the redirect route matches with same casing' do
+      it 'returns correct routes' do
+        expect(described_class.matching_path_and_descendants('gitlabb/test')).to match_array([redirect2, redirect3, redirect4, redirect5])
+      end
+    end
+
+    context 'when the redirect route matches with different casing' do
+      it 'returns correct routes' do
+        expect(described_class.matching_path_and_descendants('GitLABB/test')).to match_array([redirect2, redirect3, redirect4, redirect5])
+      end
     end
   end
 end

spec/models/route_spec.rb

@@ -145,45 +145,71 @@ describe Route do
 
   describe '#delete_conflicting_redirects' do
     context 'when a redirect route with the same path exists' do
-      let!(:redirect1) { route.create_redirect(route.path) }
+      context 'when the redirect route has matching case' do
+        let!(:redirect1) { route.create_redirect(route.path) }
 
-      it 'deletes the redirect' do
-        route.delete_conflicting_redirects
-        expect(route.conflicting_redirects).to be_empty
+        it 'deletes the redirect' do
+          expect do
+            route.delete_conflicting_redirects
+          end.to change { RedirectRoute.count }.by(-1)
+        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
+            expect do
+              route.delete_conflicting_redirects
+            end.to change { RedirectRoute.count }.by(-4)
+          end
+        end
       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") }
+      context 'when the redirect route is differently cased' do
+        let!(:redirect1) { route.create_redirect(route.path.upcase) }
 
-        it 'deletes all redirects with paths that descend from the route path' do
-          route.delete_conflicting_redirects
-          expect(route.conflicting_redirects).to be_empty
+        it 'deletes the redirect' do
+          expect do
+            route.delete_conflicting_redirects
+          end.to change { RedirectRoute.count }.by(-1)
         end
       end
     end
   end
 
   describe '#conflicting_redirects' do
+    it 'returns an ActiveRecord::Relation' do
+      expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
+    end
+
     context 'when a redirect route with the same path exists' do
-      let!(:redirect1) { route.create_redirect(route.path) }
+      context 'when the redirect route has matching case' 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])
+        it 'returns the redirect route' do
+          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 match_array([redirect1, redirect2, redirect3, redirect4])
+          end
+        end
       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") }
+      context 'when the redirect route is differently cased' do
+        let!(:redirect1) { route.create_redirect(route.path.upcase) }
 
-        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])
+        it 'returns the redirect route' do
+          expect(route.conflicting_redirects).to match_array([redirect1])
         end
       end
     end