From fccd64bb6ae0b06252cf667492ca5e5a0753c4f4 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 7 Aug 2017 16:41:39 -0700 Subject: Fix conflicting redirect search --- app/models/redirect_route.rb | 2 +- spec/models/redirect_route_spec.rb | 12 +++++-- spec/models/route_spec.rb | 74 +++++++++++++++++++++++++------------- 3 files changed, 61 insertions(+), 27 deletions(-) diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb index 964175ddab8..a034aacb23c 100644 --- a/app/models/redirect_route.rb +++ b/app/models/redirect_route.rb @@ -8,5 +8,5 @@ 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) { where('LOWER(redirect_routes.path) = LOWER(?) OR LOWER(redirect_routes.path) LIKE LOWER(?)', path, "#{sanitize_sql_like(path)}/%") } end diff --git a/spec/models/redirect_route_spec.rb b/spec/models/redirect_route_spec.rb index 80943877095..106ae59af29 100644 --- a/spec/models/redirect_route_spec.rb +++ b/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 diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index bdacc60fb53..42c2104ceb8 100644 --- a/spec/models/route_spec.rb +++ b/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 -- cgit v1.2.1 From 4f71c557be3abfb855e8489fad2c8f1614d9fb8b Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 7 Aug 2017 17:04:51 -0700 Subject: Add changelog entry --- changelogs/unreleased/mk-fix-case-insensitive-redirect-matching.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/mk-fix-case-insensitive-redirect-matching.yml diff --git a/changelogs/unreleased/mk-fix-case-insensitive-redirect-matching.yml b/changelogs/unreleased/mk-fix-case-insensitive-redirect-matching.yml new file mode 100644 index 00000000000..c539480c65f --- /dev/null +++ b/changelogs/unreleased/mk-fix-case-insensitive-redirect-matching.yml @@ -0,0 +1,4 @@ +--- +title: Fix destroy of case-insensitive conflicting redirects +merge_request: 13357 +author: -- cgit v1.2.1 From f4ecbf16231de596eb9af0ef469087b90be5b83d Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 7 Aug 2017 17:16:38 -0700 Subject: Remove unnecessary work for MySQL --- app/models/redirect_route.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb index a034aacb23c..090fbd61e6f 100644 --- a/app/models/redirect_route.rb +++ b/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('LOWER(redirect_routes.path) = LOWER(?) OR LOWER(redirect_routes.path) LIKE LOWER(?)', 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 -- cgit v1.2.1 From 3d58e30b6b5b6bbd25fcb4f59650250ee9eb83fb Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 9 Aug 2017 08:11:08 -0700 Subject: Fix style --- spec/models/route_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index 42c2104ceb8..fece370c03f 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -151,7 +151,7 @@ describe Route do it 'deletes the redirect' do expect do route.delete_conflicting_redirects - end.to change{RedirectRoute.count}.by(-1) + end.to change { RedirectRoute.count }.by(-1) end context 'when redirect routes with paths descending from the route path exists' do @@ -163,7 +163,7 @@ describe Route do 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.to change { RedirectRoute.count }.by(-4) end end end @@ -174,7 +174,7 @@ describe Route do it 'deletes the redirect' do expect do route.delete_conflicting_redirects - end.to change{RedirectRoute.count}.by(-1) + end.to change { RedirectRoute.count }.by(-1) end end end -- cgit v1.2.1