From 35c10922820e3e07a5c571e3ad81e90045b2da2b Mon Sep 17 00:00:00 2001 From: Alexander Koval Date: Tue, 26 Feb 2019 17:13:45 +0000 Subject: [PATCH] Reduce code duplication Move logic of obtaining Issuable data to separate method --- app/models/concerns/issuable.rb | 6 +- app/models/issue.rb | 4 ++ .../shared/_issuable_meta_data.html.haml | 2 +- ...x-n-1-in-issues-and-merge-requests-api.yml | 5 ++ lib/api/entities.rb | 72 ++++++------------- spec/lib/gitlab/issuable_metadata_spec.rb | 8 +-- spec/requests/api/merge_requests_spec.rb | 12 ++++ 7 files changed, 51 insertions(+), 58 deletions(-) create mode 100644 changelogs/unreleased/56726-fix-n-1-in-issues-and-merge-requests-api.yml diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 429a63f83cc..670103bc3f3 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -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 diff --git a/app/models/issue.rb b/app/models/issue.rb index 0b46e949052..071ad50fddc 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -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 diff --git a/app/views/shared/_issuable_meta_data.html.haml b/app/views/shared/_issuable_meta_data.html.haml index 6cc8c485666..31a5370a5f8 100644 --- a/app/views/shared/_issuable_meta_data.html.haml +++ b/app/views/shared/_issuable_meta_data.html.haml @@ -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') diff --git a/changelogs/unreleased/56726-fix-n-1-in-issues-and-merge-requests-api.yml b/changelogs/unreleased/56726-fix-n-1-in-issues-and-merge-requests-api.yml new file mode 100644 index 00000000000..3eb9e484647 --- /dev/null +++ b/changelogs/unreleased/56726-fix-n-1-in-issues-and-merge-requests-api.yml @@ -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 diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 7c035990fb0..237705a63d0 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -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 diff --git a/spec/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb index 42635a68ee1..6ec86163233 100644 --- a/spec/lib/gitlab/issuable_metadata_spec.rb +++ b/spec/lib/gitlab/issuable_metadata_spec.rb @@ -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 diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index b4cd3130dc5..edeb52adbb2 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -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