diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-03-19 13:41:29 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-03-19 13:41:29 +0000 |
commit | e9da2b0c752b00e0af941bf1e33278a7d38cfeb9 (patch) | |
tree | c6beef6747efb6ecd4f2f2e38ed737ef0ddf3f0c /spec | |
parent | f8171595a6fa9e3a710ef6bba125b1a49a57bd10 (diff) | |
parent | c3b489bdcb1d6cf8bcdb935c73892dc5465eacbd (diff) | |
download | gitlab-ce-e9da2b0c752b00e0af941bf1e33278a7d38cfeb9.tar.gz |
Merge branch '31114-internal-ids-are-not-atomic' into 'master'
Atomic generation of internal ids for issues.
Closes #31114
See merge request gitlab-org/gitlab-ce!17580
Diffstat (limited to 'spec')
-rw-r--r-- | spec/factories/internal_ids.rb | 7 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/all_models.yml | 1 | ||||
-rw-r--r-- | spec/models/concerns/issuable_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/internal_id_spec.rb | 106 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 8 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 2 | ||||
-rw-r--r-- | spec/support/shared_examples/models/atomic_internal_id_spec.rb | 40 |
7 files changed, 163 insertions, 3 deletions
diff --git a/spec/factories/internal_ids.rb b/spec/factories/internal_ids.rb new file mode 100644 index 00000000000..fbde07a391a --- /dev/null +++ b/spec/factories/internal_ids.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :internal_id do + project + usage :issues + last_value { project.issues.maximum(:iid) || 0 } + end +end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index bece82e531a..a204a8f1ffe 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -279,6 +279,7 @@ project: - lfs_file_locks - project_badges - source_of_merge_requests +- internal_ids award_emoji: - awardable - user diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 4b217df2e8f..f8874d14e3f 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -34,7 +34,7 @@ describe Issuable do subject { build(:issue) } before do - allow(subject).to receive(:set_iid).and_return(false) + allow(InternalId).to receive(:generate_next).and_return(nil) end it { is_expected.to validate_presence_of(:project) } diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb new file mode 100644 index 00000000000..581fd0293cc --- /dev/null +++ b/spec/models/internal_id_spec.rb @@ -0,0 +1,106 @@ +require 'spec_helper' + +describe InternalId do + let(:project) { create(:project) } + let(:usage) { :issues } + let(:issue) { build(:issue, project: project) } + let(:scope) { { project: project } } + let(:init) { ->(s) { s.project.issues.size } } + + context 'validations' do + it { is_expected.to validate_presence_of(:usage) } + end + + describe '.generate_next' do + subject { described_class.generate_next(issue, scope, usage, init) } + + context 'in the absence of a record' do + it 'creates a record if not yet present' do + expect { subject }.to change { described_class.count }.from(0).to(1) + end + + it 'stores record attributes' do + subject + + described_class.first.tap do |record| + expect(record.project).to eq(project) + expect(record.usage).to eq(usage.to_s) + end + end + + context 'with existing issues' do + before do + rand(1..10).times { create(:issue, project: project) } + described_class.delete_all + end + + it 'calculates last_value values automatically' do + expect(subject).to eq(project.issues.size + 1) + end + end + + context 'with concurrent inserts on table' do + it 'looks up the record if it was created concurrently' do + args = { **scope, usage: described_class.usages[usage.to_s] } + record = double + expect(described_class).to receive(:find_by).with(args).and_return(nil) # first call, record not present + expect(described_class).to receive(:find_by).with(args).and_return(record) # second call, record was created by another process + expect(described_class).to receive(:create!).and_raise(ActiveRecord::RecordNotUnique, 'record not unique') + expect(record).to receive(:increment_and_save!) + + subject + end + end + end + + it 'generates a strictly monotone, gapless sequence' 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 + + context 'with an insufficient schema version' do + before do + described_class.reset_column_information + expect(ActiveRecord::Migrator).to receive(:current_version).and_return(InternalId::REQUIRED_SCHEMA_VERSION - 1) + end + + let(:init) { double('block') } + + 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 + end + end + + describe '#increment_and_save!' do + let(:id) { create(:internal_id) } + subject { id.increment_and_save! } + + 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 + + context 'with last_value=nil' do + let(:id) { build(:internal_id, last_value: nil) } + + it 'returns 1' do + expect(subject).to eq(1) + end + end + end +end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index feed7968f09..11154291368 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -9,11 +9,17 @@ describe Issue do describe 'modules' do subject { described_class } - it { is_expected.to include_module(InternalId) } it { is_expected.to include_module(Issuable) } it { is_expected.to include_module(Referable) } it { is_expected.to include_module(Sortable) } it { is_expected.to include_module(Taskable) } + + it_behaves_like 'AtomicInternalId' do + let(:internal_id_attribute) { :iid } + let(:instance) { build(:issue) } + let(:scope_attrs) { { project: instance.project } } + let(:usage) { :issues } + end end subject { create(:issue) } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 4e783acbd8b..ff5a6f63010 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -17,7 +17,7 @@ describe MergeRequest do describe 'modules' do subject { described_class } - it { is_expected.to include_module(InternalId) } + it { is_expected.to include_module(NonatomicInternalId) } it { is_expected.to include_module(Issuable) } it { is_expected.to include_module(Referable) } it { is_expected.to include_module(Sortable) } diff --git a/spec/support/shared_examples/models/atomic_internal_id_spec.rb b/spec/support/shared_examples/models/atomic_internal_id_spec.rb new file mode 100644 index 00000000000..144af4fc475 --- /dev/null +++ b/spec/support/shared_examples/models/atomic_internal_id_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +shared_examples_for 'AtomicInternalId' do + describe '.has_internal_id' do + describe 'Module inclusion' do + subject { described_class } + + it { is_expected.to include_module(AtomicInternalId) } + end + + describe 'Validation' do + subject { instance } + + before do + allow(InternalId).to receive(:generate_next).and_return(nil) + end + + it { is_expected.to validate_presence_of(internal_id_attribute) } + it { is_expected.to validate_numericality_of(internal_id_attribute) } + end + + describe 'internal id generation' do + subject { instance.save! } + + 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) + end + + it 'does not overwrite an existing internal id' do + instance.public_send("#{internal_id_attribute}=", 4711) + + expect { subject }.not_to change { instance.public_send(internal_id_attribute) } + end + end + end +end |