Commit 3781cbe0 authored by Sean McGivern's avatar Sean McGivern

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
parents 5a75aa59 062efe4f
...@@ -64,6 +64,7 @@ class Issue < ActiveRecord::Base ...@@ -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 :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 :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 :public_only, -> { where(confidential: false) }
scope :confidential_only, -> { where(confidential: true) } scope :confidential_only, -> { where(confidential: true) }
......
...@@ -184,6 +184,13 @@ class MergeRequest < ActiveRecord::Base ...@@ -184,6 +184,13 @@ class MergeRequest < ActiveRecord::Base
scope :assigned, -> { where("assignee_id IS NOT NULL") } scope :assigned, -> { where("assignee_id IS NOT NULL") }
scope :unassigned, -> { where("assignee_id IS NULL") } scope :unassigned, -> { where("assignee_id IS NULL") }
scope :assigned_to, ->(u) { where(assignee_id: u.id)} 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 participant :assignee
......
...@@ -31,7 +31,13 @@ class Todo < ActiveRecord::Base ...@@ -31,7 +31,13 @@ class Todo < ActiveRecord::Base
belongs_to :note belongs_to :note
belongs_to :project belongs_to :project
belongs_to :group 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 belongs_to :user
delegate :name, :email, to: :author, prefix: true, allow_nil: true delegate :name, :email, to: :author, prefix: true, allow_nil: true
...@@ -52,6 +58,7 @@ class Todo < ActiveRecord::Base ...@@ -52,6 +58,7 @@ class Todo < ActiveRecord::Base
scope :for_type, -> (type) { where(target_type: type) } scope :for_type, -> (type) { where(target_type: type) }
scope :for_target, -> (id) { where(target_id: id) } scope :for_target, -> (id) { where(target_id: id) }
scope :for_commit, -> (id) { where(commit_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 state_machine :state, initial: :pending do
event :done do event :done do
......
---
title: Significantly reduce N+1 queries in /api/v4/todos endpoint
merge_request: 25711
author:
type: performance
...@@ -883,7 +883,8 @@ module API ...@@ -883,7 +883,8 @@ module API
expose :target_type expose :target_type
expose :target do |todo, options| 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 end
expose :target_url do |todo, options| expose :target_url do |todo, options|
......
...@@ -28,7 +28,7 @@ module API ...@@ -28,7 +28,7 @@ module API
args[:scope] = args[:scope].underscore if args[:scope] args[:scope] = args[:scope].underscore if args[:scope]
issues = IssuesFinder.new(current_user, args).execute 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) issues.reorder(order_options_with_tie_breaker)
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
......
...@@ -48,7 +48,7 @@ module API ...@@ -48,7 +48,7 @@ module API
return merge_requests if args[:view] == 'simple' return merge_requests if args[:view] == 'simple'
merge_requests merge_requests
.preload(:notes, :author, :assignee, :milestone, :latest_merge_request_diff, :labels, :timelogs, metrics: [:latest_closed_by, :merged_by]) .with_api_entity_associations
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
......
...@@ -6,6 +6,8 @@ module API ...@@ -6,6 +6,8 @@ module API
before { authenticate! } before { authenticate! }
helpers ::Gitlab::IssuableMetadata
ISSUABLE_TYPES = { ISSUABLE_TYPES = {
'merge_requests' => ->(iid) { find_merge_request_with_access(iid) }, 'merge_requests' => ->(iid) { find_merge_request_with_access(iid) },
'issues' => ->(iid) { find_project_issue(iid) } 'issues' => ->(iid) { find_project_issue(iid) }
...@@ -42,6 +44,30 @@ module API ...@@ -42,6 +44,30 @@ module API
def find_todos def find_todos
TodosFinder.new(current_user, params).execute TodosFinder.new(current_user, params).execute
end 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 end
desc 'Get a todo list' do desc 'Get a todo list' do
...@@ -51,7 +77,11 @@ module API ...@@ -51,7 +77,11 @@ module API
use :pagination use :pagination
end end
get do 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 end
desc 'Mark a todo as done' do desc 'Mark a todo as done' do
......
...@@ -8,10 +8,14 @@ describe API::Todos do ...@@ -8,10 +8,14 @@ describe API::Todos do
let(:author_2) { create(:user) } let(:author_2) { create(:user) }
let(:john_doe) { create(:user, username: 'john_doe') } let(:john_doe) { create(:user, username: 'john_doe') }
let(:merge_request) { create(:merge_request, source_project: project_1) } 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_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_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!(: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!(: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 before do
project_1.add_developer(john_doe) project_1.add_developer(john_doe)
...@@ -34,7 +38,7 @@ describe API::Todos do ...@@ -34,7 +38,7 @@ describe API::Todos do
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array 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]['id']).to eq(pending_3.id)
expect(json_response[0]['project']).to be_a Hash expect(json_response[0]['project']).to be_a Hash
expect(json_response[0]['author']).to be_a Hash expect(json_response[0]['author']).to be_a Hash
...@@ -45,6 +49,23 @@ describe API::Todos do ...@@ -45,6 +49,23 @@ describe API::Todos do
expect(json_response[0]['state']).to eq('pending') expect(json_response[0]['state']).to eq('pending')
expect(json_response[0]['action_name']).to eq('assigned') expect(json_response[0]['action_name']).to eq('assigned')
expect(json_response[0]['created_at']).to be_present 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 end
context 'and using the author filter' do context 'and using the author filter' do
...@@ -54,7 +75,7 @@ describe API::Todos do ...@@ -54,7 +75,7 @@ describe API::Todos do
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(2) expect(json_response.length).to eq(3)
end end
end end
...@@ -67,7 +88,7 @@ describe API::Todos do ...@@ -67,7 +88,7 @@ describe API::Todos do
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(1) expect(json_response.length).to eq(2)
end end
end end
...@@ -100,7 +121,7 @@ describe API::Todos do ...@@ -100,7 +121,7 @@ describe API::Todos do
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(2) expect(json_response.length).to eq(3)
end end
end end
...@@ -115,6 +136,27 @@ describe API::Todos do ...@@ -115,6 +136,27 @@ describe API::Todos do
end end
end 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 end
describe 'POST /todos/:id/mark_as_done' do describe 'POST /todos/:id/mark_as_done' do
...@@ -230,7 +272,7 @@ describe API::Todos do ...@@ -230,7 +272,7 @@ describe API::Todos do
context 'for a merge request' do context 'for a merge request' do
it_behaves_like 'an issuable', 'merge_requests' 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 end
end end
......
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