From 82e551bdac7f2792e1d6aceb1b0b674dbd3dda81 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 7 Nov 2016 15:16:04 +0200 Subject: [PATCH 1/3] Refactor routing constraints Signed-off-by: Dmitriy Zaporozhets --- lib/constraints/constrainer_helper.rb | 15 ++++++++ lib/constraints/group_url_constrainer.rb | 16 ++++++--- lib/constraints/namespace_url_constrainer.rb | 24 ------------- lib/constraints/user_url_constrainer.rb | 16 ++++++--- .../constraints/constrainer_helper_spec.rb | 20 +++++++++++ .../constraints/group_url_constrainer_spec.rb | 17 ++++++--- .../namespace_url_constrainer_spec.rb | 35 ------------------- .../constraints/user_url_constrainer_spec.rb | 12 +++++-- 8 files changed, 81 insertions(+), 74 deletions(-) create mode 100644 lib/constraints/constrainer_helper.rb delete mode 100644 lib/constraints/namespace_url_constrainer.rb create mode 100644 spec/lib/constraints/constrainer_helper_spec.rb delete mode 100644 spec/lib/constraints/namespace_url_constrainer_spec.rb diff --git a/lib/constraints/constrainer_helper.rb b/lib/constraints/constrainer_helper.rb new file mode 100644 index 00000000000..ab07a6793d9 --- /dev/null +++ b/lib/constraints/constrainer_helper.rb @@ -0,0 +1,15 @@ +module ConstrainerHelper + def extract_resource_path(path) + id = path.dup + id.sub!(/\A#{relative_url_root}/, '') if relative_url_root + id.sub(/\A\/+/, '').sub(/\/+\z/, '').sub(/.atom\z/, '') + end + + private + + def relative_url_root + if defined?(Gitlab::Application.config.relative_url_root) + Gitlab::Application.config.relative_url_root + end + end +end diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index ca39b1961ae..2bf973a73da 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -1,7 +1,15 @@ -require 'constraints/namespace_url_constrainer' +require_relative 'constrainer_helper' -class GroupUrlConstrainer < NamespaceUrlConstrainer - def find_resource(id) - Group.find_by_path(id) +class GroupUrlConstrainer + include ConstrainerHelper + + def matches?(request) + id = extract_resource_path(request.path) + + if id =~ Gitlab::Regex.namespace_regex + !!Group.find_by_path(id) + else + false + end end end diff --git a/lib/constraints/namespace_url_constrainer.rb b/lib/constraints/namespace_url_constrainer.rb deleted file mode 100644 index 91b70143f11..00000000000 --- a/lib/constraints/namespace_url_constrainer.rb +++ /dev/null @@ -1,24 +0,0 @@ -class NamespaceUrlConstrainer - def matches?(request) - id = request.path - id = id.sub(/\A#{relative_url_root}/, '') if relative_url_root - id = id.sub(/\A\/+/, '').split('/').first - id = id.sub(/.atom\z/, '') if id - - if id =~ Gitlab::Regex.namespace_regex - find_resource(id) - end - end - - def find_resource(id) - Namespace.find_by_path(id) - end - - private - - def relative_url_root - if defined?(Gitlab::Application.config.relative_url_root) - Gitlab::Application.config.relative_url_root - end - end -end diff --git a/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb index 504a0f5d93e..9583cace022 100644 --- a/lib/constraints/user_url_constrainer.rb +++ b/lib/constraints/user_url_constrainer.rb @@ -1,7 +1,15 @@ -require 'constraints/namespace_url_constrainer' +require_relative 'constrainer_helper' -class UserUrlConstrainer < NamespaceUrlConstrainer - def find_resource(id) - User.find_by('lower(username) = ?', id.downcase) +class UserUrlConstrainer + include ConstrainerHelper + + def matches?(request) + id = extract_resource_path(request.path) + + if id =~ Gitlab::Regex.namespace_regex + !!User.find_by('lower(username) = ?', id.downcase) + else + false + end end end diff --git a/spec/lib/constraints/constrainer_helper_spec.rb b/spec/lib/constraints/constrainer_helper_spec.rb new file mode 100644 index 00000000000..27c8d72aefc --- /dev/null +++ b/spec/lib/constraints/constrainer_helper_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe ConstrainerHelper, lib: true do + include ConstrainerHelper + + describe '#extract_resource_path' do + it { expect(extract_resource_path('/gitlab/')).to eq('gitlab') } + it { expect(extract_resource_path('///gitlab//')).to eq('gitlab') } + it { expect(extract_resource_path('/gitlab.atom')).to eq('gitlab') } + + context 'relative url' do + before do + allow(Gitlab::Application.config).to receive(:relative_url_root) { '/gitlab' } + end + + it { expect(extract_resource_path('/gitlab/foo')).to eq('foo') } + it { expect(extract_resource_path('/foo/bar')).to eq('foo/bar') } + end + end +end diff --git a/spec/lib/constraints/group_url_constrainer_spec.rb b/spec/lib/constraints/group_url_constrainer_spec.rb index f0b75a664f2..be69a36c2b6 100644 --- a/spec/lib/constraints/group_url_constrainer_spec.rb +++ b/spec/lib/constraints/group_url_constrainer_spec.rb @@ -1,10 +1,19 @@ require 'spec_helper' describe GroupUrlConstrainer, lib: true do - let!(:username) { create(:group, path: 'gitlab-org') } + let!(:group) { create(:group, path: 'gitlab') } - describe '#find_resource' do - it { expect(!!subject.find_resource('gitlab-org')).to be_truthy } - it { expect(!!subject.find_resource('gitlab-com')).to be_falsey } + describe '#matches?' do + context 'root group' do + it { expect(subject.matches?(request '/gitlab')).to be_truthy } + it { expect(subject.matches?(request '/gitlab.atom')).to be_truthy } + it { expect(subject.matches?(request '/gitlab/edit')).to be_falsey } + it { expect(subject.matches?(request '/gitlab-ce')).to be_falsey } + it { expect(subject.matches?(request '/.gitlab')).to be_falsey } + end + end + + def request(path) + OpenStruct.new(path: path) end end diff --git a/spec/lib/constraints/namespace_url_constrainer_spec.rb b/spec/lib/constraints/namespace_url_constrainer_spec.rb deleted file mode 100644 index 7814711fe27..00000000000 --- a/spec/lib/constraints/namespace_url_constrainer_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -require 'spec_helper' - -describe NamespaceUrlConstrainer, lib: true do - let!(:group) { create(:group, path: 'gitlab') } - - describe '#matches?' do - context 'existing namespace' do - it { expect(subject.matches?(request '/gitlab')).to be_truthy } - it { expect(subject.matches?(request '/gitlab.atom')).to be_truthy } - it { expect(subject.matches?(request '/gitlab/')).to be_truthy } - it { expect(subject.matches?(request '//gitlab/')).to be_truthy } - end - - context 'non-existing namespace' do - it { expect(subject.matches?(request '/gitlab-ce')).to be_falsey } - it { expect(subject.matches?(request '/gitlab.ce')).to be_falsey } - it { expect(subject.matches?(request '/g/gitlab')).to be_falsey } - it { expect(subject.matches?(request '/.gitlab')).to be_falsey } - end - - context 'relative url' do - before do - allow(Gitlab::Application.config).to receive(:relative_url_root) { '/gitlab' } - end - - it { expect(subject.matches?(request '/gitlab/gitlab')).to be_truthy } - it { expect(subject.matches?(request '/gitlab/gitlab-ce')).to be_falsey } - it { expect(subject.matches?(request '/gitlab/')).to be_falsey } - end - end - - def request(path) - OpenStruct.new(path: path) - end -end diff --git a/spec/lib/constraints/user_url_constrainer_spec.rb b/spec/lib/constraints/user_url_constrainer_spec.rb index 4b26692672f..d7b7d5664ff 100644 --- a/spec/lib/constraints/user_url_constrainer_spec.rb +++ b/spec/lib/constraints/user_url_constrainer_spec.rb @@ -3,8 +3,14 @@ require 'spec_helper' describe UserUrlConstrainer, lib: true do let!(:username) { create(:user, username: 'dz') } - describe '#find_resource' do - it { expect(!!subject.find_resource('dz')).to be_truthy } - it { expect(!!subject.find_resource('john')).to be_falsey } + describe '#matches?' do + it { expect(subject.matches?(request '/dz')).to be_truthy } + it { expect(subject.matches?(request '/dz.atom')).to be_truthy } + it { expect(subject.matches?(request '/dz/projects')).to be_falsey } + it { expect(subject.matches?(request '/gitlab')).to be_falsey } + end + + def request(path) + OpenStruct.new(path: path) end end From 08d21fe89927fa9ebd3695fde047a17d27893dbf Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 8 Nov 2016 14:32:42 +0200 Subject: [PATCH 2/3] Add small improvements to constrainers and specs Signed-off-by: Dmitriy Zaporozhets --- lib/constraints/group_url_constrainer.rb | 2 +- lib/constraints/user_url_constrainer.rb | 2 +- spec/lib/constraints/group_url_constrainer_spec.rb | 2 +- spec/lib/constraints/user_url_constrainer_spec.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index 2bf973a73da..2af6e1a11c8 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -7,7 +7,7 @@ class GroupUrlConstrainer id = extract_resource_path(request.path) if id =~ Gitlab::Regex.namespace_regex - !!Group.find_by_path(id) + Group.find_by(path: id).present? else false end diff --git a/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb index 9583cace022..4d722ad5af2 100644 --- a/lib/constraints/user_url_constrainer.rb +++ b/lib/constraints/user_url_constrainer.rb @@ -7,7 +7,7 @@ class UserUrlConstrainer id = extract_resource_path(request.path) if id =~ Gitlab::Regex.namespace_regex - !!User.find_by('lower(username) = ?', id.downcase) + User.find_by('lower(username) = ?', id.downcase).present? else false end diff --git a/spec/lib/constraints/group_url_constrainer_spec.rb b/spec/lib/constraints/group_url_constrainer_spec.rb index be69a36c2b6..42299b17c2b 100644 --- a/spec/lib/constraints/group_url_constrainer_spec.rb +++ b/spec/lib/constraints/group_url_constrainer_spec.rb @@ -14,6 +14,6 @@ describe GroupUrlConstrainer, lib: true do end def request(path) - OpenStruct.new(path: path) + double(:request, path: path) end end diff --git a/spec/lib/constraints/user_url_constrainer_spec.rb b/spec/lib/constraints/user_url_constrainer_spec.rb index d7b7d5664ff..b3f8530c609 100644 --- a/spec/lib/constraints/user_url_constrainer_spec.rb +++ b/spec/lib/constraints/user_url_constrainer_spec.rb @@ -11,6 +11,6 @@ describe UserUrlConstrainer, lib: true do end def request(path) - OpenStruct.new(path: path) + double(:request, path: path) end end From 5395f92e7ca93a907cdb2507b2ba2073863bc83a Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 8 Nov 2016 15:54:17 +0200 Subject: [PATCH 3/3] Fix routing spec for group controller Signed-off-by: Dmitriy Zaporozhets --- spec/routing/routing_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index c18a2d55e43..61dca5d5a62 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -266,13 +266,13 @@ describe "Groups", "routing" do end it "also display group#show on the short path" do - allow(Group).to receive(:find_by_path).and_return(true) + allow(Group).to receive(:find_by).and_return(true) expect(get('/1')).to route_to('groups#show', id: '1') end it "also display group#show with dot in the path" do - allow(Group).to receive(:find_by_path).and_return(true) + allow(Group).to receive(:find_by).and_return(true) expect(get('/group.with.dot')).to route_to('groups#show', id: 'group.with.dot') end