Commit 9ea2fc85 authored by Andreas Brandl's avatar Andreas Brandl Committed by Yorick Peterse - OOO until May 4th

Atomic internal ids for all models

parent 87e592dc
...@@ -27,8 +27,9 @@ module AtomicInternalId ...@@ -27,8 +27,9 @@ module AtomicInternalId
module ClassMethods module ClassMethods
def has_internal_id(column, scope:, 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 read_attribute(column).blank? scope_value = association(scope).reader
scope_attrs = { scope => 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 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)
......
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
class Deployment < ActiveRecord::Base class Deployment < ActiveRecord::Base
include NonatomicInternalId include AtomicInternalId
belongs_to :project, required: true belongs_to :project, required: true
belongs_to :environment, required: true belongs_to :environment, required: true
belongs_to :user belongs_to :user
belongs_to :deployable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations 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 :sha, presence: true
validates :ref, presence: true validates :ref, presence: true
......
...@@ -12,8 +12,9 @@ ...@@ -12,8 +12,9 @@
# * (Optionally) add columns to `internal_ids` if needed for scope. # * (Optionally) add columns to `internal_ids` if needed for scope.
class InternalId < ActiveRecord::Base class InternalId < ActiveRecord::Base
belongs_to :project 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 validates :usage, presence: true
......
class MergeRequest < ActiveRecord::Base class MergeRequest < ActiveRecord::Base
include NonatomicInternalId include AtomicInternalId
include Issuable include Issuable
include Noteable include Noteable
include Referable include Referable
...@@ -18,6 +18,8 @@ class MergeRequest < ActiveRecord::Base ...@@ -18,6 +18,8 @@ class MergeRequest < ActiveRecord::Base
belongs_to :source_project, class_name: "Project" belongs_to :source_project, class_name: "Project"
belongs_to :merge_user, class_name: "User" 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_many :merge_request_diffs
has_one :merge_request_diff, has_one :merge_request_diff,
......
...@@ -8,7 +8,7 @@ class Milestone < ActiveRecord::Base ...@@ -8,7 +8,7 @@ class Milestone < ActiveRecord::Base
Started = MilestoneStruct.new('Started', '#started', -3) Started = MilestoneStruct.new('Started', '#started', -3)
include CacheMarkdownField include CacheMarkdownField
include NonatomicInternalId include AtomicInternalId
include Sortable include Sortable
include Referable include Referable
include StripAttribute include StripAttribute
...@@ -21,6 +21,9 @@ class Milestone < ActiveRecord::Base ...@@ -21,6 +21,9 @@ class Milestone < ActiveRecord::Base
belongs_to :project belongs_to :project
belongs_to :group 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 :issues
has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues
has_many :merge_requests has_many :merge_requests
......
---
title: Transition to atomic internal ids for all models.
merge_request: 44259
author:
type: other
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
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
...@@ -896,12 +896,14 @@ ActiveRecord::Schema.define(version: 20180418053107) do ...@@ -896,12 +896,14 @@ ActiveRecord::Schema.define(version: 20180418053107) do
add_index "identities", ["user_id"], name: "index_identities_on_user_id", using: :btree add_index "identities", ["user_id"], name: "index_identities_on_user_id", using: :btree
create_table "internal_ids", id: :bigserial, force: :cascade do |t| 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 "usage", null: false
t.integer "last_value", null: false t.integer "last_value", null: false
t.integer "namespace_id"
end 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| create_table "issue_assignees", id: false, force: :cascade do |t|
t.integer "user_id", null: false t.integer "user_id", null: false
...@@ -2113,6 +2115,7 @@ ActiveRecord::Schema.define(version: 20180418053107) do ...@@ -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", "gpg_keys", on_delete: :nullify
add_foreign_key "gpg_signatures", "projects", on_delete: :cascade 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 "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 "internal_ids", "projects", on_delete: :cascade
add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", 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 add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade
......
...@@ -16,6 +16,15 @@ describe Deployment do ...@@ -16,6 +16,15 @@ describe Deployment do
it { is_expected.to validate_presence_of(:ref) } it { is_expected.to validate_presence_of(:ref) }
it { is_expected.to validate_presence_of(:sha) } 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 describe 'after_create callbacks' do
let(:environment) { create(:environment) } let(:environment) { create(:environment) }
let(:store) { Gitlab::EtagCaching::Store.new } let(:store) { Gitlab::EtagCaching::Store.new }
......
...@@ -17,11 +17,17 @@ describe MergeRequest do ...@@ -17,11 +17,17 @@ describe MergeRequest do
describe 'modules' do describe 'modules' do
subject { described_class } subject { described_class }
it { is_expected.to include_module(NonatomicInternalId) }
it { is_expected.to include_module(Issuable) } it { is_expected.to include_module(Issuable) }
it { is_expected.to include_module(Referable) } it { is_expected.to include_module(Referable) }
it { is_expected.to include_module(Sortable) } it { is_expected.to include_module(Sortable) }
it { is_expected.to include_module(Taskable) } 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 end
describe 'validation' do describe 'validation' do
......
require 'spec_helper' require 'spec_helper'
describe Milestone do 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 describe "Validation" do
before do before do
allow(subject).to receive(:set_iid).and_return(false) allow(subject).to receive(:set_iid).and_return(false)
......
...@@ -19,6 +19,14 @@ shared_examples_for 'AtomicInternalId' do ...@@ -19,6 +19,14 @@ shared_examples_for 'AtomicInternalId' do
it { is_expected.to validate_numericality_of(internal_id_attribute) } it { is_expected.to validate_numericality_of(internal_id_attribute) }
end 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 describe 'internal id generation' do
subject { instance.save! } subject { instance.save! }
......
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