diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-11-08 18:01:38 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-11-08 18:01:38 +0300 |
commit | c3341884107b65545f9c352d2090dc2a5cf58e19 (patch) | |
tree | 59e1e6c8522542368ffbb99126f41068715ad011 | |
parent | f8988b3d4909c4ebef3ab57bdf7e02716360b274 (diff) | |
parent | 5395f92e7ca93a907cdb2507b2ba2073863bc83a (diff) |
Merge branch 'dz-refactor-contraints' into 'master'
Refactor routing constraints
## What does this MR do?
Refactors routing constraints
## Why was this MR needed?
This refactoring make it possible to introduce nesting namespaces and project constrainer in future.
## What are the relevant issue numbers?
Extracted from https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7121/
See merge request !7327
-rw-r--r-- | lib/constraints/constrainer_helper.rb | 15 | ||||
-rw-r--r-- | lib/constraints/group_url_constrainer.rb | 16 | ||||
-rw-r--r-- | lib/constraints/namespace_url_constrainer.rb | 24 | ||||
-rw-r--r-- | lib/constraints/user_url_constrainer.rb | 16 | ||||
-rw-r--r-- | spec/lib/constraints/constrainer_helper_spec.rb | 20 | ||||
-rw-r--r-- | spec/lib/constraints/group_url_constrainer_spec.rb | 17 | ||||
-rw-r--r-- | spec/lib/constraints/namespace_url_constrainer_spec.rb | 35 | ||||
-rw-r--r-- | spec/lib/constraints/user_url_constrainer_spec.rb | 12 | ||||
-rw-r--r-- | spec/routing/routing_spec.rb | 4 |
9 files changed, 83 insertions, 76 deletions
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..2af6e1a11c8 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).present? + 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..4d722ad5af2 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).present? + 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..42299b17c2b 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) + double(:request, 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..b3f8530c609 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) + double(:request, path: path) end end 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 |