From d13c36f72de6b1c56e2063dafddd14ecbb430b9a Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 14:29:59 +0200 Subject: [PATCH 1/4] Speed up todos queries by limiting the projects set we join with Closes #20828 --- CHANGELOG | 1 + app/finders/projects_finder.rb | 3 ++- app/finders/todos_finder.rb | 20 ++++++++++---------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 658bd92a824..c6c2220b133 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 2f0a9659d15..56877b6d75a 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -1,6 +1,7 @@ class ProjectsFinder < UnionFinder - def execute(current_user = nil, options = {}) + def execute(current_user = nil, options = {}, &block) segments = all_projects(current_user) + segments.map!(&block) if block find_union(segments, Project) end diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index ff866c2faa5..9b24a86e1c1 100644 --- a/app/finders/todos_finder.rb +++ b/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,13 +93,10 @@ class TodosFinder @project end - def projects - return @projects if defined?(@projects) - - if project? - @projects = project - else - @projects = ProjectsFinder.new.execute(current_user) + def projects(items) + item_project_ids = items.reorder(nil).select(:project_id) + ProjectsFinder.new.execute(current_user) do |relation| + relation.where(id: item_project_ids) end end @@ -136,8 +135,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 From 76db0dc115316e53ed7d2189cc8789f0a0cef3c2 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 15:52:58 +0200 Subject: [PATCH 2/4] Pass project IDs relation to ProjectsFinder instead of using a block --- app/finders/projects_finder.rb | 4 ++-- app/finders/todos_finder.rb | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 56877b6d75a..c7911736812 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -1,7 +1,7 @@ class ProjectsFinder < UnionFinder - def execute(current_user = nil, options = {}, &block) + def execute(current_user = nil, project_ids_relation = nil) segments = all_projects(current_user) - segments.map!(&block) if block + segments.map! { |s| s.where(id: project_ids_relation) } if project_ids_relation find_union(segments, Project) end diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 9b24a86e1c1..4fe0070552e 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -95,9 +95,7 @@ class TodosFinder def projects(items) item_project_ids = items.reorder(nil).select(:project_id) - ProjectsFinder.new.execute(current_user) do |relation| - relation.where(id: item_project_ids) - end + ProjectsFinder.new.execute(current_user, item_project_ids) end def type? From e93de6066b8a69bc741fcdf3ef3113dd9b187878 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 17:14:39 +0200 Subject: [PATCH 3/4] Fix ProjectsFinder spec Follow-up on 1003454c --- spec/finders/projects_finder_spec.rb | 69 +++++----------------------- 1 file changed, 12 insertions(+), 57 deletions(-) diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 0a1cc3b3df7..e4b4b2d9b20 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -23,71 +23,26 @@ describe ProjectsFinder do let(:finder) { described_class.new } - describe 'without a group' do - describe 'without a user' do - subject { finder.execute } + describe 'without a user' do + subject { finder.execute } - it { is_expected.to eq([public_project]) } - end - - describe 'with a user' do - subject { finder.execute(user) } - - describe 'without private projects' do - it { is_expected.to eq([public_project, internal_project]) } - end - - describe 'with private projects' do - before do - private_project.team.add_user(user, Gitlab::Access::MASTER) - end - - it do - is_expected.to eq([public_project, internal_project, - private_project]) - end - end - end + it { is_expected.to eq([public_project]) } end - describe 'with a group' do - describe 'without a user' do - subject { finder.execute(nil, group: group) } + describe 'with a user' do + subject { finder.execute(user) } - it { is_expected.to eq([public_project]) } + describe 'without private projects' do + it { is_expected.to eq([public_project, internal_project]) } end - describe 'with a user' do - subject { finder.execute(user, group: group) } - - describe 'without shared projects' do - it { is_expected.to eq([public_project, internal_project]) } + describe 'with private projects' do + before do + private_project.team.add_user(user, Gitlab::Access::MASTER) 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 - - 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 + it do + is_expected.to eq([public_project, internal_project, private_project]) end end end From be870760d5490a394e3f3e1b7b3fadb8424c38a8 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 17:30:44 +0200 Subject: [PATCH 4/4] Add a spec for ProjectsFinder project_ids_relation option Follow-up 1003454c --- spec/finders/projects_finder_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index e4b4b2d9b20..7a3a74335e8 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -46,5 +46,13 @@ describe ProjectsFinder do end end end + + describe 'with project_ids_relation' do + let(:project_ids_relation) { Project.where(id: internal_project.id) } + + subject { finder.execute(user, project_ids_relation) } + + it { is_expected.to eq([internal_project]) } + end end end