Merge branch 'lm-show-jobs-without-needs-in-order' into 'master'

Adds previousStageJobAndNeeds field to JobType

See merge request gitlab-org/gitlab!74317

app/graphql/resolvers/project_jobs_resolver.rb

@@ -33,6 +33,7 @@ module Resolvers
 
     def preloads
       {
+        previous_stage_jobs_and_needs: [:needs, :pipeline],
         artifacts: [:job_artifacts],
         pipeline: [:user]
       }

app/graphql/resolvers/project_pipeline_resolver.rb

@@ -24,7 +24,6 @@ module Resolvers
       super
     end
 
-    # the preloads are defined on ee/app/graphql/ee/resolvers/project_pipeline_resolver.rb
     def resolve(iid: nil, sha: nil, **args)
       self.lookahead = args.delete(:lookahead)
 
@@ -42,5 +41,11 @@ module Resolvers
         end
       end
     end
+
+    def unconditional_includes
+      [
+        { statuses: [:needs] }
+      ]
+    end
   end
 end

app/graphql/resolvers/project_pipelines_resolver.rb

@@ -18,7 +18,7 @@ module Resolvers
 
     def preloads
       {
-        jobs: [:statuses],
+        jobs: { statuses: [:needs] },
         upstream: [:triggered_by_pipeline],
         downstream: [:triggered_pipelines]
       }

app/graphql/types/ci/job_type.rb

@@ -50,6 +50,8 @@ module Types
             null: true,
             description: 'How long the job was enqueued before starting.'
 
+      field :previous_stage_jobs_and_needs, Types::Ci::JobType.connection_type, null: true,
+            description: 'All prerequisite jobs.'
       field :detailed_status, Types::Ci::DetailedStatusType, null: true,
             description: 'Detailed status of the job.'
       field :artifacts, Types::Ci::JobArtifactType.connection_type, null: true,
@@ -101,6 +103,26 @@ module Types
         end
       end
 
+      def previous_stage_jobs_and_needs
+        Gitlab::Graphql::Lazy.with_value(previous_stage_jobs) do |jobs|
+          (jobs + object.needs).uniq(&:name)
+        end
+      end
+
+      def previous_stage_jobs
+        BatchLoader::GraphQL.for([object.pipeline, object.stage_idx - 1]).batch(default_value: []) do |tuples, loader|
+          tuples.group_by(&:first).each do |pipeline, keys|
+            positions = keys.map(&:second)
+
+            stages = pipeline.stages.by_position(positions)
+
+            stages.each do |stage|
+              loader.call([pipeline, stage.position], stage.latest_statuses)
+            end
+          end
+        end
+      end
+
       def stage
         ::Gitlab::Graphql::Lazy.with_value(pipeline) do |pl|
           BatchLoader::GraphQL.for([pl, object.stage]).batch do |ids, loader|

app/graphql/types/ci/stage_type.rb

@@ -31,7 +31,10 @@ module Types
 
         BatchLoader::GraphQL.for(key).batch(default_value: []) do |keys, loader|
           by_pipeline = keys.group_by(&:pipeline)
-          include_needs = keys.any? { |k| k.requires?(%i[nodes jobs nodes needs]) }
+          include_needs = keys.any? do |k|
+            k.requires?(%i[nodes jobs nodes needs]) ||
+            k.requires?(%i[nodes jobs nodes previousStageJobsAndNeeds])
+          end
 
           by_pipeline.each do |pl, key_group|
             project = pl.project

app/models/ci/stage.rb

@@ -22,6 +22,7 @@ module Ci
     scope :ordered, -> { order(position: :asc) }
     scope :in_pipelines, ->(pipelines) { where(pipeline: pipelines) }
     scope :by_name, ->(names) { where(name: names) }
+    scope :by_position, ->(positions) { where(position: positions) }
 
     with_options unless: :importing? do
       validates :project, presence: true

doc/api/graphql/reference/index.md

@@ -8663,6 +8663,7 @@ Represents the total number of issues and their weights for a particular day.
 | <a id="cijobneeds"></a>`needs` | [`CiBuildNeedConnection`](#cibuildneedconnection) | References to builds that must complete before the jobs run. (see [Connections](#connections)) |
 | <a id="cijobpipeline"></a>`pipeline` | [`Pipeline`](#pipeline) | Pipeline the job belongs to. |
 | <a id="cijobplayable"></a>`playable` | [`Boolean!`](#boolean) | Indicates the job can be played. |
+| <a id="cijobpreviousstagejobsandneeds"></a>`previousStageJobsAndNeeds` | [`CiJobConnection`](#cijobconnection) | All prerequisite jobs. (see [Connections](#connections)) |
 | <a id="cijobqueuedat"></a>`queuedAt` | [`Time`](#time) | When the job was enqueued and marked as pending. |
 | <a id="cijobqueuedduration"></a>`queuedDuration` | [`Duration`](#duration) | How long the job was enqueued before starting. |
 | <a id="cijobrefname"></a>`refName` | [`String`](#string) | Ref name of the job. |

spec/graphql/types/ci/job_type_spec.rb

@@ -25,6 +25,7 @@ RSpec.describe Types::Ci::JobType do
       needs
       pipeline
       playable
+      previousStageJobsAndNeeds
       queued_at
       queued_duration
       refName

spec/models/ci/stage_spec.rb

@@ -28,6 +28,18 @@ RSpec.describe Ci::Stage, :models do
     end
   end
 
+  describe '.by_position' do
+    it 'finds stages by position' do
+      a = create(:ci_stage_entity, position: 1)
+      b = create(:ci_stage_entity, position: 2)
+      c = create(:ci_stage_entity, position: 3)
+
+      expect(described_class.by_position(1)).to contain_exactly(a)
+      expect(described_class.by_position(2)).to contain_exactly(b)
+      expect(described_class.by_position(%w[1 3])).to contain_exactly(a, c)
+    end
+  end
+
   describe '.by_name' do
     it 'finds stages by name' do
       a = create(:ci_stage_entity, name: 'a')

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

@@ -14,7 +14,7 @@ RSpec.describe 'Query.project.pipeline' do
   describe '.stages.groups.jobs' do
     let(:pipeline) do
       pipeline = create(:ci_pipeline, project: project, user: user)
-      stage = create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'first')
+      stage = create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'first', position: 1)
       create(:ci_build, stage_id: stage.id, pipeline: pipeline, name: 'my test job')
 
       pipeline
@@ -44,13 +44,18 @@ RSpec.describe 'Query.project.pipeline' do
             name
             jobs {
               nodes {
-                detailedStatus {
-                  id
-                }
                 name
                 needs {
                   nodes { #{all_graphql_fields_for('CiBuildNeed')} }
                 }
+                previousStageJobsAndNeeds {
+                  nodes {
+                    name
+                  }
+                }
+                detailedStatus {
+                  id
+                }
                 pipeline {
                   id
                 }
@@ -62,58 +67,80 @@ RSpec.describe 'Query.project.pipeline' do
       FIELDS
     end
 
-    context 'when there are build needs' do
-      before do
-        pipeline.statuses.each do |build|
-          create_list(:ci_build_need, 2, build: build)
-        end
-      end
-
-      it 'reports the build needs' do
-        post_graphql(query, current_user: user)
-
-        expect(jobs_graphql_data).to contain_exactly a_hash_including(
-          'needs' => a_hash_including(
-            'nodes' => contain_exactly(
-              a_hash_including('name' => String),
-              a_hash_including('name' => String)
-            )
-          )
-        )
-      end
-    end
-
     it 'returns the jobs of a pipeline stage' do
       post_graphql(query, current_user: user)
 
       expect(jobs_graphql_data).to contain_exactly(a_hash_including('name' => 'my test job'))
     end
 
-    describe 'performance' do
+    context 'when there is more than one stage and job needs' 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
+        deploy_stage = create(:ci_stage_entity, position: 4, name: 'deploy', project: project, pipeline: pipeline)
 
-      it 'can find the first stage' do
-        post_graphql(query, current_user: user, variables: first_n.with(1))
+        create(:ci_build, pipeline: pipeline, stage_idx: build_stage.position, name: 'docker 1 2', stage: build_stage)
+        create(:ci_build, pipeline: pipeline, stage_idx: build_stage.position, name: 'docker 2 2', stage: build_stage)
+        create(:ci_build, pipeline: pipeline, stage_idx: test_stage.position, name: 'rspec 1 2', stage: test_stage)
+        test_job = create(:ci_bridge, pipeline: pipeline, stage_idx: test_stage.position, name: 'rspec 2 2', stage: test_stage)
+        create(:ci_build, pipeline: pipeline, stage_idx: deploy_stage.position, name: 'deploy 1 2', stage: deploy_stage)
+        deploy_job = create(:ci_build, pipeline: pipeline, stage_idx: deploy_stage.position, name: 'deploy 2 2', stage: deploy_stage)
 
-        expect(jobs_graphql_data).to contain_exactly(a_hash_including('name' => 'my test job'))
+        create(:ci_build_need, build: test_job, name: 'my test job')
+        create(:ci_build_need, build: deploy_job, name: 'rspec 1 2')
       end
 
-      it 'can find all stages' do
-        post_graphql(query, current_user: user, variables: first_n.with(3))
+      it 'reports the build needs and previous stages with no duplicates' do
+        post_graphql(query, current_user: user)
 
         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')
+          a_hash_including(
+            'name' => 'my test job',
+            'needs' => { 'nodes' => [] },
+            'previousStageJobsAndNeeds' => { 'nodes' => [] }
+          ),
+          a_hash_including(
+            'name' => 'docker 1 2',
+            'needs' => { 'nodes' => [] },
+            'previousStageJobsAndNeeds' => { 'nodes' => [
+              { "name" => "my test job" }
+            ] }
+          ),
+          a_hash_including(
+            'name' => 'docker 2 2',
+            'needs' => { 'nodes' => [] },
+            'previousStageJobsAndNeeds' => { 'nodes' => [
+              { "name" => "my test job" }
+            ] }
+          ),
+          a_hash_including(
+            'name' => 'rspec 1 2',
+            'needs' => { 'nodes' => [] },
+            'previousStageJobsAndNeeds' => { 'nodes' => [
+              { "name" => "docker 1 2" }, { "name" => "docker 2 2" }
+            ] }
+          ),
+          a_hash_including(
+            'name' => 'rspec 2 2',
+            'needs' => { 'nodes' => [a_hash_including('name' => 'my test job')] },
+            'previousStageJobsAndNeeds' => { 'nodes' => [
+              { "name" => "docker 1 2" }, { "name" => "docker 2 2" }, { "name" => "my test job" }
+            ] }
+          ),
+          a_hash_including(
+            'name' => 'deploy 1 2',
+            'needs' => { 'nodes' => [] },
+            'previousStageJobsAndNeeds' => { 'nodes' => [
+              { "name" => "rspec 1 2" }, { "name" => "rspec 2 2" }
+            ] }
+          ),
+          a_hash_including(
+            'name' => 'deploy 2 2',
+            'needs' => { 'nodes' => [a_hash_including('name' => 'rspec 1 2')] },
+            'previousStageJobsAndNeeds' => { 'nodes' => [
+              { "name" => "rspec 1 2" }, { "name" => "rspec 2 2" }
+            ] }
+          )
         )
       end
 

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

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe 'Query.project.jobs' do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project, :repository, :public) }
+  let_it_be(:user) { create(:user) }
+
+  let(:pipeline) do
+    create(:ci_pipeline, project: project, user: user)
+  end
+
+  let(:query) do
+    <<~QUERY
+    {
+      project(fullPath: "#{project.full_path}") {
+        jobs {
+          nodes {
+            name
+            previousStageJobsAndNeeds {
+              nodes {
+                name
+              }
+            }
+          }
+        }
+      }
+    }
+    QUERY
+  end
+
+  it 'does not generate N+1 queries', :request_store, :use_sql_query_cache do
+    build_stage = create(:ci_stage_entity, position: 1, name: 'build', project: project, pipeline: pipeline)
+    test_stage = create(:ci_stage_entity, position: 2, name: 'test', project: project, pipeline: pipeline)
+    create(:ci_build, pipeline: pipeline, stage_idx: build_stage.position, name: 'docker 1 2', stage: build_stage)
+    create(:ci_build, pipeline: pipeline, stage_idx: build_stage.position, name: 'docker 2 2', stage: build_stage)
+    create(:ci_build, pipeline: pipeline, stage_idx: test_stage.position, name: 'rspec 1 2', stage: test_stage)
+    test_job = create(:ci_build, pipeline: pipeline, stage_idx: test_stage.position, name: 'rspec 2 2', stage: test_stage)
+    create(:ci_build_need, build: test_job, name: 'docker 1 2')
+
+    post_graphql(query, current_user: user)
+
+    control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+      post_graphql(query, current_user: user)
+    end
+
+    create(:ci_build, name: 'test-a', stage: test_stage, stage_idx: test_stage.position, pipeline: pipeline)
+    test_b_job = create(:ci_build, name: 'test-b', stage: test_stage, stage_idx: test_stage.position, pipeline: pipeline)
+    create(:ci_build_need, build: test_b_job, name: 'docker 2 2')
+
+    expect do
+      post_graphql(query, current_user: user)
+    end.not_to exceed_all_query_limit(control)
+  end
+end

spec/requests/api/graphql/project/pipeline_spec.rb

@@ -273,6 +273,48 @@ RSpec.describe 'getting pipeline information nested in a project' do
     end
   end
 
+  context 'N+1 queries on pipeline jobs' do
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+
+    let(:fields) do
+      <<~FIELDS
+      jobs {
+        nodes {
+          previousStageJobsAndNeeds {
+            nodes {
+              name
+            }
+          }
+        }
+      }
+      FIELDS
+    end
+
+    it 'does not generate N+1 queries', :request_store, :use_sql_query_cache do
+      build_stage = create(:ci_stage_entity, position: 1, name: 'build', project: project, pipeline: pipeline)
+      test_stage = create(:ci_stage_entity, position: 2, name: 'test', project: project, pipeline: pipeline)
+      create(:ci_build, pipeline: pipeline, stage_idx: build_stage.position, name: 'docker 1 2', stage: build_stage)
+      create(:ci_build, pipeline: pipeline, stage_idx: build_stage.position, name: 'docker 2 2', stage: build_stage)
+      create(:ci_build, pipeline: pipeline, stage_idx: test_stage.position, name: 'rspec 1 2', stage: test_stage)
+      test_job = create(:ci_build, pipeline: pipeline, stage_idx: test_stage.position, name: 'rspec 2 2', stage: test_stage)
+      create(:ci_build_need, build: test_job, name: 'docker 1 2')
+
+      post_graphql(query, current_user: current_user)
+
+      control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+        post_graphql(query, current_user: current_user)
+      end
+
+      create(:ci_build, name: 'test-a', stage: test_stage, stage_idx: test_stage.position, pipeline: pipeline)
+      test_b_job = create(:ci_build, name: 'test-b', stage: test_stage, stage_idx: test_stage.position, pipeline: pipeline)
+      create(:ci_build_need, build: test_b_job, name: 'docker 2 2')
+
+      expect do
+        post_graphql(query, current_user: current_user)
+      end.not_to exceed_all_query_limit(control)
+    end
+  end
+
   context 'N+1 queries on stages jobs' do
     let(:depth) { 5 }
     let(:fields) do