BigW Consortium Gitlab

Commit 4d4ddb60 by Sean McGivern

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.
parent dc1e6b43
---
title: Speed up issues list APIs
merge_request:
author:
type: performance
...@@ -68,7 +68,7 @@ module API ...@@ -68,7 +68,7 @@ module API
desc: 'Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all`' desc: 'Return issues for the given scope: `created-by-me`, `assigned-to-me` or `all`'
end end
get do get do
issues = find_issues issues = paginate(find_issues)
options = { options = {
with: Entities::IssueBasic, with: Entities::IssueBasic,
...@@ -76,7 +76,7 @@ module API ...@@ -76,7 +76,7 @@ module API
issuable_metadata: issuable_meta_data(issues, 'Issue') issuable_metadata: issuable_meta_data(issues, 'Issue')
} }
present paginate(issues), options present issues, options
end end
end end
...@@ -95,7 +95,7 @@ module API ...@@ -95,7 +95,7 @@ module API
get ":id/issues" do get ":id/issues" do
group = find_group!(params[:id]) group = find_group!(params[:id])
issues = find_issues(group_id: group.id) issues = paginate(find_issues(group_id: group.id))
options = { options = {
with: Entities::IssueBasic, with: Entities::IssueBasic,
...@@ -103,7 +103,7 @@ module API ...@@ -103,7 +103,7 @@ module API
issuable_metadata: issuable_meta_data(issues, 'Issue') issuable_metadata: issuable_meta_data(issues, 'Issue')
} }
present paginate(issues), options present issues, options
end end
end end
...@@ -124,7 +124,7 @@ module API ...@@ -124,7 +124,7 @@ module API
get ":id/issues" do get ":id/issues" do
project = find_project!(params[:id]) project = find_project!(params[:id])
issues = find_issues(project_id: project.id) issues = paginate(find_issues(project_id: project.id))
options = { options = {
with: Entities::IssueBasic, with: Entities::IssueBasic,
...@@ -133,7 +133,7 @@ module API ...@@ -133,7 +133,7 @@ module API
issuable_metadata: issuable_meta_data(issues, 'Issue') issuable_metadata: issuable_meta_data(issues, 'Issue')
} }
present paginate(issues), options present issues, options
end end
desc 'Get a single project issue' do desc 'Get a single project issue' do
......
module Gitlab module Gitlab
module IssuableMetadata module IssuableMetadata
def issuable_meta_data(issuable_collection, collection_type) 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 # map has to be used here since using pluck or select will
# throw an error when ordering issuables by priority which inserts # throw an error when ordering issuables by priority which inserts
# a new order into the collection. # a new order into the collection.
......
...@@ -10,6 +10,10 @@ describe Gitlab::IssuableMetadata do ...@@ -10,6 +10,10 @@ describe Gitlab::IssuableMetadata do
expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({}) expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({})
end 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 context 'issues' do
let!(:issue) { create(:issue, author: user, project: project) } let!(:issue) { create(:issue, author: user, project: project) }
let!(:closed_issue) { create(:issue, state: :closed, author: user, project: project) } let!(:closed_issue) { create(:issue, state: :closed, author: user, project: project) }
...@@ -19,7 +23,7 @@ describe Gitlab::IssuableMetadata do ...@@ -19,7 +23,7 @@ describe Gitlab::IssuableMetadata do
let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) } let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) }
it 'aggregates stats on issues' do 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.count).to eq(2)
expect(data[issue.id].upvotes).to eq(1) expect(data[issue.id].upvotes).to eq(1)
...@@ -42,7 +46,7 @@ describe Gitlab::IssuableMetadata do ...@@ -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") } 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 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.count).to eq(2)
expect(data[merge_request.id].upvotes).to eq(1) expect(data[merge_request.id].upvotes).to eq(1)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment