Expose code_quality artifact with new arch

This introduces proper architecture for generalizing exposing artifacts
to frontend.

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

@@ -7,9 +7,6 @@ module EE
     module Build
       extend ActiveSupport::Concern
 
-      # CODECLIMATE_FILE is deprecated and replaced with CODE_QUALITY_FILE (#5779)
-      CODECLIMATE_FILE = 'codeclimate.json'.freeze
-      CODE_QUALITY_FILE = 'gl-code-quality-report.json'.freeze
       DEPENDENCY_SCANNING_FILE = 'gl-dependency-scanning-report.json'.freeze
       LICENSE_MANAGEMENT_FILE = 'gl-license-management-report.json'.freeze
       SAST_FILE = 'gl-sast-report.json'.freeze
@@ -34,17 +31,6 @@ module EE
         ::Gitlab::Database::LoadBalancing::Sticking.stick(:build, id)
       end
 
-      # has_codeclimate_json? is deprecated and replaced with has_code_quality_json? (#5779)
-      def has_codeclimate_json?
-        name_in?(%w[codeclimate codequality code_quality]) &&
-          has_artifact?(CODECLIMATE_FILE)
-      end
-
-      def has_code_quality_json?
-        name_in?(%w[codeclimate codequality code_quality]) &&
-          has_artifact?(CODE_QUALITY_FILE)
-      end
-
       def has_performance_json?
         name_in?(%w[performance deploy]) &&
           has_artifact?(PERFORMANCE_FILE)
@@ -87,13 +73,13 @@ module EE
         # See https://gitlab.com/gitlab-org/gitlab-ce/issues/46652
       end
 
-      private
-
       def has_artifact?(name)
         options.dig(:artifacts, :paths)&.include?(name) &&
           artifacts_metadata?
       end
 
+      private
+
       def name_in?(names)
         name.in?(Array(names))
       end

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

@@ -11,18 +11,39 @@ module EE
       prepended do
         has_one :chat_data, class_name: 'Ci::PipelineChatData'
 
+        has_many :job_artifacts, through: :builds
+
         scope :with_security_reports, -> {
           joins(:artifacts).where(ci_builds: { name: %w[sast dependency_scanning sast:container container_scanning dast] })
         }
+
+        # Deprecated, to be removed in 12.0
+        # A hash of Ci::JobArtifact file_types
+        # With mapping to the legacy job names,
+        # that has to contain given files
+        LEGACY_REPORT_FORMATS = {
+          codequality: {
+            names: %w(codeclimate codequality code_quality),
+            files: %w(codeclimate.json gl-code-quality-report.json)
+          }
+        }.freeze
       end
 
-      # codeclimate_artifact is deprecated and replaced with code_quality_artifact (#5779)
-      def codeclimate_artifact
-        @codeclimate_artifact ||= artifacts_with_files.find(&:has_codeclimate_json?)
+      def artifact_for_file_type(file_type)
+        job_artifacts.where(file_type: ::Ci::JobArtifact.file_types[file_type]).last
       end
 
-      def code_quality_artifact
-        @code_quality_artifact ||= artifacts_with_files.find(&:has_code_quality_json?)
+      def legacy_report_artifact_for_file_type(file_type)
+        legacy_names = LEGACY_REPORT_FORMATS[file_type]
+        return unless legacy_names
+
+        builds.success.latest.where(name: legacy_names[:names]).each do |build|
+          legacy_names[:files].each do |file_name|
+            next unless build.has_artifact?(file_name)
+
+            return OpenStruct.new(build: build, path: file_name)
+          end
+        end.last
       end
 
       def performance_artifact
@@ -83,15 +104,6 @@ module EE
         performance_artifact&.success?
       end
 
-      # has_codeclimate_data? is deprecated and replaced with has_code_quality_data? (#5779)
-      def has_codeclimate_data?
-        codeclimate_artifact&.success?
-      end
-
-      def has_code_quality_data?
-        code_quality_artifact&.success?
-      end
-
       def expose_sast_data?
         project.feature_available?(:sast) &&
           has_sast_data?
@@ -136,15 +148,6 @@ module EE
           expose_container_scanning_data?
       end
 
-      # expose_codeclimate_data? is deprecated and replaced with expose_code_quality_data? (#5779)
-      def expose_codeclimate_data?
-        has_codeclimate_data?
-      end
-
-      def expose_code_quality_data?
-        has_code_quality_data?
-      end
-
       private
 
       def artifacts_with_files

ee/app/models/ee/merge_request.rb

@@ -10,11 +10,6 @@ module EE
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
-      # codeclimate_artifact is deprecated and replaced with code_quality_artifact (#5779)
-      delegate :codeclimate_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
-      delegate :codeclimate_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
-      delegate :code_quality_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
-      delegate :code_quality_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
       delegate :performance_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
       delegate :performance_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
       delegate :sast_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
@@ -55,17 +50,6 @@ module EE
       false
     end
 
-    # expose_codeclimate_data? is deprecated and replaced with expose_code_quality_data? (#5779)
-    def expose_codeclimate_data?
-      !!(head_pipeline&.expose_codeclimate_data? &&
-         base_pipeline&.expose_codeclimate_data?)
-    end
-
-    def expose_code_quality_data?
-      !!(head_pipeline&.expose_code_quality_data? &&
-         base_pipeline&.expose_code_quality_data?)
-    end
-
     def expose_performance_data?
       !!(head_pipeline&.expose_performance_data? &&
          base_pipeline&.expose_performance_data?)

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

@@ -5,6 +5,24 @@ module EE
         activity_limit_exceeded: 'Pipeline activity limit exceeded!',
         size_limit_exceeded: 'Pipeline size limit exceeded!'
       }.freeze
+
+      def downloadable_url_for_report_type(file_type)
+        if (job_artifact = artifact_for_file_type(file_type)) &&
+            can?(current_user, :read_build, job_artifact.build)
+          return download_project_build_artifacts_url(
+            job_artifact.project,
+            job_artifact.build,
+            file_type: file_type)
+        end
+
+        if (build_artifact = legacy_report_artifact_for_file_type(file_type)) &&
+            can?(current_user, :read_build, build_artifact.build)
+          return raw_project_build_artifacts_url(
+            build_artifact.build.project,
+            build_artifact.build,
+            path: build_artifact.path)
+        end
+      end
     end
   end
 end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -16,33 +16,13 @@ module EE
         end
       end
 
-      # expose_codeclimate_data? is deprecated and replaced with expose_code_quality_data?
-      expose :codeclimate, if: -> (mr, _) { mr.expose_codeclimate_data? } do
-        expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_codeclimate_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.source_project,
-                                          merge_request.head_codeclimate_artifact,
-                                          path: Ci::Build::CODECLIMATE_FILE)
+      expose :codeclimate, if: -> (mr, _) { mr.head_pipeline&.present&.downloadable_url_for_report_type(:codequality) } do
+        expose :head_path do |merge_request|
+          merge_request.head_pipeline.present.downloadable_url_for_report_type(:codequality)
         end
 
-        expose :base_path, if: -> (mr, _) { can?(current_user, :read_build, mr.base_codeclimate_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.target_project,
-                                          merge_request.base_codeclimate_artifact,
-                                          path: Ci::Build::CODECLIMATE_FILE)
-        end
-      end
-
-      # We still expose it as `codeclimate` to keep compatibility with Frontend
-      expose :codeclimate, if: -> (mr, _) { mr.expose_code_quality_data? } do
-        expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_code_quality_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.source_project,
-                                          merge_request.head_code_quality_artifact,
-                                          path: Ci::Build::CODE_QUALITY_FILE)
-        end
-
-        expose :base_path, if: -> (mr, _) { can?(current_user, :read_build, mr.base_code_quality_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.target_project,
-                                          merge_request.base_code_quality_artifact,
-                                          path: Ci::Build::CODE_QUALITY_FILE)
+        expose :base_path do |merge_request|
+          merge_request.base_pipeline.present.downloadable_url_for_report_type(:codequality)
         end
       end
 

ee/spec/lib/gitlab/import_export/all_models.yml

@@ -15,6 +15,7 @@ pipelines:
 - triggered_by_pipeline
 - triggered_pipelines
 - chat_data
+- job_artifacts
 protected_branches:
 - unprotect_access_levels
 protected_environments:

ee/spec/models/ci/build_spec.rb

@@ -116,15 +116,6 @@ describe Ci::Build do
   end
 
   build_artifacts_methods = {
-    # has_codeclimate_json? is deprecated and replaced with code_quality_artifact (#5779)
-    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]

ee/spec/models/ci/pipeline_spec.rb

@@ -9,6 +9,7 @@ describe Ci::Pipeline do
   end
 
   it { is_expected.to have_one(:chat_data) }
+  it { is_expected.to have_many(:job_artifacts).through(:builds) }
 
   describe '.failure_reasons' do
     it 'contains failure reasons about exceeded limits' do
@@ -18,13 +19,6 @@ describe Ci::Pipeline do
   end
 
   PIPELINE_ARTIFACTS_METHODS = [
-    # codeclimate_artifact is deprecated and replaced with code_quality_artifact  (#5779)
-    { method: :codeclimate_artifact, options: [Ci::Build::CODECLIMATE_FILE, 'codeclimate'] },
-    { method: :codeclimate_artifact, options: [Ci::Build::CODECLIMATE_FILE, 'codequality'] },
-    { method: :codeclimate_artifact, options: [Ci::Build::CODECLIMATE_FILE, 'code_quality'] },
-    { method: :code_quality_artifact, options: [Ci::Build::CODE_QUALITY_FILE, 'codeclimate'] },
-    { method: :code_quality_artifact, options: [Ci::Build::CODE_QUALITY_FILE, 'codequality'] },
-    { method: :code_quality_artifact, options: [Ci::Build::CODE_QUALITY_FILE, 'code_quality'] },
     { method: :performance_artifact, options: [Ci::Build::PERFORMANCE_FILE, 'performance'] },
     { method: :sast_artifact, options: [Ci::Build::SAST_FILE, 'sast'] },
     { method: :dependency_scanning_artifact, options: [Ci::Build::DEPENDENCY_SCANNING_FILE, 'dependency_scanning'] },
@@ -70,7 +64,7 @@ describe Ci::Pipeline do
     end
   end
 
-  %w(sast dependency_scanning dast performance sast_container container_scanning codeclimate code_quality).each do |type|
+  %w(sast dependency_scanning dast performance sast_container container_scanning).each do |type|
     method = "has_#{type}_data?"
 
     describe "##{method}" do
@@ -84,7 +78,7 @@ describe Ci::Pipeline do
     end
   end
 
-  %w(sast dependency_scanning dast performance sast_container container_scanning codeclimate code_quality).each do |type|
+  %w(sast dependency_scanning dast performance sast_container container_scanning).each do |type|
     method = "expose_#{type}_data?"
 
     describe "##{method}" do
@@ -172,6 +166,44 @@ describe Ci::Pipeline do
     end
   end
 
+  describe '#artifact_for_file_type' do
+    let(:file_type) { :codequality }
+    let!(:build) { create(:ci_build, pipeline: pipeline) }
+    let!(:artifact) { create(:ci_job_artifact, :codequality, job: build) }
+    subject { pipeline.artifact_for_file_type(file_type) }
+
+    it 'returns the artifact' do
+      expect(subject).to eq(artifact)
+    end
+  end
+
+  describe '#legacy_report_artifact_for_file_type' do
+    let(:file_type) { :codequality }
+    let(:build_name) { ::EE::Ci::Pipeline::LEGACY_REPORT_FORMATS[file_type][:names].first }
+    let(:artifact_path) { ::EE::Ci::Pipeline::LEGACY_REPORT_FORMATS[file_type][:files].first }
+
+    let!(:build) do
+      create(
+        :ci_build,
+        :success,
+        :artifacts,
+        name: build_name,
+        pipeline: pipeline,
+        options: {
+          artifacts: {
+            paths: [artifact_path]
+          }
+        }
+      )
+    end
+
+    subject { pipeline.legacy_report_artifact_for_file_type(:codequality) }
+
+    it 'returns the artifact' do
+      expect(subject).to eq(OpenStruct.new(build: build, path: artifact_path))
+    end
+  end
+
   context 'performance' do
     def create_build(job_name, filename)
       create(
@@ -188,10 +220,10 @@ describe Ci::Pipeline do
     end
 
     it 'does not perform extra queries when calling pipeline artifacts methods after the first' do
-      create_build('codeclimate', 'codeclimate.json')
+      create_build('sast', Ci::Build::SAST_FILE)
       create_build('dependency_scanning', 'gl-dependency-scanning-report.json')
 
-      pipeline.code_quality_artifact
+      pipeline.sast_artifact
 
       expect { pipeline.dependency_scanning_artifact }.not_to exceed_query_limit(0)
     end

ee/spec/models/merge_request_spec.rb

@@ -41,28 +41,6 @@ describe MergeRequest do
     it { expect(subject.base_pipeline).to eq(pipeline) }
   end
 
-  describe '#base_codeclimate_artifact' do
-    before do
-      allow(subject.base_pipeline).to receive(:codeclimate_artifact)
-        .and_return(1)
-    end
-
-    it 'delegates to merge request diff' do
-      expect(subject.base_codeclimate_artifact).to eq(1)
-    end
-  end
-
-  describe '#head_codeclimate_artifact' do
-    before do
-      allow(subject.head_pipeline).to receive(:codeclimate_artifact)
-        .and_return(1)
-    end
-
-    it 'delegates to merge request diff' do
-      expect(subject.head_codeclimate_artifact).to eq(1)
-    end
-  end
-
   describe '#base_performance_artifact' do
     before do
       allow(subject.base_pipeline).to receive(:performance_artifact)
@@ -92,40 +70,6 @@ describe MergeRequest do
     it { is_expected.to delegate_method(:"#{type}_artifact").to(:base_pipeline).with_prefix(:base) }
   end
 
-  describe '#expose_codeclimate_data?' do
-    context 'with codeclimate data' do
-      let(:pipeline) { double(expose_codeclimate_data?: true) }
-
-      before do
-        allow(subject).to receive(:head_pipeline).and_return(pipeline)
-        allow(subject).to receive(:base_pipeline).and_return(pipeline)
-      end
-
-      it { expect(subject.expose_codeclimate_data?).to be_truthy }
-    end
-
-    context 'without codeclimate data' do
-      it { expect(subject.expose_codeclimate_data?).to be_falsey }
-    end
-  end
-
-  describe '#expose_code_quality_data?' do
-    context 'with code_quality data' do
-      let(:pipeline) { double(expose_code_quality_data?: true) }
-
-      before do
-        allow(subject).to receive(:head_pipeline).and_return(pipeline)
-        allow(subject).to receive(:base_pipeline).and_return(pipeline)
-      end
-
-      it { expect(subject.expose_code_quality_data?).to be_truthy }
-    end
-
-    context 'without code_quality data' do
-      it { expect(subject.expose_code_quality_data?).to be_falsey }
-    end
-  end
-
   describe '#expose_performance_data?' do
     context 'with performance data' do
       let(:pipeline) { double(expose_performance_data?: true) }

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -26,29 +26,12 @@ describe MergeRequestWidgetEntity do
     expect(subject.as_json[:blob_path]).to include(:head_path)
   end
 
-  # methods for old artifact are deprecated and replaced with ones for the new name (#5779)
-  it 'has codeclimate data (with old artifact name codeclimate,json)' do
-    build = create(:ci_build, name: 'job')
-
+  it 'has codeclimate data' do
     allow(merge_request).to receive_messages(
-      expose_codeclimate_data?: true,
-      expose_security_dashboard?: false,
-      base_codeclimate_artifact: build,
-      head_codeclimate_artifact: build
-    )
-
-    expect(subject.as_json).to include(:codeclimate)
-  end
-
-  it 'has codeclimate data (with new artifact name gl-code-quality-report.json)' do
-    build = create(:ci_build, name: 'job')
-
-    allow(merge_request).to receive_messages(
-      expose_code_quality_data?: true,
-      expose_security_dashboard?: false,
-      base_code_quality_artifact: build,
-      head_code_quality_artifact: build
+      base_pipeline: pipeline,
+      head_pipeline: pipeline
     )
+    allow_any_instance_of(EE::Ci::PipelinePresenter).to receive(:downloadable_url_for_report_type).and_return("dummy/url")
 
     expect(subject.as_json).to include(:codeclimate)
   end