Cache full_name and full_path methods for both Project and Namespace

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

8 files changed, 206 insertions, 47 deletions

diff --git a/app/models/concerns/full_name.rb b/app/models/concerns/full_name.rb
new file mode 100644
index 00000000000..dc3f1140ef6
--- /dev/null
+++ b/app/models/concerns/full_name.rb
@@ -0,0 +1,26 @@
+module FullName
+  extend ActiveSupport::Concern
+  extend CacheMethod::ClassMethods
+  include CacheMethod
+
+  included do
+    before_save :expire_name_cache, if: :full_name_changed?
+    cache_method :full_name
+  end
+
+  def full_name
+    if parent
+      parent.human_name + ' / ' + name
+    else
+      name
+    end
+  end
+
+  def full_name_changed?
+    name_changed? || parent_changed?
+  end
+
+  def expire_name_cache
+    expire_method_caches(%w(full_name))
+  end
+end
diff --git a/app/models/concerns/full_path.rb b/app/models/concerns/full_path.rb
new file mode 100644
index 00000000000..21517654e16
--- /dev/null
+++ b/app/models/concerns/full_path.rb
@@ -0,0 +1,26 @@
+module FullPath
+  extend ActiveSupport::Concern
+  extend CacheMethod::ClassMethods
+  include CacheMethod
+
+  included do
+    before_save :expire_path_cache, if: :full_path_changed?
+    cache_method :full_path
+  end
+
+  def full_path
+    if parent && path
+      parent.full_path + '/' + path
+    else
+      path
+    end
+  end
+
+  def full_path_changed?
+    path_changed? || parent_changed?
+  end
+
+  def expire_path_cache
+    expire_method_caches(%w(full_path))
+  end
+end
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index 2b93aa30c0f..ed1d98e3597 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 path db field and respond to _uncached_full_path and full_path_changed? methods.
 module Routable
   extend ActiveSupport::Concern
 
@@ -81,6 +81,6 @@ module Routable
 
   def update_route_path
     route || build_route(source: self)
-    route.path = full_path
+    route.path = _uncached_full_path
   end
 end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 67d8c1c2e4c..d47210cb928 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -6,6 +6,8 @@ class Namespace < ActiveRecord::Base
   include Gitlab::ShellAdapter
   include Gitlab::CurrentSettings
   include Routable
+  include FullName
+  include FullPath
 
   cache_markdown_field :description, pipeline: :description
 
@@ -169,27 +171,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
@@ -212,6 +197,22 @@ class Namespace < ActiveRecord::Base
     self.class.joins(:route).where('routes.path LIKE ?', "#{route.path}/%").reorder('routes.path ASC')
   end
 
+  def parent_changed?
+    parent_id_changed?
+  end
+
+  def expire_path_cache
+    super
+
+    children.each(&:expire_path_cache)
+  end
+
+  def expire_name_cache
+    super
+
+    children.each(&:expire_name_cache)
+  end
+
   private
 
   def repository_storage_paths
@@ -250,10 +251,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 37f4705adbd..22319b7a678 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -16,6 +16,8 @@ class Project < ActiveRecord::Base
   include ProjectFeaturesCompatibility
   include SelectForProjectAuthorization
   include Routable
+  include FullName
+  include FullPath
 
   extend Gitlab::ConfigHelper
 
@@ -811,26 +813,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)
@@ -1291,6 +1273,18 @@ class Project < ActiveRecord::Base
     end
   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)
@@ -1329,10 +1323,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/spec/models/concerns/full_name_spec.rb b/spec/models/concerns/full_name_spec.rb
new file mode 100644
index 00000000000..a85e16e4f3d
--- /dev/null
+++ b/spec/models/concerns/full_name_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe Project, 'FullName', caching: true do
+  subject { create(:empty_project) }
+
+  let(:namespace) { subject.namespace }
+  let(:full_name) { "#{namespace.human_name} / #{subject.name}" }
+  let(:new_name) { "#{namespace.human_name} / wow" }
+
+  it { is_expected.to respond_to :full_name_changed? }
+
+  describe '#full_name' do
+    it { expect(subject.full_name).to eq(full_name) }
+  end
+
+  describe '#_uncached_full_name' do
+    it { expect(subject._uncached_full_name).to eq(full_name) }
+
+    it 'returns new value even if its not saved yet' do
+      subject.name = 'wow'
+      expect(subject._uncached_full_name).to eq(new_name)
+    end
+  end
+
+  describe '#expire_name_cache' do
+    before { subject.name = 'wow' }
+
+    it 'returns new value after expire_name_cache executed' do
+      expect(subject.full_name).to eq(full_name)
+
+      subject.expire_name_cache
+
+      expect(subject.full_name).to eq(new_name)
+    end
+
+    it 'expires cache automatically when object is saved' do
+      subject.save
+
+      expect(subject.full_name).to eq(new_name)
+    end
+  end
+end
+
+describe Group, 'FullName', caching: true do
+  subject { create(:group, :nested) }
+
+  let(:parent) { subject.parent }
+  let(:full_name) { "#{parent.full_name} / #{subject.name}" }
+
+  describe '#full_name' do
+    it { expect(subject.full_name).to eq(full_name) }
+  end
+end
diff --git a/spec/models/concerns/full_path_spec.rb b/spec/models/concerns/full_path_spec.rb
new file mode 100644
index 00000000000..960376fa2f8
--- /dev/null
+++ b/spec/models/concerns/full_path_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe Project, 'FullPath', caching: true do
+  subject { create(:empty_project) }
+
+  let(:namespace) { subject.namespace }
+  let(:full_path) { "#{namespace.full_path}/#{subject.path}" }
+  let(:new_path) { "#{namespace.full_path}/wow" }
+
+  it { is_expected.to respond_to :full_path_changed? }
+
+  describe '#full_path' do
+    it { expect(subject.full_path).to eq(full_path) }
+  end
+
+  describe '#_uncached_full_path' do
+    it { expect(subject._uncached_full_path).to eq(full_path) }
+
+    it 'returns new value even if its not saved yet' do
+      subject.path = 'wow'
+      expect(subject._uncached_full_path).to eq(new_path)
+    end
+  end
+
+  describe '#expire_path_cache' do
+    before { subject.path = 'wow' }
+
+    it 'returns new value after expire_path_cache executed' do
+      expect(subject.full_path).to eq(full_path)
+
+      subject.expire_path_cache
+
+      expect(subject.full_path).to eq(new_path)
+    end
+
+    it 'expires cache automatically when object is saved' do
+      subject.save
+
+      expect(subject.full_path).to eq(new_path)
+    end
+  end
+end
+
+describe Group, 'FullPath', caching: true do
+  subject { create(:group, :nested) }
+
+  let(:parent) { subject.parent }
+  let(:full_path) { "#{parent.full_path}/#{subject.path}" }
+
+  describe '#full_path' do
+    it { expect(subject.full_path).to eq(full_path) }
+  end
+end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 9ca50555191..e3f71401460 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -300,4 +300,18 @@ describe Group, models: true do
       expect(group.members_with_parents).to include(master)
     end
   end
+
+  describe '#expire_path_cache' do
+    let(:nested_group) { create(:group, :nested, parent: group) }
+
+    before { group.path = 'wow' }
+
+    it 'clears cache for subgroups too' do
+      expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}")
+
+      group.expire_path_cache
+
+      expect(nested_group.full_path).to eq("wow/#{nested_group.path}")
+    end
+  end
 end