Refactor to use objects

For ease of testing we split into:

- connection related functions, and
- file related functions

lib/gitlab/database/postgresql_adapter/dump_schema_versions_mixin.rb

@@ -7,9 +7,7 @@ module Gitlab
         extend ActiveSupport::Concern
 
         def dump_schema_information # :nodoc:
-          versions = schema_migration.all_versions
-          file_versions = migration_context.migrations.map { |m| m.version.to_s }
-          Gitlab::Database::SchemaVersionFiles.touch_all(pool.db_config.name, versions, file_versions) if versions.any?
+          Gitlab::Database::SchemaMigrations.touch_all(self)
 
           nil
         end

lib/gitlab/database/postgresql_database_tasks/load_schema_versions_mixin.rb

@@ -9,7 +9,7 @@ module Gitlab
         def structure_load(...)
           super(...)
 
-          Gitlab::Database::SchemaVersionFiles.load_all(connection.pool.db_config.name)
+          Gitlab::Database::SchemaMigrations.load_all(connection)
         end
       end
     end

lib/gitlab/database/schema_migrations.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaMigrations
+      def self.touch_all(connection)
+        context = Gitlab::Database::SchemaMigrations::Context.new(connection)
+
+        Gitlab::Database::SchemaMigrations::Migrations.new(context).touch_all
+      end
+
+      def self.load_all(connection)
+        context = Gitlab::Database::SchemaMigrations::Context.new(connection)
+
+        Gitlab::Database::SchemaMigrations::Migrations.new(context).load_all
+      end
+    end
+  end
+end

lib/gitlab/database/schema_migrations/context.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaMigrations
+      class Context
+        attr_reader :connection
+
+        def initialize(connection)
+          @connection = connection
+        end
+
+        def schema_directory
+          @schema_directory ||=
+            if ActiveRecord::Base.configurations.primary?(database_name)
+              File.join(db_dir, 'schema_migrations')
+            else
+              File.join(db_dir, "#{database_name}_schema_migrations")
+            end
+        end
+
+        def versions_to_create
+          versions_from_database = @connection.schema_migration.all_versions
+          versions_from_migration_files = @connection.migration_context.migrations.map { |m| m.version.to_s }
+
+          versions_from_database & versions_from_migration_files
+        end
+
+        private
+
+        def database_name
+          @database_name ||= @connection.pool.db_config.name
+        end
+
+        def db_dir
+          @db_dir ||= Rails.application.config.paths["db"].first
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/schema_migrations/migrations.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module SchemaMigrations
+      class Migrations
+        MIGRATION_VERSION_GLOB = '20[0-9][0-9]*'
+
+        def initialize(context)
+          @context = context
+        end
+
+        def touch_all
+          return unless @context.versions_to_create.any?
+
+          version_filepaths = version_filenames.map { |f| File.join(schema_directory, f) }
+          FileUtils.rm(version_filepaths)
+
+          @context.versions_to_create.each do |version|
+            version_filepath = File.join(schema_directory, version)
+
+            File.open(version_filepath, 'w') do |file|
+              file << Digest::SHA256.hexdigest(version)
+            end
+          end
+        end
+
+        def load_all
+          return if version_filenames.empty?
+
+          values = version_filenames.map { |vf| "('#{@context.connection.quote_string(vf)}')" }
+
+          @context.connection.execute(<<~SQL)
+          INSERT INTO schema_migrations (version)
+          VALUES #{values.join(',')}
+          ON CONFLICT DO NOTHING
+          SQL
+        end
+
+        private
+
+        def schema_directory
+          @context.schema_directory
+        end
+
+        def version_filenames
+          @version_filenames ||= Dir.glob(MIGRATION_VERSION_GLOB, base: schema_directory)
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/schema_version_files.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Database
-    class SchemaVersionFiles
-      SCHEMA_DIRECTORY = 'schema_migrations'
-      MIGRATION_VERSION_GLOB = '20[0-9][0-9]*'
-
-      def self.touch_all(database_name, versions_from_database, versions_from_migration_files)
-        schema_directory = schema_directory_for(database_name)
-
-        version_filepaths = find_version_filenames(schema_directory).map { |f| File.join(schema_directory, f) }
-        FileUtils.rm(version_filepaths)
-
-        versions_to_create = versions_from_database & versions_from_migration_files
-        versions_to_create.each do |version|
-          version_filepath = File.join(schema_directory, version)
-
-          File.open(version_filepath, 'w') do |file|
-            file << Digest::SHA256.hexdigest(version)
-          end
-        end
-      end
-
-      def self.load_all(database_name)
-        schema_directory = schema_directory_for(database_name)
-        version_filenames = find_version_filenames(schema_directory)
-        return if version_filenames.empty?
-
-        values = version_filenames.map { |vf| "('#{connection.quote_string(vf)}')" }
-        connection.execute(<<~SQL)
-          INSERT INTO schema_migrations (version)
-          VALUES #{values.join(',')}
-          ON CONFLICT DO NOTHING
-        SQL
-      end
-
-      def self.db_dir
-        @db_dir ||= Rails.application.config.paths["db"].first
-      end
-
-      def self.schema_directory_for(database_name)
-        if ActiveRecord::Base.configurations.primary?(database_name)
-          File.join(db_dir, SCHEMA_DIRECTORY)
-        else
-          File.join(db_dir, "#{database_name}_#{SCHEMA_DIRECTORY}")
-        end
-      end
-
-      def self.find_version_filenames(schema_directory)
-        Dir.glob(MIGRATION_VERSION_GLOB, base: schema_directory)
-      end
-
-      def self.connection
-        ActiveRecord::Base.connection
-      end
-    end
-  end
-end

spec/lib/gitlab/database/postgresql_adapter/dump_schema_versions_mixin_spec.rb

@@ -3,16 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::PostgresqlAdapter::DumpSchemaVersionsMixin do
-  let(:schema_migration) { double('schema_migration', all_versions: versions) }
-  let(:migration_context) { double('migration_context', migrations: versions.map {|v| migration_struct.new(v) }) }
-
-  let(:db_name) { 'primary' }
-  let(:versions) { %w(5 2 1000 200 4 93 2) }
-
-  let(:migration_struct) do
-    Struct.new(:version)
-  end
-
   let(:instance_class) do
     klass = Class.new do
       def dump_schema_information
@@ -30,44 +20,10 @@ RSpec.describe Gitlab::Database::PostgresqlAdapter::DumpSchemaVersionsMixin do
 
   let(:instance) { instance_class.new }
 
-  before do
-    allow(instance).to receive(:schema_migration).and_return(schema_migration)
-    allow(instance).to receive(:migration_context).and_return(migration_context)
-
-    # pool is from ActiveRecord::ConnectionAdapters::PostgreSQLAdapter
-    allow(instance).to receive_message_chain(:pool, :db_config, :name).and_return(db_name)
-  end
-
-  context 'when database name is primary' do
-    context 'when version files exist' do
-      it 'touches version files' do
-        expect(Gitlab::Database::SchemaVersionFiles).to receive(:touch_all).with(db_name, versions, versions)
-        expect(instance).not_to receive(:original_dump_schema_information)
-
-        instance.dump_schema_information
-      end
-    end
+  it 'calls SchemaMigrations touch_all and skips original implementation' do
+    expect(Gitlab::Database::SchemaMigrations).to receive(:touch_all).with(instance)
+    expect(instance).not_to receive(:original_dump_schema_information)
 
-    context 'when version files do not exist' do
-      let(:versions) { [] }
-
-      it 'does not touch version files' do
-        expect(Gitlab::Database::SchemaVersionFiles).not_to receive(:touch_all)
-        expect(instance).not_to receive(:original_dump_schema_information)
-
-        instance.dump_schema_information
-      end
-    end
-  end
-
-  context 'when database name is ci' do
-    let(:db_name) { 'ci' }
-
-    it 'touches version files' do
-      expect(Gitlab::Database::SchemaVersionFiles).to receive(:touch_all).with(db_name, versions, versions)
-      expect(instance).not_to receive(:original_dump_schema_information)
-
-      instance.dump_schema_information
-    end
+    instance.dump_schema_information
   end
 end

spec/lib/gitlab/database/postgresql_database_tasks/load_schema_versions_mixin_spec.rb

@@ -3,8 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::PostgresqlDatabaseTasks::LoadSchemaVersionsMixin do
-  let(:db_name) { 'primary' }
-
   let(:instance_class) do
     klass = Class.new do
       def structure_load
@@ -22,28 +20,13 @@ RSpec.describe Gitlab::Database::PostgresqlDatabaseTasks::LoadSchemaVersionsMixi
 
   let(:instance) { instance_class.new }
 
-  before do
-    # connection is available in ActiveRecord::Tasks::PostgreSQLDatabaseTasks
-    allow(instance).to receive_message_chain(:connection, :pool, :db_config, :name).and_return(db_name)
-  end
-
-  context 'when database is primary' do
-    it 'loads version files for primary database' do
-      expect(Gitlab::Database::SchemaVersionFiles).to receive(:load_all).with(db_name)
-      expect(instance).to receive(:original_structure_load)
-
-      instance.structure_load
-    end
-  end
+  it 'calls SchemaMigrations load_all' do
+    connection = double('connection')
+    allow(instance).to receive(:connection).and_return(connection)
 
-  context 'when the database is ci' do
-    let(:db_name) { 'ci' }
+    expect(instance).to receive(:original_structure_load).ordered
+    expect(Gitlab::Database::SchemaMigrations).to receive(:load_all).with(connection).ordered
 
-    it 'loads version files for ci database' do
-      expect(Gitlab::Database::SchemaVersionFiles).to receive(:load_all).with(db_name)
-      expect(instance).to receive(:original_structure_load)
-
-      instance.structure_load
-    end
+    instance.structure_load
   end
 end

spec/lib/gitlab/database/schema_migrations/context_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::SchemaMigrations::Context do
+  let(:connection) { ActiveRecord::Base.connection }
+
+  let(:context) { described_class.new(connection) }
+
+  describe '#schema_directory' do
+    it 'returns db/schema_migrations' do
+      expect(context.schema_directory).to eq(File.join(Rails.root, 'db/schema_migrations'))
+    end
+
+    context 'multiple databases' do
+      let(:connection) { Ci::BaseModel.connection }
+
+      it 'returns a directory path that is database specific' do
+        skip_if_multiple_databases_not_setup
+
+        expect(context.schema_directory).to eq(File.join(Rails.root, 'db/ci_schema_migrations'))
+      end
+    end
+  end
+
+  describe '#versions_to_create' do
+    before do
+      allow(connection).to receive_message_chain(:schema_migration, :all_versions).and_return(migrated_versions)
+
+      migrations_struct = Struct.new(:version)
+      migrations = file_versions.map { |version| migrations_struct.new(version) }
+      allow(connection).to receive_message_chain(:migration_context, :migrations).and_return(migrations)
+    end
+
+    let(:version1) { '20200123' }
+    let(:version2) { '20200410' }
+    let(:version3) { '20200602' }
+    let(:version4) { '20200809' }
+
+    let(:migrated_versions) { file_versions }
+    let(:file_versions) { [version1, version2, version3, version4] }
+
+    context 'migrated versions is the same as migration file versions' do
+      it 'returns migrated versions' do
+        expect(context.versions_to_create).to eq(migrated_versions)
+      end
+    end
+
+    context 'migrated versions is subset of migration file versions' do
+      let(:migrated_versions) { [version1, version2] }
+
+      it 'returns migrated versions' do
+        expect(context.versions_to_create).to eq(migrated_versions)
+      end
+    end
+
+    context 'migrated versions is superset of migration file versions' do
+      let(:migrated_versions) { file_versions + ['20210809'] }
+
+      it 'returns file versions' do
+        expect(context.versions_to_create).to eq(file_versions)
+      end
+    end
+
+    context 'migrated versions has slightly different versions to migration file versions' do
+      let(:migrated_versions) { [version1, version2, version3, version4, '20210101'] }
+      let(:file_versions) { [version1, version2, version3, version4, '20210102'] }
+
+      it 'returns the common set' do
+        expect(context.versions_to_create).to eq([version1, version2, version3, version4])
+      end
+    end
+  end
+
+  def skip_if_multiple_databases_not_setup
+    skip 'Skipping because multiple databases not set up' unless Gitlab::Database.has_config?(:ci)
+  end
+end

spec/lib/gitlab/database/schema_version_files_spec.rb → spec/lib/gitlab/database/schema_migrations/migrations_spec.rb

@@ -2,44 +2,37 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::Database::SchemaVersionFiles do
-  describe '.touch_all' do
+RSpec.describe Gitlab::Database::SchemaMigrations::Migrations do
+  let(:connection) { ApplicationRecord.connection }
+  let(:context) { Gitlab::Database::SchemaMigrations::Context.new(connection) }
+
+  let(:migrations) { described_class.new(context) }
+
+  describe '#touch_all' do
     let(:version1) { '20200123' }
     let(:version2) { '20200410' }
     let(:version3) { '20200602' }
     let(:version4) { '20200809' }
 
-    let(:database_name) { 'primary' }
     let(:relative_schema_directory) { 'db/schema_migrations' }
-    let(:relative_migrate_directory) { 'db/migrate' }
-    let(:relative_post_migrate_directory) { 'db/post_migrate' }
 
     it 'creates a file containing a checksum for each version with a matching migration' do
       Dir.mktmpdir do |tmpdir|
         schema_directory = Pathname.new(tmpdir).join(relative_schema_directory)
-        migrate_directory = Pathname.new(tmpdir).join(relative_migrate_directory)
-        post_migrate_directory = Pathname.new(tmpdir).join(relative_post_migrate_directory)
-
-        FileUtils.mkdir_p(migrate_directory)
-        FileUtils.mkdir_p(post_migrate_directory)
         FileUtils.mkdir_p(schema_directory)
 
-        migration1_filepath = migrate_directory.join("#{version1}_migration.rb")
-        FileUtils.touch(migration1_filepath)
-
-        migration2_filepath = post_migrate_directory.join("#{version2}_post_migration.rb")
-        FileUtils.touch(migration2_filepath)
-
         old_version_filepath = schema_directory.join('20200101')
         FileUtils.touch(old_version_filepath)
 
         expect(File.exist?(old_version_filepath)).to be(true)
 
-        allow(described_class).to receive(:schema_directory_for).with(database_name).and_return(schema_directory)
+        allow(context).to receive(:schema_directory).and_return(schema_directory)
+        allow(context).to receive(:versions_to_create).and_return([version1, version2])
 
-        described_class.touch_all(database_name, [version1, version2, version3, version4], [version1, version2])
+        migrations.touch_all
 
         expect(File.exist?(old_version_filepath)).to be(false)
+
         [version1, version2].each do |version|
           version_filepath = schema_directory.join(version)
           expect(File.exist?(version_filepath)).to be(true)
@@ -56,13 +49,9 @@ RSpec.describe Gitlab::Database::SchemaVersionFiles do
     end
   end
 
-  describe '.load_all' do
-    let(:database_name) { 'primary' }
-    let(:connection) { double('connection') }
-
+  describe '#load_all' do
     before do
-      allow(described_class).to receive(:connection).and_return(connection)
-      allow(described_class).to receive(:find_version_filenames).and_return(filenames)
+      allow(migrations).to receive(:version_filenames).and_return(filenames)
     end
 
     context 'when there are no version files' do
@@ -72,7 +61,7 @@ RSpec.describe Gitlab::Database::SchemaVersionFiles do
         expect(connection).not_to receive(:quote_string)
         expect(connection).not_to receive(:execute)
 
-        described_class.load_all(database_name)
+        migrations.load_all
       end
     end
 
@@ -90,7 +79,7 @@ RSpec.describe Gitlab::Database::SchemaVersionFiles do
           ON CONFLICT DO NOTHING
         SQL
 
-        described_class.load_all(database_name)
+        migrations.load_all
       end
     end
   end