From 88e3ce30ae97ac8eb4b25381cfbe7772819cce0c Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 19 Nov 2017 06:35:25 -0800
Subject: [PATCH 01/21] Optimize API /groups/:id/projects by preloading
 associations

Closes #40308
---
 changelogs/unreleased/sh-optimize-groups-api.yml |  5 +++++
 lib/api/groups.rb                                |  1 +
 spec/requests/api/groups_spec.rb                 | 14 ++++++++++++++
 3 files changed, 20 insertions(+)
 create mode 100644 changelogs/unreleased/sh-optimize-groups-api.yml

diff --git a/changelogs/unreleased/sh-optimize-groups-api.yml b/changelogs/unreleased/sh-optimize-groups-api.yml
new file mode 100644
index 00000000000..283c2df5c9f
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-groups-api.yml
@@ -0,0 +1,5 @@
+---
+title: Optimize API /groups/:id/projects by preloading fork_networks table
+merge_request:
+author:
+type: performance
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index bcf2e6dae1d..7e9a5502949 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -172,6 +172,7 @@ module API
       get ":id/projects" do
         group = find_group!(params[:id])
         projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
+        projects = projects.preload(:fork_network, :forked_project_link, :project_feature, :project_group_links, :tags, :taggings, :group, :namespace)
         projects = reorder_projects(projects)
         entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project
         present paginate(projects), with: entity, current_user: current_user
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 04a658cd6c3..554723d6b1e 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -401,6 +401,20 @@ describe API::Groups do
 
         expect(response).to have_gitlab_http_status(404)
       end
+
+      it 'avoids N+1 queries' do
+        get api("/groups/#{group1.id}/projects", admin)
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          get api("/groups/#{group1.id}/projects", admin)
+        end.count
+
+        create(:project, namespace: group1)
+
+        expect do
+          get api("/groups/#{group1.id}/projects", admin)
+        end.not_to exceed_query_limit(control_count)
+      end
     end
 
     context 'when using group path in URL' do

From 39d293abd2ec135c53448552d482d17f9726701c Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 19 Nov 2017 15:25:48 -0800
Subject: [PATCH 02/21] Omit the `all` call after Project#project_group_links
 to avoid unnecessary loads

---
 lib/api/entities.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index ce332fe85d2..ca7708e366e 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -156,7 +156,7 @@ module API
       expose :public_builds, as: :public_jobs
       expose :ci_config_path
       expose :shared_with_groups do |project, options|
-        SharedGroup.represent(project.project_group_links.all, options)
+        SharedGroup.represent(project.project_group_links, options)
       end
       expose :only_allow_merge_if_pipeline_succeeds
       expose :request_access_enabled

From 02cd1702b27ddb15f15f7efeb5ce60412492e41d Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 19 Nov 2017 16:02:46 -0800
Subject: [PATCH 03/21] Only serialize namespace essentials in group's projects
 API response

---
 lib/api/entities.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index ca7708e366e..5b9b0afed0f 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -146,7 +146,7 @@ module API
       expose :shared_runners_enabled
       expose :lfs_enabled?, as: :lfs_enabled
       expose :creator_id
-      expose :namespace, using: 'API::Entities::Namespace'
+      expose :namespace, using: 'API::Entities::NamespaceBasic'
       expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? }
       expose :import_status
       expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] }
@@ -618,9 +618,11 @@ module API
       expose :created_at
     end
 
-    class Namespace < Grape::Entity
+    class NamespaceBasic < Grape::Entity
       expose :id, :name, :path, :kind, :full_path, :parent_id
+    end
 
+    class Namespace < NamespaceBasic
       expose :members_count_with_descendants, if: -> (namespace, opts) { expose_members_count_with_descendants?(namespace, opts) } do |namespace, _|
         namespace.users_with_descendants.count
       end

From 0dfb8801012de19be006026a01d7db0f04a2d3b3 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 19 Nov 2017 16:46:40 -0800
Subject: [PATCH 04/21] Preload project route to avoid N+1 query

---
 lib/api/groups.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 7e9a5502949..df2d97bceda 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -172,7 +172,7 @@ module API
       get ":id/projects" do
         group = find_group!(params[:id])
         projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
-        projects = projects.preload(:fork_network, :forked_project_link, :project_feature, :project_group_links, :tags, :taggings, :group, :namespace)
+        projects = projects.preload(:fork_network, :forked_project_link, :project_feature, :project_group_links, :tags, :taggings, :group, :namespace, :route)
         projects = reorder_projects(projects)
         entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project
         present paginate(projects), with: entity, current_user: current_user

From dd562ae749db805c7c2c764f4e8b6082009df17f Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Tue, 21 Nov 2017 21:59:00 -0800
Subject: [PATCH 05/21] Fix CHANGELOG entry to mention all associations, not
 just fork networks

---
 changelogs/unreleased/sh-optimize-groups-api.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelogs/unreleased/sh-optimize-groups-api.yml b/changelogs/unreleased/sh-optimize-groups-api.yml
index 283c2df5c9f..37b74715a81 100644
--- a/changelogs/unreleased/sh-optimize-groups-api.yml
+++ b/changelogs/unreleased/sh-optimize-groups-api.yml
@@ -1,5 +1,5 @@
 ---
-title: Optimize API /groups/:id/projects by preloading fork_networks table
+title: Optimize API /groups/:id/projects by preloading associations
 merge_request:
 author:
 type: performance

From a2babf32fe2b57850bd678919947d53e5127f6fe Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Fri, 24 Nov 2017 13:20:52 +0100
Subject: [PATCH 06/21] More preloading improvement and added batch count
 services

---
 app/services/base_count_service.rb            |  4 +--
 app/services/projects/batch_count_service.rb  | 35 +++++++++++++++++++
 .../projects/batch_forks_count_service.rb     | 14 ++++++++
 .../batch_open_issues_count_service.rb        | 15 ++++++++
 app/services/projects/count_service.rb        |  2 ++
 lib/api/groups.rb                             | 25 ++++++++++---
 6 files changed, 88 insertions(+), 7 deletions(-)
 create mode 100644 app/services/projects/batch_count_service.rb
 create mode 100644 app/services/projects/batch_forks_count_service.rb
 create mode 100644 app/services/projects/batch_open_issues_count_service.rb

diff --git a/app/services/base_count_service.rb b/app/services/base_count_service.rb
index 19873fe09a5..2e4fd1db03e 100644
--- a/app/services/base_count_service.rb
+++ b/app/services/base_count_service.rb
@@ -12,8 +12,8 @@ class BaseCountService
     Rails.cache.fetch(cache_key, cache_options) { uncached_count }.to_i
   end
 
-  def refresh_cache
-    Rails.cache.write(cache_key, uncached_count, raw: raw?)
+  def refresh_cache(&block)
+    Rails.cache.write(cache_key, block_given? ? yield : uncached_count, raw: raw?)
   end
 
   def uncached_count
diff --git a/app/services/projects/batch_count_service.rb b/app/services/projects/batch_count_service.rb
new file mode 100644
index 00000000000..5b69e24b893
--- /dev/null
+++ b/app/services/projects/batch_count_service.rb
@@ -0,0 +1,35 @@
+module Projects
+  class BatchCountService
+    def initialize(projects)
+      @projects = projects
+    end
+
+    def count
+      @projects.map do |project|
+        [project.id, current_count_service(project).count]
+      end.to_h
+    end
+
+    def refresh_cache
+      @projects.each do |project|
+        current_count_service(project).refresh_cache { global_count[project.id].to_i }
+      end
+    end
+
+    def current_count_service(project)
+      if defined? @service
+        @service.project = project
+      else
+        count_service.new(project)
+      end
+    end
+
+    def global_count(project)
+      raise NotImplementedError, 'global_count must be implemented and return an hash indexed by the project id'
+    end
+
+    def count_service
+      raise NotImplementedError, 'count_service must be implemented and return a Projects::CountService object'
+    end
+  end
+end
diff --git a/app/services/projects/batch_forks_count_service.rb b/app/services/projects/batch_forks_count_service.rb
new file mode 100644
index 00000000000..51fad1c12a4
--- /dev/null
+++ b/app/services/projects/batch_forks_count_service.rb
@@ -0,0 +1,14 @@
+module Projects
+  # Service class for getting and caching the number of forks of several projects
+  class BatchForksCountService < Projects::BatchCountService
+    def global_count
+      @global_count ||= ForkedProjectLink.where(forked_from_project: @projects.map(&:id))
+                                         .group(:forked_from_project_id)
+                                         .count
+    end
+
+    def count_service
+      ::Projects::ForksCountService
+    end
+  end
+end
diff --git a/app/services/projects/batch_open_issues_count_service.rb b/app/services/projects/batch_open_issues_count_service.rb
new file mode 100644
index 00000000000..a0d9120487c
--- /dev/null
+++ b/app/services/projects/batch_open_issues_count_service.rb
@@ -0,0 +1,15 @@
+module Projects
+  # Service class for getting and caching the number of forks of several projects
+  class BatchOpenIssuesCountService < Projects::BatchCountService
+    def global_count
+      @global_count ||= Issue.opened.public_only
+                             .where(project: @projects.map(&:id))
+                             .group(:project_id)
+                             .count
+    end
+
+    def count_service
+      ::Projects::OpenIssuesCountService
+    end
+  end
+end
diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb
index 7e575b2d6f3..a3ec52232dd 100644
--- a/app/services/projects/count_service.rb
+++ b/app/services/projects/count_service.rb
@@ -7,6 +7,8 @@ module Projects
     # all caches.
     VERSION = 1
 
+    attr_accessor :project
+
     def initialize(project)
       @project = project
     end
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index df2d97bceda..746dcc9fc91 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -52,6 +52,24 @@ module API
         groups
       end
 
+      def find_group_projects(params)
+        group = find_group!(params[:id])
+        projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
+        projects = projects.preload(:project_feature, :route, :group)
+                           .preload(namespace: [:route, :owner],
+                                    tags: :taggings,
+                                    project_group_links: :group,
+                                    fork_network: :root_project,
+                                    forked_project_link: :forked_from_project,
+                                    forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
+        projects = reorder_projects(projects)
+        paginated_projects = paginate(projects)
+        projects_with_fork = paginated_projects + paginated_projects.map(&:forked_from_project).compact
+        ::Projects::BatchForksCountService.new(projects_with_fork).refresh_cache
+        ::Projects::BatchOpenIssuesCountService.new(paginated_projects).refresh_cache
+        paginated_projects
+      end
+
       def present_groups(params, groups)
         options = {
           with: Entities::Group,
@@ -170,12 +188,9 @@ module API
         use :pagination
       end
       get ":id/projects" do
-        group = find_group!(params[:id])
-        projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
-        projects = projects.preload(:fork_network, :forked_project_link, :project_feature, :project_group_links, :tags, :taggings, :group, :namespace, :route)
-        projects = reorder_projects(projects)
+        projects = find_group_projects(params)
         entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project
-        present paginate(projects), with: entity, current_user: current_user
+        present projects, with: entity, current_user: current_user
       end
 
       desc 'Get a list of subgroups in this group.' do

From 7c7877b54dfb0a07bf128102226e338463654431 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Fri, 24 Nov 2017 13:32:01 +0100
Subject: [PATCH 07/21] Small renaming

---
 lib/api/groups.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 746dcc9fc91..05443329a32 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -63,11 +63,11 @@ module API
                                     forked_project_link: :forked_from_project,
                                     forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
         projects = reorder_projects(projects)
-        paginated_projects = paginate(projects)
-        projects_with_fork = paginated_projects + paginated_projects.map(&:forked_from_project).compact
+        projects = paginate(projects)
+        projects_with_fork = projects + projects.map(&:forked_from_project).compact
         ::Projects::BatchForksCountService.new(projects_with_fork).refresh_cache
-        ::Projects::BatchOpenIssuesCountService.new(paginated_projects).refresh_cache
-        paginated_projects
+        ::Projects::BatchOpenIssuesCountService.new(projects).refresh_cache
+        projects
       end
 
       def present_groups(params, groups)

From c85f9c5b1d320f7a6f75e3d08bbafd2fb20d3f58 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Fri, 24 Nov 2017 17:37:02 +0100
Subject: [PATCH 08/21] Code review comments applied

---
 app/services/base_count_service.rb           | 4 ++++
 app/services/projects/batch_count_service.rb | 8 ++++++--
 lib/api/groups.rb                            | 4 ++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/app/services/base_count_service.rb b/app/services/base_count_service.rb
index 2e4fd1db03e..f2844854112 100644
--- a/app/services/base_count_service.rb
+++ b/app/services/base_count_service.rb
@@ -12,6 +12,10 @@ class BaseCountService
     Rails.cache.fetch(cache_key, cache_options) { uncached_count }.to_i
   end
 
+  def count_stored?
+    Rails.cache.read(cache_key).present?
+  end
+
   def refresh_cache(&block)
     Rails.cache.write(cache_key, block_given? ? yield : uncached_count, raw: raw?)
   end
diff --git a/app/services/projects/batch_count_service.rb b/app/services/projects/batch_count_service.rb
index 5b69e24b893..b8a030e7bc5 100644
--- a/app/services/projects/batch_count_service.rb
+++ b/app/services/projects/batch_count_service.rb
@@ -12,7 +12,9 @@ module Projects
 
     def refresh_cache
       @projects.each do |project|
-        current_count_service(project).refresh_cache { global_count[project.id].to_i }
+        unless current_count_service(project).count_stored?
+          current_count_service(project).refresh_cache { global_count[project.id].to_i }
+        end
       end
     end
 
@@ -20,8 +22,10 @@ module Projects
       if defined? @service
         @service.project = project
       else
-        count_service.new(project)
+        @service = count_service.new(project)
       end
+
+      @service
     end
 
     def global_count(project)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 05443329a32..83c7898150a 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -64,8 +64,8 @@ module API
                                     forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
         projects = reorder_projects(projects)
         projects = paginate(projects)
-        projects_with_fork = projects + projects.map(&:forked_from_project).compact
-        ::Projects::BatchForksCountService.new(projects_with_fork).refresh_cache
+        projects_with_forks = projects + projects.map(&:forked_from_project).compact
+        ::Projects::BatchForksCountService.new(projects_with_forks).refresh_cache
         ::Projects::BatchOpenIssuesCountService.new(projects).refresh_cache
         projects
       end

From 58c5b463ff75618a557d067c16f49ef581cda85c Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Mon, 27 Nov 2017 12:24:33 +0100
Subject: [PATCH 09/21] Refactored /projects and /user/:user_id/projects
 endpoints

---
 lib/api/entities.rb                  | 48 ++++++++++++++++++++++------
 lib/api/groups.rb                    | 16 ++--------
 lib/api/projects.rb                  |  4 +--
 lib/api/projects_relation_builder.rb | 34 ++++++++++++++++++++
 4 files changed, 77 insertions(+), 25 deletions(-)
 create mode 100644 lib/api/projects_relation_builder.rb

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 5b9b0afed0f..8f41b930c0a 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -94,7 +94,13 @@ module API
         project.avatar_url(only_path: false)
       end
       expose :star_count, :forks_count
-      expose :last_activity_at
+      expose :created_at, :last_activity_at
+
+      def self.preload_relation(projects_relation)
+        projects_relation.preload(:project_feature, :route)
+                         .preload(namespace: [:route, :owner],
+                                  tags: :taggings)
+      end
     end
 
     class Project < BasicProjectDetails
@@ -128,6 +134,18 @@ module API
         expose :members do |project|
           expose_url(api_v4_projects_members_path(id: project.id))
         end
+
+        def self.preload_relation(projects_relation)
+          super(projects_relation).preload(:group)
+                                  .preload(project_group_links: :group,
+                                           fork_network: :root_project,
+                                           forked_project_link: :forked_from_project,
+                                           forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
+        end
+
+        def self.forks_counting_projects(projects_relation)
+          projects_relation + projects_relation.map(&:forked_from_project).compact
+        end
       end
 
       expose :archived?, as: :archived
@@ -635,9 +653,16 @@ module API
     class MemberAccess < Grape::Entity
       expose :access_level
       expose :notification_level do |member, options|
-        if member.notification_setting
-          ::NotificationSetting.levels[member.notification_setting.level]
-        end
+        notification = member_notification_setting(member)
+        ::NotificationSetting.levels[notification.level] if notification
+      end
+
+      private
+
+      def member_notification_setting(member)
+        member.user.notification_settings.select do |notification|
+          notification.source_id == member.source_id && notification.source_type == member.source_type
+        end.last
       end
     end
 
@@ -680,18 +705,21 @@ module API
       expose :permissions do
         expose :project_access, using: Entities::ProjectAccess do |project, options|
           if options.key?(:project_members)
-            (options[:project_members] || []).find { |member| member.source_id == project.id }
-          else
-            project.project_members.find_by(user_id: options[:current_user].id)
+            (options[:project_members] || []).select { |member| member.source_id == project.id }.last
+          elsif project.project_members.any?
+            # This is not the bet option to search in a CollectionProxy, but if
+            # we use find_by we will perform another query, even if the association
+            # is loaded
+            project.project_members.select { |project_member| project_member.user_id == options[:current_user].id }.last
           end
         end
 
         expose :group_access, using: Entities::GroupAccess do |project, options|
           if project.group
             if options.key?(:group_members)
-              (options[:group_members] || []).find { |member| member.source_id == project.namespace_id }
-            else
-              project.group.group_members.find_by(user_id: options[:current_user].id)
+              (options[:group_members] || []).select { |member| member.source_id == project.namespace_id }.last
+            elsif project.group.group_members.any?
+              project.group.group_members.select { |group_member| group_member.user_id == options[:current_user].id }.last
             end
           end
         end
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 83c7898150a..b81f07a1770 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -55,19 +55,8 @@ module API
       def find_group_projects(params)
         group = find_group!(params[:id])
         projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
-        projects = projects.preload(:project_feature, :route, :group)
-                           .preload(namespace: [:route, :owner],
-                                    tags: :taggings,
-                                    project_group_links: :group,
-                                    fork_network: :root_project,
-                                    forked_project_link: :forked_from_project,
-                                    forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
         projects = reorder_projects(projects)
-        projects = paginate(projects)
-        projects_with_forks = projects + projects.map(&:forked_from_project).compact
-        ::Projects::BatchForksCountService.new(projects_with_forks).refresh_cache
-        ::Projects::BatchOpenIssuesCountService.new(projects).refresh_cache
-        projects
+        paginate(projects)
       end
 
       def present_groups(params, groups)
@@ -190,7 +179,8 @@ module API
       get ":id/projects" do
         projects = find_group_projects(params)
         entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project
-        present projects, with: entity, current_user: current_user
+
+        present entity.prepare_relation(projects), with: entity, current_user: current_user
       end
 
       desc 'Get a list of subgroups in this group.' do
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 4cd7e714aa2..12506203429 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -79,9 +79,9 @@ module API
         projects = projects.with_statistics if params[:statistics]
         projects = projects.with_issues_enabled if params[:with_issues_enabled]
         projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
+        projects = paginate(projects)
 
         if current_user
-          projects = projects.includes(:route, :taggings, namespace: :route)
           project_members = current_user.project_members
           group_members = current_user.group_members
         end
@@ -95,7 +95,7 @@ module API
         )
         options[:with] = Entities::BasicProjectDetails if params[:simple]
 
-        present paginate(projects), options
+        present options[:with].prepare_relation(projects), options
       end
     end
 
diff --git a/lib/api/projects_relation_builder.rb b/lib/api/projects_relation_builder.rb
new file mode 100644
index 00000000000..57df6157225
--- /dev/null
+++ b/lib/api/projects_relation_builder.rb
@@ -0,0 +1,34 @@
+module API
+  module ProjectsRelationBuilder
+    extend ActiveSupport::Concern
+
+    module ClassMethods
+      def prepare_relation(relation)
+        relation = preload_relation(relation)
+        execute_batch_counting(relation)
+        relation
+      end
+
+      def preload_relation(relation)
+        raise NotImplementedError, 'self.preload_relation method must be defined and return a 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

From 194f7bca9a286942bcd764c836180964e33a1e92 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Thu, 30 Nov 2017 12:52:38 +0100
Subject: [PATCH 10/21] Comments from code review applied. Also switched
 forked_from_project and ForkedProjectLinks to ForkNetworkMember

---
 app/models/group.rb                           |  8 ++
 app/models/project.rb                         |  6 +-
 app/models/user.rb                            |  6 +-
 app/services/projects/batch_count_service.rb  | 21 ++----
 .../projects/batch_forks_count_service.rb     |  8 +-
 .../batch_open_issues_count_service.rb        |  7 +-
 app/services/projects/count_service.rb        |  2 -
 app/services/projects/forks_count_service.rb  |  6 +-
 .../projects/open_issues_count_service.rb     | 10 ++-
 lib/api/entities.rb                           | 75 +++++++++++--------
 lib/api/projects.rb                           | 10 +--
 lib/api/projects_relation_builder.rb          | 10 +--
 12 files changed, 96 insertions(+), 73 deletions(-)

diff --git a/app/models/group.rb b/app/models/group.rb
index 76262acf50c..27287f079a4 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -289,6 +289,14 @@ class Group < Namespace
     "#{parent.full_path}/#{path_was}"
   end
 
+  def group_member(user)
+    if group_members.loaded?
+      group_members.find { |gm| gm.user_id == user.id }
+    else
+      group_members.find_by(user_id: user)
+    end
+  end
+
   private
 
   def update_two_factor_requirement
diff --git a/app/models/project.rb b/app/models/project.rb
index eaf4f555d3b..6f57763b05b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1114,7 +1114,11 @@ class Project < ActiveRecord::Base
   end
 
   def project_member(user)
-    project_members.find_by(user_id: user)
+    if project_members.loaded?
+      project_members.find { |member| member.user_id == user.id }
+    else
+      project_members.find_by(user_id: user)
+    end
   end
 
   def default_branch
diff --git a/app/models/user.rb b/app/models/user.rb
index 14941fd7f98..ee12f541b0f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -998,7 +998,11 @@ class User < ActiveRecord::Base
   end
 
   def notification_settings_for(source)
-    notification_settings.find_or_initialize_by(source: source)
+    if notification_settings.loaded?
+      notification_settings.find { |notification| notification.source == source }
+    else
+      notification_settings.find_or_initialize_by(source: source)
+    end
   end
 
   # Lazy load global notification setting
diff --git a/app/services/projects/batch_count_service.rb b/app/services/projects/batch_count_service.rb
index b8a030e7bc5..a79fdb62d69 100644
--- a/app/services/projects/batch_count_service.rb
+++ b/app/services/projects/batch_count_service.rb
@@ -4,28 +4,19 @@ module Projects
       @projects = projects
     end
 
-    def count
-      @projects.map do |project|
-        [project.id, current_count_service(project).count]
-      end.to_h
-    end
-
     def refresh_cache
       @projects.each do |project|
-        unless current_count_service(project).count_stored?
-          current_count_service(project).refresh_cache { global_count[project.id].to_i }
+        service = count_service.new(project)
+        unless service.count_stored?
+          service.refresh_cache { global_count[project.id].to_i }
         end
       end
     end
 
-    def current_count_service(project)
-      if defined? @service
-        @service.project = project
-      else
-        @service = count_service.new(project)
-      end
+    def project_ids
+      return @projects if @projects.is_a?(ActiveRecord::Relation)
 
-      @service
+      @projects.map(&:id)
     end
 
     def global_count(project)
diff --git a/app/services/projects/batch_forks_count_service.rb b/app/services/projects/batch_forks_count_service.rb
index 51fad1c12a4..b9ed0b3cf4f 100644
--- a/app/services/projects/batch_forks_count_service.rb
+++ b/app/services/projects/batch_forks_count_service.rb
@@ -2,9 +2,11 @@ module Projects
   # Service class for getting and caching the number of forks of several projects
   class BatchForksCountService < Projects::BatchCountService
     def global_count
-      @global_count ||= ForkedProjectLink.where(forked_from_project: @projects.map(&:id))
-                                         .group(:forked_from_project_id)
-                                         .count
+      @global_count ||= begin
+        count_service.query(project_ids)
+                     .group(:forked_from_project_id)
+                     .count
+      end
     end
 
     def count_service
diff --git a/app/services/projects/batch_open_issues_count_service.rb b/app/services/projects/batch_open_issues_count_service.rb
index a0d9120487c..5087684863b 100644
--- a/app/services/projects/batch_open_issues_count_service.rb
+++ b/app/services/projects/batch_open_issues_count_service.rb
@@ -2,10 +2,9 @@ module Projects
   # Service class for getting and caching the number of forks of several projects
   class BatchOpenIssuesCountService < Projects::BatchCountService
     def global_count
-      @global_count ||= Issue.opened.public_only
-                             .where(project: @projects.map(&:id))
-                             .group(:project_id)
-                             .count
+      @global_count ||= begin
+        count_service.query(project_ids).group(:project_id).count
+      end
     end
 
     def count_service
diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb
index a3ec52232dd..7e575b2d6f3 100644
--- a/app/services/projects/count_service.rb
+++ b/app/services/projects/count_service.rb
@@ -7,8 +7,6 @@ module Projects
     # all caches.
     VERSION = 1
 
-    attr_accessor :project
-
     def initialize(project)
       @project = project
     end
diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb
index d9bdf3a8ad7..d67ae78b268 100644
--- a/app/services/projects/forks_count_service.rb
+++ b/app/services/projects/forks_count_service.rb
@@ -2,11 +2,15 @@ module Projects
   # Service class for getting and caching the number of forks of a project.
   class ForksCountService < Projects::CountService
     def relation_for_count
-      @project.forks
+      self.class.query(@project.id)
     end
 
     def cache_key_name
       'forks_count'
     end
+
+    def self.query(project_ids)
+      ForkNetworkMember.where(forked_from_project: project_ids)
+    end
   end
 end
diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb
index 25de97325e2..ab1c477936a 100644
--- a/app/services/projects/open_issues_count_service.rb
+++ b/app/services/projects/open_issues_count_service.rb
@@ -3,13 +3,17 @@ module Projects
   # project.
   class OpenIssuesCountService < Projects::CountService
     def relation_for_count
-      # We don't include confidential issues in this number since this would
-      # expose the number of confidential issues to non project members.
-      @project.issues.opened.public_only
+      self.class.query(@project.id)
     end
 
     def cache_key_name
       'open_issues_count'
     end
+
+    def self.query(project_ids)
+      # We don't include confidential issues in this number since this would
+      # expose the number of confidential issues to non project members.
+      Issue.opened.public_only.where(project: project_ids)
+    end
   end
 end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 8f41b930c0a..dada353a314 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -96,7 +96,7 @@ module API
       expose :star_count, :forks_count
       expose :created_at, :last_activity_at
 
-      def self.preload_relation(projects_relation)
+      def self.preload_relation(projects_relation, options =  {})
         projects_relation.preload(:project_feature, :route)
                          .preload(namespace: [:route, :owner],
                                   tags: :taggings)
@@ -134,18 +134,6 @@ module API
         expose :members do |project|
           expose_url(api_v4_projects_members_path(id: project.id))
         end
-
-        def self.preload_relation(projects_relation)
-          super(projects_relation).preload(:group)
-                                  .preload(project_group_links: :group,
-                                           fork_network: :root_project,
-                                           forked_project_link: :forked_from_project,
-                                           forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
-        end
-
-        def self.forks_counting_projects(projects_relation)
-          projects_relation + projects_relation.map(&:forked_from_project).compact
-        end
       end
 
       expose :archived?, as: :archived
@@ -165,7 +153,9 @@ module API
       expose :lfs_enabled?, as: :lfs_enabled
       expose :creator_id
       expose :namespace, using: 'API::Entities::NamespaceBasic'
-      expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? }
+      expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? } do |project, options|
+        project.fork_network_member.forked_from_project
+      end
       expose :import_status
       expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] }
 
@@ -182,6 +172,20 @@ module API
       expose :printing_merge_request_link_enabled
 
       expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics
+
+      def self.preload_relation(projects_relation, options =  {})
+        relation = super(projects_relation).preload(:group)
+                                           .preload(project_group_links: :group,
+                                                    fork_network: :root_project,
+                                                    fork_network_member: [forked_from_project: [:route, namespace: :route, tags: :taggings]])
+
+        # Remove this preload once forked_project_links and forked_from_project models have been removed
+        relation.preload(forked_project_link: :forked_from_project)
+      end
+
+      def self.forks_counting_projects(projects_relation)
+        projects_relation + projects_relation.map(&:fork_network_member).compact.map(&:forked_from_project).compact
+      end
     end
 
     class ProjectStatistics < Grape::Entity
@@ -653,16 +657,10 @@ module API
     class MemberAccess < Grape::Entity
       expose :access_level
       expose :notification_level do |member, options|
-        notification = member_notification_setting(member)
-        ::NotificationSetting.levels[notification.level] if notification
-      end
-
-      private
-
-      def member_notification_setting(member)
-        member.user.notification_settings.select do |notification|
-          notification.source_id == member.source_id && notification.source_type == member.source_type
-        end.last
+        # binding.pry if member.id == 5
+        if member.notification_setting
+          ::NotificationSetting.levels[member.notification_setting.level]
+        end
       end
     end
 
@@ -705,25 +703,36 @@ module API
       expose :permissions do
         expose :project_access, using: Entities::ProjectAccess do |project, options|
           if options.key?(:project_members)
-            (options[:project_members] || []).select { |member| member.source_id == project.id }.last
-          elsif project.project_members.any?
-            # This is not the bet option to search in a CollectionProxy, but if
-            # we use find_by we will perform another query, even if the association
-            # is loaded
-            project.project_members.select { |project_member| project_member.user_id == options[:current_user].id }.last
+            (options[:project_members] || []).find { |member| member.source_id == project.id }
+          else
+            project.project_member(options[:current_user])
           end
         end
 
         expose :group_access, using: Entities::GroupAccess do |project, options|
           if project.group
             if options.key?(:group_members)
-              (options[:group_members] || []).select { |member| member.source_id == project.namespace_id }.last
-            elsif project.group.group_members.any?
-              project.group.group_members.select { |group_member| group_member.user_id == options[:current_user].id }.last
+              (options[:group_members] || []).find { |member| member.source_id == project.namespace_id }
+            else
+              project.group.group_member(options[:current_user])
             end
           end
         end
       end
+
+      def self.preload_relation(projects_relation, options = {})
+        relation = super(projects_relation, options)
+
+        unless options.key?(:group_members)
+          relation = relation.preload(group: [group_members: [:source, user: [notification_settings: :source]]])
+        end
+
+        unless options.key?(:project_members)
+          relation = relation.preload(project_members: [:source, user: [notification_settings: :source]])
+        end
+
+        relation
+      end
     end
 
     class LabelBasic < Grape::Entity
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 12506203429..9e92ff1417e 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -82,20 +82,20 @@ module API
         projects = paginate(projects)
 
         if current_user
-          project_members = current_user.project_members
-          group_members = current_user.group_members
+          project_members = current_user.project_members.preload(:source, user: [notification_settings: :source])
+          group_members = current_user.group_members.preload(:source, user: [notification_settings: :source])
         end
 
         options = options.reverse_merge(
           with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails,
           statistics: params[:statistics],
-          project_members: project_members,
-          group_members: group_members,
+          project_members: nil,
+          group_members: nil,
           current_user: current_user
         )
         options[:with] = Entities::BasicProjectDetails if params[:simple]
 
-        present options[:with].prepare_relation(projects), options
+        present options[:with].prepare_relation(projects, options), options
       end
     end
 
diff --git a/lib/api/projects_relation_builder.rb b/lib/api/projects_relation_builder.rb
index 57df6157225..2e042d51c05 100644
--- a/lib/api/projects_relation_builder.rb
+++ b/lib/api/projects_relation_builder.rb
@@ -3,13 +3,13 @@ module API
     extend ActiveSupport::Concern
 
     module ClassMethods
-      def prepare_relation(relation)
-        relation = preload_relation(relation)
-        execute_batch_counting(relation)
-        relation
+      def prepare_relation(projects_relation, options = {})
+        projects_relation = preload_relation(projects_relation, options)
+        execute_batch_counting(projects_relation)
+        projects_relation
       end
 
-      def preload_relation(relation)
+      def preload_relation(projects_relation, options =  {})
         raise NotImplementedError, 'self.preload_relation method must be defined and return a relation'
       end
 

From 966f83f9653aa774ee82be65dbdae0c6e4f297da Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Thu, 30 Nov 2017 13:13:00 +0100
Subject: [PATCH 11/21] Undoing debugging change

---
 lib/api/projects.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 9e92ff1417e..14a4fc6f025 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -89,8 +89,8 @@ module API
         options = options.reverse_merge(
           with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails,
           statistics: params[:statistics],
-          project_members: nil,
-          group_members: nil,
+          project_members: project_members,
+          group_members: group_members,
           current_user: current_user
         )
         options[:with] = Entities::BasicProjectDetails if params[:simple]

From fa6b0a36bd315a4777bb8b3229b89808b95a9489 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Thu, 30 Nov 2017 14:01:56 +0100
Subject: [PATCH 12/21] Changes after rebase

---
 lib/api/entities.rb | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index dada353a314..e0eb2ecfb73 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -87,14 +87,25 @@ module API
       expose :created_at
     end
 
-    class BasicProjectDetails < ProjectIdentity
-      expose :default_branch, :tag_list
+    class BasicProjectDetails < Grape::Entity
+      include ::API::ProjectsRelationBuilder
+
+      expose :default_branch
+
+      # Avoids an N+1 query: https://github.com/mbleigh/acts-as-taggable-on/issues/91#issuecomment-168273770
+      expose :tag_list do |project|
+        # project.tags.order(:name).pluck(:name) is the most suitable option
+        # to avoid loading all the ActiveRecord objects but, if we use it here
+        # it override the preloaded associations and makes a query
+        # (fixed in https://github.com/rails/rails/pull/25976).
+        project.tags.map(&:name).sort
+      end
       expose :ssh_url_to_repo, :http_url_to_repo, :web_url
       expose :avatar_url do |project, options|
         project.avatar_url(only_path: false)
       end
       expose :star_count, :forks_count
-      expose :created_at, :last_activity_at
+      expose :last_activity_at
 
       def self.preload_relation(projects_relation, options =  {})
         projects_relation.preload(:project_feature, :route)

From c7e7f4444c791eb0ad409310394954578aa232c6 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Thu, 30 Nov 2017 16:20:13 +0100
Subject: [PATCH 13/21] Removing blank line

---
 lib/api/entities.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e0eb2ecfb73..910ac4f1814 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -91,7 +91,6 @@ module API
       include ::API::ProjectsRelationBuilder
 
       expose :default_branch
-
       # Avoids an N+1 query: https://github.com/mbleigh/acts-as-taggable-on/issues/91#issuecomment-168273770
       expose :tag_list do |project|
         # project.tags.order(:name).pluck(:name) is the most suitable option

From c0c0926accdc3c49fc2a75a0eec2f96a8a5ad15c Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Thu, 30 Nov 2017 18:24:31 +0100
Subject: [PATCH 14/21] Removed binding.pry

---
 lib/api/entities.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 910ac4f1814..248e234580f 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -667,7 +667,6 @@ module API
     class MemberAccess < Grape::Entity
       expose :access_level
       expose :notification_level do |member, options|
-        # binding.pry if member.id == 5
         if member.notification_setting
           ::NotificationSetting.levels[member.notification_setting.level]
         end

From fe95de88551bd3c8d22591764d948205f9fbc10e Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Thu, 30 Nov 2017 19:09:25 +0100
Subject: [PATCH 15/21] Fixed BasicProjectDetail parent

---
 lib/api/entities.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 248e234580f..d224f468c18 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -87,7 +87,7 @@ module API
       expose :created_at
     end
 
-    class BasicProjectDetails < Grape::Entity
+    class BasicProjectDetails < ProjectIdentity
       include ::API::ProjectsRelationBuilder
 
       expose :default_branch

From 3527d1ff2bc06ba38e820b300e49f817d2833379 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Thu, 30 Nov 2017 22:17:17 +0100
Subject: [PATCH 16/21] Undoing the change to ForkNetworkMember

---
 app/services/projects/forks_count_service.rb |  5 ++++-
 lib/api/entities.rb                          | 18 +++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb
index d67ae78b268..95ce655b157 100644
--- a/app/services/projects/forks_count_service.rb
+++ b/app/services/projects/forks_count_service.rb
@@ -10,7 +10,10 @@ module Projects
     end
 
     def self.query(project_ids)
-      ForkNetworkMember.where(forked_from_project: project_ids)
+      # We can't directly change ForkedProjectLink to ForkNetworkMember here
+      # Nowadays, when a call using v3 to projects/:id/fork is made,
+      # the relationship to ForkNetworkMember is not updated
+      ForkedProjectLink.where(forked_from_project: project_ids)
     end
   end
 end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index d224f468c18..7cec8da013d 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -163,9 +163,7 @@ module API
       expose :lfs_enabled?, as: :lfs_enabled
       expose :creator_id
       expose :namespace, using: 'API::Entities::NamespaceBasic'
-      expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? } do |project, options|
-        project.fork_network_member.forked_from_project
-      end
+      expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? }
       expose :import_status
       expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] }
 
@@ -184,17 +182,15 @@ module API
       expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics
 
       def self.preload_relation(projects_relation, options =  {})
-        relation = super(projects_relation).preload(:group)
-                                           .preload(project_group_links: :group,
-                                                    fork_network: :root_project,
-                                                    fork_network_member: [forked_from_project: [:route, namespace: :route, tags: :taggings]])
-
-        # Remove this preload once forked_project_links and forked_from_project models have been removed
-        relation.preload(forked_project_link: :forked_from_project)
+        super(projects_relation).preload(:group)
+                                .preload(project_group_links: :group,
+                                         fork_network: :root_project,
+                                         forked_project_link: :forked_from_project,
+                                         forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
       end
 
       def self.forks_counting_projects(projects_relation)
-        projects_relation + projects_relation.map(&:fork_network_member).compact.map(&:forked_from_project).compact
+        projects_relation + projects_relation.map(&:forked_from_project).compact
       end
     end
 

From 3dc331c6131040b8c2a0e682c34e5a19c803e000 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Fri, 1 Dec 2017 09:14:55 +0100
Subject: [PATCH 17/21] Fixed test

---
 spec/models/project_spec.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 521b7bd70ba..a069ae784af 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -315,7 +315,6 @@ describe Project do
 
     it { is_expected.to delegate_method(:empty_repo?).to(:repository) }
     it { is_expected.to delegate_method(:members).to(:team).with_prefix(true) }
-    it { is_expected.to delegate_method(:count).to(:forks).with_prefix(true) }
     it { is_expected.to delegate_method(:name).to(:owner).with_prefix(true).with_arguments(allow_nil: true) }
   end
 
@@ -2473,7 +2472,7 @@ describe Project do
     it 'returns the number of forks' do
       project = build(:project)
 
-      allow(project.forks).to receive(:count).and_return(1)
+      expect_any_instance_of(Projects::ForksCountService).to receive(:count).and_return(1)
 
       expect(project.forks_count).to eq(1)
     end

From c5a89b35f0d43b54df8767522428a006770673ac Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Fri, 1 Dec 2017 11:34:24 +0100
Subject: [PATCH 18/21] Using map to retrieve the element ids because of some
 issues with mysql

---
 app/services/projects/batch_count_service.rb             | 5 +++--
 app/services/projects/batch_forks_count_service.rb       | 4 +++-
 app/services/projects/batch_open_issues_count_service.rb | 4 +++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/app/services/projects/batch_count_service.rb b/app/services/projects/batch_count_service.rb
index a79fdb62d69..178ebc5a143 100644
--- a/app/services/projects/batch_count_service.rb
+++ b/app/services/projects/batch_count_service.rb
@@ -1,3 +1,6 @@
+# Service class for getting and caching the number of elements of several projects
+# Warning: do not user this service with a really large set of projects
+# because the service use maps to retrieve the project ids.
 module Projects
   class BatchCountService
     def initialize(projects)
@@ -14,8 +17,6 @@ module Projects
     end
 
     def project_ids
-      return @projects if @projects.is_a?(ActiveRecord::Relation)
-
       @projects.map(&:id)
     end
 
diff --git a/app/services/projects/batch_forks_count_service.rb b/app/services/projects/batch_forks_count_service.rb
index b9ed0b3cf4f..e61fe6c86b2 100644
--- a/app/services/projects/batch_forks_count_service.rb
+++ b/app/services/projects/batch_forks_count_service.rb
@@ -1,5 +1,7 @@
+# Service class for getting and caching the number of forks of several projects
+# Warning: do not user this service with a really large set of projects
+# because the service use maps to retrieve the project ids
 module Projects
-  # Service class for getting and caching the number of forks of several projects
   class BatchForksCountService < Projects::BatchCountService
     def global_count
       @global_count ||= begin
diff --git a/app/services/projects/batch_open_issues_count_service.rb b/app/services/projects/batch_open_issues_count_service.rb
index 5087684863b..3b0ade2419b 100644
--- a/app/services/projects/batch_open_issues_count_service.rb
+++ b/app/services/projects/batch_open_issues_count_service.rb
@@ -1,5 +1,7 @@
+# Service class for getting and caching the number of issues of several projects
+# Warning: do not user this service with a really large set of projects
+# because the service use maps to retrieve the project ids
 module Projects
-  # Service class for getting and caching the number of forks of several projects
   class BatchOpenIssuesCountService < Projects::BatchCountService
     def global_count
       @global_count ||= begin

From 979056e964827a9d6efc979843ac567a3dd5cdfd Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Fri, 1 Dec 2017 19:21:04 +0100
Subject: [PATCH 19/21] Not forcing to redefine preload_relation

---
 lib/api/projects_relation_builder.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/api/projects_relation_builder.rb b/lib/api/projects_relation_builder.rb
index 2e042d51c05..6482fd94ab8 100644
--- a/lib/api/projects_relation_builder.rb
+++ b/lib/api/projects_relation_builder.rb
@@ -10,7 +10,7 @@ module API
       end
 
       def preload_relation(projects_relation, options =  {})
-        raise NotImplementedError, 'self.preload_relation method must be defined and return a relation'
+        projects_relation
       end
 
       def forks_counting_projects(projects_relation)

From 7f2b6b11bd7aaeab73f6f1b5431e9d8f3f034cb6 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Mon, 4 Dec 2017 11:20:20 +0100
Subject: [PATCH 20/21] Moving query to base count service

---
 app/services/projects/count_service.rb             | 4 ++++
 app/services/projects/forks_count_service.rb       | 4 ----
 app/services/projects/open_issues_count_service.rb | 4 ----
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb
index 7e575b2d6f3..42ebb38f676 100644
--- a/app/services/projects/count_service.rb
+++ b/app/services/projects/count_service.rb
@@ -11,6 +11,10 @@ module Projects
       @project = project
     end
 
+    def relation_for_count
+      self.class.query(@project.id)
+    end
+
     def cache_key_name
       raise(
         NotImplementedError,
diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb
index 95ce655b157..dc6eb19affd 100644
--- a/app/services/projects/forks_count_service.rb
+++ b/app/services/projects/forks_count_service.rb
@@ -1,10 +1,6 @@
 module Projects
   # Service class for getting and caching the number of forks of a project.
   class ForksCountService < Projects::CountService
-    def relation_for_count
-      self.class.query(@project.id)
-    end
-
     def cache_key_name
       'forks_count'
     end
diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb
index ab1c477936a..a975a06a05c 100644
--- a/app/services/projects/open_issues_count_service.rb
+++ b/app/services/projects/open_issues_count_service.rb
@@ -2,10 +2,6 @@ module Projects
   # Service class for counting and caching the number of open issues of a
   # project.
   class OpenIssuesCountService < Projects::CountService
-    def relation_for_count
-      self.class.query(@project.id)
-    end
-
     def cache_key_name
       'open_issues_count'
     end

From 264171f72d4ef3e5dfafaf32d3d267ab279469e1 Mon Sep 17 00:00:00 2001
From: Francisco Lopez <fjlopez@gitlab.com>
Date: Mon, 4 Dec 2017 12:33:19 +0100
Subject: [PATCH 21/21] Fixed bug

---
 app/services/projects/count_service.rb       |  7 +++++++
 spec/services/projects/count_service_spec.rb | 12 ++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb
index 42ebb38f676..933829b557b 100644
--- a/app/services/projects/count_service.rb
+++ b/app/services/projects/count_service.rb
@@ -25,5 +25,12 @@ module Projects
     def cache_key
       ['projects', 'count_service', VERSION, @project.id, cache_key_name]
     end
+
+    def self.query(project_ids)
+      raise(
+        NotImplementedError,
+        '"query" must be implemented and return an ActiveRecord::Relation'
+      )
+    end
   end
 end
diff --git a/spec/services/projects/count_service_spec.rb b/spec/services/projects/count_service_spec.rb
index cc496501bad..183f6128c7b 100644
--- a/spec/services/projects/count_service_spec.rb
+++ b/spec/services/projects/count_service_spec.rb
@@ -4,9 +4,17 @@ describe Projects::CountService do
   let(:project) { build(:project, id: 1) }
   let(:service) { described_class.new(project) }
 
-  describe '#relation_for_count' do
+  describe '.query' do
     it 'raises NotImplementedError' do
-      expect { service.relation_for_count }.to raise_error(NotImplementedError)
+      expect { described_class.query(project.id) }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#relation_for_count' do
+    it 'calls the class method query with the project id' do
+      expect(described_class).to receive(:query).with(project.id)
+
+      service.relation_for_count
     end
   end