Refactor schema and use proper insert query

Use INSERT DO UPDATE for better handling of conflicts instead
of using with_lock.

app/models/ci/daily_code_coverage.rb

+# frozen_string_literal: true
+
+module Ci
+  class DailyCodeCoverage < ApplicationRecord
+    extend Gitlab::Ci::Model
+
+    def self.create_or_update_for_build(build)
+      ref = connection.quote(build.ref)
+      name = connection.quote(build.name)
+      date = connection.quote(build.created_at.to_date)
+
+      connection.execute <<-EOF.strip_heredoc
+        INSERT INTO #{table_name} (project_id, ref, name, date, last_build_id, coverage)
+        VALUES (#{build.project_id}, #{ref}, #{name}, #{date}, #{build.id}, #{build.coverage})
+        ON CONFLICT (project_id, ref, name, date)
+        DO UPDATE SET coverage = #{build.coverage}, last_build_id = #{build.id} WHERE #{table_name}.last_build_id < #{build.id};
+      EOF
+    end
+  end
+end

app/models/ci/pipeline.rb

@@ -189,7 +189,10 @@ module Ci
       end
 
       after_transition [:created, :waiting_for_resource, :preparing, :pending, :running] => :success do |pipeline|
-        pipeline.run_after_commit { PipelineSuccessWorker.perform_async(pipeline.id) }
+        # We want to wait a little bit to ensure that all BuildFinishedWorkers finish first
+        # because this is where code coverage is parsed and stored in CI build records which
+        # the daily code coverage worker relies on.
+        pipeline.run_after_commit { Ci::DailyCodeCoverageWorker.perform_in(5.minutes, pipeline.id) }
       end
 
       after_transition do |pipeline, transition|

app/models/daily_code_coverage.rb

-# frozen_string_literal: true
-
-class DailyCodeCoverage < ApplicationRecord
-  validates :project_id, presence: true, uniqueness: { scope: [:ref, :name, :date], case_sensitive: false }
-  validates :last_pipeline_id, presence: true
-  validates :ref, presence: true
-  validates :name, presence: true
-  validates :coverage, presence: true
-  validates :date, presence: true
-  validate :newer_pipeline
-
-  private
-
-  def newer_pipeline
-    return if new_record?
-    return unless last_pipeline_id_changed?
-
-    old_pipeline_id, new_pipeline_id = last_pipeline_id_change
-    return if new_pipeline_id > old_pipeline_id
-
-    errors.add(:last_pipeline_id, 'new pipeline ID must be newer than the existing one')
-  end
-end

app/services/ci/daily_code_coverage_service.rb

@@ -3,27 +3,11 @@
 module Ci
   class DailyCodeCoverageService
     def execute(pipeline)
+      return unless Feature.enabled?(:ci_daily_code_coverage, default_enabled: true)
+
       pipeline.builds.with_coverage.each do |build|
-        daily_coverage = daily_coverage_for(pipeline, build)
-        daily_coverage.with_lock do
-          daily_coverage.coverage = build.coverage
-          daily_coverage.last_pipeline_id = pipeline.id
-          daily_coverage.save
-        end
-      end
+        DailyCodeCoverage.create_or_update_for_build(build)
       end
-
-    private
-
-    def daily_coverage_for(pipeline, build)
-      # rubocop: disable CodeReuse/ActiveRecord
-      DailyCodeCoverage.find_or_initialize_by(
-        project_id: pipeline.project_id,
-        ref: pipeline.ref,
-        name: build.name,
-        date: pipeline.created_at.to_date
-      )
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

app/workers/all_queues.yml

@@ -605,7 +605,7 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: 
-- :name: pipeline_background:daily_code_coverage
+- :name: pipeline_background:ci_daily_code_coverage
   :feature_category: :continuous_integration
   :has_external_dependencies: 
   :urgency: :default

app/workers/ci/daily_code_coverage_worker.rb

+# frozen_string_literal: true
+
+module Ci
+  class DailyCodeCoverageWorker
+    include ApplicationWorker
+    include PipelineBackgroundQueue
+
+    def perform(pipeline_id)
+      Ci::Pipeline.find_by_id(pipeline_id).try do |pipeline|
+        Ci::DailyCodeCoverageService.new.execute(pipeline)
+      end
+    end
+  end
+end

app/workers/daily_code_coverage_worker.rb

-# frozen_string_literal: true
-
-class DailyCodeCoverageWorker
-  include ApplicationWorker
-  include PipelineBackgroundQueue
-
-  # rubocop: disable CodeReuse/ActiveRecord
-  def perform(pipeline_id)
-    Ci::Pipeline.find_by(id: pipeline_id).try do |pipeline|
-      Ci::DailyCodeCoverageService.new.execute(pipeline)
-    end
-  end
-  # rubocop: enable CodeReuse/ActiveRecord
-end

db/migrate/20200204131831_create_daily_code_coverages.rb

@@ -4,17 +4,18 @@ class CreateDailyCodeCoverages < ActiveRecord::Migration[6.0]
   DOWNTIME = false
 
   def change
-    create_table :daily_code_coverages do |t|
+    create_table :ci_daily_code_coverages do |t|
       t.date :date, null: false
-      t.integer :project_id, null: false
-      t.integer :last_pipeline_id, null: false
+      t.bigint :project_id, null: false
+      t.bigint :last_build_id, null: false
       t.float :coverage, null: false
       t.string :ref, null: false # rubocop:disable Migration/AddLimitToStringColumns
       t.string :name, null: false # rubocop:disable Migration/AddLimitToStringColumns
 
+      t.index :last_build_id
       t.index [:project_id, :ref, :name, :date], name: 'index_daily_code_coverage_unique_columns', unique: true, order: { date: :desc }
       t.foreign_key :projects, on_delete: :cascade
-      t.foreign_key :ci_pipelines, column: :last_pipeline_id, on_delete: :cascade
+      t.foreign_key :ci_builds, column: :last_build_id, on_delete: :cascade
     end
   end
 end

db/schema.rb

@@ -738,6 +738,17 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
     t.index ["build_id"], name: "index_ci_builds_runner_session_on_build_id", unique: true
   end
 
+  create_table "ci_daily_code_coverages", force: :cascade do |t|
+    t.date "date", null: false
+    t.bigint "project_id", null: false
+    t.bigint "last_build_id", null: false
+    t.float "coverage", null: false
+    t.string "ref", null: false
+    t.string "name", null: false
+    t.index ["last_build_id"], name: "index_ci_daily_code_coverages_on_last_build_id"
+    t.index ["project_id", "ref", "name", "date"], name: "index_daily_code_coverage_unique_columns", unique: true, order: { date: :desc }
+  end
+
   create_table "ci_group_variables", id: :serial, force: :cascade do |t|
     t.string "key", null: false
     t.text "value"
@@ -1327,16 +1338,6 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
     t.float "percentage_service_desk_issues", default: 0.0, null: false
   end
 
-  create_table "daily_code_coverages", force: :cascade do |t|
-    t.date "date", null: false
-    t.integer "project_id", null: false
-    t.integer "last_pipeline_id", null: false
-    t.float "coverage", null: false
-    t.string "ref", null: false
-    t.string "name", null: false
-    t.index ["project_id", "ref", "name", "date"], name: "index_daily_code_coverage_unique_columns", unique: true, order: { date: :desc }
-  end
-
   create_table "dependency_proxy_blobs", id: :serial, force: :cascade do |t|
     t.integer "group_id", null: false
     t.datetime_with_timezone "created_at", null: false
@@ -4775,6 +4776,8 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
   add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", on_delete: :cascade
   add_foreign_key "ci_builds_metadata", "projects", on_delete: :cascade
   add_foreign_key "ci_builds_runner_session", "ci_builds", column: "build_id", on_delete: :cascade
+  add_foreign_key "ci_daily_code_coverages", "ci_builds", column: "last_build_id", on_delete: :cascade
+  add_foreign_key "ci_daily_code_coverages", "projects", on_delete: :cascade
   add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade
   add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade
   add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade
@@ -4842,8 +4845,6 @@ ActiveRecord::Schema.define(version: 2020_03_13_123934) do
   add_foreign_key "commit_user_mentions", "notes", on_delete: :cascade
   add_foreign_key "container_expiration_policies", "projects", on_delete: :cascade
   add_foreign_key "container_repositories", "projects"
-  add_foreign_key "daily_code_coverages", "ci_pipelines", column: "last_pipeline_id", on_delete: :cascade
-  add_foreign_key "daily_code_coverages", "projects", on_delete: :cascade
   add_foreign_key "dependency_proxy_blobs", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "dependency_proxy_group_settings", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "deploy_keys_projects", "projects", name: "fk_58a901ca7e", on_delete: :cascade

spec/factories/daily_code_coverages.rb → spec/factories/ci/daily_code_coverages.rb

 # frozen_string_literal: true
 
 FactoryBot.define do
-  factory :daily_code_coverage do
+  factory :ci_daily_code_coverage, class: 'Ci::DailyCodeCoverage' do
     ref { 'test_branch' }
     name { 'test_coverage_job' }
     coverage { 77 }
     date { Time.zone.now.to_date }
 
     after(:build) do |dcc|
-      pipeline = create(:ci_pipeline)
+      build = create(:ci_build)
 
-      dcc.project_id = pipeline.project_id unless dcc.project_id
-      dcc.last_pipeline_id = pipeline.id unless dcc.last_pipeline_id
+      dcc.project_id = build.project_id unless dcc.project_id
+      dcc.last_build_id = build.id unless dcc.last_build_id
     end
   end
 end

spec/models/ci/daily_code_coverage_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::DailyCodeCoverage do
+  describe '::create_or_update_for_build' do
+    let!(:build) { create(:ci_build, created_at: '2020-02-06 00:01:10', name: 'rspec', coverage: 80) }
+
+    context 'when there is no existing record with matching project_id, ref, name, date' do
+      it 'creates a new record for the given build' do
+        described_class.create_or_update_for_build(build)
+
+        expect(described_class.last).to have_attributes(
+          project_id: build.project.id,
+          last_build_id: build.id,
+          ref: build.ref,
+          name: build.name,
+          coverage: build.coverage,
+          date: build.created_at.to_date
+        )
+      end
+    end
+
+    context 'when there is existing record with matching project_id, ref, name, date' do
+      let!(:new_build) { create(:ci_build, project: build.project, created_at: build.created_at, ref: build.ref, name: build.name, coverage: 99) }
+      let!(:existing) do
+        create(
+          :ci_daily_code_coverage,
+          project_id: existing_build.project.id,
+          last_build_id: existing_build.id,
+          ref: existing_build.ref,
+          name: existing_build.name,
+          coverage: existing_build.coverage,
+          date: existing_build.created_at.to_date
+        )
+      end
+
+      context 'and build ID is newer than last_build_id' do
+        let(:existing_build) { build }
+
+        it 'updates the last_build_id and coverage' do
+          described_class.create_or_update_for_build(new_build)
+
+          existing.reload
+
+          expect(existing).to have_attributes(
+            last_build_id: new_build.id,
+            coverage: new_build.coverage
+          )
+        end
+      end
+
+      context 'and build ID is not newer than last_build_id' do
+        let(:existing_build) { new_build }
+
+        it 'does not update the last_build_id and coverage' do
+          described_class.create_or_update_for_build(build)
+
+          existing.reload
+
+          expect(existing).to have_attributes(
+            last_build_id: new_build.id,
+            coverage: new_build.coverage
+          )
+        end
+      end
+    end
+  end
+end

spec/models/ci/pipeline_spec.rb

@@ -1120,7 +1120,7 @@ describe Ci::Pipeline, :mailer do
           let(:from_status) { status }
 
           it 'schedules pipeline success worker' do
-            expect(PipelineSuccessWorker).to receive(:perform_async).with(pipeline.id)
+            expect(Ci::DailyCodeCoverageWorker).to receive(:perform_in).with(5.minutes, pipeline.id)
 
             pipeline.succeed
           end

spec/models/daily_code_coverage_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe DailyCodeCoverage do
-  describe 'validation' do
-    subject { described_class.new }
-
-    it { is_expected.to validate_presence_of(:project_id) }
-    it { is_expected.to validate_presence_of(:last_pipeline_id) }
-    it { is_expected.to validate_presence_of(:ref) }
-    it { is_expected.to validate_presence_of(:name) }
-    it { is_expected.to validate_presence_of(:coverage) }
-    it { is_expected.to validate_presence_of(:date) }
-
-    context 'uniqueness' do
-      before do
-        create(:daily_code_coverage)
-      end
-
-      it { is_expected.to validate_uniqueness_of(:project_id).scoped_to([:ref, :name, :date]) }
-    end
-
-    context 'ensuring newer pipeline' do
-      context 'on new records' do
-        subject { build(:daily_code_coverage, last_pipeline_id: 1) }
-
-        it { is_expected.to be_valid }
-      end
-
-      context 'on existing records' do
-        subject { create(:daily_code_coverage, last_pipeline_id: 12) }
-
-        context 'and new pipeline ID is older' do
-          before do
-            subject.last_pipeline_id = 10
-          end
-
-          it { is_expected.not_to be_valid }
-        end
-
-        context 'and new pipeline ID is newer' do
-          before do
-            subject.last_pipeline_id = 15
-          end
-
-          it { is_expected.to be_valid }
-        end
-      end
-    end
-  end
-end

spec/services/ci/daily_code_coverage_service_spec.rb

@@ -3,37 +3,37 @@
 require 'spec_helper'
 
 describe Ci::DailyCodeCoverageService, '#execute' do
-  let!(:pipeline) { create(:ci_pipeline, created_at: '2020-02-06 00:01:10') }
-  let!(:rspec_job) { create(:ci_build, pipeline: pipeline, name: 'rspec', coverage: 80) }
-  let!(:karma_job) { create(:ci_build, pipeline: pipeline, name: 'karma', coverage: 90) }
-  let!(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) }
+  let!(:pipeline) { create(:ci_pipeline) }
+  let!(:rspec_job) { create(:ci_build, pipeline: pipeline, created_at: '2020-02-06 00:01:10', name: 'rspec', coverage: 80) }
+  let!(:karma_job) { create(:ci_build, pipeline: pipeline, created_at: '2020-02-06 00:01:12', name: 'karma', coverage: 90) }
+  let!(:extra_job) { create(:ci_build, pipeline: pipeline, created_at: '2020-02-06 00:01:14', name: 'extra', coverage: nil) }
 
   it 'creates daily code coverage record for each job in the pipeline that has coverage value' do
     described_class.new.execute(pipeline)
 
-    DailyCodeCoverage.find_by(name: 'rspec').tap do |coverage|
+    Ci::DailyCodeCoverage.find_by(name: 'rspec').tap do |coverage|
       expect(coverage).to have_attributes(
         project_id: pipeline.project.id,
-        last_pipeline_id: pipeline.id,
+        last_build_id: rspec_job.id,
         ref: pipeline.ref,
         name: rspec_job.name,
         coverage: rspec_job.coverage,
-        date: pipeline.created_at.to_date
+        date: rspec_job.created_at.to_date
       )
     end
 
-    DailyCodeCoverage.find_by(name: 'karma').tap do |coverage|
+    Ci::DailyCodeCoverage.find_by(name: 'karma').tap do |coverage|
       expect(coverage).to have_attributes(
         project_id: pipeline.project.id,
-        last_pipeline_id: pipeline.id,
+        last_build_id: karma_job.id,
         ref: pipeline.ref,
         name: karma_job.name,
         coverage: karma_job.coverage,
-        date: pipeline.created_at.to_date
+        date: karma_job.created_at.to_date
       )
     end
 
-    expect(DailyCodeCoverage.find_by(name: 'extra')).to be_nil
+    expect(Ci::DailyCodeCoverage.find_by(name: 'extra')).to be_nil
   end
 
   context 'when there is an existing daily code coverage for the matching date, project, ref, and name' do
@@ -41,21 +41,20 @@ describe Ci::DailyCodeCoverageService, '#execute' do
       create(
         :ci_pipeline,
         project: pipeline.project,
-        ref: pipeline.ref,
-        created_at: '2020-02-06 00:02:20'
+        ref: pipeline.ref
       )
     end
-    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: 'rspec', coverage: 84) }
-    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: 'karma', coverage: 92) }
+    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, created_at: '2020-02-06 00:02:20', name: 'rspec', coverage: 84) }
+    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, created_at: '2020-02-06 00:02:22', name: 'karma', coverage: 92) }
 
     before do
       # Create the existing daily code coverage records
       described_class.new.execute(pipeline)
     end
 
-    it "updates the existing record's coverage value and last_pipeline_id" do
-      rspec_coverage = DailyCodeCoverage.find_by(name: 'rspec')
-      karma_coverage = DailyCodeCoverage.find_by(name: 'karma')
+    it "updates the existing record's coverage value and last_build_id" do
+      rspec_coverage = Ci::DailyCodeCoverage.find_by(name: 'rspec')
+      karma_coverage = Ci::DailyCodeCoverage.find_by(name: 'karma')
 
       # Bump up the coverage values
       described_class.new.execute(new_pipeline)
@@ -64,28 +63,27 @@ describe Ci::DailyCodeCoverageService, '#execute' do
       karma_coverage.reload
 
       expect(rspec_coverage).to have_attributes(
-        last_pipeline_id: new_pipeline.id,
+        last_build_id: new_rspec_job.id,
         coverage: new_rspec_job.coverage
       )
 
       expect(karma_coverage).to have_attributes(
-        last_pipeline_id: new_pipeline.id,
+        last_build_id: new_karma_job.id,
         coverage: new_karma_job.coverage
       )
     end
   end
 
-  context 'when the ID of the given pipeline is older than the last_pipeline_id' do
+  context 'when the ID of the build is older than the last_build_id' do
     let!(:new_pipeline) do
       create(
         :ci_pipeline,
         project: pipeline.project,
-        ref: pipeline.ref,
-        created_at: '2020-02-06 00:02:20'
+        ref: pipeline.ref
       )
     end
-    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: 'rspec', coverage: 84) }
-    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: 'karma', coverage: 92) }
+    let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, created_at: '2020-02-06 00:02:20', name: 'rspec', coverage: 84) }
+    let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, created_at: '2020-02-06 00:02:22', name: 'karma', coverage: 92) }
 
     before do
       # Create the existing daily code coverage records
@@ -94,8 +92,8 @@ describe Ci::DailyCodeCoverageService, '#execute' do
     end
 
     it 'does not update the existing daily code coverage records' do
-      rspec_coverage = DailyCodeCoverage.find_by(name: 'rspec')
-      karma_coverage = DailyCodeCoverage.find_by(name: 'karma')
+      rspec_coverage = Ci::DailyCodeCoverage.find_by(name: 'rspec')
+      karma_coverage = Ci::DailyCodeCoverage.find_by(name: 'karma')
 
       # Run another one but for the older pipeline.
       # This simulates the scenario wherein the success worker
@@ -108,12 +106,12 @@ describe Ci::DailyCodeCoverageService, '#execute' do
       karma_coverage.reload
 
       expect(rspec_coverage).to have_attributes(
-        last_pipeline_id: new_pipeline.id,
+        last_build_id: new_rspec_job.id,
         coverage: new_rspec_job.coverage
       )
 
       expect(karma_coverage).to have_attributes(
-        last_pipeline_id: new_pipeline.id,
+        last_build_id: new_karma_job.id,
         coverage: new_karma_job.coverage
       )
     end

spec/workers/daily_code_coverage_worker_spec.rb → spec/workers/ci/daily_code_coverage_worker_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe DailyCodeCoverageWorker do
+describe Ci::DailyCodeCoverageWorker do
   describe '#perform' do
     let!(:pipeline) { create(:ci_pipeline) }
 

spec/workers/pipeline_success_worker_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe PipelineSuccessWorker do
-  describe '#perform' do
-    context 'when pipeline exists' do
-      let(:pipeline) { create(:ci_pipeline) }
-
-      it 'asynchronously executes the daily code coverage worker' do
-        expect(DailyCodeCoverageWorker)
-          .to receive(:perform_async).with(pipeline.id)
-
-        described_class.new.perform(pipeline.id)
-      end
-    end
-
-    context 'when pipeline does not exist' do
-      it 'does not raise exception' do
-        expect { described_class.new.perform(123) }
-          .not_to raise_error
-      end
-    end
-  end
-end