Merge branch 'dz-refactor-full-path' into 'master'

Store group and project full name and full path in routes table

See merge request !8979

16 files changed, 191 insertions, 98 deletions

diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb
index c491e5c7550..8360ce08bdc 100644
--- a/app/controllers/admin/dashboard_controller.rb
+++ b/app/controllers/admin/dashboard_controller.rb
@@ -1,7 +1,7 @@
 class Admin::DashboardController < Admin::ApplicationController
   def index
-    @projects = Project.limit(10)
+    @projects = Project.with_route.limit(10)
     @users = User.limit(10)
-    @groups = Group.limit(10)
+    @groups = Group.with_route.limit(10)
   end
 end
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 8d0198f8797..cea3d088e94 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -2,7 +2,7 @@ class Admin::GroupsController < Admin::ApplicationController
   before_action :group, only: [:edit, :update, :destroy, :project_update, :members_update]
 
   def index
-    @groups = Group.with_statistics
+    @groups = Group.with_statistics.with_route
     @groups = @groups.sort(@sort = params[:sort])
     @groups = @groups.search(params[:name]) if params[:name].present?
     @groups = @groups.page(params[:page])
diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb
index de6bc689bb7..0b7cf8167f0 100644
--- a/app/controllers/dashboard/groups_controller.rb
+++ b/app/controllers/dashboard/groups_controller.rb
@@ -1,5 +1,5 @@
 class Dashboard::GroupsController < Dashboard::ApplicationController
   def index
-    @group_members = current_user.group_members.includes(:source).page(params[:page])
+    @group_members = current_user.group_members.includes(source: :route).page(params[:page])
   end
 end
diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index 4e43f42e9e1..d932a17883f 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -2,7 +2,7 @@ class GroupsFinder < UnionFinder
   def execute(current_user = nil)
     segments = all_groups(current_user)
 
-    find_union(segments, Group).order_id_desc
+    find_union(segments, Group).with_route.order_id_desc
   end
 
   private
diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb
index c7911736812..18ec45f300d 100644
--- a/app/finders/projects_finder.rb
+++ b/app/finders/projects_finder.rb
@@ -3,7 +3,7 @@ class ProjectsFinder < UnionFinder
     segments = all_projects(current_user)
     segments.map! { |s| s.where(id: project_ids_relation) } if project_ids_relation
 
-    find_union(segments, Project)
+    find_union(segments, Project).with_route
   end
 
   private
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index 2b93aa30c0f..9f6d215ceb3 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -1,5 +1,5 @@
 # Store object full path in separate table for easy lookup and uniq validation
-# Object must have path db field and respond to full_path and full_path_changed? methods.
+# Object must have name and path db fields and respond to parent and parent_changed? methods.
 module Routable
   extend ActiveSupport::Concern
 
@@ -9,7 +9,13 @@ module Routable
     validates_associated :route
     validates :route, presence: true
 
-    before_validation :update_route_path, if: :full_path_changed?
+    scope :with_route, -> { includes(:route) }
+
+    before_validation do
+      if full_path_changed? || full_name_changed?
+        prepare_route
+      end
+    end
   end
 
   class_methods do
@@ -77,10 +83,62 @@ module Routable
     end
   end
 
+  def full_name
+    if route && route.name.present?
+      @full_name ||= route.name
+    else
+      update_route if persisted?
+
+      build_full_name
+    end
+  end
+
+  def full_path
+    if route && route.path.present?
+      @full_path ||= route.path
+    else
+      update_route if persisted?
+
+      build_full_path
+    end
+  end
+
   private
 
-  def update_route_path
+  def full_name_changed?
+    name_changed? || parent_changed?
+  end
+
+  def full_path_changed?
+    path_changed? || parent_changed?
+  end
+
+  def build_full_name
+    if parent && name
+      parent.human_name + ' / ' + name
+    else
+      name
+    end
+  end
+
+  def build_full_path
+    if parent && path
+      parent.full_path + '/' + path
+    else
+      path
+    end
+  end
+
+  def update_route
+    prepare_route
+    route.save
+  end
+
+  def prepare_route
     route || build_route(source: self)
-    route.path = full_path
+    route.path = build_full_path
+    route.name = build_full_name
+    @full_path = nil
+    @full_name = nil
   end
 end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 8352565da1c..6de4d08fc28 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -177,27 +177,10 @@ class Namespace < ActiveRecord::Base
     Gitlab.config.lfs.enabled
   end
 
-  def full_path
-    if parent
-      parent.full_path + '/' + path
-    else
-      path
-    end
-  end
-
   def shared_runners_enabled?
     projects.with_shared_runners.any?
   end
 
-  def full_name
-    @full_name ||=
-      if parent
-        parent.full_name + ' / ' + name
-      else
-        name
-      end
-  end
-
   # Scopes the model on ancestors of the record
   def ancestors
     if parent_id
@@ -224,6 +207,10 @@ class Namespace < ActiveRecord::Base
     [owner_id]
   end
 
+  def parent_changed?
+    parent_id_changed?
+  end
+
   private
 
   def repository_storage_paths
@@ -262,10 +249,6 @@ class Namespace < ActiveRecord::Base
       find_each(&:refresh_members_authorized_projects)
   end
 
-  def full_path_changed?
-    path_changed? || parent_id_changed?
-  end
-
   def remove_exports!
     Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete))
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index b45f22d94d9..c17bcedf7b2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -228,7 +228,12 @@ class Project < ActiveRecord::Base
   scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
   scope :with_statistics, -> { includes(:statistics) }
   scope :with_shared_runners, -> { where(shared_runners_enabled: true) }
-  scope :inside_path, ->(path) { joins(:route).where('routes.path LIKE ?', "#{path}/%") }
+  scope :inside_path, ->(path) do
+    # We need routes alias rs for JOIN so it does not conflict with
+    # includes(:route) which we use in ProjectsFinder.
+    joins("INNER JOIN routes rs ON rs.source_id = projects.id AND rs.source_type = 'Project'").
+      where('rs.path LIKE ?', "#{path}/%")
+  end
 
   # "enabled" here means "not disabled". It includes private features!
   scope :with_feature_enabled, ->(feature) {
@@ -810,26 +815,6 @@ class Project < ActiveRecord::Base
     end
   end
 
-  def name_with_namespace
-    @name_with_namespace ||= begin
-                               if namespace
-                                 namespace.human_name + ' / ' + name
-                               else
-                                 name
-                               end
-                             end
-  end
-  alias_method :human_name, :name_with_namespace
-
-  def full_path
-    if namespace && path
-      namespace.full_path + '/' + path
-    else
-      path
-    end
-  end
-  alias_method :path_with_namespace, :full_path
-
   def execute_hooks(data, hooks_scope = :push_hooks)
     hooks.send(hooks_scope).each do |hook|
       hook.async_execute(data, hooks_scope.to_s)
@@ -1328,6 +1313,18 @@ class Project < ActiveRecord::Base
     map.public_path_for_source_path(path)
   end
 
+  def parent
+    namespace
+  end
+
+  def parent_changed?
+    namespace_id_changed?
+  end
+
+  alias_method :name_with_namespace, :full_name
+  alias_method :human_name, :full_name
+  alias_method :path_with_namespace, :full_path
+
   private
 
   def cross_namespace_reference?(from)
@@ -1366,10 +1363,6 @@ class Project < ActiveRecord::Base
     raise BoardLimitExceeded, 'Number of permitted boards exceeded' if boards.size >= NUMBER_OF_PERMITTED_BOARDS
   end
 
-  def full_path_changed?
-    path_changed? || namespace_id_changed?
-  end
-
   def update_project_statistics
     stats = statistics || build_statistics
     stats.update(namespace_id: namespace_id)
diff --git a/app/models/route.rb b/app/models/route.rb
index dd171fdb069..4eec3e73d5b 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -8,16 +8,22 @@ class Route < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false }
 
-  after_update :rename_descendants, if: :path_changed?
+  after_update :rename_descendants
 
   def rename_descendants
-    # We update each row separately because MySQL does not have regexp_replace.
-    # rubocop:disable Rails/FindEach
-    Route.where('path LIKE ?', "#{path_was}/%").each do |route|
-      # Note that update column skips validation and callbacks.
-      # We need this to avoid recursive call of rename_descendants method
-      route.update_column(:path, route.path.sub(path_was, path))
+    if path_changed? || name_changed?
+      descendants = Route.where('path LIKE ?', "#{path_was}/%")
+
+      descendants.each do |route|
+        attributes = {
+          path: route.path.sub(path_was, path),
+          name: route.name.sub(name_was, name)
+        }
+
+        # Note that update_columns skips validation and callbacks.
+        # We need this to avoid recursive call of rename_descendants method
+        route.update_columns(attributes)
+      end
     end
-    # rubocop:enable Rails/FindEach
   end
 end
diff --git a/changelogs/unreleased/dz-refactor-full-path.yml b/changelogs/unreleased/dz-refactor-full-path.yml
new file mode 100644
index 00000000000..da8568fd220
--- /dev/null
+++ b/changelogs/unreleased/dz-refactor-full-path.yml
@@ -0,0 +1,4 @@
+---
+title: Store group and project full name and full path in routes table
+merge_request: 8979
+author:
diff --git a/db/migrate/20170204172458_add_name_to_route.rb b/db/migrate/20170204172458_add_name_to_route.rb
new file mode 100644
index 00000000000..38ed1ad9039
--- /dev/null
+++ b/db/migrate/20170204172458_add_name_to_route.rb
@@ -0,0 +1,12 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddNameToRoute < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :routes, :name, :string
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 850772ba356..3fef5b82073 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1037,6 +1037,7 @@ ActiveRecord::Schema.define(version: 20170206101030) do
     t.string "path", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string "name"
   end
 
   add_index "routes", ["path"], name: "index_routes_on_path", unique: true, using: :btree
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index 30443534cca..e008ec28fa4 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -14,12 +14,14 @@ describe Group, 'Routable' do
   describe 'Callbacks' do
     it 'creates route record on create' do
       expect(group.route.path).to eq(group.path)
+      expect(group.route.name).to eq(group.name)
     end
 
     it 'updates route record on path change' do
-      group.update_attributes(path: 'wow')
+      group.update_attributes(path: 'wow', name: 'much')
 
       expect(group.route.path).to eq('wow')
+      expect(group.route.name).to eq('much')
     end
 
     it 'ensure route path uniqueness across different objects' do
@@ -78,4 +80,34 @@ describe Group, 'Routable' do
 
     it { is_expected.to eq([nested_group]) }
   end
+
+  describe '#full_path' do
+    let(:group) { create(:group) }
+    let(:nested_group) { create(:group, parent: group) }
+
+    it { expect(group.full_path).to eq(group.path) }
+    it { expect(nested_group.full_path).to eq("#{group.path}/#{nested_group.path}") }
+  end
+
+  describe '#full_name' do
+    let(:group) { create(:group) }
+    let(:nested_group) { create(:group, parent: group) }
+
+    it { expect(group.full_name).to eq(group.name) }
+    it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") }
+  end
+end
+
+describe Project, 'Routable' do
+  describe '#full_path' do
+    let(:project) { build_stubbed(:empty_project) }
+
+    it { expect(project.full_path).to eq "#{project.namespace.path}/#{project.path}" }
+  end
+
+  describe '#full_name' do
+    let(:project) { build_stubbed(:empty_project) }
+
+    it { expect(project.full_name).to eq "#{project.namespace.human_name} / #{project.name}" }
+  end
 end
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index ec389f20d78..35d932f1c64 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -186,22 +186,6 @@ describe Namespace, models: true do
     end
   end
 
-  describe '#full_path' do
-    let(:group) { create(:group) }
-    let(:nested_group) { create(:group, parent: group) }
-
-    it { expect(group.full_path).to eq(group.path) }
-    it { expect(nested_group.full_path).to eq("#{group.path}/#{nested_group.path}") }
-  end
-
-  describe '#full_name' do
-    let(:group) { create(:group) }
-    let(:nested_group) { create(:group, parent: group) }
-
-    it { expect(group.full_name).to eq(group.name) }
-    it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") }
-  end
-
   describe '#ancestors' do
     let(:group) { create(:group) }
     let(:nested_group) { create(:group, parent: group) }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2129bcbd74d..35f3dd00870 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -275,13 +275,6 @@ describe Project, models: true do
     it { is_expected.to delegate_method(:add_master).to(:team) }
   end
 
-  describe '#name_with_namespace' do
-    let(:project) { build_stubbed(:empty_project) }
-
-    it { expect(project.name_with_namespace).to eq "#{project.namespace.human_name} / #{project.name}" }
-    it { expect(project.human_name).to eq project.name_with_namespace }
-  end
-
   describe '#to_reference' do
     let(:owner)     { create(:user, name: 'Gitlab') }
     let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) }
@@ -1840,6 +1833,20 @@ describe Project, models: true do
     end
   end
 
+  describe '#parent' do
+    let(:project) { create(:empty_project) }
+
+    it { expect(project.parent).to eq(project.namespace) }
+  end
+
+  describe '#parent_changed?' do
+    let(:project) { create(:empty_project) }
+
+    before { project.namespace_id = 7 }
+
+    it { expect(project.parent_changed?).to be_truthy }
+  end
+
   def enable_lfs
     allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
   end
diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb
index dd2a5109abc..a276aa4b13a 100644
--- a/spec/models/route_spec.rb
+++ b/spec/models/route_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Route, models: true do
-  let!(:group) { create(:group, path: 'gitlab') }
+  let!(:group) { create(:group, path: 'gitlab', name: 'gitlab') }
   let!(:route) { group.route }
 
   describe 'relationships' do
@@ -15,17 +15,30 @@ describe Route, models: true do
   end
 
   describe '#rename_descendants' do
-    let!(:nested_group) { create(:group, path: "test", parent: group) }
-    let!(:deep_nested_group) { create(:group, path: "foo", parent: nested_group) }
-    let!(:similar_group) { create(:group, path: 'gitlab-org') }
+    let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) }
+    let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) }
+    let!(:similar_group) { create(:group, path: 'gitlab-org', name: 'gitlab-org') }
 
-    before { route.update_attributes(path: 'bar') }
+    context 'path update' do
+      before { route.update_attributes(path: 'bar') }
 
-    it "updates children routes with new path" do
-      expect(described_class.exists?(path: 'bar')).to be_truthy
-      expect(described_class.exists?(path: 'bar/test')).to be_truthy
-      expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy
-      expect(described_class.exists?(path: 'gitlab-org')).to be_truthy
+      it "updates children routes with new path" do
+        expect(described_class.exists?(path: 'bar')).to be_truthy
+        expect(described_class.exists?(path: 'bar/test')).to be_truthy
+        expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy
+        expect(described_class.exists?(path: 'gitlab-org')).to be_truthy
+      end
+    end
+
+    context 'name update' do
+      before { route.update_attributes(name: 'bar') }
+
+      it "updates children routes with new path" do
+        expect(described_class.exists?(name: 'bar')).to be_truthy
+        expect(described_class.exists?(name: 'bar / test')).to be_truthy
+        expect(described_class.exists?(name: 'bar / test / foo')).to be_truthy
+        expect(described_class.exists?(name: 'gitlab-org')).to be_truthy
+      end
     end
   end
 end