Merge branch 'fix/slow-todos' into 'master'

Speed up todos queries by limiting the projects set we join with

See merge request !5791

CHANGELOG

@@ -110,6 +110,7 @@ v 8.11.0 (unreleased)
   - Each `File::exists?` replaced to `File::exist?` because of deprecate since ruby version 2.2.0
   - Add auto-completition in pipeline (Katarzyna Kobierska Ula Budziszewska)
   - Fix a memory leak caused by Banzai::Filter::SanitizationFilter
+  - Speed up todos queries by limiting the projects set we join with
 
 v 8.10.5
   - Add a data migration to fix some missing timestamps in the members table. !5670

app/finders/projects_finder.rb

 class ProjectsFinder < UnionFinder
-  def execute(current_user = nil, options = {})
+  def execute(current_user = nil, project_ids_relation = nil)
     segments = all_projects(current_user)
+    segments.map! { |s| s.where(id: project_ids_relation) } if project_ids_relation
 
     find_union(segments, Project)
   end

app/finders/todos_finder.rb

@@ -27,9 +27,11 @@ class TodosFinder
     items = by_action_id(items)
     items = by_action(items)
     items = by_author(items)
-    items = by_project(items)
     items = by_state(items)
     items = by_type(items)
+    # Filtering by project HAS TO be the last because we use
+    # the project IDs yielded by the todos query thus far
+    items = by_project(items)
 
     items.reorder(id: :desc)
   end
@@ -91,14 +93,9 @@ class TodosFinder
     @project
   end
 
-  def projects
-    return @projects if defined?(@projects)
-
-    if project?
-      @projects = project
-    else
-      @projects = ProjectsFinder.new.execute(current_user)
-    end
+  def projects(items)
+    item_project_ids = items.reorder(nil).select(:project_id)
+    ProjectsFinder.new.execute(current_user, item_project_ids)
   end
 
   def type?
@@ -136,8 +133,9 @@ class TodosFinder
   def by_project(items)
     if project?
       items = items.where(project: project)
-    elsif projects
-      items = items.merge(projects).joins(:project)
+    else
+      item_projects = projects(items)
+      items = items.merge(item_projects).joins(:project)
     end
 
     items

spec/finders/projects_finder_spec.rb

@@ -23,7 +23,6 @@ describe ProjectsFinder do
 
     let(:finder) { described_class.new }
 
-    describe 'without a group' do
     describe 'without a user' do
       subject { finder.execute }
 
@@ -43,53 +42,17 @@ describe ProjectsFinder do
         end
 
         it do
-            is_expected.to eq([public_project, internal_project,
-                               private_project])
+          is_expected.to eq([public_project, internal_project, private_project])
         end
       end
     end
-    end
-
-    describe 'with a group' do
-      describe 'without a user' do
-        subject { finder.execute(nil, group: group) }
 
-        it { is_expected.to eq([public_project]) }
-      end
-
-      describe 'with a user' do
-        subject { finder.execute(user, group: group) }
+    describe 'with project_ids_relation' do
+      let(:project_ids_relation) { Project.where(id: internal_project.id) }
 
-        describe 'without shared projects' do
-          it { is_expected.to eq([public_project, internal_project]) }
-        end
-
-        describe 'with shared projects and group membership' do
-          before do
-            group.add_user(user, Gitlab::Access::DEVELOPER)
-
-            shared_project.project_group_links.
-              create(group_access: Gitlab::Access::MASTER, group: group)
-          end
+      subject { finder.execute(user, project_ids_relation) }
 
-          it do
-            is_expected.to eq([shared_project, public_project, internal_project])
-          end
-        end
-
-        describe 'with shared projects and project membership' do
-          before do
-            shared_project.team.add_user(user, Gitlab::Access::DEVELOPER)
-
-            shared_project.project_group_links.
-              create(group_access: Gitlab::Access::MASTER, group: group)
-          end
-
-          it do
-            is_expected.to eq([shared_project, public_project, internal_project])
-          end
-        end
-      end
+      it { is_expected.to eq([internal_project]) }
     end
   end
 end