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

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

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

lib/constraints/namespace_url_constrainer.rb

@@ -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

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

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

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

spec/lib/constraints/namespace_url_constrainer_spec.rb

@@ -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

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

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