Merge branch 'sh-optimize-merge-request-controller' into 'master'

Reduce SQL calls for loading CI artifacts

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

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

@@ -123,6 +123,12 @@ module EE
         source_bridge.success!
       end
 
+      def batch_lookup_report_artifact_for_file_type(file_type)
+        return unless available_licensed_report_type?(file_type)
+
+        latest_report_artifacts[file_type.to_s]&.last
+      end
+
       def any_report_artifact_for_type(file_type)
         report_artifact_for_file_type(file_type) || legacy_report_artifact_for_file_type(file_type)
       end
@@ -198,6 +204,30 @@ module EE
 
       private
 
+      # This batch loads the latest reports for each CI job artifact
+      # type (e.g. sast, dast, etc.) in a single SQL query to eliminate
+      # the need to do N different `job_artifacts.where(file_type:
+      # X).last` calls.
+      #
+      # Return a hash of file type => array of 1 job artifact
+      def latest_report_artifacts
+        ::Gitlab::SafeRequestStore.fetch("pipeline:#{self.id}:latest_report_artifacts") do
+          # Note we use read_attribute(:project_id) to read the project
+          # ID instead of self.project_id. The latter appears to load
+          # the Project model. This extra filter doesn't appear to
+          # affect query plan but included to ensure we don't leak the
+          # wrong informaiton.
+          ::Ci::JobArtifact.where(
+            id: job_artifacts.with_reports
+              .select('max(ci_job_artifacts.id) as id')
+              .where(project_id: self.read_attribute(:project_id))
+              .group(:file_type)
+          )
+            .preload(:job)
+            .group_by(&:file_type)
+        end
+      end
+
       def available_licensed_report_type?(file_type)
         feature_names = REPORT_LICENSED_FEATURES.fetch(file_type)
         feature_names.nil? || feature_names.any? { |feature| project.feature_available?(feature) }

ee/app/presenters/ee/ci/pipeline_presenter.rb

@@ -25,7 +25,7 @@ module EE
       end
 
       def downloadable_path_for_report_type(file_type)
-        if (job_artifact = report_artifact_for_file_type(file_type)) &&
+        if (job_artifact = batch_lookup_report_artifact_for_file_type(file_type)) &&
             can?(current_user, :read_build, job_artifact.job)
           return download_project_job_artifacts_path(
             job_artifact.project,

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -33,7 +33,35 @@ describe MergeRequestWidgetEntity do
     expect(subject.as_json[:approvals_before_merge]).to eq(0)
   end
 
-  describe 'test report artifacts' do
+  def create_all_artifacts
+    artifacts = %i(codequality sast dependency_scanning container_scanning dast license_management performance)
+
+    artifacts.each do |artifact_type|
+      create(:ee_ci_build, artifact_type, :success, pipeline: pipeline, project: pipeline.project)
+    end
+
+    pipeline.reload
+  end
+
+  it 'avoids N+1 queries', :request_store do
+    allow(pipeline).to receive(:available_licensed_report_type?).and_return(true)
+    allow(merge_request).to receive_messages(base_pipeline: pipeline, head_pipeline: pipeline)
+    create_all_artifacts
+    serializer = MergeRequestSerializer.new(current_user: user, project: project)
+
+    serializer.represent(merge_request)
+
+    RequestStore.clear!
+
+    control = ActiveRecord::QueryRecorder.new { serializer.represent(merge_request) }
+
+    create_all_artifacts
+    RequestStore.clear!
+
+    expect { serializer.represent(merge_request) }.not_to exceed_query_limit(control)
+  end
+
+  describe 'test report artifacts', :request_store do
     using RSpec::Parameterized::TableSyntax
 
     where(:json_entry, :artifact_type) do
@@ -88,7 +116,7 @@ describe MergeRequestWidgetEntity do
     end
   end
 
-  describe '#license_management' do
+  describe '#license_management', :request_store do
     before do
       allow(merge_request).to receive_messages(
         head_pipeline: pipeline, target_project: project)