Merge branch 'pedropombeiro/421889/fix-n+1' into 'master'

GraphQL: Fix N+1 issue in jobs resolver

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130720



Merged-by: A Browne <abrowne@gitlab.com>
Approved-by: A Browne <abrowne@gitlab.com>
Reviewed-by: Pedro Pombeiro <noreply@pedro.pombei.ro>
Reviewed-by: A Browne <abrowne@gitlab.com>
Co-authored-by: Pedro Pombeiro <noreply@pedro.pombei.ro>

app/graphql/resolvers/ci/all_jobs_resolver.rb

@@ -40,6 +40,7 @@ def preloads
           play_path: [{ project: { namespace: [:route] } }],
           web_path: [{ project: { namespace: [:route] } }],
           tags: [:tags],
+          ai_failure_analysis: [{ project: [:project_feature, :namespace] }],
           trace: [{ project: [:namespace] }, :job_artifacts_trace]
         }
       end

app/graphql/resolvers/ci/runner_jobs_resolver.rb

@@ -45,6 +45,7 @@ def preloads
           web_path: [{ project: { namespace: [:route] } }],
           short_sha: [:pipeline],
           tags: [:tags],
+          ai_failure_analysis: [{ project: [:project_feature, :namespace] }],
           trace: [{ project: [:namespace] }, :job_artifacts_trace]
         }
       end

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

@@ -5,6 +5,8 @@
 RSpec.describe 'Query.runner(id)', :freeze_time, feature_category: :runner_fleet do
   include GraphqlHelpers
+  using RSpec::Parameterized::TableSyntax
+
   let_it_be(:user) { create(:user, :admin) }
   let_it_be(:another_admin) { create(:user, :admin) }
   let_it_be_with_reload(:group) { create(:group) }
@@ -144,23 +146,6 @@
       )
       expect(runner_data['tagList']).to match_array runner.tag_list
     end
-
-    it 'does not execute more queries per runner', :use_sql_query_cache, :aggregate_failures do
-      # warm-up license cache and so on:
-      personal_access_token = create(:personal_access_token, user: user)
-      args = { current_user: user, token: { personal_access_token: personal_access_token } }
-      post_graphql(query, **args)
-      expect(graphql_data_at(:runner)).not_to be_nil
-
-      personal_access_token = create(:personal_access_token, user: another_admin)
-      args = { current_user: another_admin, token: { personal_access_token: personal_access_token } }
-      control = ActiveRecord::QueryRecorder.new(skip_cached: false) { post_graphql(query, **args) }
-
-      create(:ci_runner, :instance, version: '14.0.0', tag_list: %w[tag5 tag6], creator: another_admin)
-      create(:ci_runner, :project, version: '14.0.1', projects: [project1], tag_list: %w[tag3 tag8], creator: another_admin)
-
-      expect { post_graphql(query, **args) }.not_to exceed_all_query_limit(control)
-    end
   end
   shared_examples 'retrieval with no admin url' do
@@ -248,23 +233,25 @@
   end
   describe 'for project runner' do
-    describe 'locked' do
-      using RSpec::Parameterized::TableSyntax
+    let_it_be_with_refind(:project_runner) do
+      create(:ci_runner, :project,
+             description: 'Runner 3',
+             contacted_at: 1.day.ago,
+             active: false,
+             locked: false,
+             version: 'adfe157',
+             revision: 'b',
+             ip_address: '10.10.10.10',
+             access_level: 1,
+             run_untagged: true)
+    end
+    describe 'locked' do
       where(is_locked: [true, false])
       with_them do
-        let(:project_runner) do
-          create(:ci_runner, :project,
-            description: 'Runner 3',
-            contacted_at: 1.day.ago,
-            active: false,
-            locked: is_locked,
-            version: 'adfe157',
-            revision: 'b',
-            ip_address: '10.10.10.10',
-            access_level: 1,
-            run_untagged: true)
+        before do
+          project_runner.update!(locked: is_locked)
         end
         let(:query) do
@@ -428,6 +415,38 @@
         end
       end
     end
+
+    describe 'a query fetching all fields' do
+      let(:query) do
+        wrap_fields(query_graphql_path(query_path, all_graphql_fields_for('CiRunner')))
+      end
+
+      let(:query_path) do
+        [
+          [:runner, { id: project_runner.to_global_id.to_s }]
+        ]
+      end
+
+      it 'does not execute more queries per runner', :use_sql_query_cache, :aggregate_failures do
+        create(:ci_build, :failed, runner: project_runner)
+        create(:ci_runner_machine, runner: project_runner, version: '16.4.0')
+
+        # warm-up license cache and so on:
+        personal_access_token = create(:personal_access_token, user: user)
+        args = { current_user: user, token: { personal_access_token: personal_access_token } }
+        post_graphql(query, **args)
+        expect(graphql_data_at(:runner)).not_to be_nil
+
+        personal_access_token = create(:personal_access_token, user: another_admin)
+        args = { current_user: another_admin, token: { personal_access_token: personal_access_token } }
+        control = ActiveRecord::QueryRecorder.new(skip_cached: false) { post_graphql(query, **args) }
+
+        create(:ci_build, :failed, runner: project_runner)
+        create(:ci_runner_machine, runner: project_runner, version: '16.4.1')
+
+        expect { post_graphql(query, **args) }.not_to exceed_all_query_limit(control)
+      end
+    end
   end
   describe 'for inactive runner' do
@@ -995,8 +1014,6 @@ def runner_query(runner)
     end
     context 'when requesting individual fields' do
-      using RSpec::Parameterized::TableSyntax
-
       where(:field) do
         [
           'detailedStatus { id detailsPath group icon text }',

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

@@ -72,10 +72,16 @@
           args = { current_user: admin2, token: { personal_access_token: personal_access_token } }
           control = ActiveRecord::QueryRecorder.new { post_graphql(query, **args) }
-          create(:ci_runner, :instance, version: '14.0.0', tag_list: %w[tag5 tag6], creator: admin2)
-          create(:ci_runner, :project, version: '14.0.1', projects: [project], tag_list: %w[tag3 tag8],
+          runner2 = create(:ci_runner, :instance, version: '14.0.0', tag_list: %w[tag5 tag6], creator: admin2)
+          runner3 = create(:ci_runner, :project, version: '14.0.1', projects: [project], tag_list: %w[tag3 tag8],
             creator: current_user)
+          create(:ci_build, :failed, runner: runner2)
+          create(:ci_runner_machine, runner: runner2, version: '16.4.1')
+
+          create(:ci_build, :failed, runner: runner3)
+          create(:ci_runner_machine, runner: runner3, version: '16.4.0')
+
           expect { post_graphql(query, **args) }.not_to exceed_query_limit(control)
         end
       end

spec/requests/api/graphql/jobs_query_spec.rb

@@ -10,7 +10,7 @@
       :jobs, {}, %(
       count
       nodes {
-        #{all_graphql_fields_for(::Types::Ci::JobType, max_depth: 1, excluded: %w[aiFailureAnalysis])}
+        #{all_graphql_fields_for(::Types::Ci::JobType, max_depth: 1)}
       })
     )
   end