Atomic internal ids for all models

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! }