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

Reduce code duplication

Browse source 
Move logic of obtaining Issuable data to separate method
This commit is contained in:
Alexander Koval 2019-02-26 17:13:45 +00:00 committed by Kamil Trzciński
parent 3a29b6af82
commit 35c1092282
7 changed files with 51 additions and 58 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
app/models/concerns/issuable.rb
View file

@ -28,7 +28,7 @@ module Issuable
# This object is used to gather issuable meta data for displaying
# upvotes, downvotes, notes and closing merge requests count for issues and merge requests
# 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
cache_markdown_field :title, pipeline: :single_line
@ -36,8 +36,8 @@ module Issuable
redact_field :description
belongs_to :author, class_name: "User"
belongs_to :updated_by, class_name: "User"
belongs_to :author, class_name: 'User'
belongs_to :updated_by, class_name: 'User'
belongs_to :last_edited_by, class_name: 'User'
belongs_to :milestone

4
app/models/issue.rb
View file

@ -263,6 +263,10 @@ class Issue < ActiveRecord::Base
end
# rubocop: enable CodeReuse/ServiceClass
def merge_requests_count
merge_requests_closing_issues.count
end
private
def ensure_metrics

2
app/views/shared/_issuable_meta_data.html.haml
View file

@ -1,4 +1,4 @@
- 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]
- upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes
- issuable_url = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes')

5
changelogs/unreleased/56726-fix-n-1-in-issues-and-merge-requests-api.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Fix N+1 query in Issues and MergeRequest API when issuable_metadata is present
merge_request: 25042
author: Alex Koval
type: other

72
lib/api/entities.rb
View file

@ -468,11 +468,17 @@ module API
end
end
class ProjectEntity < Grape::Entity
class IssueableEntity < Grape::Entity
expose :id, :iid
expose(:project_id) { |entity| entity&.project.try(:id) }
expose :title, :description
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
class Diff < Grape::Entity
@ -515,57 +521,35 @@ module API
end
end
class IssueBasic < ProjectEntity
class IssueBasic < IssueableEntity
expose :closed_at
expose :closed_by, using: Entities::UserBasic
expose :labels do |issue, options|
expose :labels do |issue|
# Avoids an N+1 query since labels are preloaded
issue.labels.map(&:title).sort
end
expose :milestone, using: Entities::Milestone
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
end
expose :user_notes_count
expose :upvotes do |issue, options|
if options[:issuable_metadata]
# Avoids an N+1 query when metadata is included
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(:user_notes_count) { |issue, options| issuable_metadata(issue, options, :user_notes_count) }
expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count) }
expose(:upvotes) { |issue, options| issuable_metadata(issue, options, :upvotes) }
expose(:downvotes) { |issue, options| issuable_metadata(issue, options, :downvotes) }
expose :due_date
expose :confidential
expose :discussion_locked
expose :web_url do |issue, options|
expose :web_url do |issue|
Gitlab::UrlBuilder.build(issue)
end
expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |issue|
issue
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
class Issue < IssueBasic
@ -628,14 +612,14 @@ module API
end
end
class MergeRequestSimple < ProjectEntity
class MergeRequestSimple < IssueableEntity
expose :title
expose :web_url do |merge_request, options|
Gitlab::UrlBuilder.build(merge_request)
end
end
class MergeRequestBasic < ProjectEntity
class MergeRequestBasic < IssueableEntity
expose :merged_by, using: Entities::UserBasic do |merge_request, _options|
merge_request.metrics&.merged_by
end
@ -659,23 +643,12 @@ module API
MarkupHelper.markdown_field(entity, :description)
end
expose :target_branch, :source_branch
expose :upvotes do |merge_request, options|
if options[:issuable_metadata]
options[:issuable_metadata][merge_request.id].upvotes
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(:user_notes_count) { |merge_request, options| issuable_metadata(merge_request, options, :user_notes_count) }
expose(:upvotes) { |merge_request, options| issuable_metadata(merge_request, options, :upvotes) }
expose(:downvotes) { |merge_request, options| issuable_metadata(merge_request, options, :downvotes) }
expose :author, :assignee, using: Entities::UserBasic
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
merge_request.labels.map(&:title).sort
end
@ -693,7 +666,6 @@ module API
end
expose :diff_head_sha, as: :sha
expose :merge_commit_sha
expose :user_notes_count
expose :discussion_locked
expose :should_remove_source_branch?, as: :should_remove_source_branch
expose :force_remove_source_branch?, as: :force_remove_source_branch
@ -701,7 +673,7 @@ module API
# Deprecated
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)
end

8
spec/lib/gitlab/issuable_metadata_spec.rb
View file

@ -28,12 +28,12 @@ describe Gitlab::IssuableMetadata do
expect(data.count).to eq(2)
expect(data[issue.id].upvotes).to eq(1)
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[closed_issue.id].upvotes).to eq(0)
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)
end
end
@ -51,12 +51,12 @@ describe Gitlab::IssuableMetadata do
expect(data.count).to eq(2)
expect(data[merge_request.id].upvotes).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_closed.id].upvotes).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)
end
end

12
spec/requests/api/merge_requests_spec.rb
View file

@ -320,6 +320,18 @@ describe API::MergeRequests do
expect(json_response.first['title']).to eq merge_request_closed.title
expect(json_response.first['id']).to eq merge_request_closed.id
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
describe "GET /groups/:id/merge_requests" do