diff --git a/.rubocop.yml b/.rubocop.yml index f613a43b2247c173dde294c08e519e70d86e1ee7..0ebb17ce59358d23ce1c06dff5bc89eb8e0df1b9 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 00a9b4ddc6ed421dad8eef28cd9ded5702ef2e12..21478c462590814b8ded7a53151e5b9368e4b2ed 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 2b5f6bcb4c13253bfd1d138fc20dd758ba365d7c..ca13a06b92cf3a58fdab58d8eec05267b9102a96 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 fbb71c1ccfd0f65084b7d693abcfd7a897fe6439..41044816de908d566f5161364af95c309ff3f58d 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 0000000000000000000000000000000000000000..10bc0c192e74487f8a0d2dada94f4e4dcfb7a92c --- /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 0000000000000000000000000000000000000000..27c4c7d0746599d2b763748e8d517477972d2837 --- /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 0000000000000000000000000000000000000000..e375af494a2efcfc4305cbee6f190790b79e2532 --- /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 0000000000000000000000000000000000000000..821a215653921bb291b083998c5012b231b3f2a3 --- /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