Merge branch 'route-caching' into 'master'

Cache path lookups for namespaces [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57027

app/models/concerns/routable.rb

@@ -96,11 +96,49 @@ module Routable
   end
 
   def full_name
-    route&.name || build_full_name
+    # We have to test for persistence as the cache key uses #updated_at
+    return (route&.name || build_full_name) unless persisted? && Feature.enabled?(:cached_route_lookups, self, type: :ops, default_enabled: :yaml)
+
+    # Return the name as-is if the parent is missing
+    return name if route.nil? && parent.nil? && name.present?
+
+    # If the route is already preloaded, return directly, preventing an extra load
+    return route.name if route_loaded? && route.present?
+
+    # Similarly, we can allow the build if the parent is loaded
+    return build_full_name if parent_loaded?
+
+    Gitlab::Cache.fetch_once([cache_key, :full_name]) do
+      route&.name || build_full_name
+    end
   end
 
   def full_path
-    route&.path || build_full_path
+    # We have to test for persistence as the cache key uses #updated_at
+    return (route&.path || build_full_path) unless persisted? && Feature.enabled?(:cached_route_lookups, self, type: :ops, default_enabled: :yaml)
+
+    # Return the path as-is if the parent is missing
+    return path if route.nil? && parent.nil? && path.present?
+
+    # If the route is already preloaded, return directly, preventing an extra load
+    return route.path if route_loaded? && route.present?
+
+    # Similarly, we can allow the build if the parent is loaded
+    return build_full_path if parent_loaded?
+
+    Gitlab::Cache.fetch_once([cache_key, :full_path]) do
+      route&.path || build_full_path
+    end
+  end
+
+  # Overriden in the Project model
+  # parent_id condition prevents issues with parent reassignment
+  def parent_loaded?
+    association(:parent).loaded?
+  end
+
+  def route_loaded?
+    association(:route).loaded?
   end
 
   def full_path_components

app/models/namespace.rb

@@ -14,6 +14,7 @@ class Namespace < ApplicationRecord
   include IgnorableColumns
   include Namespaces::Traversal::Recursive
   include Namespaces::Traversal::Linear
+  include EachBatch
 
   ignore_column :delayed_project_removal, remove_with: '14.1', remove_after: '2021-05-22'
 
@@ -88,6 +89,10 @@ class Namespace < ApplicationRecord
   after_update :move_dir, if: :saved_change_to_path_or_parent?
   before_destroy(prepend: true) { prepare_for_destroy }
   after_destroy :rm_dir
+  after_commit :expire_child_caches, on: :update, if: -> {
+    Feature.enabled?(:cached_route_lookups, self, type: :ops, default_enabled: :yaml) &&
+      saved_change_to_name? || saved_change_to_path? || saved_change_to_parent_id?
+  }
 
   scope :for_user, -> { where(type: nil) }
   scope :sort_by_type, -> { order(Gitlab::Database.nulls_first_order(:type)) }
@@ -422,6 +427,16 @@ class Namespace < ApplicationRecord
 
   private
 
+  def expire_child_caches
+    Namespace.where(id: descendants).each_batch do |namespaces|
+      namespaces.touch_all
+    end
+
+    all_projects.each_batch do |projects|
+      projects.touch_all
+    end
+  end
+
   def all_projects_with_pages
     if all_projects.pages_metadata_not_migrated.exists?
       Gitlab::BackgroundMigration::MigratePagesMetadata.new.perform_on_relation(

app/models/project.rb

@@ -831,6 +831,10 @@ class Project < ApplicationRecord
     super
   end
 
+  def parent_loaded?
+    association(:namespace).loaded?
+  end
+
   def project_setting
     super.presence || build_project_setting
   end

changelogs/unreleased/route-caching.yml

+---
+title: Cache path lookups for namespaces
+merge_request: 57027
+author:
+type: performance

config/feature_flags/ops/cached_route_lookups.yml

+---
+name: cached_route_lookups
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57027
+rollout_issue_url:
+milestone: '13.11'
+type: ops
+group: group::source code
+default_enabled: false

lib/gitlab/cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Cache
+    class << self
+      # Utility method for performing a fetch but only
+      # once per request, storing the returned value in
+      # the request store, if active.
+      def fetch_once(key, **kwargs)
+        Gitlab::SafeRequestStore.fetch(key) do
+          Rails.cache.fetch(key, **kwargs) do
+            yield
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/cache_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Gitlab::Cache, :request_store do
+  describe "#fetch_once" do
+    subject do
+      proc do
+        described_class.fetch_once([:test, "key"], expires_in: 10.minutes) do
+          "return value"
+        end
+      end
+    end
+
+    it "fetches from the cache once" do
+      expect(Rails.cache).to receive(:fetch).once.with([:test, "key"], expires_in: 10.minutes).and_call_original
+
+      expect(subject.call).to eq("return value")
+      expect(subject.call).to eq("return value")
+    end
+
+    it "always returns from the request store" do
+      expect(Gitlab::SafeRequestStore).to receive(:fetch).twice.with([:test, "key"]).and_call_original
+
+      expect(subject.call).to eq("return value")
+      expect(subject.call).to eq("return value")
+    end
+  end
+end

spec/models/concerns/routable_spec.rb

@@ -62,7 +62,7 @@ RSpec.describe Routable do
   end
 end
 
-RSpec.describe Group, 'Routable' do
+RSpec.describe Group, 'Routable', :with_clean_rails_cache do
   let_it_be_with_reload(:group) { create(:group, name: 'foo') }
   let_it_be(:nested_group) { create(:group, parent: group) }
 
@@ -165,19 +165,63 @@ RSpec.describe Group, 'Routable' do
     end
   end
 
+  describe '#parent_loaded?' do
+    before do
+      group.parent = create(:group)
+      group.save!
+
+      group.reload
+    end
+
+    it 'is false when the parent is not loaded' do
+      expect(group.parent_loaded?).to be_falsey
+    end
+
+    it 'is true when the parent is loaded' do
+      group.parent
+
+      expect(group.parent_loaded?).to be_truthy
+    end
+  end
+
+  describe '#route_loaded?' do
+    it 'is false when the route is not loaded' do
+      expect(group.route_loaded?).to be_falsey
+    end
+
+    it 'is true when the route is loaded' do
+      group.route
+
+      expect(group.route_loaded?).to be_truthy
+    end
+  end
+
   describe '#full_path' do
     it { expect(group.full_path).to eq(group.path) }
     it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") }
+
+    it 'hits the cache when not preloaded' do
+      forcibly_hit_cached_lookup(nested_group, :full_path)
+
+      expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}")
+    end
   end
 
   describe '#full_name' do
     it { expect(group.full_name).to eq(group.name) }
     it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") }
+
+    it 'hits the cache when not preloaded' do
+      forcibly_hit_cached_lookup(nested_group, :full_name)
+
+      expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}")
+    end
   end
 end
 
-RSpec.describe Project, 'Routable' do
-  let_it_be(:project) { create(:project) }
+RSpec.describe Project, 'Routable', :with_clean_rails_cache do
+  let_it_be(:namespace) { create(:namespace) }
+  let_it_be(:project) { create(:project, namespace: namespace) }
 
   it_behaves_like '.find_by_full_path' do
     let_it_be(:record) { project }
@@ -192,10 +236,30 @@ RSpec.describe Project, 'Routable' do
   end
 
   describe '#full_path' do
-    it { expect(project.full_path).to eq "#{project.namespace.full_path}/#{project.path}" }
+    it { expect(project.full_path).to eq "#{namespace.full_path}/#{project.path}" }
+
+    it 'hits the cache when not preloaded' do
+      forcibly_hit_cached_lookup(project, :full_path)
+
+      expect(project.full_path).to eq("#{namespace.full_path}/#{project.path}")
+    end
   end
 
   describe '#full_name' do
-    it { expect(project.full_name).to eq "#{project.namespace.human_name} / #{project.name}" }
+    it { expect(project.full_name).to eq "#{namespace.human_name} / #{project.name}" }
+
+    it 'hits the cache when not preloaded' do
+      forcibly_hit_cached_lookup(project, :full_name)
+
+      expect(project.full_name).to eq("#{namespace.human_name} / #{project.name}")
+    end
   end
 end
+
+def forcibly_hit_cached_lookup(record, method)
+  stub_feature_flags(cached_route_lookups: true)
+  expect(record).to receive(:persisted?).and_return(true)
+  expect(record).to receive(:route_loaded?).and_return(false)
+  expect(record).to receive(:parent_loaded?).and_return(false)
+  expect(Gitlab::Cache).to receive(:fetch_once).with([record.cache_key, method]).and_call_original
+end

spec/models/namespace_spec.rb

@@ -212,6 +212,54 @@ RSpec.describe Namespace do
     end
   end
 
+  describe "after_commit :expire_child_caches" do
+    let(:namespace) { create(:group) }
+
+    it "expires the child caches when updated" do
+      child_1 = create(:group, parent: namespace, updated_at: 1.week.ago)
+      child_2 = create(:group, parent: namespace, updated_at: 1.day.ago)
+      grandchild = create(:group, parent: child_1, updated_at: 1.week.ago)
+      project_1 = create(:project, namespace: namespace, updated_at: 2.days.ago)
+      project_2 = create(:project, namespace: child_1, updated_at: 3.days.ago)
+      project_3 = create(:project, namespace: grandchild, updated_at: 4.years.ago)
+
+      freeze_time do
+        namespace.update!(path: "foo")
+
+        [namespace, child_1, child_2, grandchild, project_1, project_2, project_3].each do |record|
+          expect(record.reload.updated_at).to eq(Time.zone.now)
+        end
+      end
+    end
+
+    it "expires on name changes" do
+      expect(namespace).to receive(:expire_child_caches).once
+
+      namespace.update!(name: "Foo")
+    end
+
+    it "expires on path changes" do
+      expect(namespace).to receive(:expire_child_caches).once
+
+      namespace.update!(path: "bar")
+    end
+
+    it "expires on parent changes" do
+      expect(namespace).to receive(:expire_child_caches).once
+
+      namespace.update!(parent: create(:group))
+    end
+
+    it "doesn't expire on other field changes" do
+      expect(namespace).not_to receive(:expire_child_caches)
+
+      namespace.update!(
+        description: "Foo bar",
+        max_artifacts_size: 10
+      )
+    end
+  end
+
   describe '#visibility_level_field' do
     it { expect(namespace.visibility_level_field).to eq(:visibility_level) }
   end

spec/models/project_spec.rb

@@ -6824,6 +6824,26 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  describe '#parent_loaded?' do
+    let_it_be(:project) { create(:project) }
+
+    before do
+      project.namespace = create(:namespace)
+
+      project.reload
+    end
+
+    it 'is false when the parent is not loaded' do
+      expect(project.parent_loaded?).to be_falsey
+    end
+
+    it 'is true when the parent is loaded' do
+      project.parent
+
+      expect(project.parent_loaded?).to be_truthy
+    end
+  end
+
   describe '#bots' do
     subject { project.bots }