diff options
author | Andreas Brandl <abrandl@gitlab.com> | 2018-03-16 13:34:08 +0100 |
---|---|---|
committer | Andreas Brandl <abrandl@gitlab.com> | 2018-03-16 13:35:27 +0100 |
commit | fb6d6fce5a4d0fd833dc1cd231dd284a6c89471a (patch) | |
tree | cd60653ee507ae673a6da8a12ebd731bf449f525 | |
parent | bc3fc8ec3eec74876a0e2125248c27cde153e32b (diff) | |
download | gitlab-ce-fb6d6fce5a4d0fd833dc1cd231dd284a6c89471a.tar.gz |
Address review comments.
-rw-r--r-- | app/models/concerns/atomic_internal_id.rb | 12 | ||||
-rw-r--r-- | app/models/internal_id.rb | 7 | ||||
-rw-r--r-- | config/initializers/ar_native_database_types.rb | 2 | ||||
-rw-r--r-- | db/migrate/20180305095250_create_internal_ids_table.rb | 6 | ||||
-rw-r--r-- | spec/models/internal_id_spec.rb | 6 | ||||
-rw-r--r-- | spec/support/shared_examples/models/atomic_internal_id_spec.rb | 8 |
6 files changed, 22 insertions, 19 deletions
diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 343edc237c9..6895c7d7e95 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -1,6 +1,6 @@ # 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. # # For example, let's generate internal ids for Issue per Project: @@ -25,18 +25,18 @@ module AtomicInternalId extend ActiveSupport::Concern 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 - 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 - 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) - self.public_send("#{on}=", new_iid) # rubocop:disable GitlabSecurity/PublicSend + self.public_send("#{column}=", new_iid) # rubocop:disable GitlabSecurity/PublicSend end end - validates on, presence: true, numericality: true + validates column, presence: true, numericality: true end end diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index f3630ed1ac5..cbec735c2dd 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -66,6 +66,7 @@ class InternalId < ActiveRecord::Base # scope: Attributes that define the scope for id generation. # 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 + # Make sure to not throw exceptions in the absence of records (if this is expected). attr_reader :subject, :scope, :init, :scope_attrs, :usage def initialize(subject, scope, usage, init) @@ -74,9 +75,9 @@ class InternalId < ActiveRecord::Base @init = init @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" end end @@ -85,7 +86,7 @@ class InternalId < ActiveRecord::Base def generate subject.transaction do # 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 (lookup || create_record).increment_and_save! diff --git a/config/initializers/ar_native_database_types.rb b/config/initializers/ar_native_database_types.rb index d7b4b348957..3522b1db536 100644 --- a/config/initializers/ar_native_database_types.rb +++ b/config/initializers/ar_native_database_types.rb @@ -4,7 +4,7 @@ module ActiveRecord module ConnectionAdapters class AbstractMysqlAdapter NATIVE_DATABASE_TYPES.merge!( - bigserial: { name: 'bigint(20) auto_increment PRIMARY KEY' } + bigserial: { name: 'bigint(20) auto_increment PRIMARY KEY' } ) end end diff --git a/db/migrate/20180305095250_create_internal_ids_table.rb b/db/migrate/20180305095250_create_internal_ids_table.rb index e972432fb98..432086fe98b 100644 --- a/db/migrate/20180305095250_create_internal_ids_table.rb +++ b/db/migrate/20180305095250_create_internal_ids_table.rb @@ -3,7 +3,7 @@ class CreateInternalIdsTable < ActiveRecord::Migration DOWNTIME = false - def up + def change create_table :internal_ids, id: :bigserial do |t| t.references :project, null: false, foreign_key: { on_delete: :cascade } t.integer :usage, null: false @@ -12,8 +12,4 @@ class CreateInternalIdsTable < ActiveRecord::Migration t.index [:usage, :project_id], unique: true end end - - def down - drop_table :internal_ids - end end diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb index ef6db2daa95..40d777c46cc 100644 --- a/spec/models/internal_id_spec.rb +++ b/spec/models/internal_id_spec.rb @@ -41,10 +41,11 @@ describe InternalId do end 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) end normalized = seq.map { |i| i - seq.min } + expect(normalized).to eq((0..seq.size - 1).to_a) end @@ -58,6 +59,7 @@ describe InternalId do it 'calculates next internal ids on the fly' do val = rand(1..100) + expect(init).to receive(:call).with(issue).and_return(val) expect(subject).to eq(val + 1) end @@ -70,11 +72,13 @@ describe InternalId do it 'returns incremented iid' do value = id.last_value + expect(subject).to eq(value + 1) end it 'saves the record' do subject + expect(id.changed?).to be_falsey end diff --git a/spec/support/shared_examples/models/atomic_internal_id_spec.rb b/spec/support/shared_examples/models/atomic_internal_id_spec.rb index 671aa314bd6..144af4fc475 100644 --- a/spec/support/shared_examples/models/atomic_internal_id_spec.rb +++ b/spec/support/shared_examples/models/atomic_internal_id_spec.rb @@ -24,14 +24,16 @@ shared_examples_for 'AtomicInternalId' do it 'calls InternalId.generate_next and sets internal id attribute' do iid = rand(1..1000) + expect(InternalId).to receive(:generate_next).with(instance, scope_attrs, usage, any_args).and_return(iid) 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 it 'does not overwrite an existing internal id' do - instance.public_send("#{internal_id_attribute}=", 4711) # rubocop:disable GitlabSecurity/PublicSend - expect { subject }.not_to change { instance.public_send(internal_id_attribute) } # rubocop:disable GitlabSecurity/PublicSend + instance.public_send("#{internal_id_attribute}=", 4711) + + expect { subject }.not_to change { instance.public_send(internal_id_attribute) } end end end |