kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

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

Browse source 
By preloading associations and batching issuable metadata lookups,
we can significantly cut the number of SQL queries needed to load
the Todos API endpoint.

On GitLab.com, my own tests showed my user's SQL queries went
from 365 to under 60 SQL queries.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/40378
This commit is contained in:
Stan Hu 2019-03-02 09:31:36 -08:00
parent a592a78072
commit 062efe4f7a
9 changed files with 103 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
app/models/issue.rb
View file

@ -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) }

7
app/models/merge_request.rb
View file

@ -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

9
app/models/todo.rb
View file

@ -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

5
changelogs/unreleased/sh-optimize-todos-api.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Significantly reduce N+1 queries in /api/v4/todos endpoint
merge_request: 25711
author:
type: performance

3
lib/api/entities.rb
View file

@ -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|

2
lib/api/issues.rb
View file

@ -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

2
lib/api/merge_requests.rb
View file

@ -45,7 +45,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

32
lib/api/todos.rb
View file

@ -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

52
spec/requests/api/todos_spec.rb
View file

@ -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