Commit fb6d6fce authored by Andreas Brandl's avatar Andreas Brandl

Address review comments.

parent bc3fc8ec
# Include atomic internal id generation scheme for a model # Include atomic internal id generation scheme for a model
# #
# This allows to atomically generate internal ids that are # This allows us to atomically generate internal ids that are
# unique within a given scope. # unique within a given scope.
# #
# For example, let's generate internal ids for Issue per Project: # For example, let's generate internal ids for Issue per Project:
...@@ -25,18 +25,18 @@ module AtomicInternalId ...@@ -25,18 +25,18 @@ module AtomicInternalId
extend ActiveSupport::Concern extend ActiveSupport::Concern
module ClassMethods module ClassMethods
def has_internal_id(on, scope:, usage: nil, init:) # rubocop:disable Naming/PredicateName def has_internal_id(column, scope:, init:) # rubocop:disable Naming/PredicateName
before_validation(on: :create) do before_validation(on: :create) do
if self.public_send(on).blank? # rubocop:disable GitlabSecurity/PublicSend if self.public_send(column).blank? # rubocop:disable GitlabSecurity/PublicSend
scope_attrs = { scope => self.public_send(scope) } # rubocop:disable GitlabSecurity/PublicSend scope_attrs = { scope => self.public_send(scope) } # rubocop:disable GitlabSecurity/PublicSend
usage = (usage || self.class.table_name).to_sym usage = self.class.table_name.to_sym
new_iid = InternalId.generate_next(self, scope_attrs, usage, init) new_iid = InternalId.generate_next(self, scope_attrs, usage, init)
self.public_send("#{on}=", new_iid) # rubocop:disable GitlabSecurity/PublicSend self.public_send("#{column}=", new_iid) # rubocop:disable GitlabSecurity/PublicSend
end end
end end
validates on, presence: true, numericality: true validates column, presence: true, numericality: true
end end
end end
......
...@@ -66,6 +66,7 @@ class InternalId < ActiveRecord::Base ...@@ -66,6 +66,7 @@ class InternalId < ActiveRecord::Base
# scope: Attributes that define the scope for id generation. # scope: Attributes that define the scope for id generation.
# usage: Symbol to define the usage of the internal id, see InternalId.usages # usage: Symbol to define the usage of the internal id, see InternalId.usages
# init: Block that gets called to initialize InternalId record if not present # init: Block that gets called to initialize InternalId record if not present
# Make sure to not throw exceptions in the absence of records (if this is expected).
attr_reader :subject, :scope, :init, :scope_attrs, :usage attr_reader :subject, :scope, :init, :scope_attrs, :usage
def initialize(subject, scope, usage, init) def initialize(subject, scope, usage, init)
...@@ -74,9 +75,9 @@ class InternalId < ActiveRecord::Base ...@@ -74,9 +75,9 @@ class InternalId < ActiveRecord::Base
@init = init @init = init
@usage = usage @usage = usage
raise ArgumentError, 'scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty? raise ArgumentError, 'Scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty?
unless InternalId.usages.keys.include?(usage.to_s) unless InternalId.usages.has_key?(usage.to_s)
raise ArgumentError, "Usage '#{usage}' is unknown. Supported values are #{InternalId.usages.keys} from InternalId.usages" raise ArgumentError, "Usage '#{usage}' is unknown. Supported values are #{InternalId.usages.keys} from InternalId.usages"
end end
end end
...@@ -85,7 +86,7 @@ class InternalId < ActiveRecord::Base ...@@ -85,7 +86,7 @@ class InternalId < ActiveRecord::Base
def generate def generate
subject.transaction do subject.transaction do
# Create a record in internal_ids if one does not yet exist # Create a record in internal_ids if one does not yet exist
# and increment it's last value # and increment its last value
# #
# Note this will acquire a ROW SHARE lock on the InternalId record # Note this will acquire a ROW SHARE lock on the InternalId record
(lookup || create_record).increment_and_save! (lookup || create_record).increment_and_save!
......
...@@ -3,7 +3,7 @@ class CreateInternalIdsTable < ActiveRecord::Migration ...@@ -3,7 +3,7 @@ class CreateInternalIdsTable < ActiveRecord::Migration
DOWNTIME = false DOWNTIME = false
def up def change
create_table :internal_ids, id: :bigserial do |t| create_table :internal_ids, id: :bigserial do |t|
t.references :project, null: false, foreign_key: { on_delete: :cascade } t.references :project, null: false, foreign_key: { on_delete: :cascade }
t.integer :usage, null: false t.integer :usage, null: false
...@@ -12,8 +12,4 @@ class CreateInternalIdsTable < ActiveRecord::Migration ...@@ -12,8 +12,4 @@ class CreateInternalIdsTable < ActiveRecord::Migration
t.index [:usage, :project_id], unique: true t.index [:usage, :project_id], unique: true
end end
end end
def down
drop_table :internal_ids
end
end end
...@@ -41,10 +41,11 @@ describe InternalId do ...@@ -41,10 +41,11 @@ describe InternalId do
end end
it 'generates a strictly monotone, gapless sequence' do it 'generates a strictly monotone, gapless sequence' do
seq = (0..rand(1000)).map do seq = (0..rand(100)).map do
described_class.generate_next(issue, scope, usage, init) described_class.generate_next(issue, scope, usage, init)
end end
normalized = seq.map { |i| i - seq.min } normalized = seq.map { |i| i - seq.min }
expect(normalized).to eq((0..seq.size - 1).to_a) expect(normalized).to eq((0..seq.size - 1).to_a)
end end
...@@ -58,6 +59,7 @@ describe InternalId do ...@@ -58,6 +59,7 @@ describe InternalId do
it 'calculates next internal ids on the fly' do it 'calculates next internal ids on the fly' do
val = rand(1..100) val = rand(1..100)
expect(init).to receive(:call).with(issue).and_return(val) expect(init).to receive(:call).with(issue).and_return(val)
expect(subject).to eq(val + 1) expect(subject).to eq(val + 1)
end end
...@@ -70,11 +72,13 @@ describe InternalId do ...@@ -70,11 +72,13 @@ describe InternalId do
it 'returns incremented iid' do it 'returns incremented iid' do
value = id.last_value value = id.last_value
expect(subject).to eq(value + 1) expect(subject).to eq(value + 1)
end end
it 'saves the record' do it 'saves the record' do
subject subject
expect(id.changed?).to be_falsey expect(id.changed?).to be_falsey
end end
......
...@@ -24,14 +24,16 @@ shared_examples_for 'AtomicInternalId' do ...@@ -24,14 +24,16 @@ shared_examples_for 'AtomicInternalId' do
it 'calls InternalId.generate_next and sets internal id attribute' do it 'calls InternalId.generate_next and sets internal id attribute' do
iid = rand(1..1000) iid = rand(1..1000)
expect(InternalId).to receive(:generate_next).with(instance, scope_attrs, usage, any_args).and_return(iid) expect(InternalId).to receive(:generate_next).with(instance, scope_attrs, usage, any_args).and_return(iid)
subject subject
expect(instance.public_send(internal_id_attribute)).to eq(iid) # rubocop:disable GitlabSecurity/PublicSend expect(instance.public_send(internal_id_attribute)).to eq(iid)
end end
it 'does not overwrite an existing internal id' do it 'does not overwrite an existing internal id' do
instance.public_send("#{internal_id_attribute}=", 4711) # rubocop:disable GitlabSecurity/PublicSend instance.public_send("#{internal_id_attribute}=", 4711)
expect { subject }.not_to change { instance.public_send(internal_id_attribute) } # rubocop:disable GitlabSecurity/PublicSend
expect { subject }.not_to change { instance.public_send(internal_id_attribute) }
end end
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