From af2e1919e8cdc93e30e4cbb2616d29f1605b1f25 Mon Sep 17 00:00:00 2001
From: Jon Jenkins <jjenkins@gitlab.com>
Date: Wed, 27 Sep 2023 10:20:35 +0000
Subject: [PATCH] Add code to support running migrations in milestone order
In order to support running of migrations in milestone order, this
commit adds some of the necessary classes and tests to make this work.
---
.rubocop.yml | 6 +
gems/gitlab-utils/lib/gitlab/version_info.rb | 12 ++
.../spec/gitlab/version_info_spec.rb | 34 +++++
lib/gitlab/database/migration.rb | 6 +-
.../database/migrations/milestone_mixin.rb | 35 +++++
lib/gitlab/database/migrations/version.rb | 76 +++++++++++
.../migrations/milestone_mixin_spec.rb | 48 +++++++
.../database/migrations/version_spec.rb | 120 ++++++++++++++++++
8 files changed, 334 insertions(+), 3 deletions(-)
create mode 100644 lib/gitlab/database/migrations/milestone_mixin.rb
create mode 100644 lib/gitlab/database/migrations/version.rb
create mode 100644 spec/lib/gitlab/database/migrations/milestone_mixin_spec.rb
create mode 100644 spec/lib/gitlab/database/migrations/version_spec.rb
diff --git a/.rubocop.yml b/.rubocop.yml
index f613a43b2247c..0ebb17ce59358 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -119,6 +119,12 @@ Lint/EmptyFile:
- 'ee/db/embedding/seeds.rb'
- 'ee/db/geo/seeds.rb'
+# This file has a lot of these, and how we name classes here is essential for how we
+# implement migration versions
+Naming/ClassAndModuleCamelCase:
+ Exclude:
+ - 'lib/gitlab/database/migration.rb'
+
# This cop checks whether some constant value isn't a
# mutable literal (e.g. array or hash).
Style/MutableConstant:
diff --git a/gems/gitlab-utils/lib/gitlab/version_info.rb b/gems/gitlab-utils/lib/gitlab/version_info.rb
index 00a9b4ddc6ed4..21478c4625908 100644
--- a/gems/gitlab-utils/lib/gitlab/version_info.rb
+++ b/gems/gitlab-utils/lib/gitlab/version_info.rb
@@ -7,10 +7,22 @@ class VersionInfo
attr_reader :major, :minor, :patch
VERSION_REGEX = /(\d+)\.(\d+)\.(\d+)/
+ MILESTONE_REGEX = /\A(\d+)\.(\d+)\z/
# To mitigate ReDoS, limit the length of the version string we're
# willing to check
MAX_VERSION_LENGTH = 128
+ InvalidMilestoneError = Class.new(StandardError)
+
+ def self.parse_from_milestone(str)
+ raise InvalidMilestoneError if str.length > MAX_VERSION_LENGTH
+
+ m = MILESTONE_REGEX.match(str)
+ raise InvalidMilestoneError if m.nil?
+
+ VersionInfo.new(m[1].to_i, m[2].to_i)
+ end
+
def self.parse(str, parse_suffix: false)
return str if str.is_a?(self)
diff --git a/gems/gitlab-utils/spec/gitlab/version_info_spec.rb b/gems/gitlab-utils/spec/gitlab/version_info_spec.rb
index 2b5f6bcb4c132..ca13a06b92cf3 100644
--- a/gems/gitlab-utils/spec/gitlab/version_info_spec.rb
+++ b/gems/gitlab-utils/spec/gitlab/version_info_spec.rb
@@ -130,6 +130,40 @@
end
end
+ describe '.parse_from_milestone' do
+ subject(:milestone) { described_class.parse_from_milestone(milestone_str) }
+
+ context 'when the milestone string is valid' do
+ let(:milestone_str) { '14.7' }
+
+ it "creates a #{described_class.class} with patch version zero" do
+ expect(milestone.major).to eq 14
+ expect(milestone.minor).to eq 7
+ expect(milestone.patch).to eq 0
+ end
+ end
+
+ context 'when the milestone string is not valid' do
+ let(:milestone_str) { 'foo' }
+
+ it 'raises InvalidMilestoneError' do
+ expect do
+ milestone
+ end.to raise_error "#{described_class}::InvalidMilestoneError".constantize
+ end
+ end
+
+ context 'when the milestone string is too long' do
+ let(:milestone_str) { 'a' * 129 }
+
+ it 'raises InvalidMilestoneError' do
+ expect do
+ milestone
+ end.to raise_error "#{described_class}::InvalidMilestoneError".constantize
+ end
+ end
+ end
+
describe '.to_s' do
it { expect(@v1_0_0.to_s).to eq("1.0.0") }
it { expect(@v1_0_1_rc1.to_s).to eq("1.0.1-rc1") }
diff --git a/lib/gitlab/database/migration.rb b/lib/gitlab/database/migration.rb
index fbb71c1ccfd0f..41044816de908 100644
--- a/lib/gitlab/database/migration.rb
+++ b/lib/gitlab/database/migration.rb
@@ -34,7 +34,7 @@ def enable_lock_retries?
# to indicate backwards-compatible or otherwise minor changes (e.g. a Rails version bump).
# However, this hasn't been strictly formalized yet.
- class V1_0 < ActiveRecord::Migration[6.1] # rubocop:disable Naming/ClassAndModuleCamelCase
+ class V1_0 < ActiveRecord::Migration[6.1]
include LockRetriesConcern
include Gitlab::Database::MigrationHelpers::V2
include Gitlab::Database::MigrationHelpers::AnnounceDatabase
@@ -47,11 +47,11 @@ class MigrationRecord < ActiveRecord::Base
end
end
- class V2_0 < V1_0 # rubocop:disable Naming/ClassAndModuleCamelCase
+ class V2_0 < V1_0
include Gitlab::Database::MigrationHelpers::RestrictGitlabSchema
end
- class V2_1 < V2_0 # rubocop:disable Naming/ClassAndModuleCamelCase
+ class V2_1 < V2_0
include Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables
include Gitlab::Database::Migrations::RunnerBackoff::MigrationHelpers
end
diff --git a/lib/gitlab/database/migrations/milestone_mixin.rb b/lib/gitlab/database/migrations/milestone_mixin.rb
new file mode 100644
index 0000000000000..10bc0c192e744
--- /dev/null
+++ b/lib/gitlab/database/migrations/milestone_mixin.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Database
+ module Migrations
+ module MilestoneMixin
+ extend ActiveSupport::Concern
+ include Gitlab::ClassAttributes
+
+ MilestoneNotSetError = Class.new(StandardError)
+
+ class_methods do
+ def milestone(milestone_str = nil)
+ if milestone_str.present?
+ set_class_attribute(:migration_milestone, milestone_str)
+ else
+ get_class_attribute(:migration_milestone)
+ end
+ end
+ end
+
+ def initialize(name = class_name, version = nil, type = nil)
+ raise MilestoneNotSetError, "Milestone is not set for #{self.class.name}" if milestone.nil?
+
+ super(name, version)
+ @version = Gitlab::Database::Migrations::Version.new(version, milestone, type)
+ end
+
+ def milestone # rubocop:disable Lint/DuplicateMethods
+ @milestone ||= self.class.milestone
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/database/migrations/version.rb b/lib/gitlab/database/migrations/version.rb
new file mode 100644
index 0000000000000..27c4c7d074659
--- /dev/null
+++ b/lib/gitlab/database/migrations/version.rb
@@ -0,0 +1,76 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Database
+ module Migrations
+ class Version
+ InvalidTypeError = Class.new(StandardError)
+
+ include Comparable
+
+ TYPE_VALUES = {
+ regular: 0,
+ post: 1
+ }.freeze
+
+ attr_reader :timestamp, :milestone, :type_value
+
+ def initialize(timestamp, milestone, type)
+ @timestamp = timestamp
+ @milestone = milestone
+ self.type = type
+ end
+
+ def type
+ TYPE_VALUES.key(@type_value)
+ end
+
+ def type=(value)
+ @type_value = TYPE_VALUES.fetch(value.to_sym) { raise InvalidTypeError }
+ end
+
+ def regular?
+ @type_value == TYPE_VALUES[:regular]
+ end
+
+ def post_deployment?
+ @type_value == TYPE_VALUES[:post]
+ end
+
+ def <=>(other)
+ return 1 unless other.is_a?(self.class)
+
+ return milestone <=> other.milestone if milestone != other.milestone
+
+ return @type_value <=> other.type_value if @type_value != other.type_value
+
+ @timestamp <=> other.timestamp
+ end
+
+ def to_s
+ @timestamp.to_s
+ end
+
+ def to_i
+ @timestamp.to_i
+ end
+
+ def coerce(_other)
+ [-1, timestamp.to_i]
+ end
+
+ def eql?(other)
+ (self <=> other) == 0
+ end
+
+ def ==(other)
+ eql?(other)
+ end
+
+ def hash
+ [timestamp, milestone, @type_value].hash
+ end
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/database/migrations/milestone_mixin_spec.rb b/spec/lib/gitlab/database/migrations/milestone_mixin_spec.rb
new file mode 100644
index 0000000000000..e375af494a2ef
--- /dev/null
+++ b/spec/lib/gitlab/database/migrations/milestone_mixin_spec.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::Migrations::MilestoneMixin, feature_category: :database do
+ let(:migration_no_mixin) do
+ Class.new(Gitlab::Database::Migration[2.1]) do
+ def change
+ # no-op here to make rubocop happy
+ end
+ end
+ end
+
+ let(:migration_mixin) do
+ Class.new(Gitlab::Database::Migration[2.1]) do
+ include Gitlab::Database::Migrations::MilestoneMixin
+ end
+ end
+
+ let(:migration_mixin_version) do
+ Class.new(Gitlab::Database::Migration[2.1]) do
+ include Gitlab::Database::Migrations::MilestoneMixin
+ milestone '16.4'
+ end
+ end
+
+ context 'when the mixin is not included' do
+ it 'does not raise an error' do
+ expect { migration_no_mixin.new(4, 4) }.not_to raise_error
+ end
+ end
+
+ context 'when the mixin is included' do
+ context 'when a milestone is not specified' do
+ it "raises MilestoneNotSetError" do
+ expect { migration_mixin.new(4, 4, :regular) }.to raise_error(
+ "#{described_class}::MilestoneNotSetError".constantize
+ )
+ end
+ end
+
+ context 'when a milestone is specified' do
+ it "does not raise an error" do
+ expect { migration_mixin_version.new(4, 4, :regular) }.not_to raise_error
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/database/migrations/version_spec.rb b/spec/lib/gitlab/database/migrations/version_spec.rb
new file mode 100644
index 0000000000000..821a215653921
--- /dev/null
+++ b/spec/lib/gitlab/database/migrations/version_spec.rb
@@ -0,0 +1,120 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::Migrations::Version, feature_category: :database do
+ let(:test_versions) do
+ [
+ 4,
+ 5,
+ described_class.new(6, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ 7,
+ described_class.new(8, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ described_class.new(9, Gitlab::VersionInfo.parse_from_milestone('10.4'), :regular),
+ described_class.new(10, Gitlab::VersionInfo.parse_from_milestone('10.3'), :post),
+ described_class.new(11, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular)
+ ]
+ end
+
+ describe "#<=>" do
+ it 'sorts by existence of milestone, then by milestone, then by type, then by timestamp when sorted by version' do
+ expect(test_versions.sort.map(&:to_i)).to eq [4, 5, 7, 6, 8, 11, 10, 9]
+ end
+ end
+
+ describe 'initialize' do
+ context 'when the type is :post or :regular' do
+ it 'does not raise an error' do
+ expect { described_class.new(4, 4, :regular) }.not_to raise_error
+ expect { described_class.new(4, 4, :post) }.not_to raise_error
+ end
+ end
+
+ context 'when the type is anything else' do
+ it 'does not raise an error' do
+ expect { described_class.new(4, 4, 'foo') }.to raise_error("#{described_class}::InvalidTypeError".constantize)
+ end
+ end
+ end
+
+ describe 'eql?' do
+ where(:version1, :version2, :expected_equality) do
+ [
+ [
+ described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ true
+ ],
+ [
+ described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.4'), :regular),
+ false
+ ],
+ [
+ described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :post),
+ false
+ ],
+ [
+ described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ described_class.new(5, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular),
+ false
+ ]
+ ]
+ end
+
+ with_them do
+ it 'correctly evaluates deep equality' do
+ expect(version1.eql?(version2)).to eq(expected_equality)
+ end
+
+ it 'correctly evaluates deep equality using ==' do
+ expect(version1 == version2).to eq(expected_equality)
+ end
+ end
+ end
+
+ describe 'type' do
+ subject { described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), migration_type) }
+
+ context 'when the migration is regular' do
+ let(:migration_type) { :regular }
+
+ it 'correctly identifies the migration type' do
+ expect(subject.type).to eq(:regular)
+ expect(subject.regular?).to eq(true)
+ expect(subject.post_deployment?).to eq(false)
+ end
+ end
+
+ context 'when the migration is post_deployment' do
+ let(:migration_type) { :post }
+
+ it 'correctly identifies the migration type' do
+ expect(subject.type).to eq(:post)
+ expect(subject.regular?).to eq(false)
+ expect(subject.post_deployment?).to eq(true)
+ end
+ end
+ end
+
+ describe 'to_s' do
+ subject { described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular) }
+
+ it 'returns the given timestamp value as a string' do
+ expect(subject.to_s).to eql('4')
+ end
+ end
+
+ describe 'hash' do
+ subject { described_class.new(4, Gitlab::VersionInfo.parse_from_milestone('10.3'), :regular) }
+
+ let(:expected_hash) { subject.hash }
+
+ it 'deterministically returns a hash of the timestamp, milestone, and type value' do
+ 3.times do
+ expect(subject.hash).to eq(expected_hash)
+ end
+ end
+ end
+end
--
GitLab