Merge branch 'dashboard-projects-controller-query-performance' into 'master'

Improve various parts of Dashboard::ProjectsController

See merge request !13319

app/controllers/dashboard/projects_controller.rb

@@ -45,8 +45,10 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
   end
 
   def load_projects(finder_params)
-    ProjectsFinder.new(params: finder_params, current_user: current_user)
-      .execute.includes(:route, namespace: :route)
+    ProjectsFinder
+      .new(params: finder_params, current_user: current_user)
+      .execute
+      .includes(:route, :creator, namespace: :route)
   end
 
   def load_events

app/helpers/projects_helper.rb

@@ -225,6 +225,26 @@ module ProjectsHelper
     end
   end
 
+  # Returns true if any projects are present.
+  #
+  # If the relation has a LIMIT applied we'll cast the relation to an Array
+  # since repeated any? checks would otherwise result in multiple COUNT queries
+  # being executed.
+  #
+  # If no limit is applied we'll just issue a COUNT since the result set could
+  # be too large to load into memory.
+  def any_projects?(projects)
+    if projects.limit_value
+      projects.to_a.any?
+    else
+      projects.except(:offset).any?
+    end
+  end
+
+  def has_projects_or_name?(projects, params)
+    !!(params[:name] || any_projects?(projects))
+  end
+
   private
 
   def repo_children_classes(field)

app/models/user.rb

@@ -632,7 +632,11 @@ class User < ActiveRecord::Base
   end
 
   def projects_limit_left
-    projects_limit - personal_projects.count
+    projects_limit - personal_projects_count
+  end
+
+  def personal_projects_count
+    @personal_projects_count ||= personal_projects.count
   end
 
   def projects_limit_percent
@@ -646,16 +650,14 @@ class User < ActiveRecord::Base
     events = events.where(project_id: project_ids) if project_ids
 
     # Use the latest event that has not been pushed or merged recently
-    events.recent.find do |event|
-      project = Project.find_by_id(event.project_id)
-      next unless project
-
-      if project.repository.branch_exists?(event.branch_name)
-        merge_requests = MergeRequest.where("created_at >= ?", event.created_at)
-          .where(source_project_id: project.id,
-                 source_branch: event.branch_name)
-        merge_requests.empty?
-      end
+    events.includes(:project).recent.find do |event|
+      next unless event.project.repository.branch_exists?(event.branch_name)
+
+      merge_requests = MergeRequest.where("created_at >= ?", event.created_at)
+        .where(source_project_id: event.project.id,
+               source_branch: event.branch_name)
+
+      merge_requests.empty?
     end
   end
 

app/views/dashboard/projects/index.html.haml

@@ -13,10 +13,8 @@
   - if show_callout?('user_callout_dismissed')
     = render 'shared/user_callout'
 
-  - if @projects.any? || params[:name]
+  - if has_projects_or_name?(@projects, params)
     = render 'dashboard/projects_head'
-
-  - if @projects.any? || params[:name]
     = render 'projects'
   - else
     = render "zero_authorized_projects"

app/views/dashboard/projects/starred.html.haml

@@ -9,7 +9,7 @@
 %div{ class: container_class }
   = render 'dashboard/projects_head'
 
-  - if @projects.any? || params[:filter_projects]
+  - if params[:filter_projects] || any_projects?(@projects)
     = render 'projects'
   - else
     %h3 You don't have starred projects yet

app/views/shared/_new_project_item_select.html.haml

-- if @projects.any?
+- if any_projects?(@projects)
   .project-item-select-holder
     = project_select_tag :project_path, class: "project-item-select", data: { include_groups: local_assigns[:include_groups], order_by: 'last_activity_at', relative_path: local_assigns[:path] }, with_feature_enabled: local_assigns[:with_feature_enabled]
     %a.btn.btn-new.new-project-item-select-button

app/views/shared/projects/_list.html.haml

@@ -10,7 +10,7 @@
 - load_pipeline_status(projects)
 
 .js-projects-list-holder
-  - if projects.any?
+  - if any_projects?(projects)
     %ul.projects-list
       - projects.each_with_index do |project, i|
         - css_class = (i >= projects_limit) || project.pending_delete? ? 'hide' : nil

changelogs/unreleased/dont-use-limit-offset-when-counting-projects.yml

+---
+title: "Improve performance of checking for projects on the projects dashboard"
+merge_request:
+author:

changelogs/unreleased/eager-load-project-creators-for-project-dashboards.yml

+---
+title: Eager load project creators for project dashboards
+merge_request:
+author:

changelogs/unreleased/memoize-user-personal-projects-count.yml

+---
+title: Memoize the number of personal projects a user has to reduce COUNT queries
+merge_request:
+author:

changelogs/unreleased/remove-redundant-query-when-retrieving-recent-pushes.yml

+---
+title: Remove redundant query when retrieving the most recent push of a user
+merge_request:
+author:

spec/helpers/projects_helper_spec.rb

@@ -411,4 +411,48 @@ describe ProjectsHelper do
       end
     end
   end
+
+  describe '#has_projects_or_name?' do
+    let(:projects) do
+      create(:project)
+      Project.all
+    end
+
+    it 'returns true when there are projects' do
+      expect(helper.has_projects_or_name?(projects, {})).to eq(true)
+    end
+
+    it 'returns true when there are no projects but a name is given' do
+      expect(helper.has_projects_or_name?(Project.none, name: 'foo')).to eq(true)
+    end
+
+    it 'returns false when there are no projects and there is no name' do
+      expect(helper.has_projects_or_name?(Project.none, {})).to eq(false)
+    end
+  end
+
+  describe '#any_projects?' do
+    before do
+      create(:project)
+    end
+
+    it 'returns true when projects will be returned' do
+      expect(helper.any_projects?(Project.all)).to eq(true)
+    end
+
+    it 'returns false when no projects will be returned' do
+      expect(helper.any_projects?(Project.none)).to eq(false)
+    end
+
+    it 'only executes a single query when a LIMIT is applied' do
+      relation = Project.limit(1)
+      recorder = ActiveRecord::QueryRecorder.new do
+        2.times do
+          helper.any_projects?(relation)
+        end
+      end
+
+      expect(recorder.count).to eq(1)
+    end
+  end
 end

spec/models/user_spec.rb

@@ -1976,4 +1976,28 @@ describe User do
       expect(user.allow_password_authentication?).to be_falsey
     end
   end
+
+  describe '#personal_projects_count' do
+    it 'returns the number of personal projects using a single query' do
+      user = build(:user)
+      projects = double(:projects, count: 1)
+
+      expect(user).to receive(:personal_projects).once.and_return(projects)
+
+      2.times do
+        expect(user.personal_projects_count).to eq(1)
+      end
+    end
+  end
+
+  describe '#projects_limit_left' do
+    it 'returns the number of projects that can be created by the user' do
+      user = build(:user)
+
+      allow(user).to receive(:projects_limit).and_return(10)
+      allow(user).to receive(:personal_projects_count).and_return(5)
+
+      expect(user.projects_limit_left).to eq(5)
+    end
+  end
 end