Merge branch 'reduce-artifacts-queries-in-mr-widget' into 'master'

Reduce queries for CI artifacts from MR widget

Closes gitlab-ce#43394

See merge request gitlab-org/gitlab-ee!6978

ee/app/models/ee/ci/build.rb

@@ -20,23 +20,9 @@ module EE
       DAST_FILE = 'gl-dast-report.json'.freeze
 
       included do
-        scope :code_quality, -> { where(name: %w[codeclimate codequality code_quality]) }
-        scope :performance, -> { where(name: %w[performance deploy]) }
-        scope :sast, -> { where(name: 'sast') }
-        scope :dependency_scanning, -> { where(name: 'dependency_scanning') }
-        scope :license_management, -> { where(name: 'license_management') }
-        scope :sast_container, -> { where(name: %w[sast:container container_scanning]) }
-        scope :dast, -> { where(name: 'dast') }
-
         after_save :stick_build_if_status_changed
       end
 
-      class_methods do
-        def find_dast
-          dast.find(&:has_dast_json?)
-        end
-      end
-
       def shared_runners_minutes_limit_enabled?
         runner && runner.instance_type? && project.shared_runners_minutes_limit_enabled?
       end
@@ -50,40 +36,49 @@ module EE
 
       # has_codeclimate_json? is deprecated and replaced with has_code_quality_json? (#5779)
       def has_codeclimate_json?
-        has_artifact?(CODECLIMATE_FILE)
+        name_in?(%w[codeclimate codequality code_quality]) &&
+          has_artifact?(CODECLIMATE_FILE)
       end
 
       def has_code_quality_json?
-        has_artifact?(CODE_QUALITY_FILE)
+        name_in?(%w[codeclimate codequality code_quality]) &&
+          has_artifact?(CODE_QUALITY_FILE)
       end
 
       def has_performance_json?
-        has_artifact?(PERFORMANCE_FILE)
+        name_in?(%w[performance deploy]) &&
+          has_artifact?(PERFORMANCE_FILE)
       end
 
       def has_sast_json?
-        has_artifact?(SAST_FILE)
+        name_in?('sast') &&
+          has_artifact?(SAST_FILE)
       end
 
       def has_dependency_scanning_json?
-        has_artifact?(DEPENDENCY_SCANNING_FILE)
+        name_in?('dependency_scanning') &&
+          has_artifact?(DEPENDENCY_SCANNING_FILE)
       end
 
       def has_license_management_json?
-        has_artifact?(LICENSE_MANAGEMENT_FILE)
+        name_in?('license_management') &&
+          has_artifact?(LICENSE_MANAGEMENT_FILE)
       end
 
       # has_sast_container_json? is deprecated and replaced with has_container_scanning_json? (#5778)
       def has_sast_container_json?
-        has_artifact?(SAST_CONTAINER_FILE)
+        name_in?(%w[sast:container container_scanning]) &&
+          has_artifact?(SAST_CONTAINER_FILE)
       end
 
       def has_container_scanning_json?
-        has_artifact?(CONTAINER_SCANNING_FILE)
+        name_in?(%w[sast:container container_scanning]) &&
+          has_artifact?(CONTAINER_SCANNING_FILE)
       end
 
       def has_dast_json?
-        has_artifact?(DAST_FILE)
+        name_in?('dast') &&
+          has_artifact?(DAST_FILE)
       end
 
       private
@@ -92,6 +87,10 @@ module EE
         options.dig(:artifacts, :paths)&.include?(name) &&
           artifacts_metadata?
       end
+
+      def name_in?(names)
+        name.in?(Array(names))
+      end
     end
   end
 end

ee/app/models/ee/ci/pipeline.rb

@@ -18,40 +18,40 @@ module EE
 
       # codeclimate_artifact is deprecated and replaced with code_quality_artifact (#5779)
       def codeclimate_artifact
-        @codeclimate_artifact ||= artifacts.code_quality.find(&:has_codeclimate_json?)
+        @codeclimate_artifact ||= artifacts_with_files.find(&:has_codeclimate_json?)
       end
 
       def code_quality_artifact
-        @code_quality_artifact ||= artifacts.code_quality.find(&:has_code_quality_json?)
+        @code_quality_artifact ||= artifacts_with_files.find(&:has_code_quality_json?)
       end
 
       def performance_artifact
-        @performance_artifact ||= artifacts.performance.find(&:has_performance_json?)
+        @performance_artifact ||= artifacts_with_files.find(&:has_performance_json?)
       end
 
       def sast_artifact
-        @sast_artifact ||= artifacts.sast.find(&:has_sast_json?)
+        @sast_artifact ||= artifacts_with_files.find(&:has_sast_json?)
       end
 
       def dependency_scanning_artifact
-        @dependency_scanning_artifact ||= artifacts.dependency_scanning.find(&:has_dependency_scanning_json?)
+        @dependency_scanning_artifact ||= artifacts_with_files.find(&:has_dependency_scanning_json?)
       end
 
       def license_management_artifact
-        @license_management_artifact ||= artifacts.license_management.find(&:has_license_management_json?)
+        @license_management_artifact ||= artifacts_with_files.find(&:has_license_management_json?)
       end
 
       # sast_container_artifact is deprecated and replaced with container_scanning_artifact (#5778)
       def sast_container_artifact
-        @sast_container_artifact ||= artifacts.sast_container.find(&:has_sast_container_json?)
+        @sast_container_artifact ||= artifacts_with_files.find(&:has_sast_container_json?)
       end
 
       def container_scanning_artifact
-        @container_scanning_artifact ||= artifacts.sast_container.find(&:has_container_scanning_json?)
+        @container_scanning_artifact ||= artifacts_with_files.find(&:has_container_scanning_json?)
       end
 
       def dast_artifact
-        @dast_artifact ||= artifacts.dast.find(&:has_dast_json?)
+        @dast_artifact ||= artifacts_with_files.find(&:has_dast_json?)
       end
 
       def initialize_yaml_processor
@@ -148,6 +148,12 @@ module EE
       def expose_code_quality_data?
         has_code_quality_data?
       end
+
+      private
+
+      def artifacts_with_files
+        @artifacts_with_files ||= artifacts.includes(:job_artifacts_metadata, :job_artifacts_archive).to_a
+      end
     end
   end
 end

ee/changelogs/unreleased/reduce-artifacts-queries-in-mr-widget.yml

+---
+title: Reduce queries needed for CI artifacts on merge request widget
+merge_request: 6978
+author:
+type: performance

ee/spec/models/ci/build_spec.rb

@@ -13,34 +13,6 @@ describe Ci::Build do
 
   let(:job) { create(:ci_build, pipeline: pipeline) }
 
-  describe '.code_quality' do
-    subject { described_class.code_quality }
-
-    context 'when a job name is codeclimate' do
-      let!(:job) { create(:ci_build, pipeline: pipeline, name: 'codeclimate') }
-
-      it { is_expected.to include(job) }
-    end
-
-    context 'when a job name is codequality' do
-      let!(:job) { create(:ci_build, pipeline: pipeline, name: 'codequality') }
-
-      it { is_expected.to include(job) }
-    end
-
-    context 'when a job name is code_quality' do
-      let!(:job) { create(:ci_build, pipeline: pipeline, name: 'code_quality') }
-
-      it { is_expected.to include(job) }
-    end
-
-    context 'when a job name is irrelevant' do
-      let!(:job) { create(:ci_build, pipeline: pipeline, name: 'codechecker') }
-
-      it { is_expected.not_to include(job) }
-    end
-  end
-
   describe '#shared_runners_minutes_limit_enabled?' do
     subject { job.shared_runners_minutes_limit_enabled? }
 
@@ -143,46 +115,97 @@ describe Ci::Build do
     end
   end
 
-  BUILD_ARTIFACTS_METHODS = {
+  build_artifacts_methods = {
     # has_codeclimate_json? is deprecated and replaced with code_quality_artifact (#5779)
-    has_codeclimate_json?: Ci::Build::CODECLIMATE_FILE,
-    has_code_quality_json?: Ci::Build::CODE_QUALITY_FILE,
-    has_performance_json?: Ci::Build::PERFORMANCE_FILE,
-    has_sast_json?: Ci::Build::SAST_FILE,
-    has_dependency_scanning_json?: Ci::Build::DEPENDENCY_SCANNING_FILE,
-    has_license_management_json?: Ci::Build::LICENSE_MANAGEMENT_FILE,
+    has_codeclimate_json?: {
+      filename: Ci::Build::CODECLIMATE_FILE,
+      job_names: %w[codeclimate codequality code_quality]
+    },
+    has_code_quality_json?: {
+      filename: Ci::Build::CODE_QUALITY_FILE,
+      job_names: %w[codeclimate codequality code_quality]
+    },
+    has_performance_json?: {
+      filename: Ci::Build::PERFORMANCE_FILE,
+      job_names: %w[performance deploy]
+    },
+    has_sast_json?: {
+      filename: Ci::Build::SAST_FILE,
+      job_names: %w[sast]
+    },
+    has_dependency_scanning_json?: {
+      filename: Ci::Build::DEPENDENCY_SCANNING_FILE,
+      job_names: %w[dependency_scanning]
+    },
+    has_license_management_json?: {
+      filename: Ci::Build::LICENSE_MANAGEMENT_FILE,
+      job_names: %w[license_management]
+    },
     # has_sast_container_json? is deprecated and replaced with has_container_scanning_json (#5778)
-    has_sast_container_json?: Ci::Build::SAST_CONTAINER_FILE,
-    has_container_scanning_json?: Ci::Build::CONTAINER_SCANNING_FILE,
-    has_dast_json?: Ci::Build::DAST_FILE
-  }.freeze
+    has_sast_container_json?: {
+      filename: Ci::Build::SAST_CONTAINER_FILE,
+      job_names: %w[sast:container container_scanning]
+    },
+    has_container_scanning_json?: {
+      filename: Ci::Build::CONTAINER_SCANNING_FILE,
+      job_names: %w[sast:container container_scanning]
+    },
+    has_dast_json?: {
+      filename: Ci::Build::DAST_FILE,
+      job_names: %w[dast]
+    }
+  }
+
+  build_artifacts_methods.each do |method, requirements|
+    filename = requirements[:filename]
+    job_names = requirements[:job_names]
 
-  BUILD_ARTIFACTS_METHODS.each do |method, filename|
     describe "##{method}" do
-      context 'valid build' do
-        let!(:build) do
+      job_names.each do |job_name|
+        context "with a job named #{job_name} and a file named #{filename}" do
+          let(:build) do
+            create(
+              :ci_build,
+              :artifacts,
+              name: job_name,
+              pipeline: pipeline,
+              options: {
+                artifacts: {
+                  paths: [filename, 'some-other-artifact.txt']
+                }
+              }
+            )
+          end
+
+          it { expect(build.send(method)).to be_truthy }
+        end
+      end
+
+      context 'with an invalid filename' do
+        let(:build) do
           create(
             :ci_build,
             :artifacts,
+            name: job_names.first,
             pipeline: pipeline,
-            options: {
-              artifacts: {
-                paths: [filename, 'some-other-artifact.txt']
-              }
-            }
+            options: {}
           )
         end
 
-        it { expect(build.send(method)).to be_truthy }
+        it { expect(build.send(method)).to be_falsey }
       end
 
-      context 'invalid build' do
-        let!(:build) do
+      context 'with an invalid job name' do
+        let(:build) do
           create(
             :ci_build,
             :artifacts,
             pipeline: pipeline,
-            options: {}
+            options: {
+              artifacts: {
+                paths: [filename, 'some-other-artifact.txt']
+              }
+            }
           )
         end
 

ee/spec/models/ci/pipeline_spec.rb

@@ -171,4 +171,29 @@ describe Ci::Pipeline do
       expect(described_class.with_security_reports).to eq([pipeline_1, pipeline_2, pipeline_3, pipeline_4])
     end
   end
+
+  context 'performance' do
+    def create_build(job_name, filename)
+      create(
+        :ci_build,
+        :artifacts,
+        name: job_name,
+        pipeline: pipeline,
+        options: {
+          artifacts: {
+            paths: [filename]
+          }
+        }
+      )
+    end
+
+    it 'does not perform extra queries when calling pipeline artifacts methods after the first' do
+      create_build('codeclimate', 'codeclimate.json')
+      create_build('dependency_scanning', 'gl-dependency-scanning-report.json')
+
+      pipeline.code_quality_artifact
+
+      expect { pipeline.dependency_scanning_artifact }.not_to exceed_query_limit(0)
+    end
+  end
 end