Merge branch '56726-fix-n+1-in-issues-and-merge-requests-api' into 'master'

Fix N+1 query in Issues and MergeRequest API when issuable_metadata presense

See merge request gitlab-org/gitlab-ce!25042

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

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

app/views/shared/_issuable_meta_data.html.haml

-- 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')

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

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

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

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

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