Merge branch 'sh-optimize-todos-controller' into 'master'

Eliminate N+1 queries in Dashboard::TodosController

Closes #43042

See merge request gitlab-org/gitlab-ce!29954

6 files changed, 37 insertions, 3 deletions

diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index f173c263474..27980466a42 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -10,6 +10,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   def index
     @sort = params[:sort]
     @todos = @todos.page(params[:page])
+    @todos = @todos.with_entity_associations
 
     return if redirect_out_of_range(@todos)
   end
diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index 6bd78336ed3..645160077f5 100644
--- a/app/helpers/todos_helper.rb
+++ b/app/helpers/todos_helper.rb
@@ -170,7 +170,7 @@ module TodosHelper
   end
 
   def todo_group_options
-    groups = current_user.authorized_groups.map do |group|
+    groups = current_user.authorized_groups.with_route.map do |group|
       { id: group.id, text: group.full_name }
     end
 
diff --git a/app/models/todo.rb b/app/models/todo.rb
index f1fc5e599eb..240c91da5b6 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -60,7 +60,7 @@ class Todo < ApplicationRecord
   scope :for_type, -> (type) { where(target_type: type) }
   scope :for_target, -> (id) { where(target_id: id) }
   scope :for_commit, -> (id) { where(commit_id: id) }
-  scope :with_api_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: :route }]) }
+  scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: :route }]) }
   scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) }
 
   state_machine :state, initial: :pending do
diff --git a/changelogs/unreleased/sh-optimize-todos-controller.yml b/changelogs/unreleased/sh-optimize-todos-controller.yml
new file mode 100644
index 00000000000..181ddd1b3bc
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-todos-controller.yml
@@ -0,0 +1,5 @@
+---
+title: Eliminate N+1 queries in Dashboard::TodosController
+merge_request: 29954
+author:
+type: performance
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index d2196f05173..871eaabc887 100644
--- a/lib/api/todos.rb
+++ b/lib/api/todos.rb
@@ -77,7 +77,7 @@ module API
         use :pagination
       end
       get do
-        todos = paginate(find_todos.with_api_entity_associations)
+        todos = paginate(find_todos.with_entity_associations)
         options = { with: Entities::Todo, current_user: current_user }
         batch_load_issuable_metadata(todos, options)
 
diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb
index 6243ddc03c0..9a3fbfaac51 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -44,6 +44,34 @@ describe Dashboard::TodosController do
       end
     end
 
+    context "with render_views" do
+      render_views
+
+      it 'avoids N+1 queries', :request_store do
+        merge_request = create(:merge_request, source_project: project)
+        create(:todo, project: project, author: author, user: user, target: merge_request)
+        create(:issue, project: project, assignees: [user])
+
+        group = create(:group)
+        group.add_owner(user)
+
+        get :index
+
+        control = ActiveRecord::QueryRecorder.new { get :index }
+
+        create(:issue, project: project, assignees: [user])
+        group_2 = create(:group)
+        group_2.add_owner(user)
+        project_2 = create(:project)
+        project_2.add_developer(user)
+        merge_request_2 = create(:merge_request, source_project: project_2)
+        create(:todo, project: project, author: author, user: user, target: merge_request_2)
+
+        expect { get :index }.not_to exceed_query_limit(control)
+        expect(response.status).to eq(200)
+      end
+    end
+
     context 'group authorization' do
       it 'renders 404 when user does not have read access on given group' do
         unauthorized_group = create(:group, :private)