From 4d4ddb6004e6f7f56b337a49c6eedaad70d70862 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 7 Nov 2017 14:00:21 +0000 Subject: Fail when issuable_meta_data is called on an unlimited collection This method can be called with an array, or a relation: 1. Arrays always have a limited amount of values, so that's fine. 2. If the relation does not have a limit value applied, then we will load every single object in that collection, and prevent N+1 queries for the metadata for that. But that's wrong, because we should never call this without an explicit limit set. So we raise in that case, and this commit will see which specs fail. The only failing specs here were the issues API specs, and the specs for IssuableMetadata itself, and both have been addressed. --- changelogs/unreleased/fix-issues-api-list-performance.yml | 5 +++++ lib/api/issues.rb | 12 ++++++------ lib/gitlab/issuable_metadata.rb | 8 ++++++++ spec/lib/gitlab/issuable_metadata_spec.rb | 12 ++++++++---- 4 files changed, 27 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/fix-issues-api-list-performance.yml diff --git a/changelogs/unreleased/fix-issues-api-list-performance.yml b/changelogs/unreleased/fix-issues-api-list-performance.yml new file mode 100644 index 00000000000..9c180f4d55e --- /dev/null +++ b/changelogs/unreleased/fix-issues-api-list-performance.yml @@ -0,0 +1,5 @@ +--- +title: Speed up issues list APIs +merge_request: +author: +type: performance diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 0df41dcc903..74dfd9f96de 100644 --- a/lib/api/issues.rb +++ b/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 diff --git a/lib/gitlab/issuable_metadata.rb b/lib/gitlab/issuable_metadata.rb index 977c05910d3..0c9de72329c 100644 --- a/lib/gitlab/issuable_metadata.rb +++ b/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. diff --git a/spec/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb index 2455969a183..42635a68ee1 100644 --- a/spec/lib/gitlab/issuable_metadata_spec.rb +++ b/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) -- cgit v1.2.1