Commit fa296825 authored by Alessio Caiazza's avatar Alessio Caiazza Committed by Douwe Maan

Extract ProjectStatistics updates into a concern

Refactor existing tests as a shared example
parent 1aa0ceae
......@@ -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
......@@ -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?
......
# 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
......@@ -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 }
......
......@@ -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) }
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment