Commit 97fa44cf authored by Alex Kalderimis's avatar Alex Kalderimis Committed by Markus Koller

Add IID to design management designs

This adds a new column to design_management_designs, an internal ID
scoped to project.

The column is null by default for now, and a background migration runs
to update all designs in batches by project.
parent 8be38eb8
......@@ -34,7 +34,7 @@ module AlertManagement
has_many :ordered_notes, -> { fresh }, as: :noteable, class_name: 'Note'
has_many :user_mentions, class_name: 'AlertManagement::AlertUserMention', foreign_key: :alert_management_alert_id
has_internal_id :iid, scope: :project, init: ->(s) { s.project.alert_management_alerts.maximum(:iid) }
has_internal_id :iid, scope: :project
sha_attribute :fingerprint
......
......@@ -42,9 +42,16 @@ module Ci
belongs_to :external_pull_request
belongs_to :ci_ref, class_name: 'Ci::Ref', foreign_key: :ci_ref_id, inverse_of: :pipelines
has_internal_id :iid, scope: :project, presence: false, track_if: -> { !importing? }, ensure_if: -> { !importing? }, init: ->(s) do
s&.project&.all_pipelines&.maximum(:iid) || s&.project&.all_pipelines&.count
end
has_internal_id :iid, scope: :project, presence: false,
track_if: -> { !importing? },
ensure_if: -> { !importing? },
init: ->(pipeline, scope) do
if pipeline
pipeline.project&.all_pipelines&.maximum(:iid) || pipeline.project&.all_pipelines&.count
elsif scope
::Ci::Pipeline.where(**scope).maximum(:iid)
end
end
has_many :stages, -> { order(position: :asc) }, inverse_of: :pipeline
has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
......
......@@ -27,16 +27,42 @@ module AtomicInternalId
extend ActiveSupport::Concern
class_methods do
def has_internal_id(column, scope:, init:, ensure_if: nil, track_if: nil, presence: true, backfill: false) # rubocop:disable Naming/PredicateName
# We require init here to retain the ability to recalculate in the absence of a
# InternalId record (we may delete records in `internal_ids` for example).
raise "has_internal_id requires a init block, none given." unless init
def has_internal_id( # rubocop:disable Naming/PredicateName
column, scope:, init: :not_given, ensure_if: nil, track_if: nil,
presence: true, backfill: false, hook_names: :create)
raise "has_internal_id init must not be nil if given." if init.nil?
raise "has_internal_id needs to be defined on association." unless self.reflect_on_association(scope)
before_validation :"track_#{scope}_#{column}!", on: :create, if: track_if
before_validation :"ensure_#{scope}_#{column}!", on: :create, if: ensure_if
init = infer_init(scope) if init == :not_given
before_validation :"track_#{scope}_#{column}!", on: hook_names, if: track_if
before_validation :"ensure_#{scope}_#{column}!", on: hook_names, if: ensure_if
validates column, presence: presence
define_singleton_internal_id_methods(scope, column, init)
define_instance_internal_id_methods(scope, column, init, backfill)
end
private
def infer_init(scope)
case scope
when :project
AtomicInternalId.project_init(self)
when :group
AtomicInternalId.group_init(self)
else
# We require init here to retain the ability to recalculate in the absence of a
# InternalId record (we may delete records in `internal_ids` for example).
raise "has_internal_id - cannot infer init for scope: #{scope}"
end
end
# Defines instance methods:
# - ensure_{scope}_{column}!
# - track_{scope}_{column}!
# - reset_{scope}_{column}
# - {column}=
def define_instance_internal_id_methods(scope, column, init, backfill)
define_method("ensure_#{scope}_#{column}!") do
return if backfill && self.class.where(column => nil).exists?
......@@ -103,19 +129,95 @@ module AtomicInternalId
read_attribute(column)
end
end
# Defines class methods:
#
# - with_{scope}_{column}_supply
# This method can be used to allocate a block of IID values during
# bulk operations (importing/copying, etc). This can be more efficient
# than creating instances one-by-one.
#
# Pass in a block that receives a `Supply` instance. To allocate a new
# IID value, call `Supply#next_value`.
#
# Example:
#
# MyClass.with_project_iid_supply(project) do |supply|
# attributes = MyClass.where(project: project).find_each do |record|
# record.attributes.merge(iid: supply.next_value)
# end
#
# bulk_insert(attributes)
# end
def define_singleton_internal_id_methods(scope, column, init)
define_singleton_method("with_#{scope}_#{column}_supply") do |scope_value, &block|
subject = find_by(scope => scope_value) || self
scope_attrs = ::AtomicInternalId.scope_attrs(scope_value)
usage = ::AtomicInternalId.scope_usage(self)
generator = InternalId::InternalIdGenerator.new(subject, scope_attrs, usage, init)
generator.with_lock do
supply = Supply.new(generator.record.last_value)
block.call(supply)
ensure
generator.track_greatest(supply.current_value) if supply
end
end
end
end
def self.scope_attrs(scope_value)
{ scope_value.class.table_name.singularize.to_sym => scope_value } if scope_value
end
def internal_id_scope_attrs(scope)
scope_value = internal_id_read_scope(scope)
{ scope_value.class.table_name.singularize.to_sym => scope_value } if scope_value
::AtomicInternalId.scope_attrs(scope_value)
end
def internal_id_scope_usage
self.class.table_name.to_sym
::AtomicInternalId.scope_usage(self.class)
end
def self.scope_usage(including_class)
including_class.table_name.to_sym
end
def self.project_init(klass, column_name = :iid)
->(instance, scope) do
if instance
klass.where(project_id: instance.project_id).maximum(column_name)
elsif scope.present?
klass.where(**scope).maximum(column_name)
end
end
end
def self.group_init(klass, column_name = :iid)
->(instance, scope) do
if instance
klass.where(group_id: instance.group_id).maximum(column_name)
elsif scope.present?
klass.where(group: scope[:namespace]).maximum(column_name)
end
end
end
def internal_id_read_scope(scope)
association(scope).reader
end
class Supply
attr_reader :current_value
def initialize(start_value)
@current_value = start_value
end
def next_value
@current_value += 1
end
end
end
......@@ -14,7 +14,8 @@ module Enums
operations_feature_flags: 6,
operations_user_lists: 7,
alert_management_alerts: 8,
sprints: 9 # iterations
sprints: 9, # iterations
design_management_designs: 10
}
end
end
......
......@@ -21,9 +21,7 @@ class Deployment < ApplicationRecord
has_one :deployment_cluster
has_internal_id :iid, scope: :project, track_if: -> { !importing? }, init: ->(s) do
Deployment.where(project: s.project).maximum(:iid) if s&.project
end
has_internal_id :iid, scope: :project, track_if: -> { !importing? }
validates :sha, presence: true
validates :ref, presence: true
......
......@@ -2,6 +2,7 @@
module DesignManagement
class Design < ApplicationRecord
include AtomicInternalId
include Importable
include Noteable
include Gitlab::FileTypeDetection
......@@ -26,6 +27,10 @@ module DesignManagement
has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_internal_id :iid, scope: :project, presence: true,
hook_names: %i[create update], # Deal with old records
track_if: -> { !importing? }
validates :project, :filename, presence: true
validates :issue, presence: true, unless: :importing?
validates :filename, uniqueness: { scope: :issue_id }, length: { maximum: 255 }
......
......@@ -61,13 +61,13 @@ class InternalId < ApplicationRecord
class << self
def track_greatest(subject, scope, usage, new_value, init)
InternalIdGenerator.new(subject, scope, usage)
.track_greatest(init, new_value)
InternalIdGenerator.new(subject, scope, usage, init)
.track_greatest(new_value)
end
def generate_next(subject, scope, usage, init)
InternalIdGenerator.new(subject, scope, usage)
.generate(init)
InternalIdGenerator.new(subject, scope, usage, init)
.generate
end
def reset(subject, scope, usage, value)
......@@ -99,15 +99,18 @@ class InternalId < ApplicationRecord
# 4) In the absence of a record in the internal_ids table, one will be created
# and last_value will be calculated on the fly.
#
# subject: The instance we're generating an internal id for. Gets passed to init if called.
# subject: The instance or class we're generating an internal id for.
# scope: Attributes that define the scope for id generation.
# Valid keys are `project/project_id` and `namespace/namespace_id`.
# usage: Symbol to define the usage of the internal id, see InternalId.usages
attr_reader :subject, :scope, :scope_attrs, :usage
# init: Proc that accepts the subject and the scope and returns Integer|NilClass
attr_reader :subject, :scope, :scope_attrs, :usage, :init
def initialize(subject, scope, usage)
def initialize(subject, scope, usage, init = nil)
@subject = subject
@scope = scope
@usage = usage
@init = init
raise ArgumentError, 'Scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty?
......@@ -119,13 +122,13 @@ class InternalId < ApplicationRecord
# Generates next internal id and returns it
# 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).
def generate(init)
def generate
subject.transaction do
# Create a record in internal_ids if one does not yet exist
# and increment its last value
#
# Note this will acquire a ROW SHARE lock on the InternalId record
(lookup || create_record(init)).increment_and_save!
record.increment_and_save!
end
end
......@@ -148,12 +151,20 @@ class InternalId < ApplicationRecord
# and set its new_value if it is higher than the current last_value
#
# Note this will acquire a ROW SHARE lock on the InternalId record
def track_greatest(init, new_value)
def track_greatest(new_value)
subject.transaction do
(lookup || create_record(init)).track_greatest_and_save!(new_value)
record.track_greatest_and_save!(new_value)
end
end
def record
@record ||= (lookup || create_record)
end
def with_lock(&block)
record.with_lock(&block)
end
private
# Retrieve InternalId record for (project, usage) combination, if it exists
......@@ -171,12 +182,16 @@ class InternalId < ApplicationRecord
# was faster in doing this, we'll realize once we hit the unique key constraint
# violation. We can safely roll-back the nested transaction and perform
# a lookup instead to retrieve the record.
def create_record(init)
def create_record
raise ArgumentError, 'Cannot initialize without init!' unless init
instance = subject.is_a?(::Class) ? nil : subject
subject.transaction(requires_new: true) do
InternalId.create!(
**scope,
usage: usage_value,
last_value: init.call(subject) || 0
last_value: init.call(instance, scope) || 0
)
end
rescue ActiveRecord::RecordNotUnique
......
......@@ -48,7 +48,7 @@ class Issue < ApplicationRecord
belongs_to :moved_to, class_name: 'Issue'
has_one :moved_from, class_name: 'Issue', foreign_key: :moved_to_id
has_internal_id :iid, scope: :project, track_if: -> { !importing? }, init: ->(s) { s&.project&.issues&.maximum(:iid) }
has_internal_id :iid, scope: :project, track_if: -> { !importing? }
has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
......
......@@ -17,8 +17,8 @@ class Iteration < ApplicationRecord
belongs_to :project
belongs_to :group
has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.iterations&.maximum(:iid) }
has_internal_id :iid, scope: :group, init: ->(s) { s&.group&.iterations&.maximum(:iid) }
has_internal_id :iid, scope: :project
has_internal_id :iid, scope: :group
validates :start_date, presence: true
validates :due_date, presence: true
......
......@@ -41,7 +41,14 @@ class MergeRequest < ApplicationRecord
belongs_to :merge_user, class_name: "User"
belongs_to :iteration, foreign_key: 'sprint_id'
has_internal_id :iid, scope: :target_project, track_if: -> { !importing? }, init: ->(s) { s&.target_project&.merge_requests&.maximum(:iid) }
has_internal_id :iid, scope: :target_project, track_if: -> { !importing? },
init: ->(mr, scope) do
if mr
mr.target_project&.merge_requests&.maximum(:iid)
elsif scope[:project]
where(target_project: scope[:project]).maximum(:iid)
end
end
has_many :merge_request_diffs
has_many :merge_request_context_commits, inverse_of: :merge_request
......
......@@ -12,8 +12,8 @@ class Milestone < ApplicationRecord
has_many :milestone_releases
has_many :releases, through: :milestone_releases
has_internal_id :iid, scope: :project, track_if: -> { !importing? }, init: ->(s) { s&.project&.milestones&.maximum(:iid) }
has_internal_id :iid, scope: :group, track_if: -> { !importing? }, init: ->(s) { s&.group&.milestones&.maximum(:iid) }
has_internal_id :iid, scope: :project, track_if: -> { !importing? }
has_internal_id :iid, scope: :group, track_if: -> { !importing? }
has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
......
......@@ -13,7 +13,7 @@ module Operations
belongs_to :project
has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.operations_feature_flags&.maximum(:iid) }
has_internal_id :iid, scope: :project
default_value_for :active, true
......
......@@ -13,7 +13,7 @@ module Operations
has_many :strategy_user_lists
has_many :strategies, through: :strategy_user_lists
has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.operations_feature_flags_user_lists&.maximum(:iid) }, presence: true
has_internal_id :iid, scope: :project, presence: true
validates :project, presence: true
validates :name,
......
......@@ -172,20 +172,26 @@ module DesignManagement
def copy_designs!
design_attributes = attributes_config[:design_attributes]
new_rows = designs.map do |design|
design.attributes.slice(*design_attributes).merge(
issue_id: target_issue.id,
project_id: target_project.id
::DesignManagement::Design.with_project_iid_supply(target_project) do |supply|
new_rows = designs.each_with_index.map do |design, i|
design.attributes.slice(*design_attributes).merge(
issue_id: target_issue.id,
project_id: target_project.id,
iid: supply.next_value
)
end
# TODO Replace `Gitlab::Database.bulk_insert` with `BulkInsertSafe`
# once https://gitlab.com/gitlab-org/gitlab/-/issues/247718 is fixed.
# When this is fixed, we can remove the call to
# `with_project_iid_supply` above, since the objects will be instantiated
# and callbacks (including `ensure_project_iid!`) will fire.
::Gitlab::Database.bulk_insert( # rubocop:disable Gitlab/BulkInsert
DesignManagement::Design.table_name,
new_rows,
return_ids: true
)
end
# TODO Replace `Gitlab::Database.bulk_insert` with `BulkInsertSafe`
# once https://gitlab.com/gitlab-org/gitlab/-/issues/247718 is fixed.
::Gitlab::Database.bulk_insert( # rubocop:disable Gitlab/BulkInsert
DesignManagement::Design.table_name,
new_rows,
return_ids: true
)
end
def copy_versions!
......
---
title: Add iid column to design_management_designs
merge_request: 46596
author:
type: added
# frozen_string_literal: true
class AddIidToDesignManagementDesign < ActiveRecord::Migration[6.0]
DOWNTIME = false
def change
add_column :design_management_designs, :iid, :integer
end
end
# frozen_string_literal: true
class AddIndexOnDesignManagementDesignsIidProjectId < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
INDEX_NAME = 'index_design_management_designs_on_iid_and_project_id'
def up
add_concurrent_index :design_management_designs, [:project_id, :iid],
name: INDEX_NAME,
unique: true
end
def down
remove_concurrent_index_by_name :design_management_designs, INDEX_NAME
end
end
# frozen_string_literal: true
class BackfillDesignIids < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
class Designs < ActiveRecord::Base
include EachBatch
self.table_name = 'design_management_designs'
end
def up
backfill = ::Gitlab::BackgroundMigration::BackfillDesignInternalIds.new(Designs)
Designs.select(:project_id).distinct.each_batch(of: 100, column: :project_id) do |relation|
backfill.perform(relation)
end
end
def down
# NOOP
end
end
# frozen_string_literal: true
class AddNotNullCheckOnIidOnDesignManangementDesigns < ActiveRecord::Migration[6.0]
include ::Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_not_null_constraint(:design_management_designs, :iid)
end
def down
remove_not_null_constraint(:design_management_designs, :iid)
end
end
bef50f2417b9676c89aea838f7b9c85fb88af9f52c197d8eb4613a9c91bc7741
\ No newline at end of file
2f6c7efc1716d02dd40adb08bd09b9f1e63e4248619678c0562f4b8d581e6065
\ No newline at end of file
3937235469c8fb1f2b0af9cdf38933db5ae61552d1a9050755cec5f7c16ebb66
\ No newline at end of file
7ec73c06ccc4c9f618e0455d0a7aae3b591bf52b5ddb1b3f1678d2fd50b9fd5e
\ No newline at end of file
......@@ -11650,7 +11650,9 @@ CREATE TABLE design_management_designs (
issue_id integer,
filename character varying NOT NULL,
relative_position integer,
CONSTRAINT check_07155e2715 CHECK ((char_length((filename)::text) <= 255))
iid integer,
CONSTRAINT check_07155e2715 CHECK ((char_length((filename)::text) <= 255)),
CONSTRAINT check_cfb92df01a CHECK ((iid IS NOT NULL))
);
CREATE SEQUENCE design_management_designs_id_seq
......@@ -20592,6 +20594,8 @@ CREATE INDEX index_description_versions_on_merge_request_id ON description_versi
CREATE INDEX index_design_management_designs_issue_id_relative_position_id ON design_management_designs USING btree (issue_id, relative_position, id);
CREATE UNIQUE INDEX index_design_management_designs_on_iid_and_project_id ON design_management_designs USING btree (project_id, iid);
CREATE UNIQUE INDEX index_design_management_designs_on_issue_id_and_filename ON design_management_designs USING btree (issue_id, filename);
CREATE INDEX index_design_management_designs_on_project_id ON design_management_designs USING btree (project_id);
......
......@@ -51,7 +51,7 @@ module EE
has_many :children, class_name: "Epic", foreign_key: :parent_id
has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_internal_id :iid, scope: :group, init: ->(s) { s&.group&.epics&.maximum(:iid) }
has_internal_id :iid, scope: :group
has_many :epic_issues
has_many :issues, through: :epic_issues
......
......@@ -23,7 +23,7 @@ module RequirementsManagement
has_many :test_reports, inverse_of: :requirement
has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.requirements&.maximum(:iid) }
has_internal_id :iid, scope: :project
validates :author, :project, :title, presence: true
......
......@@ -106,7 +106,7 @@ RSpec.describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMent
let(:design_user_mentions) { table(:design_user_mentions) }
let(:project) { projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1', namespace_id: group.id, visibility_level: 0) }
let!(:design) { designs.create!(filename: 'test.png', project_id: project.id) }
let!(:design) { designs.create!(filename: 'test.png', project_id: project.id, iid: 1000) }
let!(:note1) { notes.create!(noteable_id: design.id, noteable_type: 'DesignManagement::Design', project_id: project.id, author_id: author.id, note: description_mentions) }
let!(:note2) { notes.create!(noteable_id: design.id, noteable_type: 'DesignManagement::Design', project_id: project.id, author_id: author.id, note: 'sample note') }
......
# frozen_string_literal: true
module Gitlab
module BackgroundMigration
# Backfill design.iid for a range of projects
class BackfillDesignInternalIds
# See app/models/internal_id
# This is a direct copy of the application code with the following changes:
# - usage enum is hard-coded to the value for design_management_designs
# - init is not passed around, but ignored
class InternalId < ActiveRecord::Base
def self.track_greatest(subject, scope, new_value)
InternalIdGenerator.new(subject, scope).track_greatest(new_value)
end
# Increments #last_value with new_value if it is greater than the current,
# and saves the record
#
# The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL).
# As such, the increment is atomic and safe to be called concurrently.
def track_greatest_and_save!(new_value)
update_and_save { self.last_value = [last_value || 0, new_value].max }
end
private
def update_and_save(&block)
lock!
yield
# update_and_save_counter.increment(usage: usage, changed: last_value_changed?)
save!
last_value
end
end
# See app/models/internal_id
class InternalIdGenerator
attr_reader :subject, :scope, :scope_attrs
def initialize(subject, scope)
@subject = subject
@scope = scope
raise ArgumentError, 'Scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty?
end
# Create a record in internal_ids if one does not yet exist
# and set its new_value if it is higher than the current last_value
#
# Note this will acquire a ROW SHARE lock on the InternalId record
def track_greatest(new_value)
subject.transaction do
record.track_greatest_and_save!(new_value)
end
end
def record
@record ||= (lookup || create_record)
end
def lookup
InternalId.find_by(**scope, usage: usage_value)
end
def usage_value
10 # see Enums::InternalId - this is the value for design_management_designs
end
# Create InternalId record for (scope, usage) combination, if it doesn't exist
#
# We blindly insert without synchronization. If another process
# was faster in doing this, we'll realize once we hit the unique key constraint
# violation. We can safely roll-back the nested transaction and perform
# a lookup instead to retrieve the record.
def create_record
subject.transaction(requires_new: true) do
InternalId.create!(
**scope,
usage: usage_value,
last_value: 0
)
end
rescue ActiveRecord::RecordNotUnique
lookup
end
end
attr_reader :design_class
def initialize(design_class)
@design_class = design_class
end
def perform(relation)
start_id, end_id = relation.pluck("min(project_id), max(project_id)").flatten
table = 'design_management_designs'
ActiveRecord::Base.connection.execute <<~SQL
WITH
starting_iids(project_id, iid) as (
SELECT project_id, MAX(COALESCE(iid, 0))
FROM #{table}
WHERE project_id BETWEEN #{start_id} AND #{end_id}
GROUP BY project_id
),
with_calculated_iid(id, iid) as (
SELECT design.id,
init.iid + ROW_NUMBER() OVER (PARTITION BY design.project_id ORDER BY design.id ASC)
FROM #{table} as design, starting_iids as init
WHERE design.project_id BETWEEN #{start_id} AND #{end_id}
AND design.iid IS NULL
AND init.project_id = design.project_id
)
UPDATE #{table}
SET iid = with_calculated_iid.iid
FROM with_calculated_iid
WHERE #{table}.id = with_calculated_iid.id
SQL
# track the new greatest IID value
relation.each do |design|
current_max = design_class.where(project_id: design.project_id).maximum(:iid)
scope = { project_id: design.project_id }
InternalId.track_greatest(design, scope, current_max)
end
end
end
end
end
......@@ -29,6 +29,7 @@ ignore_design_attributes:
- id
- issue_id
- project_id
- iid
ignore_version_attributes:
- id
......
# frozen_string_literal: true
FactoryBot.define do
factory :design, class: 'DesignManagement::Design' do
factory :design, traits: [:has_internal_id], class: 'DesignManagement::Design' do
issue { association(:issue) }
project { issue&.project || association(:project) }
sequence(:filename) { |n| "homescreen-#{n}.jpg" }
......
......@@ -98,6 +98,7 @@
"designs":[
{
"id":38,
"iid": 1,
"project_id":30,
"issue_id":469,
"filename":"chirrido3.jpg",
......@@ -107,6 +108,7 @@
},
{
"id":39,
"iid": 2,
"project_id":30,
"issue_id":469,
"filename":"jonathan_richman.jpg",
......@@ -116,6 +118,7 @@
},
{
"id":40,
"iid": 3,
"project_id":30,
"issue_id":469,
"filename":"mariavontrap.jpeg",
......@@ -137,6 +140,7 @@
"event":0,
"design":{
"id":38,
"iid": 1,
"project_id":30,
"issue_id":469,
"filename":"chirrido3.jpg"
......@@ -156,6 +160,7 @@
"event":1,
"design":{
"id":38,
"iid": 1,
"project_id":30,
"issue_id":469,
"filename":"chirrido3.jpg"
......@@ -167,6 +172,7 @@
"event":0,
"design":{
"id":39,
"iid": 2,
"project_id":30,
"issue_id":469,
"filename":"jonathan_richman.jpg"
......@@ -186,6 +192,7 @@
"event":1,
"design":{
"id":38,
"iid": 1,
"project_id":30,
"issue_id":469,
"filename":"chirrido3.jpg"
......@@ -197,6 +204,7 @@
"event":2,
"design":{
"id":39,
"iid": 2,
"project_id":30,
"issue_id":469,
"filename":"jonathan_richman.jpg"
......@@ -208,6 +216,7 @@
"event":0,
"design":{
"id":40,
"iid": 3,
"project_id":30,
"issue_id":469,
"filename":"mariavontrap.jpeg"
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::BackgroundMigration::BackfillDesignInternalIds, :migration, schema: 20201030203854 do
subject { described_class.new(designs) }
let_it_be(:namespaces) { table(:namespaces) }
let_it_be(:projects) { table(:projects) }
let_it_be(:designs) { table(:design_management_designs) }
let(:namespace) { namespaces.create!(name: 'foo', path: 'foo') }
let(:project) { projects.create!(namespace_id: namespace.id) }
let(:project_2) { projects.create!(namespace_id: namespace.id) }
def create_design!(proj = project)
designs.create!(project_id: proj.id, filename: generate(:filename))
end
def migrate!
relation = designs.where(project_id: [project.id, project_2.id]).select(:project_id).distinct
subject.perform(relation)
end
it 'backfills the iid for designs' do
3.times { create_design! }
expect do
migrate!
end.to change { designs.pluck(:iid) }.from(contain_exactly(nil, nil, nil)).to(contain_exactly(1, 2, 3))
end
it 'scopes IIDs and handles range and starting-point correctly' do
create_design!.update!(iid: 10)
create_design!.update!(iid: 12)
create_design!(project_2).update!(iid: 7)
project_3 = projects.create!(namespace_id: namespace.id)
2.times { create_design! }
2.times { create_design!(project_2) }
2.times { create_design!(project_3) }
migrate!
expect(designs.where(project_id: project.id).pluck(:iid)).to contain_exactly(10, 12, 13, 14)
expect(designs.where(project_id: project_2.id).pluck(:iid)).to contain_exactly(7, 8, 9)
expect(designs.where(project_id: project_3.id).pluck(:iid)).to contain_exactly(nil, nil)
end
it 'updates the internal ID records' do
design = create_design!
2.times { create_design! }
design.update!(iid: 10)
scope = { project_id: project.id }
usage = :design_management_designs
init = ->(_d, _s) { 0 }
::InternalId.track_greatest(design, scope, usage, 10, init)
migrate!
next_iid = ::InternalId.generate_next(design, scope, usage, init)
expect(designs.pluck(:iid)).to contain_exactly(10, 11, 12)
expect(design.reload.iid).to eq(10)
expect(next_iid).to eq(13)
end
end
......@@ -1680,7 +1680,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
has_internal_id :iid,
scope: :project,
init: ->(s) { s&.project&.issues&.maximum(:iid) },
init: ->(s, _scope) { s&.project&.issues&.maximum(:iid) },
backfill: true,
presence: false
end
......
......@@ -774,6 +774,7 @@ ExternalPullRequest:
- target_sha
DesignManagement::Design:
- id
- iid
- project_id
- filename
- relative_position
......
......@@ -86,4 +86,20 @@ RSpec.describe AtomicInternalId do
expect { subject }.to change { milestone.iid }.from(nil).to(iid.to_i)
end
end
describe '.with_project_iid_supply' do
let(:iid) { 100 }
it 'wraps generate and track_greatest in a concurrency-safe lock' do
expect_next_instance_of(InternalId::InternalIdGenerator) do |g|
expect(g).to receive(:with_lock).and_call_original
expect(g.record).to receive(:last_value).and_return(iid)
expect(g).to receive(:track_greatest).with(iid + 4)
end
::Milestone.with_project_iid_supply(milestone.project) do |supply|
4.times { supply.next_value }
end
end
end
end
......@@ -11,6 +11,14 @@ RSpec.describe DesignManagement::Design do
let_it_be(:design3) { create(:design, :with_versions, issue: issue, versions_count: 1) }
let_it_be(:deleted_design) { create(:design, :with_versions, deleted: true) }
it_behaves_like 'AtomicInternalId', validate_presence: true do
let(:internal_id_attribute) { :iid }
let(:instance) { build(:design, issue: issue) }
let(:scope) { :project }
let(:scope_attrs) { { project: instance.project } }
let(:usage) { :design_management_designs }
end
it_behaves_like 'a class that supports relative positioning' do
let_it_be(:relative_parent) { create(:issue) }
......
......@@ -6,8 +6,9 @@ RSpec.describe InternalId do
let(:project) { create(:project) }
let(:usage) { :issues }
let(:issue) { build(:issue, project: project) }
let(:id_subject) { issue }
let(:scope) { { project: project } }
let(:init) { ->(s) { s.project.issues.size } }
let(:init) { ->(issue, scope) { issue&.project&.issues&.size || Issue.where(**scope).count } }
it_behaves_like 'having unique enum values'
......@@ -39,7 +40,7 @@ RSpec.describe InternalId do
end
describe '.generate_next' do
subject { described_class.generate_next(issue, scope, usage, init) }
subject { described_class.generate_next(id_subject, scope, usage, init) }
context 'in the absence of a record' do
it 'creates a record if not yet present' do
......@@ -88,6 +89,14 @@ RSpec.describe InternalId do
expect(normalized).to eq((0..seq.size - 1).to_a)
end
context 'there are no instances to pass in' do
let(:id_subject) { Issue }
it 'accepts classes instead' do
expect(subject).to eq(1)
end
end
end
describe '.reset' do
......@@ -130,7 +139,7 @@ RSpec.describe InternalId do
describe '.track_greatest' do
let(:value) { 9001 }
subject { described_class.track_greatest(issue, scope, usage, value, init) }
subject { described_class.track_greatest(id_subject, scope, usage, value, init) }
context 'in the absence of a record' do
it 'creates a record if not yet present' do
......@@ -166,6 +175,14 @@ RSpec.describe InternalId do
expect(subject).to eq 10_001
end
end
context 'there are no instances to pass in' do
let(:id_subject) { Issue }
it 'accepts classes instead' do
expect(subject).to eq(value)
end
end
end
describe '#increment_and_save!' do
......
......@@ -68,6 +68,31 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla
include_examples 'service error', message: 'Target design collection already has designs'
end
context 'when target project already has designs' do
let!(:issue_x) { create(:issue, project: target_issue.project) }
let!(:existing) { create(:design, issue: issue_x, project: target_issue.project) }
let(:new_designs) do
target_issue.reset
target_issue.designs.where.not(id: existing.id)
end
it 'sets IIDs for new designs above existing ones' do
subject
expect(new_designs).to all(have_attributes(iid: (be > existing.iid)))
end
it 'does not allow for IID collisions' do
subject
create(:design, issue: issue_x, project: target_issue.project)
design_iids = target_issue.project.designs.map(&:id)
expect(design_iids).to match_array(design_iids.uniq)
end
end
include_examples 'service success'
it 'creates a design repository for the target project' do
......@@ -162,9 +187,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla
it 'copies the Git repository data', :aggregate_failures do
subject
commit_shas = target_repository.commits('master', limit: 99).map(&:id)
expect(commit_shas).to include(*target_issue.design_versions.ordered.pluck(:sha))
expect(commits_on_master(limit: 99)).to include(*target_issue.design_versions.ordered.pluck(:sha))
end
it 'creates a master branch if none previously existed' do
......@@ -212,9 +235,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla
issue_2 = create(:issue, project: target_issue.project)
create(:design, :with_file, issue: issue_2, project: target_issue.project)
expect { subject }.not_to change {
expect(target_repository.commits('master', limit: 10).size).to eq(1)
}
expect { subject }.not_to change { commits_on_master }
end
it 'sets the design collection copy state' do
......@@ -223,6 +244,10 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla
expect(target_issue.design_collection).to be_copy_error
end
end
def commits_on_master(limit: 10)
target_repository.commits('master', limit: limit).map(&:id)
end
end
end
end
......
......@@ -76,6 +76,26 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true|
end
end
describe 'supply of internal ids' do
let(:scope_value) { scope_attrs.each_value.first }
let(:method_name) { :"with_#{scope}_#{internal_id_attribute}_supply" }
it 'provides a persistent supply of IID values, sensitive to the current state' do
iid = rand(1..1000)
write_internal_id(iid)
instance.public_send(:"track_#{scope}_#{internal_id_attribute}!")
# Allocate 3 IID values
described_class.public_send(method_name, scope_value) do |supply|
3.times { supply.next_value }
end
current_value = described_class.public_send(method_name, scope_value, &:current_value)
expect(current_value).to eq(iid + 3)
end
end
describe "#reset_scope_internal_id_attribute" do
it 'rewinds the allocated IID' do
expect { ensure_scope_attribute! }.not_to raise_error
......
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