diff options
-rw-r--r-- | app/models/concerns/atomic_internal_id.rb | 20 | ||||
-rw-r--r-- | app/models/internal_id.rb | 40 | ||||
-rw-r--r-- | app/models/issue.rb | 2 | ||||
-rw-r--r-- | spec/factories/internal_ids.rb | 3 | ||||
-rw-r--r-- | spec/models/internal_id_spec.rb | 26 |
5 files changed, 42 insertions, 49 deletions
diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 3cc9ce7f03f..eef5c0bfcd1 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -2,15 +2,19 @@ module AtomicInternalId extend ActiveSupport::Concern included do - before_validation(on: :create) do - set_iid - end - - validates :iid, presence: true, numericality: true - end + class << self + def has_internal_id(on, scope:, usage: nil, init: nil) + before_validation(on: :create) do + if self.public_send(on).blank? # rubocop:disable GitlabSecurity/PublicSend + usage = (usage || self.class.name.tableize).to_sym + new_iid = InternalId.generate_next(self, scope, usage, init) + self.public_send("#{on}=", new_iid) # rubocop:disable GitlabSecurity/PublicSend + end + end - def set_iid - self.iid = InternalId.generate_next(self.project, :issues) if iid.blank? + validates on, presence: true, numericality: true + end + end end def to_param diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index 24c7cbf988f..58e71b623d0 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -23,17 +23,6 @@ class InternalId < ActiveRecord::Base last_value end - before_create :calculate_last_value! - - # Calculate #last_value by counting the number of - # existing records for this usage. - def calculate_last_value! - return if last_value - - parent = project # ??|| group - self.last_value = parent.send(usage.to_sym).maximum(:iid) || 0 # rubocop:disable GitlabSecurity/PublicSend - end - class << self # Generate next internal id for a given project and usage. # @@ -45,12 +34,21 @@ class InternalId < ActiveRecord::Base # 3) The generated sequence is gapless. # 4) In the absence of a record in the internal_ids table, one will be created # and last_value will be calculated on the fly. - def generate_next(project, usage) - raise 'project not set - this is required' unless project + def generate_next(subject, scope, usage, init) + scope = [scope].flatten.compact + raise 'scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty? + raise "usage #{usage} is unknown. Supported values are InternalId.usages = #{InternalId.usages.keys.to_s}" unless InternalId.usages.include?(usage.to_sym) + + init ||= ->(s) { 0 } + + scope_attrs = scope.inject({}) do |h, e| + h[e] = subject.public_send(e) + h + end - project.transaction do + transaction do # Create a record in internal_ids if one does not yet exist - id = (lookup(project, usage) || create_record(project, usage)) + id = (lookup(scope_attrs, usage) || create_record(scope_attrs, usage, init, subject)) # This will lock the InternalId record with ROW SHARE # and increment #last_value @@ -61,8 +59,8 @@ class InternalId < ActiveRecord::Base private # Retrieve InternalId record for (project, usage) combination, if it exists - def lookup(project, usage) - project.internal_ids.find_by(usage: usages[usage.to_s]) + def lookup(scope_attrs, usage) + InternalId.find_by(usage: usages[usage.to_s], **scope_attrs) end # Create InternalId record for (project, usage) combination, if it doesn't exist @@ -71,13 +69,13 @@ class InternalId < ActiveRecord::Base # was faster in doing this, we'll realize once we hit the unique key constraint # violation. We can safely roll-back the nested transaction and perform # a lookup instead to retrieve the record. - def create_record(project, usage) + def create_record(scope_attrs, usage, init, subject) begin - project.transaction(requires_new: true) do - create!(project: project, usage: usages[usage.to_s]) + transaction(requires_new: true) do + create!(usage: usages[usage.to_s], **scope_attrs, last_value: init.call(subject) || 0) end rescue ActiveRecord::RecordNotUnique - lookup(project, usage) + lookup(scope_attrs, usage) end end end diff --git a/app/models/issue.rb b/app/models/issue.rb index cf547106122..509e214c601 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -24,6 +24,8 @@ class Issue < ActiveRecord::Base belongs_to :project belongs_to :moved_to, class_name: 'Issue' + has_internal_id :iid, scope: :project, init: ->(s) { s.project.issues.maximum(:iid) } + has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :merge_requests_closing_issues, diff --git a/spec/factories/internal_ids.rb b/spec/factories/internal_ids.rb index b4c14d22a29..fbde07a391a 100644 --- a/spec/factories/internal_ids.rb +++ b/spec/factories/internal_ids.rb @@ -1,6 +1,7 @@ FactoryBot.define do factory :internal_id do project - usage { InternalId.usages.keys.first } + usage :issues + last_value { project.issues.maximum(:iid) || 0 } end end diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb index b953b6a2df8..5971d8f47a6 100644 --- a/spec/models/internal_id_spec.rb +++ b/spec/models/internal_id_spec.rb @@ -3,6 +3,9 @@ require 'spec_helper' describe InternalId do let(:project) { create(:project) } let(:usage) { :issues } + let(:issue) { build(:issue, project: project) } + let(:scope) { :project } + let(:init) { ->(s) { project.issues.size } } context 'validations' do it { is_expected.to validate_presence_of(:usage) } @@ -11,7 +14,7 @@ describe InternalId do describe '.generate_next' do context 'in the absence of a record' do - subject { described_class.generate_next(project, usage) } + subject { described_class.generate_next(issue, scope, usage, init) } it 'creates a record if not yet present' do expect { subject }.to change { described_class.count }.from(0).to(1) @@ -22,13 +25,14 @@ describe InternalId do described_class.first.tap do |record| expect(record.project).to eq(project) - expect(record.usage).to eq(usage.to_s) # TODO + expect(record.usage).to eq(usage.to_s) end end context 'with existing issues' do before do rand(10).times { create(:issue, project: project) } + InternalId.delete_all end it 'calculates last_value values automatically' do @@ -39,7 +43,7 @@ describe InternalId do it 'generates a strictly monotone, gapless sequence' do seq = (0..rand(1000)).map do - described_class.generate_next(project, usage) + described_class.generate_next(issue, scope, usage, init) end normalized = seq.map { |i| i - seq.min } expect(normalized).to eq((0..seq.size - 1).to_a) @@ -68,20 +72,4 @@ describe InternalId do end end end - - describe '#calculate_last_value! (for issues)' do - subject do - build(:internal_id, project: project, usage: :issues) - end - - context 'with existing issues' do - before do - rand(10).times { create(:issue, project: project) } - end - - it 'counts related issues and saves' do - expect { subject.calculate_last_value! }.to change { subject.last_value }.from(nil).to(project.issues.size) - end - end - end end |