Merge branch 'ajk-gql-ci-performance-improvements' into 'master'

GraphQL CI performance improvements

See merge request gitlab-org/gitlab!50386

app/graphql/types/ci/stage_type.rb

@@ -16,6 +16,26 @@ module Types
       def detailed_status
         object.detailed_status(context[:current_user])
       end
+
+      # Issues one query per pipeline
+      def groups
+        BatchLoader::GraphQL.for([object.pipeline, object]).batch(default_value: []) do |keys, loader|
+          by_pipeline = keys.group_by(&:first)
+
+          by_pipeline.each do |pl, key_group|
+            project = pl.project
+            stages = key_group.map(&:second).uniq
+            indexed = stages.index_by(&:id)
+            results = pl.latest_statuses.where(stage_id: stages.map(&:id)) # rubocop: disable CodeReuse/ActiveRecord
+
+            results.group_by(&:stage_id).each do |stage_id, statuses|
+              stage = indexed[stage_id]
+              groups = ::Ci::Group.fabricate(project, stage, statuses)
+              loader.call([pl, stage], groups)
+            end
+          end
+        end
+      end
     end
   end
 end

app/models/ci/group.rb

@@ -39,8 +39,13 @@ module Ci
       end
     end
 
-    def self.fabricate(project, stage)
-      stage.latest_statuses
+    # Construct a grouping of statuses for this stage.
+    # We allow the caller to pass in statuses for efficiency (avoiding N+1
+    # queries).
+    def self.fabricate(project, stage, statuses = nil)
+      statuses ||= stage.latest_statuses
+
+      statuses
         .sort_by(&:sortable_name).group_by(&:group_name)
         .map do |group_name, grouped_statuses|
           self.new(project, stage, name: group_name, jobs: grouped_statuses)

changelogs/unreleased/ajk-gql-ci-performance-improvements.yml

+---
+title: Performance improvements for CI GraphQL resources
+merge_request: 50386
+author:
+type: performance

spec/graphql/resolvers/ci/jobs_resolver_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe Resolvers::Ci::JobsResolver do
   include GraphqlHelpers
 
-  let_it_be(:pipeline) { create(:ci_pipeline) }
+  let_it_be(:project) { create(:project, :repository, :public) }
+  let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
 
   before_all do
     create(:ci_build, name: 'Normal job', pipeline: pipeline)

spec/requests/api/graphql/ci/jobs_spec.rb

@@ -7,27 +7,33 @@ RSpec.describe 'Query.project.pipeline' do
   let_it_be(:project) { create(:project, :repository, :public) }
   let_it_be(:user) { create(:user) }
 
-  def first(field)
-    [field.pluralize, 'nodes', 0]
+  def all(*fields)
+    fields.flat_map { |f| [f, :nodes] }
   end
 
   describe '.stages.groups.jobs' do
     let(:pipeline) do
       pipeline = create(:ci_pipeline, project: project, user: user)
-      stage = create(:ci_stage_entity, pipeline: pipeline, name: 'first')
+      stage = create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'first')
       create(:commit_status, stage_id: stage.id, pipeline: pipeline, name: 'my test job')
 
       pipeline
     end
 
-    let(:jobs_graphql_data) { graphql_data.dig(*%w[project pipeline], *first('stage'), *first('group'), 'jobs', 'nodes') }
+    let(:jobs_graphql_data) { graphql_data_at(:project, :pipeline, *all(:stages, :groups, :jobs)) }
+
+    let(:first_n) { var('Int') }
 
     let(:query) do
-      %(
-        query {
-          project(fullPath: "#{project.full_path}") {
-            pipeline(iid: "#{pipeline.iid}") {
-              stages {
+      with_signature([first_n], wrap_fields(query_graphql_path([
+        [:project,  { full_path: project.full_path }],
+        [:pipeline, { iid: pipeline.iid.to_s }],
+        [:stages,   { first: first_n }]
+      ], stage_fields)))
+    end
+
+    let(:stage_fields) do
+      <<~FIELDS
       nodes {
         name
         groups {
@@ -44,11 +50,7 @@ RSpec.describe 'Query.project.pipeline' do
           }
         }
       }
-              }
-            }
-          }
-        }
-      )
+      FIELDS
     end
 
     it 'returns the jobs of a pipeline stage' do
@@ -57,60 +59,43 @@ RSpec.describe 'Query.project.pipeline' do
       expect(jobs_graphql_data).to contain_exactly(a_hash_including('name' => 'my test job'))
     end
 
-    it 'avoids N+1 queries', :aggregate_failures do
-      control_count = ActiveRecord::QueryRecorder.new do
-        post_graphql(query, current_user: user)
-      end
-
-      build_stage = create(:ci_stage_entity, name: 'build', pipeline: pipeline)
-      test_stage = create(:ci_stage_entity, name: 'test', pipeline: pipeline)
+    describe 'performance' do
+      before do
+        build_stage = create(:ci_stage_entity, position: 2, name: 'build', project: project, pipeline: pipeline)
+        test_stage = create(:ci_stage_entity, position: 3, name: 'test', project: project, pipeline: pipeline)
         create(:commit_status, pipeline: pipeline, stage_id: build_stage.id, name: 'docker 1 2')
         create(:commit_status, pipeline: pipeline, stage_id: build_stage.id, name: 'docker 2 2')
         create(:commit_status, pipeline: pipeline, stage_id: test_stage.id, name: 'rspec 1 2')
         create(:commit_status, pipeline: pipeline, stage_id: test_stage.id, name: 'rspec 2 2')
+      end
 
-      expect do
-        post_graphql(query, current_user: user)
-      end.not_to exceed_query_limit(control_count)
-
-      expect(response).to have_gitlab_http_status(:ok)
+      it 'can find the first stage' do
+        post_graphql(query, current_user: user, variables: first_n.with(1))
 
-      build_stage = graphql_data.dig('project', 'pipeline', 'stages', 'nodes').find do |stage|
-        stage['name'] == 'build'
-      end
-      test_stage = graphql_data.dig('project', 'pipeline', 'stages', 'nodes').find do |stage|
-        stage['name'] == 'test'
+        expect(jobs_graphql_data).to contain_exactly(a_hash_including('name' => 'my test job'))
       end
-      docker_group = build_stage.dig('groups', 'nodes').first
-      rspec_group = test_stage.dig('groups', 'nodes').first
 
-      expect(docker_group['name']).to eq('docker')
-      expect(rspec_group['name']).to eq('rspec')
+      it 'can find all stages' do
+        post_graphql(query, current_user: user, variables: first_n.with(3))
+
+        expect(jobs_graphql_data).to contain_exactly(
+          a_hash_including('name' => 'my test job'),
+          a_hash_including('name' => 'docker 1 2'),
+          a_hash_including('name' => 'docker 2 2'),
+          a_hash_including('name' => 'rspec 1 2'),
+          a_hash_including('name' => 'rspec 2 2')
+        )
+      end
 
-      docker_jobs = docker_group.dig('jobs', 'nodes')
-      rspec_jobs = rspec_group.dig('jobs', 'nodes')
+      it 'avoids N+1 queries' do
+        control_count = ActiveRecord::QueryRecorder.new do
+          post_graphql(query, current_user: user, variables: first_n.with(1))
+        end
 
-      expect(docker_jobs).to eq([
-        {
-          'name' => 'docker 1 2',
-          'pipeline' => { 'id' => pipeline.to_global_id.to_s }
-        },
-        {
-          'name' => 'docker 2 2',
-          'pipeline' => { 'id' => pipeline.to_global_id.to_s }
-        }
-      ])
-
-      expect(rspec_jobs).to eq([
-        {
-          'name' => 'rspec 1 2',
-          'pipeline' => { 'id' => pipeline.to_global_id.to_s }
-        },
-        {
-          'name' => 'rspec 2 2',
-          'pipeline' => { 'id' => pipeline.to_global_id.to_s }
-        }
-      ])
+        expect do
+          post_graphql(query, current_user: user, variables: first_n.with(3))
+        end.not_to exceed_query_limit(control_count)
+      end
     end
   end
 

spec/requests/api/graphql/ci/pipelines_spec.rb

@@ -6,53 +6,59 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
   include GraphqlHelpers
 
   let_it_be(:project) { create(:project, :repository, :public) }
-  let_it_be(:first_user) { create(:user) }
-  let_it_be(:second_user) { create(:user) }
+  let_it_be(:user) { create(:user) }
 
   describe '.jobs' do
-    let_it_be(:query) do
-      %(
-        query {
-          project(fullPath: "#{project.full_path}") {
-            pipelines {
-              nodes {
-                jobs {
-                  nodes {
-                    name
-                  }
-                }
-              }
-            }
-          }
-        }
-      )
+    let(:first_n) { var('Int') }
+    let(:query_path) do
+      [
+        [:project, { full_path: project.full_path }],
+        [:pipelines, { first: first_n }],
+        [:nodes],
+        [:jobs],
+        [:nodes]
+      ]
     end
 
-    it 'fetches the jobs without an N+1' do
-      pipeline = create(:ci_pipeline, project: project)
-      create(:ci_build, pipeline: pipeline, name: 'Job 1')
-
-      control_count = ActiveRecord::QueryRecorder.new do
-        post_graphql(query, current_user: first_user)
+    let(:query) do
+      with_signature([first_n], wrap_fields(query_graphql_path(query_path, :name)))
     end
 
+    before_all do
+      pipeline = create(:ci_pipeline, project: project)
+      create(:ci_build, pipeline: pipeline, name: 'Job 1')
       pipeline = create(:ci_pipeline, project: project)
       create(:ci_build, pipeline: pipeline, name: 'Job 2')
+    end
 
-      expect do
-        post_graphql(query, current_user: second_user)
-      end.not_to exceed_query_limit(control_count)
+    it 'limits the results' do
+      post_graphql(query, current_user: user, variables: first_n.with(1))
 
-      expect(response).to have_gitlab_http_status(:ok)
+      expect(graphql_data_at(*query_path.map(&:first))).to contain_exactly a_hash_including(
+        'name' => 'Job 2'
+      )
+    end
 
-      pipelines_data = graphql_data.dig('project', 'pipelines', 'nodes')
+    it 'fetches all results' do
+      post_graphql(query, current_user: user)
 
-      job_names = pipelines_data.map do |pipeline_data|
-        jobs_data = pipeline_data.dig('jobs', 'nodes')
-        jobs_data.map { |job_data| job_data['name'] }
-      end.flatten
+      expect(graphql_data_at(*query_path.map(&:first))).to contain_exactly(
+        a_hash_including('name' => 'Job 1'),
+        a_hash_including('name' => 'Job 2')
+      )
+    end
 
-      expect(job_names).to contain_exactly('Job 1', 'Job 2')
+    it 'fetches the jobs without an N+1' do
+      first_user = create(:personal_access_token).user
+      second_user = create(:personal_access_token).user
+
+      control_count = ActiveRecord::QueryRecorder.new do
+        post_graphql(query, current_user: first_user, variables: first_n.with(1))
+      end
+
+      expect do
+        post_graphql(query, current_user: second_user)
+      end.not_to exceed_query_limit(control_count)
     end
   end
 
@@ -80,7 +86,7 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
       create(:ci_build, :dast, name: 'DAST Job 1', pipeline: pipeline)
       create(:ci_build, :sast, name: 'SAST Job 1', pipeline: pipeline)
 
-      post_graphql(query, current_user: first_user)
+      post_graphql(query, current_user: user)
 
       expect(response).to have_gitlab_http_status(:ok)
 
@@ -96,9 +102,9 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
   end
 
   describe 'upstream' do
-    let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: first_user) }
+    let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user) }
     let_it_be(:upstream_project) { create(:project, :repository, :public) }
-    let_it_be(:upstream_pipeline) { create(:ci_pipeline, project: upstream_project, user: first_user) }
+    let_it_be(:upstream_pipeline) { create(:ci_pipeline, project: upstream_project, user: user) }
     let(:upstream_pipelines_graphql_data) { graphql_data.dig(*%w[project pipelines nodes]).first['upstream'] }
 
     let(:query) do
@@ -120,7 +126,7 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
     before do
       create(:ci_sources_pipeline, source_pipeline: upstream_pipeline, pipeline: pipeline )
 
-      post_graphql(query, current_user: first_user)
+      post_graphql(query, current_user: user)
     end
 
     it_behaves_like 'a working graphql query'
@@ -131,15 +137,18 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
 
     context 'when fetching the upstream pipeline from the pipeline' do
       it 'avoids N+1 queries' do
+        first_user = create(:user)
+        second_user = create(:user)
+
         control_count = ActiveRecord::QueryRecorder.new do
           post_graphql(query, current_user: first_user)
         end
 
-        pipeline_2 = create(:ci_pipeline, project: project, user: first_user)
-        upstream_pipeline_2 = create(:ci_pipeline, project: upstream_project, user: first_user)
+        pipeline_2 = create(:ci_pipeline, project: project, user: user)
+        upstream_pipeline_2 = create(:ci_pipeline, project: upstream_project, user: user)
         create(:ci_sources_pipeline, source_pipeline: upstream_pipeline_2, pipeline: pipeline_2 )
-        pipeline_3 = create(:ci_pipeline, project: project, user: first_user)
-        upstream_pipeline_3 = create(:ci_pipeline, project: upstream_project, user: first_user)
+        pipeline_3 = create(:ci_pipeline, project: project, user: user)
+        upstream_pipeline_3 = create(:ci_pipeline, project: upstream_project, user: user)
         create(:ci_sources_pipeline, source_pipeline: upstream_pipeline_3, pipeline: pipeline_3 )
 
         expect do
@@ -152,12 +161,12 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
   end
 
   describe 'downstream' do
-    let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: first_user) }
-    let(:pipeline_2) { create(:ci_pipeline, project: project, user: first_user) }
+    let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user) }
+    let(:pipeline_2) { create(:ci_pipeline, project: project, user: user) }
 
     let_it_be(:downstream_project) { create(:project, :repository, :public) }
-    let_it_be(:downstream_pipeline_a) { create(:ci_pipeline, project: downstream_project, user: first_user) }
-    let_it_be(:downstream_pipeline_b) { create(:ci_pipeline, project: downstream_project, user: first_user) }
+    let_it_be(:downstream_pipeline_a) { create(:ci_pipeline, project: downstream_project, user: user) }
+    let_it_be(:downstream_pipeline_b) { create(:ci_pipeline, project: downstream_project, user: user) }
 
     let(:pipelines_graphql_data) { graphql_data.dig(*%w[project pipelines nodes]) }
 
@@ -183,7 +192,7 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
       create(:ci_sources_pipeline, source_pipeline: pipeline, pipeline: downstream_pipeline_a)
       create(:ci_sources_pipeline, source_pipeline: pipeline_2, pipeline: downstream_pipeline_b)
 
-      post_graphql(query, current_user: first_user)
+      post_graphql(query, current_user: user)
     end
 
     it_behaves_like 'a working graphql query'
@@ -198,16 +207,19 @@ RSpec.describe 'Query.project(fullPath).pipelines' do
 
     context 'when fetching the downstream pipelines from the pipeline' do
       it 'avoids N+1 queries' do
+        first_user = create(:user)
+        second_user = create(:user)
+
         control_count = ActiveRecord::QueryRecorder.new do
           post_graphql(query, current_user: first_user)
         end
 
-        downstream_pipeline_2a = create(:ci_pipeline, project: downstream_project, user: first_user)
+        downstream_pipeline_2a = create(:ci_pipeline, project: downstream_project, user: user)
         create(:ci_sources_pipeline, source_pipeline: pipeline, pipeline: downstream_pipeline_2a)
-        downsteam_pipeline_3a = create(:ci_pipeline, project: downstream_project, user: first_user)
+        downsteam_pipeline_3a = create(:ci_pipeline, project: downstream_project, user: user)
         create(:ci_sources_pipeline, source_pipeline: pipeline, pipeline: downsteam_pipeline_3a)
 
-        downstream_pipeline_2b = create(:ci_pipeline, project: downstream_project, user: first_user)
+        downstream_pipeline_2b = create(:ci_pipeline, project: downstream_project, user: user)
         create(:ci_sources_pipeline, source_pipeline: pipeline_2, pipeline: downstream_pipeline_2b)
         downsteam_pipeline_3b = create(:ci_pipeline, project: downstream_project, user: first_user)
         create(:ci_sources_pipeline, source_pipeline: pipeline_2, pipeline: downsteam_pipeline_3b)