Merge branch 'fix-issues-api-list-performance' into 'master'

Fail when issuable_meta_data is called on an unlimited collection

Closes #39845

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

changelogs/unreleased/fix-issues-api-list-performance.yml

@@ -0,0 +1,5 @@
+---
+title: Speed up issues list APIs
+merge_request:
+author:
+type: performance

lib/api/issues.rb

@@ -68,7 +68,7 @@ module API
                          desc: 'Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all`'
       end
       get do
-        issues = find_issues
+        issues = paginate(find_issues)
 
         options = {
           with: Entities::IssueBasic,
@@ -76,7 +76,7 @@ module API
           issuable_metadata: issuable_meta_data(issues, 'Issue')
         }
 
-        present paginate(issues), options
+        present issues, options
       end
     end
 
@@ -95,7 +95,7 @@ module API
       get ":id/issues" do
         group = find_group!(params[:id])
 
-        issues = find_issues(group_id: group.id)
+        issues = paginate(find_issues(group_id: group.id))
 
         options = {
           with: Entities::IssueBasic,
@@ -103,7 +103,7 @@ module API
           issuable_metadata: issuable_meta_data(issues, 'Issue')
         }
 
-        present paginate(issues), options
+        present issues, options
       end
     end
 
@@ -124,7 +124,7 @@ module API
       get ":id/issues" do
         project = find_project!(params[:id])
 
-        issues = find_issues(project_id: project.id)
+        issues = paginate(find_issues(project_id: project.id))
 
         options = {
           with: Entities::IssueBasic,
@@ -133,7 +133,7 @@ module API
           issuable_metadata: issuable_meta_data(issues, 'Issue')
         }
 
-        present paginate(issues), options
+        present issues, options
       end
 
       desc 'Get a single project issue' do

lib/gitlab/issuable_metadata.rb

@@ -1,6 +1,14 @@
 module Gitlab
   module IssuableMetadata
     def issuable_meta_data(issuable_collection, collection_type)
+      # ActiveRecord uses Object#extend for null relations.
+      if !(issuable_collection.singleton_class < ActiveRecord::NullRelation) &&
+          issuable_collection.respond_to?(:limit_value) &&
+          issuable_collection.limit_value.nil?
+
+        raise 'Collection must have a limit applied for preloading meta-data'
+      end
+
       # map has to be used here since using pluck or select will
       # throw an error when ordering issuables by priority which inserts
       # a new order into the collection.

spec/lib/gitlab/issuable_metadata_spec.rb

@@ -1,8 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::IssuableMetadata do
-  let(:user)        { create(:user) }
-  let!(:project)    { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
+  let(:user)     { create(:user) }
+  let!(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
 
   subject { Class.new { include Gitlab::IssuableMetadata }.new }
 
@@ -10,6 +10,10 @@ describe Gitlab::IssuableMetadata do
     expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({})
   end
 
+  it 'raises an error when given a collection with no limit' do
+    expect { subject.issuable_meta_data(Issue.all, 'Issue') }.to raise_error(/must have a limit/)
+  end
+
   context 'issues' do
     let!(:issue) { create(:issue, author: user, project: project) }
     let!(:closed_issue) { create(:issue, state: :closed, author: user, project: project) }
@@ -19,7 +23,7 @@ describe Gitlab::IssuableMetadata do
     let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) }
 
     it 'aggregates stats on issues' do
-      data = subject.issuable_meta_data(Issue.all, 'Issue')
+      data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue')
 
       expect(data.count).to eq(2)
       expect(data[issue.id].upvotes).to eq(1)
@@ -42,7 +46,7 @@ describe Gitlab::IssuableMetadata do
     let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
 
     it 'aggregates stats on merge requests' do
-      data = subject.issuable_meta_data(MergeRequest.all, 'MergeRequest')
+      data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest')
 
       expect(data.count).to eq(2)
       expect(data[merge_request.id].upvotes).to eq(1)