Merge branch 'ac-refactor-project-stats' into 'master'

Extract ProjectStatistics updates into a concern

See merge request gitlab-org/gitlab-ce!27420

app/models/ci/build.rb

@@ -15,6 +15,7 @@ module Ci
     include Gitlab::Utils::StrongMemoize
     include Deployable
     include HasRef
+    include UpdateProjectStatistics
 
     BuildArchivedError = Class.new(StandardError)
 
@@ -157,8 +158,7 @@ module Ci
       run_after_commit { BuildHooksWorker.perform_async(build.id) }
     end
 
-    after_save :update_project_statistics_after_save, if: :artifacts_size_changed?
-    after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed?
+    update_project_statistics stat: :build_artifacts_size, attribute: :artifacts_size
 
     class << self
       # This is needed for url_for to work,
@@ -847,21 +847,5 @@ module Ci
 
       pipeline.config_processor.build_attributes(name)
     end
-
-    def update_project_statistics_after_save
-      update_project_statistics(read_attribute(:artifacts_size).to_i - artifacts_size_was.to_i)
-    end
-
-    def update_project_statistics_after_destroy
-      update_project_statistics(-artifacts_size)
-    end
-
-    def update_project_statistics(difference)
-      ProjectStatistics.increment_statistic(project_id, :build_artifacts_size, difference)
-    end
-
-    def project_destroyed?
-      project.pending_delete?
-    end
   end
 end

app/models/ci/job_artifact.rb

@@ -4,6 +4,7 @@ module Ci
   class JobArtifact < ApplicationRecord
     include AfterCommitQueue
     include ObjectStorage::BackgroundMove
+    include UpdateProjectStatistics
     extend Gitlab::Ci::Model
 
     NotSupportedAdapterError = Class.new(StandardError)
@@ -52,8 +53,8 @@ module Ci
     validates :file_format, presence: true, unless: :trace?, on: :create
     validate :valid_file_format?, unless: :trace?, on: :create
     before_save :set_size, if: :file_changed?
-    after_save :update_project_statistics_after_save, if: :size_changed?
-    after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed?
+
+    update_project_statistics stat: :build_artifacts_size
 
     after_save :update_file_store, if: :file_changed?
 
@@ -176,18 +177,6 @@ module Ci
       self.size = file.size
     end
 
-    def update_project_statistics_after_save
-      update_project_statistics(size.to_i - size_was.to_i)
-    end
-
-    def update_project_statistics_after_destroy
-      update_project_statistics(-self.size.to_i)
-    end
-
-    def update_project_statistics(difference)
-      ProjectStatistics.increment_statistic(project_id, :build_artifacts_size, difference)
-    end
-
     def project_destroyed?
       # Use job.project to avoid extra DB query for project
       job.project.pending_delete?

app/models/concerns/update_project_statistics.rb

+# frozen_string_literal: true
+
+# This module is providing helpers for updating `ProjectStatistics` with `after_save` and `before_destroy` hooks.
+#
+# It deals with `ProjectStatistics.increment_statistic` making sure not to update statistics on a cascade delete from the
+# project, and keeping track of value deltas on each save. It updates the DB only when a change is needed.
+#
+# How to use
+# - Invoke `update_project_statistics stat: :a_project_statistics_column, attribute: :an_attr_to_track` in a model class body.
+#
+# Expectation
+# - `attribute` must be an ActiveRecord attribute
+# - The model must implement `project` and `project_id`. i.e. direct Project relationship or delegation
+module UpdateProjectStatistics
+  extend ActiveSupport::Concern
+
+  class_methods do
+    attr_reader :statistic_name, :statistic_attribute
+
+    # Configure the model to update +stat+ on ProjectStatistics when +attribute+ changes
+    #
+    # +stat+:: a column of ProjectStatistics to update
+    # +attribute+:: an attribute of the current model, default to +:size+
+    def update_project_statistics(stat:, attribute: :size)
+      @statistic_name = stat
+      @statistic_attribute = attribute
+
+      after_save(:update_project_statistics_after_save, if: :update_project_statistics_attribute_changed?)
+      after_destroy(:update_project_statistics_after_destroy, unless: :project_destroyed?)
+    end
+    private :update_project_statistics
+  end
+
+  included do
+    private
+
+    def project_destroyed?
+      project.pending_delete?
+    end
+
+    def update_project_statistics_attribute_changed?
+      attribute_changed?(self.class.statistic_attribute)
+    end
+
+    def update_project_statistics_after_save
+      attr = self.class.statistic_attribute
+      delta = read_attribute(attr).to_i - attribute_was(attr).to_i
+
+      update_project_statistics(delta)
+    end
+
+    def update_project_statistics_after_destroy
+      update_project_statistics(-read_attribute(self.class.statistic_attribute).to_i)
+    end
+
+    def update_project_statistics(delta)
+      ProjectStatistics.increment_statistic(project_id, self.class.statistic_name, delta)
+    end
+  end
+end

spec/models/ci/build_spec.rb

@@ -31,6 +31,10 @@ describe Ci::Build do
 
   it { is_expected.to be_a(ArtifactMigratable) }
 
+  it_behaves_like 'UpdateProjectStatistics' do
+    subject { FactoryBot.build(:ci_build, pipeline: pipeline, artifacts_size: 23) }
+  end
+
   describe 'associations' do
     it 'has a bidirectional relationship with projects' do
       expect(described_class.reflect_on_association(:project).has_inverse?).to eq(:builds)
@@ -2119,54 +2123,6 @@ describe Ci::Build do
     end
   end
 
-  context 'when updating the build' do
-    let(:build) { create(:ci_build, artifacts_size: 23) }
-
-    it 'updates project statistics' do
-      build.artifacts_size = 42
-
-      expect(build).to receive(:update_project_statistics_after_save).and_call_original
-
-      expect { build.save! }
-        .to change { build.project.statistics.reload.build_artifacts_size }
-        .by(19)
-    end
-
-    context 'when the artifact size stays the same' do
-      it 'does not update project statistics' do
-        build.name = 'changed'
-
-        expect(build).not_to receive(:update_project_statistics_after_save)
-
-        build.save!
-      end
-    end
-  end
-
-  context 'when destroying the build' do
-    let!(:build) { create(:ci_build, artifacts_size: 23) }
-
-    it 'updates project statistics' do
-      expect(ProjectStatistics)
-        .to receive(:increment_statistic)
-        .and_call_original
-
-      expect { build.destroy! }
-        .to change { build.project.statistics.reload.build_artifacts_size }
-        .by(-23)
-    end
-
-    context 'when the build is destroyed due to the project being destroyed' do
-      it 'does not update the project statistics' do
-        expect(ProjectStatistics)
-          .not_to receive(:increment_statistic)
-
-        build.project.update(pending_delete: true)
-        build.project.destroy!
-      end
-    end
-  end
-
   describe '#variables' do
     let(:container_registry_enabled) { false }
 

spec/models/ci/job_artifact_spec.rb

@@ -5,6 +5,10 @@ require 'spec_helper'
 describe Ci::JobArtifact do
   let(:artifact) { create(:ci_job_artifact, :archive) }
 
+  it_behaves_like 'UpdateProjectStatistics' do
+    subject { build(:ci_job_artifact, :archive, size: 106365) }
+  end
+
   describe "Associations" do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:job) }
@@ -102,12 +106,6 @@ describe Ci::JobArtifact do
     it 'sets the size from the file size' do
       expect(artifact.size).to eq(106365)
     end
-
-    it 'updates the project statistics' do
-      expect { artifact }
-        .to change { project.statistics.reload.build_artifacts_size }
-        .by(106365)
-    end
   end
 
   context 'updating the artifact file' do
@@ -115,12 +113,6 @@ describe Ci::JobArtifact do
       artifact.update!(file: fixture_file_upload('spec/fixtures/dk.png'))
       expect(artifact.size).to eq(1062)
     end
-
-    it 'updates the project statistics' do
-      expect { artifact.update!(file: fixture_file_upload('spec/fixtures/dk.png')) }
-        .to change { artifact.project.statistics.reload.build_artifacts_size }
-        .by(1062 - 106365)
-    end
   end
 
   describe 'validates file format' do
@@ -259,34 +251,6 @@ describe Ci::JobArtifact do
     end
   end
 
-  context 'when destroying the artifact' do
-    let(:project) { create(:project, :repository) }
-    let(:pipeline) { create(:ci_pipeline, project: project) }
-    let!(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
-
-    it 'updates the project statistics' do
-      artifact = build.job_artifacts.first
-
-      expect(ProjectStatistics)
-        .to receive(:increment_statistic)
-        .and_call_original
-
-      expect { artifact.destroy }
-        .to change { project.statistics.reload.build_artifacts_size }
-        .by(-106365)
-    end
-
-    context 'when it is destroyed from the project level' do
-      it 'does not update the project statistics' do
-        expect(ProjectStatistics)
-          .not_to receive(:increment_statistic)
-
-        project.update(pending_delete: true)
-        project.destroy!
-      end
-    end
-  end
-
   describe 'file is being stored' do
     subject { create(:ci_job_artifact, :archive) }
 

spec/support/shared_examples/models/update_project_statistics_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+shared_examples_for 'UpdateProjectStatistics' do
+  let(:project)   { subject.project }
+  let(:stat)      { described_class.statistic_name }
+  let(:attribute) { described_class.statistic_attribute }
+
+  def reload_stat
+    project.statistics.reload.send(stat).to_i
+  end
+
+  def read_attribute
+    subject.read_attribute(attribute).to_i
+  end
+
+  it { is_expected.to be_new_record }
+
+  context 'when creating' do
+    it 'updates the project statistics' do
+      delta = read_attribute
+
+      expect { subject.save! }
+        .to change { reload_stat }
+        .by(delta)
+    end
+  end
+
+  context 'when updating' do
+    before do
+      subject.save!
+    end
+
+    it 'updates project statistics' do
+      delta = 42
+
+      expect(ProjectStatistics)
+        .to receive(:increment_statistic)
+        .and_call_original
+
+      subject.write_attribute(attribute, read_attribute + delta)
+      expect { subject.save! }
+        .to change { reload_stat }
+        .by(delta)
+    end
+  end
+
+  context 'when destroying' do
+    before do
+      subject.save!
+    end
+
+    it 'updates the project statistics' do
+      delta = -read_attribute
+
+      expect(ProjectStatistics)
+        .to receive(:increment_statistic)
+        .and_call_original
+
+      expect { subject.destroy }
+        .to change { reload_stat }
+        .by(delta)
+    end
+
+    context 'when it is destroyed from the project level' do
+      it 'does not update the project statistics' do
+        expect(ProjectStatistics)
+          .not_to receive(:increment_statistic)
+
+        project.update(pending_delete: true)
+        project.destroy!
+      end
+    end
+  end
+end