diff --git a/app/models/issue.rb b/app/models/issue.rb index 071ad50fddc..deab53d25e7 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -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) } diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b67797f5404..acf80addc6a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -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 diff --git a/app/models/todo.rb b/app/models/todo.rb index d9b86d941b6..2b0dee875a3 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -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 diff --git a/changelogs/unreleased/sh-optimize-todos-api.yml b/changelogs/unreleased/sh-optimize-todos-api.yml new file mode 100644 index 00000000000..936ac31b853 --- /dev/null +++ b/changelogs/unreleased/sh-optimize-todos-api.yml @@ -0,0 +1,5 @@ +--- +title: Significantly reduce N+1 queries in /api/v4/todos endpoint +merge_request: 25711 +author: +type: performance diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5176e9713c1..2cd0d93b205 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -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| diff --git a/lib/api/issues.rb b/lib/api/issues.rb index f43f4d961d6..7b8c070ad57 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -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 diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 44f1e81caf2..28072bdf586 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -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 diff --git a/lib/api/todos.rb b/lib/api/todos.rb index 64ac8ece56c..d2196f05173 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -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 diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index f121a1d3b78..9f0d5ad5d12 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -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