Use upsert_all to simplify code and less IO

Use Rails 6 upsert_all to reduce queries and simplify code.

app/models/ci/daily_report_result.rb

@@ -13,25 +13,27 @@ module Ci
       coverage: 12
     }.freeze
 
-    enum param: REPORT_PARAMS
+    enum param_type: REPORT_PARAMS
 
     def self.store_coverage(pipeline)
       return unless Feature.enabled?(:ci_daily_code_coverage, default_enabled: true)
 
-      ref_path = connection.quote(pipeline.source_ref_path)
-      date = connection.quote(pipeline.created_at.to_date)
-      param = params[:coverage]
-
-      pipeline.builds.with_coverage.each do |build|
-        title = connection.quote(build.group_name)
-
-        connection.execute <<-EOF.strip_heredoc
-          INSERT INTO #{table_name} (project_id, ref_path, param, title, date, last_pipeline_id, value)
-          VALUES (#{build.project_id}, #{ref_path}, #{param}, #{title}, #{date}, #{pipeline.id}, #{build.coverage})
-          ON CONFLICT (project_id, ref_path, param, title, date)
-          DO UPDATE SET value = #{build.coverage}, last_pipeline_id = #{pipeline.id} WHERE #{table_name}.last_pipeline_id < #{pipeline.id};
-        EOF
+      base_attrs = {
+        project_id: pipeline.project_id,
+        ref_path: pipeline.source_ref_path,
+        param_type: param_types[:coverage],
+        date: pipeline.created_at.to_date,
+        last_pipeline_id: pipeline.id
+      }
+
+      data = pipeline.builds.with_coverage.map do |build|
+        base_attrs.merge(
+          title: build.group_name,
+          value: build.coverage
+        )
       end
+
+      upsert_all(data, unique_by: :index_daily_report_results_unique_columns) if data.any?
     end
   end
 end

app/models/ci/pipeline.rb

@@ -948,9 +948,9 @@ module Ci
 
     def source_ref_path
       if branch? || merge_request?
-        Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
+        Gitlab::Git::BRANCH_REF_PREFIX + source_ref.to_s
       elsif tag?
-        Gitlab::Git::TAG_REF_PREFIX + ref.to_s
+        Gitlab::Git::TAG_REF_PREFIX + source_ref.to_s
       end
     end
 

db/migrate/20200204131831_create_daily_report_results.rb

@@ -9,12 +9,12 @@ class CreateDailyReportResults < ActiveRecord::Migration[6.0]
       t.bigint :project_id, null: false
       t.bigint :last_pipeline_id, null: false
       t.float :value, null: false
-      t.integer :param, limit: 2, null: false
+      t.integer :param_type, limit: 2, null: false
       t.string :ref_path, null: false # rubocop:disable Migration/AddLimitToStringColumns
       t.string :title, null: false # rubocop:disable Migration/AddLimitToStringColumns
 
       t.index :last_pipeline_id
-      t.index [:project_id, :ref_path, :param, :date, :title], name: 'index_daily_report_results_unique_columns', unique: true
+      t.index [:project_id, :ref_path, :param_type, :date, :title], name: 'index_daily_report_results_unique_columns', unique: true
       t.foreign_key :projects, on_delete: :cascade
       t.foreign_key :ci_pipelines, column: :last_pipeline_id, on_delete: :cascade
     end

db/schema.rb

@@ -743,11 +743,11 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
     t.bigint "project_id", null: false
     t.bigint "last_pipeline_id", null: false
     t.float "value", null: false
-    t.integer "param", limit: 2, null: false
+    t.integer "param_type", limit: 2, null: false
     t.string "ref_path", null: false
     t.string "title", null: false
     t.index ["last_pipeline_id"], name: "index_ci_daily_report_results_on_last_pipeline_id"
-    t.index ["project_id", "ref_path", "param", "date", "title"], name: "index_daily_report_results_unique_columns", unique: true
+    t.index ["project_id", "ref_path", "param_type", "date", "title"], name: "index_daily_report_results_unique_columns", unique: true
   end
 
   create_table "ci_group_variables", id: :serial, force: :cascade do |t|

spec/factories/ci/daily_report_results.rb

@@ -6,7 +6,7 @@ FactoryBot.define do
     date { Time.zone.now.to_date }
     project
     last_pipeline factory: :ci_pipeline
-    param { Ci::DailyReportResult.params[:coverage] }
+    param_type { Ci::DailyReportResult.param_types[:coverage] }
     title { 'rspec' }
     value { 77.0 }
   end

spec/models/ci/daily_report_result_spec.rb

@@ -17,7 +17,7 @@ describe Ci::DailyReportResult do
           project_id: pipeline.project.id,
           last_pipeline_id: pipeline.id,
           ref_path: pipeline.source_ref_path,
-          param: 'coverage',
+          param_type: 'coverage',
           title: rspec_job.group_name,
           value: rspec_job.coverage,
           date: pipeline.created_at.to_date
@@ -29,7 +29,7 @@ describe Ci::DailyReportResult do
           project_id: pipeline.project.id,
           last_pipeline_id: pipeline.id,
           ref_path: pipeline.source_ref_path,
-          param: 'coverage',
+          param_type: 'coverage',
           title: karma_job.group_name,
           value: karma_job.coverage,
           date: pipeline.created_at.to_date
@@ -96,7 +96,7 @@ describe Ci::DailyReportResult do
         described_class.store_coverage(new_pipeline)
       end
 
-      it 'does not update the existing daily code coverage records' do
+      it 'updates the existing daily code coverage records' do
         rspec_coverage = Ci::DailyReportResult.find_by(title: 'rspec')
         karma_coverage = Ci::DailyReportResult.find_by(title: 'karma')
 
@@ -104,22 +104,39 @@ describe Ci::DailyReportResult do
         # This simulates the scenario wherein the success worker
         # of an older pipeline, for some network hiccup, was delayed
         # and only got executed right after the newer pipeline's success worker.
-        # In this case, we don't want to bump the coverage value with an older one.
+        # Ideally, we don't want to bump the coverage value with an older one
+        # but given this can be a rare edge case and can be remedied by re-running
+        # the pipeline we'll just let it be for now. In return, we are able to use
+        # Rails 6 shiny new method, upsert_all, and simplify the code a lot.
         described_class.store_coverage(pipeline)
 
         rspec_coverage.reload
         karma_coverage.reload
 
         expect(rspec_coverage).to have_attributes(
-          last_pipeline_id: new_pipeline.id,
-          value: new_rspec_job.coverage
+          last_pipeline_id: pipeline.id,
+          value: rspec_job.coverage
         )
 
         expect(karma_coverage).to have_attributes(
-          last_pipeline_id: new_pipeline.id,
-          value: new_karma_job.coverage
+          last_pipeline_id: pipeline.id,
+          value: karma_job.coverage
+        )
+      end
+    end
+
+    context 'when pipeline has no builds with coverage' do
+      let!(:new_pipeline) do
+        create(
+          :ci_pipeline,
+          created_at: '2020-02-06 00:02:20'
         )
       end
+      let!(:some_job) { create(:ci_build, pipeline: new_pipeline, name: 'foo') }
+
+      it 'does nothing' do
+        expect { described_class.store_coverage(new_pipeline) }.not_to raise_error
+      end
     end
   end
 end

spec/models/ci/pipeline_spec.rb

@@ -3119,20 +3119,20 @@ describe Ci::Pipeline, :mailer do
     subject { pipeline.source_ref_path }
 
     context 'when pipeline is for a branch' do
-      it { is_expected.to eq(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref.to_s) }
+      it { is_expected.to eq(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.source_ref.to_s) }
     end
 
     context 'when pipeline is for a merge request' do
       let(:merge_request) { create(:merge_request, source_project: project) }
       let(:pipeline) { create(:ci_pipeline, project: project, head_pipeline_of: merge_request) }
 
-      it { is_expected.to eq(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref.to_s) }
+      it { is_expected.to eq(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.source_ref.to_s) }
     end
 
     context 'when pipeline is for a tag' do
       let(:pipeline) { create(:ci_pipeline, project: project, tag: true) }
 
-      it { is_expected.to eq(Gitlab::Git::TAG_REF_PREFIX + pipeline.ref.to_s) }
+      it { is_expected.to eq(Gitlab::Git::TAG_REF_PREFIX + pipeline.source_ref.to_s) }
     end
   end
 end