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

Significantly reduce N+1 queries in /api/v4/todos endpoint

Closes #40378

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

9 files changed, 103 insertions, 10 deletions

diff --git a/app/models/issue.rb b/app/models/issue.rb
index 071ad50fddc..deab53d25e7 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -64,6 +64,7 @@ class Issue < ActiveRecord::Base
   scope :order_closest_future_date, -> { reorder('CASE WHEN issues.due_date >= CURRENT_DATE THEN 0 ELSE 1 END ASC, ABS(CURRENT_DATE - issues.due_date) ASC') }
 
   scope :preload_associations, -> { preload(:labels, project: :namespace) }
+  scope :with_api_entity_associations, -> { preload(:timelogs, :assignees, :author, :notes, :labels, project: [:route, { namespace: :route }] ) }
 
   scope :public_only, -> { where(confidential: false) }
   scope :confidential_only, -> { where(confidential: true) }
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b67797f5404..acf80addc6a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -184,6 +184,13 @@ class MergeRequest < ActiveRecord::Base
   scope :assigned, -> { where("assignee_id IS NOT NULL") }
   scope :unassigned, -> { where("assignee_id IS NULL") }
   scope :assigned_to, ->(u) { where(assignee_id: u.id)}
+  scope :with_api_entity_associations, -> {
+    preload(:author, :assignee, :notes, :labels, :milestone, :timelogs,
+            latest_merge_request_diff: [:merge_request_diff_commits],
+            metrics: [:latest_closed_by, :merged_by],
+            target_project: [:route, { namespace: :route }],
+            source_project: [:route, { namespace: :route }])
+  }
 
   participant :assignee
 
diff --git a/app/models/todo.rb b/app/models/todo.rb
index d9b86d941b6..2b0dee875a3 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -31,7 +31,13 @@ class Todo < ActiveRecord::Base
   belongs_to :note
   belongs_to :project
   belongs_to :group
-  belongs_to :target, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
+  belongs_to :target, -> {
+    if self.klass.respond_to?(:with_api_entity_associations)
+      self.with_api_entity_associations
+    else
+      self
+    end
+  }, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
   belongs_to :user
 
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
@@ -52,6 +58,7 @@ class Todo < ActiveRecord::Base
   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 }]) }
 
   state_machine :state, initial: :pending do
     event :done do
diff --git a/changelogs/unreleased/sh-optimize-todos-api.yml b/changelogs/unreleased/sh-optimize-todos-api.yml
new file mode 100644
index 00000000000..936ac31b853
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-todos-api.yml
@@ -0,0 +1,5 @@
+---
+title: Significantly reduce N+1 queries in /api/v4/todos endpoint
+merge_request: 25711
+author:
+type: performance
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 5176e9713c1..2cd0d93b205 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -883,7 +883,8 @@ module API
       expose :target_type
 
       expose :target do |todo, options|
-        todo_target_class(todo.target_type).represent(todo.target, options)
+        todo_options = options.fetch(todo.target_type, {})
+        todo_target_class(todo.target_type).represent(todo.target, todo_options)
       end
 
       expose :target_url do |todo, options|
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index d59d2f5a098..b2ec4ed898e 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -28,7 +28,7 @@ module API
         args[:scope] = args[:scope].underscore if args[:scope]
 
         issues = IssuesFinder.new(current_user, args).execute
-          .preload(:assignees, :labels, :notes, :timelogs, :project, :author, :closed_by)
+                   .with_api_entity_associations
         issues.reorder(order_options_with_tie_breaker)
       end
       # rubocop: enable CodeReuse/ActiveRecord
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 6518ebbcff5..98dcc388f44 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -48,7 +48,7 @@ module API
         return merge_requests if args[:view] == 'simple'
 
         merge_requests
-          .preload(:notes, :author, :assignee, :milestone, :latest_merge_request_diff, :labels, :timelogs, metrics: [:latest_closed_by, :merged_by])
+          .with_api_entity_associations
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index 64ac8ece56c..d2196f05173 100644
--- a/lib/api/todos.rb
+++ b/lib/api/todos.rb
@@ -6,6 +6,8 @@ module API
 
     before { authenticate! }
 
+    helpers ::Gitlab::IssuableMetadata
+
     ISSUABLE_TYPES = {
       'merge_requests' => ->(iid) { find_merge_request_with_access(iid) },
       'issues' => ->(iid) { find_project_issue(iid) }
@@ -42,6 +44,30 @@ module API
         def find_todos
           TodosFinder.new(current_user, params).execute
         end
+
+        def issuable_and_awardable?(type)
+          obj_type = Object.const_get(type)
+
+          (obj_type < Issuable) && (obj_type < Awardable)
+        rescue NameError
+          false
+        end
+
+        def batch_load_issuable_metadata(todos, options)
+          # This should be paginated and will cause Rails to SELECT for all the Todos
+          todos_by_type = todos.group_by(&:target_type)
+
+          todos_by_type.keys.each do |type|
+            next unless issuable_and_awardable?(type)
+
+            collection = todos_by_type[type]
+
+            next unless collection
+
+            targets = collection.map(&:target)
+            options[type] = { issuable_metadata: issuable_meta_data(targets, type) }
+          end
+        end
       end
 
       desc 'Get a todo list' do
@@ -51,7 +77,11 @@ module API
         use :pagination
       end
       get do
-        present paginate(find_todos), with: Entities::Todo, current_user: current_user
+        todos = paginate(find_todos.with_api_entity_associations)
+        options = { with: Entities::Todo, current_user: current_user }
+        batch_load_issuable_metadata(todos, options)
+
+        present todos, options
       end
 
       desc 'Mark a todo as done' do
diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb
index f121a1d3b78..9f0d5ad5d12 100644
--- a/spec/requests/api/todos_spec.rb
+++ b/spec/requests/api/todos_spec.rb
@@ -8,10 +8,14 @@ describe API::Todos do
   let(:author_2) { create(:user) }
   let(:john_doe) { create(:user, username: 'john_doe') }
   let(:merge_request) { create(:merge_request, source_project: project_1) }
+  let!(:merge_request_todo) { create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request) }
   let!(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe) }
   let!(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe) }
   let!(:pending_3) { create(:on_commit_todo, project: project_1, author: author_2, user: john_doe) }
   let!(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe) }
+  let!(:award_emoji_1) { create(:award_emoji, awardable: merge_request, user: author_1, name: 'thumbsup') }
+  let!(:award_emoji_2) { create(:award_emoji, awardable: pending_1.target, user: author_1, name: 'thumbsup') }
+  let!(:award_emoji_3) { create(:award_emoji, awardable: pending_2.target, user: author_2, name: 'thumbsdown') }
 
   before do
     project_1.add_developer(john_doe)
@@ -34,7 +38,7 @@ describe API::Todos do
         expect(response.status).to eq(200)
         expect(response).to include_pagination_headers
         expect(json_response).to be_an Array
-        expect(json_response.length).to eq(3)
+        expect(json_response.length).to eq(4)
         expect(json_response[0]['id']).to eq(pending_3.id)
         expect(json_response[0]['project']).to be_a Hash
         expect(json_response[0]['author']).to be_a Hash
@@ -45,6 +49,23 @@ describe API::Todos do
         expect(json_response[0]['state']).to eq('pending')
         expect(json_response[0]['action_name']).to eq('assigned')
         expect(json_response[0]['created_at']).to be_present
+        expect(json_response[0]['target_type']).to eq('Commit')
+
+        expect(json_response[1]['target_type']).to eq('Issue')
+        expect(json_response[1]['target']['upvotes']).to eq(0)
+        expect(json_response[1]['target']['downvotes']).to eq(1)
+        expect(json_response[1]['target']['merge_requests_count']).to eq(0)
+
+        expect(json_response[2]['target_type']).to eq('Issue')
+        expect(json_response[2]['target']['upvotes']).to eq(1)
+        expect(json_response[2]['target']['downvotes']).to eq(0)
+        expect(json_response[2]['target']['merge_requests_count']).to eq(0)
+
+        expect(json_response[3]['target_type']).to eq('MergeRequest')
+        # Only issues get a merge request count at the moment
+        expect(json_response[3]['target']['merge_requests_count']).to be_nil
+        expect(json_response[3]['target']['upvotes']).to eq(1)
+        expect(json_response[3]['target']['downvotes']).to eq(0)
       end
 
       context 'and using the author filter' do
@@ -54,7 +75,7 @@ describe API::Todos do
           expect(response.status).to eq(200)
           expect(response).to include_pagination_headers
           expect(json_response).to be_an Array
-          expect(json_response.length).to eq(2)
+          expect(json_response.length).to eq(3)
         end
       end
 
@@ -67,7 +88,7 @@ describe API::Todos do
           expect(response.status).to eq(200)
           expect(response).to include_pagination_headers
           expect(json_response).to be_an Array
-          expect(json_response.length).to eq(1)
+          expect(json_response.length).to eq(2)
         end
       end
 
@@ -100,7 +121,7 @@ describe API::Todos do
           expect(response.status).to eq(200)
           expect(response).to include_pagination_headers
           expect(json_response).to be_an Array
-          expect(json_response.length).to eq(2)
+          expect(json_response.length).to eq(3)
         end
       end
 
@@ -115,6 +136,27 @@ describe API::Todos do
         end
       end
     end
+
+    it 'avoids N+1 queries', :request_store do
+      create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request)
+
+      get api('/todos', john_doe)
+
+      control = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) }
+
+      merge_request_2 = create(:merge_request, source_project: project_2)
+      create(:todo, project: project_2, author: author_2, user: john_doe, target: merge_request_2)
+
+      project_3 = create(:project, :repository)
+      project_3.add_developer(john_doe)
+      merge_request_3 = create(:merge_request, source_project: project_3)
+      create(:todo, project: project_3, author: author_2, user: john_doe, target: merge_request_3)
+      create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe)
+      create(:on_commit_todo, project: project_3, author: author_1, user: john_doe)
+
+      expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control)
+      expect(response.status).to eq(200)
+    end
   end
 
   describe 'POST /todos/:id/mark_as_done' do
@@ -230,7 +272,7 @@ describe API::Todos do
 
     context 'for a merge request' do
       it_behaves_like 'an issuable', 'merge_requests' do
-        let(:issuable) { merge_request }
+        let(:issuable) { create(:merge_request, :simple, source_project: project_1) }
       end
     end
   end