Merge branch 'ab/pagination-batch-counts' into 'master'

Refactor project API helpers

Closes #37919

See merge request gitlab-org/gitlab!21130

lib/api/entities.rb

@@ -176,7 +176,7 @@ module API
     end
 
     class BasicProjectDetails < ProjectIdentity
-      include ::API::ProjectsRelationBuilder
+      include ::API::ProjectsBatchCounting
 
       expose :default_branch, if: -> (project, options) { Ability.allowed?(options[:current_user], :download_code, project) }
       # Avoids an N+1 query: https://github.com/mbleigh/acts-as-taggable-on/issues/91#issuecomment-168273770
@@ -418,7 +418,7 @@ module API
           options: { only_owned: true }
         ).execute
 
-        Entities::Project.prepare_relation(projects)
+        Entities::Project.preload_and_batch_count!(projects)
       end
 
       expose :shared_projects, using: Entities::Project do |group, options|
@@ -428,7 +428,7 @@ module API
           options: { only_shared: true }
         ).execute
 
-        Entities::Project.prepare_relation(projects)
+        Entities::Project.preload_and_batch_count!(projects)
       end
     end
 

lib/api/groups.rb

@@ -231,7 +231,7 @@ module API
 
         projects, options = with_custom_attributes(projects, options)
 
-        present options[:with].prepare_relation(projects), options
+        present options[:with].preload_and_batch_count!(projects), options
       end
 
       desc 'Get a list of subgroups in this group.' do

lib/api/helpers/pagination.rb

@@ -6,6 +6,12 @@ module API
       def paginate(relation)
         ::Gitlab::Pagination::OffsetPagination.new(self).paginate(relation)
       end
+
+      # This applies pagination and executes the query
+      # It always returns an array instead of an ActiveRecord relation
+      def paginate_and_retrieve!(relation)
+        paginate(relation).to_a
+      end
     end
   end
 end

lib/api/projects.rb

@@ -75,15 +75,17 @@ module API
         mutually_exclusive :import_url, :template_name, :template_project_id
       end
 
-      def load_projects
+      def find_projects
         ProjectsFinder.new(current_user: current_user, params: project_finder_params).execute
       end
 
-      def present_projects(projects, options = {})
+      # Prepare the full projects query
+      # None of this is supposed to actually execute any database query
+      def prepare_query(projects)
         projects = reorder_projects(projects)
         projects = apply_filters(projects)
-        projects = paginate(projects)
-        projects, options = with_custom_attributes(projects, options)
+
+        projects, options = with_custom_attributes(projects)
 
         options = options.reverse_merge(
           with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails,
@@ -91,9 +93,23 @@ module API
           current_user: current_user,
           license: false
         )
+
         options[:with] = Entities::BasicProjectDetails if params[:simple]
 
-        present options[:with].prepare_relation(projects, options), options
+        projects = options[:with].preload_relation(projects, options)
+
+        [projects, options]
+      end
+
+      def prepare_and_present(project_relation)
+        projects, options = prepare_query(project_relation)
+
+        projects = paginate_and_retrieve!(projects)
+
+        # Refresh count caches
+        options[:with].execute_batch_counting(projects)
+
+        present projects, options
       end
 
       def translate_params_for_compatibility(params)
@@ -118,7 +134,7 @@ module API
 
         params[:user] = user
 
-        present_projects load_projects
+        prepare_and_present find_projects
       end
 
       desc 'Get projects starred by a user' do
@@ -134,7 +150,7 @@ module API
         not_found!('User') unless user
 
         starred_projects = StarredProjectsFinder.new(user, params: project_finder_params, current_user: current_user).execute
-        present_projects starred_projects
+        prepare_and_present starred_projects
       end
     end
 
@@ -150,7 +166,7 @@ module API
         use :with_custom_attributes
       end
       get do
-        present_projects load_projects
+        prepare_and_present find_projects
       end
 
       desc 'Create new project' do
@@ -287,7 +303,7 @@ module API
       get ':id/forks' do
         forks = ForkProjectsFinder.new(user_project, params: project_finder_params, current_user: current_user).execute
 
-        present_projects forks
+        prepare_and_present forks
       end
 
       desc 'Check pages access of this project'

lib/api/projects_batch_counting.rb

+# frozen_string_literal: true
+
+module API
+  module ProjectsBatchCounting
+    extend ActiveSupport::Concern
+
+    class_methods do
+      # This adds preloading to the query and executes batch counting
+      # Side-effect: The query will be executed during batch counting
+      def preload_and_batch_count!(projects_relation)
+        preload_relation(projects_relation).tap do |projects|
+          execute_batch_counting(projects)
+        end
+      end
+
+      def execute_batch_counting(projects)
+        ::Projects::BatchForksCountService.new(forks_counting_projects(projects)).refresh_cache
+
+        ::Projects::BatchOpenIssuesCountService.new(projects).refresh_cache
+      end
+
+      def forks_counting_projects(projects)
+        projects
+      end
+    end
+  end
+end

lib/api/projects_relation_builder.rb

-# frozen_string_literal: true
-
-module API
-  module ProjectsRelationBuilder
-    extend ActiveSupport::Concern
-
-    class_methods do
-      def prepare_relation(projects_relation, options = {})
-        projects_relation = preload_relation(projects_relation, options)
-        execute_batch_counting(projects_relation)
-        projects_relation
-      end
-
-      def preload_relation(projects_relation, options = {})
-        projects_relation
-      end
-
-      def forks_counting_projects(projects_relation)
-        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
-  end
-end

spec/lib/api/helpers/pagination_spec.rb

@@ -19,4 +19,18 @@ describe API::Helpers::Pagination do
       expect(result).to eq(expected_result)
     end
   end
+
+  describe '#paginate_and_retrieve!' do
+    let(:relation) { double("relation") }
+
+    let(:paginated_result) { double }
+    let(:result) { double }
+
+    it 'applies pagination and returns an array' do
+      expect(subject).to receive(:paginate).with(relation).and_return(paginated_result)
+      expect(paginated_result).to receive(:to_a).and_return(result)
+
+      expect(subject.paginate_and_retrieve!(relation)).to eq(result)
+    end
+  end
 end

spec/lib/api/projects_batch_counting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::ProjectsBatchCounting do
+  subject do
+    Class.new do
+      include ::API::ProjectsBatchCounting
+    end
+  end
+
+  describe '.preload_and_batch_count!' do
+    let(:projects) { double }
+    let(:preloaded_projects) { double }
+
+    it 'preloads the relation' do
+      allow(subject).to receive(:execute_batch_counting).with(preloaded_projects)
+
+      expect(subject).to receive(:preload_relation).with(projects).and_return(preloaded_projects)
+
+      expect(subject.preload_and_batch_count!(projects)).to eq(preloaded_projects)
+    end
+
+    it 'executes batch counting' do
+      allow(subject).to receive(:preload_relation).with(projects).and_return(preloaded_projects)
+
+      expect(subject).to receive(:execute_batch_counting).with(preloaded_projects)
+
+      subject.preload_and_batch_count!(projects)
+    end
+  end
+
+  describe '.execute_batch_counting' do
+    let(:projects) { create_list(:project, 2) }
+    let(:count_service) { double }
+
+    it 'counts forks' do
+      allow(::Projects::BatchForksCountService).to receive(:new).with(projects).and_return(count_service)
+
+      expect(count_service).to receive(:refresh_cache)
+
+      subject.execute_batch_counting(projects)
+    end
+
+    it 'counts open issues' do
+      allow(::Projects::BatchOpenIssuesCountService).to receive(:new).with(projects).and_return(count_service)
+
+      expect(count_service).to receive(:refresh_cache)
+
+      subject.execute_batch_counting(projects)
+    end
+
+    context 'custom fork counting' do
+      subject do
+        Class.new do
+          include ::API::ProjectsBatchCounting
+          def self.forks_counting_projects(projects)
+            [projects.first]
+          end
+        end
+      end
+
+      it 'counts forks for other projects' do
+        allow(::Projects::BatchForksCountService).to receive(:new).with([projects.first]).and_return(count_service)
+
+        expect(count_service).to receive(:refresh_cache)
+
+        subject.execute_batch_counting(projects)
+      end
+    end
+  end
+end

spec/requests/api/projects_spec.rb

@@ -155,6 +155,35 @@ describe API::Projects do
       project4
     end
 
+    # This is a regression spec for https://gitlab.com/gitlab-org/gitlab/issues/37919
+    context 'batch counting forks and open issues and refreshing count caches' do
+      # We expect to count these projects (only the ones on the first page, not all matching ones)
+      let(:projects) { Project.public_to_user(nil).order(id: :desc).first(per_page) }
+      let(:per_page) { 2 }
+      let(:count_service) { double }
+
+      before do
+        # Create more projects, so we have more than one page
+        create_list(:project, 5, :public)
+      end
+
+      it 'batch counts project forks' do
+        expect(::Projects::BatchForksCountService).to receive(:new).with(projects).and_return(count_service)
+        expect(count_service).to receive(:refresh_cache)
+
+        get api("/projects?per_page=#{per_page}")
+        expect(response.status).to eq 200
+      end
+
+      it 'batch counts open issues' do
+        expect(::Projects::BatchOpenIssuesCountService).to receive(:new).with(projects).and_return(count_service)
+        expect(count_service).to receive(:refresh_cache)
+
+        get api("/projects?per_page=#{per_page}")
+        expect(response.status).to eq 200
+      end
+    end
+
     context 'when unauthenticated' do
       it_behaves_like 'projects response' do
         let(:filter) { { search: project.name } }