From 00e4921fd2b20c40a6adc8fc922837fb2c93c545 Mon Sep 17 00:00:00 2001
From: Leonardo da Rosa <ldarosa@gitlab.com>
Date: Wed, 16 Aug 2023 11:53:25 +0000
Subject: [PATCH] Avoid name collision between migrations
Adds a `pre-push` hook to check name clashing between `db` migrations and `Elasticsearch` migrations.
---
lefthook.yml | 5 ++
.../database/migration_collision_checker.rb | 56 +++++++++++++++++++
...alidate_name_collisions_between_migrations | 12 ++++
.../db/migrate/database_migration.txt | 11 ++++
.../db/migrate/database_migration_two.txt | 11 ++++
.../post_migrate/database_clash_migration.txt | 11 ++++
.../database_clash_migration_two.txt | 11 ++++
.../elasticsearch_clash_migration.txt | 15 +++++
.../elasticsearch_clash_migration_two.txt | 11 ++++
.../elasticsearch/elasticsearch_migration.txt | 22 ++++++++
.../migration_collision_checker_spec.rb | 48 ++++++++++++++++
11 files changed, 213 insertions(+)
create mode 100644 scripts/database/migration_collision_checker.rb
create mode 100755 scripts/validate_name_collisions_between_migrations
create mode 100644 spec/fixtures/migrations/db/migrate/database_migration.txt
create mode 100644 spec/fixtures/migrations/db/migrate/database_migration_two.txt
create mode 100644 spec/fixtures/migrations/db/post_migrate/database_clash_migration.txt
create mode 100644 spec/fixtures/migrations/db/post_migrate/database_clash_migration_two.txt
create mode 100644 spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration.txt
create mode 100644 spec/fixtures/migrations/elasticsearch/elasticsearch_clash_migration_two.txt
create mode 100644 spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt
create mode 100644 spec/scripts/database/migration_collision_checker_spec.rb
diff --git a/lefthook.yml b/lefthook.yml
index f6a2b4b070f1..27a237a8fb39 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 000000000000..0c9daee9f4c7
--- /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 000000000000..b6bc46cf4808
--- /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 000000000000..594d06f84315
--- /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 000000000000..236529b4b2df
--- /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 000000000000..a936dafd69f5
--- /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 000000000000..6af29a498bd7
--- /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 000000000000..e723d9f44e7e
--- /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 000000000000..6af29a498bd7
--- /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 000000000000..122d3ef6d2b3
--- /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 000000000000..a3afbae50b6e
--- /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
--
GitLab