Commit 72cc9f92 authored by Sean McGivern's avatar Sean McGivern Committed by Jose Ivan Vargas

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

Fix conflicting redirect search

See merge request !13357
parent 345a304a
...@@ -8,5 +8,13 @@ class RedirectRoute < ActiveRecord::Base ...@@ -8,5 +8,13 @@ class RedirectRoute < ActiveRecord::Base
presence: true, presence: true,
uniqueness: { case_sensitive: false } 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 end
---
title: Fix destroy of case-insensitive conflicting redirects
merge_request: 13357
author:
...@@ -20,8 +20,16 @@ describe RedirectRoute do ...@@ -20,8 +20,16 @@ describe RedirectRoute do
let!(:redirect4) { group.redirect_routes.create(path: 'gitlabb/test/foo/bar') } let!(:redirect4) { group.redirect_routes.create(path: 'gitlabb/test/foo/bar') }
let!(:redirect5) { group.redirect_routes.create(path: 'gitlabb/test/baz') } let!(:redirect5) { group.redirect_routes.create(path: 'gitlabb/test/baz') }
context 'when the redirect route matches with same casing' do
it 'returns correct routes' do it 'returns correct routes' do
expect(described_class.matching_path_and_descendants('gitlabb/test')).to match_array([redirect2, redirect3, redirect4, redirect5]) expect(described_class.matching_path_and_descendants('gitlabb/test')).to match_array([redirect2, redirect3, redirect4, redirect5])
end end
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 end
...@@ -145,11 +145,13 @@ describe Route do ...@@ -145,11 +145,13 @@ describe Route do
describe '#delete_conflicting_redirects' do describe '#delete_conflicting_redirects' do
context 'when a redirect route with the same path exists' do context 'when a redirect route with the same path exists' do
context 'when the redirect route has matching case' do
let!(:redirect1) { route.create_redirect(route.path) } let!(:redirect1) { route.create_redirect(route.path) }
it 'deletes the redirect' do it 'deletes the redirect' do
expect do
route.delete_conflicting_redirects route.delete_conflicting_redirects
expect(route.conflicting_redirects).to be_empty end.to change { RedirectRoute.count }.by(-1)
end end
context 'when redirect routes with paths descending from the route path exists' do context 'when redirect routes with paths descending from the route path exists' do
...@@ -159,19 +161,35 @@ describe Route do ...@@ -159,19 +161,35 @@ describe Route do
let!(:other_redirect) { route.create_redirect("other") } let!(:other_redirect) { route.create_redirect("other") }
it 'deletes all redirects with paths that descend from the route path' do it 'deletes all redirects with paths that descend from the route path' do
expect do
route.delete_conflicting_redirects route.delete_conflicting_redirects
expect(route.conflicting_redirects).to be_empty end.to change { RedirectRoute.count }.by(-4)
end
end
end
context 'when the redirect route is differently cased' do
let!(:redirect1) { route.create_redirect(route.path.upcase) }
it 'deletes the redirect' do
expect do
route.delete_conflicting_redirects
end.to change { RedirectRoute.count }.by(-1)
end end
end end
end end
end end
describe '#conflicting_redirects' do 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 context 'when a redirect route with the same path exists' do
context 'when the redirect route has matching case' do
let!(:redirect1) { route.create_redirect(route.path) } let!(:redirect1) { route.create_redirect(route.path) }
it 'returns the redirect route' do it 'returns the redirect route' do
expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
expect(route.conflicting_redirects).to match_array([redirect1]) expect(route.conflicting_redirects).to match_array([redirect1])
end end
...@@ -182,10 +200,18 @@ describe Route do ...@@ -182,10 +200,18 @@ describe Route do
let!(:other_redirect) { route.create_redirect("other") } let!(:other_redirect) { route.create_redirect("other") }
it 'returns the redirect routes' do 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]) expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3, redirect4])
end end
end end
end end
context 'when the redirect route is differently cased' do
let!(:redirect1) { route.create_redirect(route.path.upcase) }
it 'returns the redirect route' do
expect(route.conflicting_redirects).to match_array([redirect1])
end
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment