Merge branch 'ajk-quality-triage-ops-532' into 'master'

Eliminate N+1 performance issues in MergeRequest.pipelines

See merge request gitlab-org/gitlab!47784

app/graphql/resolvers/merge_request_pipelines_resolver.rb

@@ -5,14 +5,32 @@ module Resolvers
   class MergeRequestPipelinesResolver < BaseResolver
     # The GraphQL type here gets defined in this include
     include ::ResolvesPipelines
+    include ::CachingArrayResolver
 
     alias_method :merge_request, :object
 
+    # Return at most 500 pipelines for each MR.
+    # Merge requests generally have many fewer pipelines than this.
+    def self.field_options
+      super.merge(max_page_size: 500)
+    end
+
     def resolve(**args)
       return unless project
 
-      resolve_pipelines(project, args)
-        .merge(merge_request.all_pipelines)
+      super
+    end
+
+    def query_for(args)
+      resolve_pipelines(project, args).merge(merge_request.all_pipelines)
+    end
+
+    def model_class
+      ::Ci::Pipeline
+    end
+
+    def query_input(**args)
+      args
     end
 
     def project

app/graphql/types/merge_request_type.rb

@@ -115,7 +115,7 @@ module Types
           description: 'The pipeline running on the branch HEAD of the merge request'
     field :pipelines,
           null: true,
-          description: 'Pipelines for the merge request',
+          description: 'Pipelines for the merge request. Note: for performance reasons, no more than the most recent 500 pipelines will be returned.',
           resolver: Resolvers::MergeRequestPipelinesResolver
 
     field :milestone, Types::MilestoneType, null: true,

changelogs/unreleased/ajk-quality-triage-ops-532.yml

+---
+title: Eliminate N+1 performance issues in MergeRequest.pipelines in GraphQL API
+merge_request: 47784
+author:
+type: fixed

doc/api/graphql/reference/gitlab_schema.graphql

@@ -13193,7 +13193,8 @@ type MergeRequest implements CurrentUserTodos & Noteable {
   ): UserConnection
 
   """
-  Pipelines for the merge request
+  Pipelines for the merge request. Note: for performance reasons, no more than
+  the most recent 500 pipelines will be returned.
   """
   pipelines(
     """

doc/api/graphql/reference/gitlab_schema.json

@@ -36406,7 +36406,7 @@
             },
             {
               "name": "pipelines",
-              "description": "Pipelines for the merge request",
+              "description": "Pipelines for the merge request. Note: for performance reasons, no more than the most recent 500 pipelines will be returned.",
               "args": [
                 {
                   "name": "status",

doc/api/graphql/reference/index.md

@@ -2050,7 +2050,7 @@ Autogenerated return type of MarkAsSpamSnippet.
 | `milestone` | Milestone | The milestone of the merge request |
 | `notes` | NoteConnection! | All notes on this noteable |
 | `participants` | UserConnection | Participants in the merge request |
-| `pipelines` | PipelineConnection | Pipelines for the merge request |
+| `pipelines` | PipelineConnection | Pipelines for the merge request. Note: for performance reasons, no more than the most recent 500 pipelines will be returned. |
 | `project` | Project! | Alias for target_project |
 | `projectId` | Int! | ID of the merge request project |
 | `rebaseCommitSha` | String | Rebase commit SHA of the merge request |

spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Resolvers::MergeRequestPipelinesResolver do
   end
 
   def resolve_pipelines
-    resolve(described_class, obj: merge_request, ctx: { current_user: current_user })
+    sync(resolve(described_class, obj: merge_request, ctx: { current_user: current_user }))
   end
 
   it 'resolves only MRs for the passed merge request' do

spec/graphql/types/merge_request_type_spec.rb

@@ -39,6 +39,12 @@ RSpec.describe GitlabSchema.types['MergeRequest'] do
     expect(described_class).to have_graphql_fields(*expected_fields)
   end
 
+  describe '#pipelines' do
+    subject { described_class.fields['pipelines'] }
+
+    it { is_expected.to have_attributes(max_page_size: 500) }
+  end
+
   describe '#diff_stats_summary' do
     subject { GitlabSchema.execute(query, context: { current_user: current_user }).as_json }
 

spec/requests/api/graphql/project/merge_request/pipelines_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Query.project.mergeRequests.pipelines' do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let_it_be(:author) { create(:user) }
+  let_it_be(:merge_requests) do
+    %i[with_diffs with_image_diffs conflict].map do |trait|
+      create(:merge_request, trait, author: author, source_project: project)
+    end
+  end
+
+  describe '.count' do
+    let(:query) do
+      <<~GQL
+      query($path: ID!, $first: Int) {
+        project(fullPath: $path) {
+          mergeRequests(first: $first) {
+            nodes {
+              iid
+              pipelines {
+                count
+              }
+            }
+          }
+        }
+      }
+      GQL
+    end
+
+    def run_query(first = nil)
+      post_graphql(query, current_user: author, variables: { path: project.full_path, first: first })
+    end
+
+    before do
+      merge_requests.each do |mr|
+        shas = mr.all_commits.limit(2).pluck(:sha)
+
+        shas.each do |sha|
+          create(:ci_pipeline, :success, project: project, ref: mr.source_branch, sha: sha)
+        end
+      end
+    end
+
+    it 'produces correct results' do
+      run_query(2)
+
+      p_nodes = graphql_data_at(:project, :merge_requests, :nodes)
+
+      expect(p_nodes).to all(match('iid' => be_present, 'pipelines' => match('count' => 2)))
+    end
+
+    it 'is scalable', :request_store, :use_clean_rails_memory_store_caching do
+      # warm up
+      run_query
+
+      expect { run_query(2) }.to(issue_same_number_of_queries_as { run_query(1) }.ignoring_cached_queries)
+    end
+  end
+end