diff --git a/lefthook.yml b/lefthook.yml index f6a2b4b070f11cd982280e2128bc392df403a364..27a237a8fb39a3d9bcebc7763b2bf053462aa236 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -83,6 +83,11 @@ pre-push: files: git diff --name-only --diff-filter=d $(git merge-base origin/master HEAD)..HEAD glob: 'db/structure.sql' run: scripts/validate_schema_changes + db-migration-name-collisions: + tags: database + files: git diff --name-only --diff-filter=d $(git merge-base origin/master HEAD)..HEAD + glob: '{db/migrate/*.rb,db/post_migrate/*.rb,ee/elastic/migrate/*.rb}' + run: scripts/validate_name_collisions_between_migrations static-verification: skip: true # This is disabled by default. You can enable this check by adding skip: false in lefthook-local.yml https://github.com/evilmartians/lefthook/blob/master/docs/configuration.md#skip tags: backend diff --git a/scripts/database/migration_collision_checker.rb b/scripts/database/migration_collision_checker.rb new file mode 100644 index 0000000000000000000000000000000000000000..0c9daee9f4c7dab8f8dfd545bf00a9480867c701 --- /dev/null +++ b/scripts/database/migration_collision_checker.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'pathname' +require 'open3' + +# Checks for class name collisions between Database migrations and Elasticsearch migrations +class MigrationCollisionChecker + MIGRATION_FOLDERS = %w[db/migrate/*.rb db/post_migrate/*.rb ee/elastic/migrate/*.rb].freeze + + CLASS_MATCHER = /^\s*class\s+:*([A-Z][A-Za-z0-9_]+\S+)/ + + ERROR_CODE = 1 + + # To be removed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129012 + SKIP_MIGRATIONS = %w[AddInternalToNotes BackfillInternalOnNotes].freeze + + Result = Struct.new(:error_code, :error_message) + + def initialize + @collisions = Hash.new { |h, k| h[k] = [] } + end + + def check + check_for_collisions + + return if collisions.empty? + + Result.new(ERROR_CODE, "\e[31mError: Naming collisions were found between migrations\n\n#{message}\e[0m") + end + + private + + attr_reader :collisions + + def check_for_collisions + MIGRATION_FOLDERS.each do |migration_folder| + Dir.glob(base_path.join(migration_folder)).each do |migration_path| + klass_name = CLASS_MATCHER.match(File.read(migration_path))[1] + + next if SKIP_MIGRATIONS.include?(klass_name) + + collisions[klass_name] << migration_path + end + end + + collisions.select! { |_, v| v.size > 1 } + end + + def message + collisions.map { |klass_name, paths| "#{klass_name}: #{paths.join(', ')}\n" }.join('') + end + + def base_path + Pathname.new(File.expand_path('../../', __dir__)) + end +end diff --git a/scripts/validate_name_collisions_between_migrations b/scripts/validate_name_collisions_between_migrations new file mode 100755 index 0000000000000000000000000000000000000000..b6bc46cf480854bd59727c9228edcf22e55cd6fe --- /dev/null +++ b/scripts/validate_name_collisions_between_migrations @@ -0,0 +1,12 @@ +#!/usr/bin/env ruby + +# frozen_string_literal: true + +require_relative './database/migration_collision_checker' + +result = MigrationCollisionChecker.new.check + +if result + puts result.error_message + exit result.error_code +end diff --git a/spec/fixtures/migrations/db/migrate/database_migration.txt b/spec/fixtures/migrations/db/migrate/database_migration.txt new file mode 100644 index 0000000000000000000000000000000000000000..594d06f843151822ddccbc63b96ba3cdb5779b96 --- /dev/null +++ b/spec/fixtures/migrations/db/migrate/database_migration.txt @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class DatabaseMigration < Gitlab::Database::Migration[2.0] + def up + add_column :dast_site_profiles, :scan_file_path, :text + end + + def down + remove_column :dast_site_profiles, :scan_file_path, :text + end +end diff --git a/spec/fixtures/migrations/db/migrate/database_migration_two.txt b/spec/fixtures/migrations/db/migrate/database_migration_two.txt new file mode 100644 index 0000000000000000000000000000000000000000..236529b4b2df45223bddb6afdf851d5500f9d655 --- /dev/null +++ b/spec/fixtures/migrations/db/migrate/database_migration_two.txt @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class Gitlab::Database::DatabaseMigration < Gitlab::Database::Migration[2.0] + def up + add_column :dast_site_profiles, :scan_file_path, :text + end + + def down + remove_column :dast_site_profiles, :scan_file_path, :text + end +end diff --git a/spec/fixtures/migrations/db/post_migrate/database_clash_migration.txt b/spec/fixtures/migrations/db/post_migrate/database_clash_migration.txt new file mode 100644 index 0000000000000000000000000000000000000000..a936dafd69f5e6b2b7879dcfe71e6ab2d9ee8b31 --- /dev/null +++ b/spec/fixtures/migrations/db/post_migrate/database_clash_migration.txt @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class ::ClashMigration < Gitlab::Database::Migration[2.0] + def up + add_column :dast_site_profiles, :scan_file_path, :text + end + + def down + remove_column :dast_site_profiles, :scan_file_path, :text + end +end diff --git a/spec/fixtures/migrations/db/post_migrate/database_clash_migration_two.txt b/spec/fixtures/migrations/db/post_migrate/database_clash_migration_two.txt new file mode 100644 index 0000000000000000000000000000000000000000..6af29a498bd7e22bab7811a2730a23b5d62466b3 --- /dev/null +++ b/spec/fixtures/migrations/db/post_migrate/database_clash_migration_two.txt @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class Gitlab::ClashMigrationTwo < Gitlab::Database::Migration[2.0] + def up + add_column :dast_site_profiles, :scan_file_path, :text + end + + def down + remove_column :dast_site_profiles, :scan_file_path, :text + end +end diff --git a/spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration.txt b/spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration.txt new file mode 100644 index 0000000000000000000000000000000000000000..e723d9f44e7e3eee850e510c667b0297b9a58c81 --- /dev/null +++ b/spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration.txt @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class ClashMigration < Elastic::Migration + include Elastic::MigrationCreateIndex + + retry_on_failure + + def document_type + :epic + end + + def target_class + Epic + end +end diff --git a/spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration_two.txt b/spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration_two.txt new file mode 100644 index 0000000000000000000000000000000000000000..6af29a498bd7e22bab7811a2730a23b5d62466b3 --- /dev/null +++ b/spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration_two.txt @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class Gitlab::ClashMigrationTwo < Gitlab::Database::Migration[2.0] + def up + add_column :dast_site_profiles, :scan_file_path, :text + end + + def down + remove_column :dast_site_profiles, :scan_file_path, :text + end +end diff --git a/spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt b/spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt new file mode 100644 index 0000000000000000000000000000000000000000..122d3ef6d2b36a72c5b7110b256e652ec88deb03 --- /dev/null +++ b/spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class ElasticsearchMigration < Elastic::Migration + include Elastic::MigrationUpdateMappingsHelper + + private + + def index_name + Project.__elasticsearch__.index_name + end + + def new_mappings + { + readme_content: { + type: 'text' + }, + ci_catalog: { + type: 'boolean' + } + } + end +end diff --git a/spec/scripts/database/migration_collision_checker_spec.rb b/spec/scripts/database/migration_collision_checker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..a3afbae50b6e6499ae6d2a26ee8d998d63f8297f --- /dev/null +++ b/spec/scripts/database/migration_collision_checker_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +require_relative '../../../scripts/database/migration_collision_checker' + +RSpec.describe MigrationCollisionChecker, feature_category: :database do + subject(:checker) { described_class.new } + + before do + stub_const('MigrationCollisionChecker::MIGRATION_FOLDERS', [db_migration_path, elasticsearch_migration_path]) + end + + describe "#check" do + context "when there's no collision between migrations" do + let(:db_migration_path) { 'spec/fixtures/migrations/db/migrate/*.txt' } + let(:elasticsearch_migration_path) { 'spec/fixtures/migrations/elasticsearch/*.txt' } + + it { expect(checker.check).to be_nil } + end + + context 'when migration class name clashes' do + let(:db_migration_path) { 'spec/fixtures/migrations/db/*/*.txt' } + let(:elasticsearch_migration_path) { 'spec/fixtures/migrations/elasticsearch/*.txt' } + + it 'returns the error code' do + expect(checker.check.error_code).to eq(1) + end + + it 'returns the error message' do + expect(checker.check.error_message).to include( + 'Naming collisions were found between migrations', 'ClashMigration', 'Gitlab::ClashMigrationTwo' + ) + end + end + + context 'when migration class name clashes but they are marked to be skipped' do + let(:db_migration_path) { 'spec/fixtures/migrations/db/*/*.txt' } + let(:elasticsearch_migration_path) { 'spec/fixtures/migrations/elasticsearch/*.txt' } + + before do + stub_const('MigrationCollisionChecker::SKIP_MIGRATIONS', %w[ClashMigration Gitlab::ClashMigrationTwo]) + end + + it { expect(checker.check).to be_nil } + end + end +end