Merge branch '220316-redis-n-1-in-api-v4-groups-id-projects-forks-count-key' into 'master'

Resolve "Redis N+1 in /api/v4/groups/:id/projects - forks count key"

Closes #220316

See merge request gitlab-org/gitlab!35328

app/controllers/concerns/renders_member_access.rb

@@ -7,12 +7,6 @@ module RendersMemberAccess
     groups
   end
 
-  def prepare_projects_for_rendering(projects)
-    preload_max_member_access_for_collection(Project, projects)
-
-    projects
-  end
-
   private
 
   # rubocop: disable CodeReuse/ActiveRecord

app/controllers/concerns/renders_projects_list.rb

+# frozen_string_literal: true
+
+module RendersProjectsList
+  def prepare_projects_for_rendering(projects)
+    preload_max_member_access_for_collection(Project, projects)
+
+    # Call the forks count method on every project, so the BatchLoader would load them all at
+    # once when the entities are rendered
+    projects.each(&:forks_count)
+
+    projects
+  end
+end

app/controllers/dashboard/projects_controller.rb

@@ -3,6 +3,7 @@
 class Dashboard::ProjectsController < Dashboard::ApplicationController
   include ParamsBackwardCompatibility
   include RendersMemberAccess
+  include RendersProjectsList
   include SortingHelper
   include SortingPreference
   include FiltersEvents

app/controllers/explore/projects_controller.rb

@@ -4,6 +4,7 @@ class Explore::ProjectsController < Explore::ApplicationController
   include PageLimiter
   include ParamsBackwardCompatibility
   include RendersMemberAccess
+  include RendersProjectsList
   include SortingHelper
   include SortingPreference
 

app/controllers/projects/forks_controller.rb

@@ -3,6 +3,7 @@
 class Projects::ForksController < Projects::ApplicationController
   include ContinueParams
   include RendersMemberAccess
+  include RendersProjectsList
   include Gitlab::Utils::StrongMemoize
 
   # Authorize

app/controllers/root_controller.rb

@@ -13,10 +13,15 @@ class RootController < Dashboard::ProjectsController
 
   before_action :redirect_unlogged_user, if: -> { current_user.nil? }
   before_action :redirect_logged_user, if: -> { current_user.present? }
+  # We only need to load the projects when the user is logged in but did not
+  # configure a dashboard. In which case we render projects. We can do that straight
+  # from the #index action.
+  skip_before_action :projects
 
   def index
     # n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/40260
     Gitlab::GitalyClient.allow_n_plus_1_calls do
+      projects
       super
     end
   end

app/controllers/users_controller.rb

@@ -3,6 +3,7 @@
 class UsersController < ApplicationController
   include RoutableActions
   include RendersMemberAccess
+  include RendersProjectsList
   include ControllerWithCrossProjectAccessCheck
   include Gitlab::NoteableMetadata
 

app/models/project.rb

@@ -2153,7 +2153,13 @@ class Project < ApplicationRecord
 
   # rubocop: disable CodeReuse/ServiceClass
   def forks_count
-    Projects::ForksCountService.new(self).count
+    BatchLoader.for(self).batch do |projects, loader|
+      fork_count_per_project = ::Projects::BatchForksCountService.new(projects).refresh_cache_and_retrieve_data
+
+      fork_count_per_project.each do |project, count|
+        loader.call(project, count)
+      end
+    end
   end
   # rubocop: enable CodeReuse/ServiceClass
 

app/services/projects/batch_forks_count_service.rb

@@ -5,6 +5,21 @@
 # because the service use maps to retrieve the project ids
 module Projects
   class BatchForksCountService < Projects::BatchCountService
+    def refresh_cache_and_retrieve_data
+      count_services = @projects.map { |project| count_service.new(project) }
+
+      values = Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
+        Rails.cache.fetch_multi(*(count_services.map { |ser| ser.cache_key } )) { |key| nil }
+      end
+
+      results_per_service = Hash[count_services.zip(values.values)]
+      projects_to_refresh = results_per_service.select { |_k, value| value.nil? }
+      projects_to_refresh = recreate_cache(projects_to_refresh)
+
+      results_per_service.update(projects_to_refresh)
+      results_per_service.transform_keys { |k| k.project }
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def global_count
       @global_count ||= begin
@@ -18,5 +33,13 @@ module Projects
     def count_service
       ::Projects::ForksCountService
     end
+
+    def recreate_cache(projects_to_refresh)
+      projects_to_refresh.each_with_object({}) do |(service, _v), hash|
+        count = global_count[service.project.id].to_i
+        service.refresh_cache { count }
+        hash[service] = count
+      end
+    end
   end
 end

app/services/projects/forks_count_service.rb

@@ -3,6 +3,8 @@
 module Projects
   # Service class for getting and caching the number of forks of a project.
   class ForksCountService < Projects::CountService
+    attr_reader :project
+
     def cache_key_name
       'forks_count'
     end

changelogs/unreleased/220316-redis-n-1-in-api-v4-groups-id-projects-forks-count-key.yml

+---
+title: Use BatchLoader for Project.forks_count to limit calls to Redis
+merge_request: 35328
+author:
+type: performance

lib/api/entities/basic_project_details.rb

@@ -33,7 +33,8 @@ module API
         project.avatar_url(only_path: false)
       end
 
-      expose :star_count, :forks_count
+      expose :forks_count
+      expose :star_count
       expose :last_activity_at
       expose :namespace, using: 'API::Entities::NamespaceBasic'
       expose :custom_attributes, using: 'API::Entities::CustomAttribute', if: :with_custom_attributes

lib/api/entities/group_detail.rb

@@ -7,6 +7,7 @@ module API
         SharedGroupWithGroup.represent(group.shared_with_group_links.public_or_visible_to_user(group, options[:current_user]))
       end
       expose :runners_token, if: lambda { |group, options| options[:user_can_admin_group] }
+
       expose :projects, using: Entities::Project do |group, options|
         projects = GroupProjectsFinder.new(
           group: group,

lib/api/projects_relation_builder.rb

@@ -8,6 +8,10 @@ module API
       def prepare_relation(projects_relation, options = {})
         projects_relation = preload_relation(projects_relation, options)
         execute_batch_counting(projects_relation)
+        # Call the forks count method on every project, so the BatchLoader would load them all at
+        # once when the entities are rendered
+        projects_relation.each(&:forks_count)
+
         projects_relation
       end
 
@@ -19,16 +23,11 @@ module API
         projects_relation
       end
 
-      def batch_forks_counting(projects_relation)
-        ::Projects::BatchForksCountService.new(forks_counting_projects(projects_relation)).refresh_cache
-      end
-
       def batch_open_issues_counting(projects_relation)
         ::Projects::BatchOpenIssuesCountService.new(projects_relation).refresh_cache
       end
 
       def execute_batch_counting(projects_relation)
-        batch_forks_counting(projects_relation)
         batch_open_issues_counting(projects_relation)
       end
     end

spec/models/project_spec.rb

@@ -4116,7 +4116,7 @@ RSpec.describe Project do
     it 'returns the number of forks' do
       project = build(:project)
 
-      expect_any_instance_of(Projects::ForksCountService).to receive(:count).and_return(1)
+      expect_any_instance_of(::Projects::BatchForksCountService).to receive(:refresh_cache_and_retrieve_data).and_return({ project => 1 })
 
       expect(project.forks_count).to eq(1)
     end

spec/services/projects/fork_service_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe Projects::ForkService do
       expect(from_project.forks_count).to be_zero
 
       fork_project(from_project, to_user)
+      BatchLoader::Executor.clear_current
 
       expect(from_project.forks_count).to eq(1)
     end
@@ -405,6 +406,7 @@ RSpec.describe Projects::ForkService do
         expect(fork_from_project.forks_count).to be_zero
 
         subject.execute(fork_to_project)
+        BatchLoader::Executor.clear_current
 
         expect(fork_from_project.forks_count).to eq(1)
       end

spec/services/projects/unlink_fork_service_spec.rb

@@ -53,6 +53,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
     expect(source.forks_count).to eq(1)
 
     subject.execute
+    BatchLoader::Executor.clear_current
 
     expect(source.forks_count).to be_zero
   end
@@ -146,6 +147,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
       expect(project.forks_count).to eq(2)
 
       subject.execute
+      BatchLoader::Executor.clear_current
 
       expect(project.forks_count).to be_zero
     end
@@ -212,6 +214,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
         expect(forked_project.forks_count).to eq(1)
 
         subject.execute
+        BatchLoader::Executor.clear_current
 
         expect(project.forks_count).to eq(1)
         expect(forked_project.forks_count).to eq(0)