diff options
| -rw-r--r-- | app/models/concerns/atomic_internal_id.rb | 5 | ||||
| -rw-r--r-- | app/models/concerns/nonatomic_internal_id.rb | 22 | ||||
| -rw-r--r-- | app/models/deployment.rb | 4 | ||||
| -rw-r--r-- | app/models/internal_id.rb | 3 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 4 | ||||
| -rw-r--r-- | app/models/milestone.rb | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/ab-44259-atomic-internal-ids-for-all-models.yml | 5 | ||||
| -rw-r--r-- | db/migrate/20180416155103_add_further_scope_columns_to_internal_id_table.rb | 15 | ||||
| -rw-r--r-- | db/migrate/20180417090132_add_index_constraints_to_internal_id_table.rb | 40 | ||||
| -rw-r--r-- | db/schema.rb | 7 | ||||
| -rw-r--r-- | spec/models/deployment_spec.rb | 9 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 8 | ||||
| -rw-r--r-- | spec/models/milestone_spec.rb | 20 | ||||
| -rw-r--r-- | spec/support/shared_examples/models/atomic_internal_id_spec.rb | 8 |
14 files changed, 124 insertions, 31 deletions
diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 4b66725a3e6..22f516a172f 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -27,8 +27,9 @@ module AtomicInternalId module ClassMethods def has_internal_id(column, scope:, init:) # rubocop:disable Naming/PredicateName before_validation(on: :create) do - if read_attribute(column).blank? - scope_attrs = { scope => association(scope).reader } + scope_value = association(scope).reader + if read_attribute(column).blank? && scope_value + scope_attrs = { scope_value.class.table_name.singularize.to_sym => scope_value } usage = self.class.table_name.to_sym new_iid = InternalId.generate_next(self, scope_attrs, usage, init) diff --git a/app/models/concerns/nonatomic_internal_id.rb b/app/models/concerns/nonatomic_internal_id.rb deleted file mode 100644 index 9d0c9b8512f..00000000000 --- a/app/models/concerns/nonatomic_internal_id.rb +++ /dev/null @@ -1,22 +0,0 @@ -module NonatomicInternalId - extend ActiveSupport::Concern - - included do - validate :set_iid, on: :create - validates :iid, presence: true, numericality: true - end - - def set_iid - if iid.blank? - parent = project || group - records = parent.public_send(self.class.name.tableize) # rubocop:disable GitlabSecurity/PublicSend - max_iid = records.maximum(:iid) - - self.iid = max_iid.to_i + 1 - end - end - - def to_param - iid.to_s - end -end diff --git a/app/models/deployment.rb b/app/models/deployment.rb index e18ea8bfea4..254764eefde 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -1,11 +1,13 @@ class Deployment < ActiveRecord::Base - include NonatomicInternalId + include AtomicInternalId belongs_to :project, required: true belongs_to :environment, required: true belongs_to :user belongs_to :deployable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations + has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.deployments&.maximum(:iid) } + validates :sha, presence: true validates :ref, presence: true diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index 96a43006642..189942c5ad8 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -12,8 +12,9 @@ # * (Optionally) add columns to `internal_ids` if needed for scope. class InternalId < ActiveRecord::Base belongs_to :project + belongs_to :namespace - enum usage: { issues: 0 } + enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4 } validates :usage, presence: true diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 91d8be5559b..8f964a488aa 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1,5 +1,5 @@ class MergeRequest < ActiveRecord::Base - include NonatomicInternalId + include AtomicInternalId include Issuable include Noteable include Referable @@ -18,6 +18,8 @@ class MergeRequest < ActiveRecord::Base belongs_to :source_project, class_name: "Project" belongs_to :merge_user, class_name: "User" + has_internal_id :iid, scope: :target_project, init: ->(s) { s&.target_project&.merge_requests&.maximum(:iid) } + has_many :merge_request_diffs has_one :merge_request_diff, diff --git a/app/models/milestone.rb b/app/models/milestone.rb index a66a0015827..d14e3a4ded5 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -8,7 +8,7 @@ class Milestone < ActiveRecord::Base Started = MilestoneStruct.new('Started', '#started', -3) include CacheMarkdownField - include NonatomicInternalId + include AtomicInternalId include Sortable include Referable include StripAttribute @@ -21,6 +21,9 @@ class Milestone < ActiveRecord::Base belongs_to :project belongs_to :group + has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.milestones&.maximum(:iid) } + has_internal_id :iid, scope: :group, init: ->(s) { s&.group&.milestones&.maximum(:iid) } + has_many :issues has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues has_many :merge_requests diff --git a/changelogs/unreleased/ab-44259-atomic-internal-ids-for-all-models.yml b/changelogs/unreleased/ab-44259-atomic-internal-ids-for-all-models.yml new file mode 100644 index 00000000000..e154f7dbc85 --- /dev/null +++ b/changelogs/unreleased/ab-44259-atomic-internal-ids-for-all-models.yml @@ -0,0 +1,5 @@ +--- +title: Transition to atomic internal ids for all models. +merge_request: 44259 +author: +type: other diff --git a/db/migrate/20180416155103_add_further_scope_columns_to_internal_id_table.rb b/db/migrate/20180416155103_add_further_scope_columns_to_internal_id_table.rb new file mode 100644 index 00000000000..37e2d19e022 --- /dev/null +++ b/db/migrate/20180416155103_add_further_scope_columns_to_internal_id_table.rb @@ -0,0 +1,15 @@ +class AddFurtherScopeColumnsToInternalIdTable < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + change_column_null :internal_ids, :project_id, true + add_column :internal_ids, :namespace_id, :integer, null: true + end + + def down + change_column_null :internal_ids, :project_id, false + remove_column :internal_ids, :namespace_id + end +end diff --git a/db/migrate/20180417090132_add_index_constraints_to_internal_id_table.rb b/db/migrate/20180417090132_add_index_constraints_to_internal_id_table.rb new file mode 100644 index 00000000000..582b89a3948 --- /dev/null +++ b/db/migrate/20180417090132_add_index_constraints_to_internal_id_table.rb @@ -0,0 +1,40 @@ +class AddIndexConstraintsToInternalIdTable < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :internal_ids, [:usage, :namespace_id], unique: true, where: 'namespace_id IS NOT NULL' + + replace_index(:internal_ids, [:usage, :project_id], name: 'index_internal_ids_on_usage_and_project_id') do + add_concurrent_index :internal_ids, [:usage, :project_id], unique: true, where: 'project_id IS NOT NULL' + end + + add_concurrent_foreign_key :internal_ids, :namespaces, column: :namespace_id, on_delete: :cascade + end + + def down + remove_concurrent_index :internal_ids, [:usage, :namespace_id] + + replace_index(:internal_ids, [:usage, :project_id], name: 'index_internal_ids_on_usage_and_project_id') do + add_concurrent_index :internal_ids, [:usage, :project_id], unique: true + end + + remove_foreign_key :internal_ids, column: :namespace_id + end + + private + def replace_index(table, columns, name:) + temporary_name = "#{name}_old" + + if index_exists?(table, columns, name: name) + rename_index table, name, temporary_name + end + + yield + + remove_concurrent_index_by_name table, temporary_name + end +end diff --git a/db/schema.rb b/db/schema.rb index 1d272bdb779..df621956c80 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -896,12 +896,14 @@ ActiveRecord::Schema.define(version: 20180418053107) do add_index "identities", ["user_id"], name: "index_identities_on_user_id", using: :btree create_table "internal_ids", id: :bigserial, force: :cascade do |t| - t.integer "project_id", null: false + t.integer "project_id" t.integer "usage", null: false t.integer "last_value", null: false + t.integer "namespace_id" end - add_index "internal_ids", ["usage", "project_id"], name: "index_internal_ids_on_usage_and_project_id", unique: true, using: :btree + add_index "internal_ids", ["usage", "namespace_id"], name: "index_internal_ids_on_usage_and_namespace_id", unique: true, where: "(namespace_id IS NOT NULL)", using: :btree + add_index "internal_ids", ["usage", "project_id"], name: "index_internal_ids_on_usage_and_project_id", unique: true, where: "(project_id IS NOT NULL)", using: :btree create_table "issue_assignees", id: false, force: :cascade do |t| t.integer "user_id", null: false @@ -2113,6 +2115,7 @@ ActiveRecord::Schema.define(version: 20180418053107) do add_foreign_key "gpg_signatures", "gpg_keys", on_delete: :nullify add_foreign_key "gpg_signatures", "projects", on_delete: :cascade add_foreign_key "group_custom_attributes", "namespaces", column: "group_id", on_delete: :cascade + add_foreign_key "internal_ids", "namespaces", name: "fk_162941d509", on_delete: :cascade add_foreign_key "internal_ids", "projects", on_delete: :cascade add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index ac30cd27e0c..aee70bcfb29 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -16,6 +16,15 @@ describe Deployment do it { is_expected.to validate_presence_of(:ref) } it { is_expected.to validate_presence_of(:sha) } + describe 'modules' do + it_behaves_like 'AtomicInternalId' do + let(:internal_id_attribute) { :iid } + let(:instance) { build(:deployment) } + let(:scope_attrs) { { project: instance.project } } + let(:usage) { :deployments } + end + end + describe 'after_create callbacks' do let(:environment) { create(:environment) } let(:store) { Gitlab::EtagCaching::Store.new } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index f73f44ca0ad..becb146422e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -17,11 +17,17 @@ describe MergeRequest do describe 'modules' do subject { described_class } - 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) } it { is_expected.to include_module(Taskable) } + + it_behaves_like 'AtomicInternalId' do + let(:internal_id_attribute) { :iid } + let(:instance) { build(:merge_request) } + let(:scope_attrs) { { project: instance.target_project } } + let(:usage) { :merge_requests } + end end describe 'validation' do diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index c7460981a32..4bb9717d33e 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -1,6 +1,26 @@ require 'spec_helper' describe Milestone do + describe 'modules' do + context 'with a project' do + it_behaves_like 'AtomicInternalId' do + let(:internal_id_attribute) { :iid } + let(:instance) { build(:milestone, project: build(:project), group: nil) } + let(:scope_attrs) { { project: instance.project } } + let(:usage) { :milestones } + end + end + + context 'with a group' do + it_behaves_like 'AtomicInternalId' do + let(:internal_id_attribute) { :iid } + let(:instance) { build(:milestone, project: nil, group: build(:group)) } + let(:scope_attrs) { { namespace: instance.group } } + let(:usage) { :milestones } + end + end + end + describe "Validation" do before do allow(subject).to receive(:set_iid).and_return(false) 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 144af4fc475..6a6e13418a9 100644 --- a/spec/support/shared_examples/models/atomic_internal_id_spec.rb +++ b/spec/support/shared_examples/models/atomic_internal_id_spec.rb @@ -19,6 +19,14 @@ shared_examples_for 'AtomicInternalId' do it { is_expected.to validate_numericality_of(internal_id_attribute) } end + describe 'Creating an instance' do + subject { instance.save! } + + it 'saves a new instance properly' do + expect { subject }.not_to raise_error + end + end + describe 'internal id generation' do subject { instance.save! } |