Commit 1874aa9a authored by Alexander Koval's avatar Alexander Koval Committed by Kamil Trzciński

Reduce code duplication

Move logic of obtaining Issuable data to separate method
parent d2336e02
...@@ -28,7 +28,7 @@ module Issuable ...@@ -28,7 +28,7 @@ module Issuable
# This object is used to gather issuable meta data for displaying # This object is used to gather issuable meta data for displaying
# upvotes, downvotes, notes and closing merge requests count for issues and merge requests # upvotes, downvotes, notes and closing merge requests count for issues and merge requests
# lists avoiding n+1 queries and improving performance. # lists avoiding n+1 queries and improving performance.
IssuableMeta = Struct.new(:upvotes, :downvotes, :notes_count, :merge_requests_count) IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :merge_requests_count)
included do included do
cache_markdown_field :title, pipeline: :single_line cache_markdown_field :title, pipeline: :single_line
...@@ -36,8 +36,8 @@ module Issuable ...@@ -36,8 +36,8 @@ module Issuable
redact_field :description redact_field :description
belongs_to :author, class_name: "User" belongs_to :author, class_name: 'User'
belongs_to :updated_by, class_name: "User" belongs_to :updated_by, class_name: 'User'
belongs_to :last_edited_by, class_name: 'User' belongs_to :last_edited_by, class_name: 'User'
belongs_to :milestone belongs_to :milestone
......
...@@ -263,6 +263,10 @@ class Issue < ActiveRecord::Base ...@@ -263,6 +263,10 @@ class Issue < ActiveRecord::Base
end end
# rubocop: enable CodeReuse/ServiceClass # rubocop: enable CodeReuse/ServiceClass
def merge_requests_count
merge_requests_closing_issues.count
end
private private
def ensure_metrics def ensure_metrics
......
- note_count = @issuable_meta_data[issuable.id].notes_count - note_count = @issuable_meta_data[issuable.id].user_notes_count
- issue_votes = @issuable_meta_data[issuable.id] - issue_votes = @issuable_meta_data[issuable.id]
- upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes - upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes
- issuable_url = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes') - issuable_url = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes')
......
---
title: Fix N+1 query in Issues and MergeRequest API when issuable_metadata is present
merge_request: 25042
author: Alex Koval
type: other
...@@ -473,6 +473,12 @@ module API ...@@ -473,6 +473,12 @@ module API
expose(:project_id) { |entity| entity&.project.try(:id) } expose(:project_id) { |entity| entity&.project.try(:id) }
expose :title, :description expose :title, :description
expose :state, :created_at, :updated_at expose :state, :created_at, :updated_at
# Avoids an N+1 query when metadata is included
def issuable_metadata(subject, options, method)
cached_subject = options.dig(:issuable_metadata, subject.id)
(cached_subject || subject).public_send(method) # rubocop: disable GitlabSecurity/PublicSend
end
end end
class Diff < Grape::Entity class Diff < Grape::Entity
...@@ -518,54 +524,32 @@ module API ...@@ -518,54 +524,32 @@ module API
class IssueBasic < ProjectEntity class IssueBasic < ProjectEntity
expose :closed_at expose :closed_at
expose :closed_by, using: Entities::UserBasic expose :closed_by, using: Entities::UserBasic
expose :labels do |issue, options| expose :labels do |issue|
# Avoids an N+1 query since labels are preloaded # Avoids an N+1 query since labels are preloaded
issue.labels.map(&:title).sort issue.labels.map(&:title).sort
end end
expose :milestone, using: Entities::Milestone expose :milestone, using: Entities::Milestone
expose :assignees, :author, using: Entities::UserBasic expose :assignees, :author, using: Entities::UserBasic
expose :assignee, using: ::API::Entities::UserBasic do |issue, options| expose :assignee, using: ::API::Entities::UserBasic do |issue|
issue.assignees.first issue.assignees.first
end end
expose :user_notes_count expose(:user_notes_count) { |issue, options| issuable_metadata(issue, options, :user_notes_count) }
expose :upvotes do |issue, options| expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count) }
if options[:issuable_metadata] expose(:upvotes) { |issue, options| issuable_metadata(issue, options, :upvotes) }
# Avoids an N+1 query when metadata is included expose(:downvotes) { |issue, options| issuable_metadata(issue, options, :downvotes) }
options[:issuable_metadata][issue.id].upvotes
else
issue.upvotes
end
end
expose :downvotes do |issue, options|
if options[:issuable_metadata]
# Avoids an N+1 query when metadata is included
options[:issuable_metadata][issue.id].downvotes
else
issue.downvotes
end
end
expose :due_date expose :due_date
expose :confidential expose :confidential
expose :discussion_locked expose :discussion_locked
expose :web_url do |issue, options| expose :web_url do |issue|
Gitlab::UrlBuilder.build(issue) Gitlab::UrlBuilder.build(issue)
end end
expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |issue| expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |issue|
issue issue
end end
expose :merge_requests_count do |issue, options|
if options[:issuable_metadata]
# Avoids an N+1 query when metadata is included
options[:issuable_metadata][issue.id].merge_requests_count
else
issue.merge_requests_closing_issues.count
end
end
end end
class Issue < IssueBasic class Issue < IssueBasic
...@@ -659,23 +643,12 @@ module API ...@@ -659,23 +643,12 @@ module API
MarkupHelper.markdown_field(entity, :description) MarkupHelper.markdown_field(entity, :description)
end end
expose :target_branch, :source_branch expose :target_branch, :source_branch
expose :upvotes do |merge_request, options| expose(:user_notes_count) { |merge_request, options| issuable_metadata(merge_request, options, :user_notes_count) }
if options[:issuable_metadata] expose(:upvotes) { |merge_request, options| issuable_metadata(merge_request, options, :upvotes) }
options[:issuable_metadata][merge_request.id].upvotes expose(:downvotes) { |merge_request, options| issuable_metadata(merge_request, options, :downvotes) }
else
merge_request.upvotes
end
end
expose :downvotes do |merge_request, options|
if options[:issuable_metadata]
options[:issuable_metadata][merge_request.id].downvotes
else
merge_request.downvotes
end
end
expose :author, :assignee, using: Entities::UserBasic expose :author, :assignee, using: Entities::UserBasic
expose :source_project_id, :target_project_id expose :source_project_id, :target_project_id
expose :labels do |merge_request, options| expose :labels do |merge_request|
# Avoids an N+1 query since labels are preloaded # Avoids an N+1 query since labels are preloaded
merge_request.labels.map(&:title).sort merge_request.labels.map(&:title).sort
end end
...@@ -693,7 +666,6 @@ module API ...@@ -693,7 +666,6 @@ module API
end end
expose :diff_head_sha, as: :sha expose :diff_head_sha, as: :sha
expose :merge_commit_sha expose :merge_commit_sha
expose :user_notes_count
expose :discussion_locked expose :discussion_locked
expose :should_remove_source_branch?, as: :should_remove_source_branch expose :should_remove_source_branch?, as: :should_remove_source_branch
expose :force_remove_source_branch?, as: :force_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch
...@@ -701,7 +673,7 @@ module API ...@@ -701,7 +673,7 @@ module API
# Deprecated # Deprecated
expose :allow_collaboration, as: :allow_maintainer_to_push, if: -> (merge_request, _) { merge_request.for_fork? } expose :allow_collaboration, as: :allow_maintainer_to_push, if: -> (merge_request, _) { merge_request.for_fork? }
expose :web_url do |merge_request, options| expose :web_url do |merge_request|
Gitlab::UrlBuilder.build(merge_request) Gitlab::UrlBuilder.build(merge_request)
end end
......
...@@ -28,12 +28,12 @@ describe Gitlab::IssuableMetadata do ...@@ -28,12 +28,12 @@ describe Gitlab::IssuableMetadata do
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)
expect(data[issue.id].downvotes).to eq(0) expect(data[issue.id].downvotes).to eq(0)
expect(data[issue.id].notes_count).to eq(0) expect(data[issue.id].user_notes_count).to eq(0)
expect(data[issue.id].merge_requests_count).to eq(1) expect(data[issue.id].merge_requests_count).to eq(1)
expect(data[closed_issue.id].upvotes).to eq(0) expect(data[closed_issue.id].upvotes).to eq(0)
expect(data[closed_issue.id].downvotes).to eq(1) expect(data[closed_issue.id].downvotes).to eq(1)
expect(data[closed_issue.id].notes_count).to eq(0) expect(data[closed_issue.id].user_notes_count).to eq(0)
expect(data[closed_issue.id].merge_requests_count).to eq(0) expect(data[closed_issue.id].merge_requests_count).to eq(0)
end end
end end
...@@ -51,12 +51,12 @@ describe Gitlab::IssuableMetadata do ...@@ -51,12 +51,12 @@ describe Gitlab::IssuableMetadata do
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)
expect(data[merge_request.id].downvotes).to eq(1) expect(data[merge_request.id].downvotes).to eq(1)
expect(data[merge_request.id].notes_count).to eq(1) expect(data[merge_request.id].user_notes_count).to eq(1)
expect(data[merge_request.id].merge_requests_count).to eq(0) expect(data[merge_request.id].merge_requests_count).to eq(0)
expect(data[merge_request_closed.id].upvotes).to eq(0) expect(data[merge_request_closed.id].upvotes).to eq(0)
expect(data[merge_request_closed.id].downvotes).to eq(0) expect(data[merge_request_closed.id].downvotes).to eq(0)
expect(data[merge_request_closed.id].notes_count).to eq(0) expect(data[merge_request_closed.id].user_notes_count).to eq(0)
expect(data[merge_request_closed.id].merge_requests_count).to eq(0) expect(data[merge_request_closed.id].merge_requests_count).to eq(0)
end end
end end
......
...@@ -320,6 +320,18 @@ describe API::MergeRequests do ...@@ -320,6 +320,18 @@ describe API::MergeRequests do
expect(json_response.first['title']).to eq merge_request_closed.title expect(json_response.first['title']).to eq merge_request_closed.title
expect(json_response.first['id']).to eq merge_request_closed.id expect(json_response.first['id']).to eq merge_request_closed.id
end end
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new do
get api("/projects/#{project.id}/merge_requests", user)
end.count
create(:merge_request, author: user, assignee: user, source_project: project, target_project: project, created_at: base_time)
expect do
get api("/projects/#{project.id}/merge_requests", user)
end.not_to exceed_query_limit(control)
end
end end
describe "GET /groups/:id/merge_requests" do describe "GET /groups/:id/merge_requests" do
......
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