From f08e8c4bb703c7f0e84da51ccd70bbd9c203e80d Mon Sep 17 00:00:00 2001
From: Igor Drozdov <idrozdov@gitlab.com>
Date: Mon, 12 Aug 2024 19:46:32 +0000
Subject: [PATCH] Add ActiveRecord::Enum patch
It removes newly introduced check that doesn't allow defining
an enum without backed column.
A migration job fails because it:
- Performs database migration on a particular version
- The version does not contain email_confirmation_setting column
- Performs another migration that initializes Rails env
- Initializers call Gitlab::CurrentSetting
- ApplicationSetting tries to define email_confirmation_setting enum
- Fails because the enum is not backed by a column
---
config/initializers/00_active_record_enum.rb | 31 +++++++++++
.../database/migrations/pg_backend_pid.rb | 19 ++++++-
.../postgresql_adapter/type_map_cache.rb | 8 ++-
.../00_active_record_enum_spec.rb | 22 ++++++++
.../migrations/pg_backend_pid_spec.rb | 53 +++++++++++++++++--
.../postgresql_adapter/type_map_cache_spec.rb | 2 +
6 files changed, 128 insertions(+), 7 deletions(-)
create mode 100644 config/initializers/00_active_record_enum.rb
create mode 100644 spec/initializers/00_active_record_enum_spec.rb
diff --git a/config/initializers/00_active_record_enum.rb b/config/initializers/00_active_record_enum.rb
new file mode 100644
index 0000000000000..d25651154e503
--- /dev/null
+++ b/config/initializers/00_active_record_enum.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+# Override ActiveRecord::Enum.load_schema! to remove the check that verifies that
+# defined enum attributes are backed by a database column.
+#
+# The check has been introduced in Rails 7.1: https://github.com/rails/rails/pull/45734
+#
+# However, there are valid use cases when the enum definition should not raise an error
+# when the column does not exist in the column list:
+#
+# - Perform database migration on a particular version, the one that doesn't contain the column
+# - Try performing another operation that loads Rails environment (for example, another migration):
+# - Rails environment is being initialized and configured
+# - Gitlab::CurrentSetting is being called to customize the configuration
+# - ApplicationSetting model tries to define a enum attribute that is not backed by a column on that patricular
+# migration version
+# - An error is raised
+
+module ActiveRecordAttributesPatch
+ def attribute(name, *args, **options)
+ return super unless defined_enums.key?(name)
+ return unless block_given?
+
+ super do |subtype|
+ subtype = subtype.subtype if ActiveRecord::Enum::EnumType === subtype
+ ActiveRecord::Enum::EnumType.new(name, defined_enums[name], subtype)
+ end
+ end
+end
+
+ActiveRecord::Attributes::ClassMethods.prepend(ActiveRecordAttributesPatch)
diff --git a/lib/gitlab/database/migrations/pg_backend_pid.rb b/lib/gitlab/database/migrations/pg_backend_pid.rb
index 52f309e405821..c4dea00cc278f 100644
--- a/lib/gitlab/database/migrations/pg_backend_pid.rb
+++ b/lib/gitlab/database/migrations/pg_backend_pid.rb
@@ -7,6 +7,19 @@ module PgBackendPid
module MigratorPgBackendPid
extend ::Gitlab::Utils::Override
+ override :with_advisory_lock
+ def with_advisory_lock
+ Gitlab::Database::Migrations::PgBackendPid.say(connection)
+
+ super
+ ensure
+ Gitlab::Database::Migrations::PgBackendPid.say(connection)
+ end
+ end
+
+ module OldMigratorPgBackendPid
+ extend ::Gitlab::Utils::Override
+
override :with_advisory_lock_connection
def with_advisory_lock_connection
super do |conn|
@@ -20,7 +33,11 @@ def with_advisory_lock_connection
end
def self.patch!
- ActiveRecord::Migrator.prepend(MigratorPgBackendPid)
+ if ::Gitlab.next_rails?
+ ActiveRecord::Migrator.prepend(MigratorPgBackendPid)
+ else
+ ActiveRecord::Migrator.prepend(OldMigratorPgBackendPid)
+ end
end
def self.say(conn)
diff --git a/lib/gitlab/database/postgresql_adapter/type_map_cache.rb b/lib/gitlab/database/postgresql_adapter/type_map_cache.rb
index ff66d9115ab1f..0e6cbded10ab8 100644
--- a/lib/gitlab/database/postgresql_adapter/type_map_cache.rb
+++ b/lib/gitlab/database/postgresql_adapter/type_map_cache.rb
@@ -1,7 +1,7 @@
# frozen_string_literal: true
# Caches loading of additional types from the DB
-# https://github.com/rails/rails/blob/v6.0.3.2/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L521-L589
+# https://github.com/rails/rails/blob/v7.1.3.4/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L997
# rubocop:disable Gitlab/ModuleWithInstanceVariables
@@ -32,6 +32,12 @@ def initialize_type_map(map = type_map)
end
def reload_type_map
+ if ::Gitlab.next_rails? && @type_map.blank?
+ @type_map = ActiveRecord::Type::HashLookupTypeMap.new
+
+ return initialize_type_map
+ end
+
TYPE_MAP_CACHE_MONITOR.synchronize do
self.class.type_map_cache[@connection_parameters.hash] = nil
end
diff --git a/spec/initializers/00_active_record_enum_spec.rb b/spec/initializers/00_active_record_enum_spec.rb
new file mode 100644
index 0000000000000..f413d14e642e3
--- /dev/null
+++ b/spec/initializers/00_active_record_enum_spec.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# Adds a missing test to provide full coverage for the patch
+RSpec.describe 'ActiveRecord::Enum Patch', feature_category: :database do
+ context 'when enum is not backed by a database column' do
+ let(:klass) do
+ Class.new(ActiveRecord::Base) do
+ self.table_name = :projects
+
+ enum values_without_backed_column: [:one, :two]
+ end
+ end
+
+ it 'does not raise an error' do
+ expect do
+ klass.new.values_without_backed_column
+ end.not_to raise_error
+ end
+ end
+end
diff --git a/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb b/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb
index a9ef28a4b5189..6c2ecb0b1f097 100644
--- a/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb
+++ b/spec/lib/gitlab/database/migrations/pg_backend_pid_spec.rb
@@ -4,6 +4,47 @@
RSpec.describe Gitlab::Database::Migrations::PgBackendPid, feature_category: :database do
describe Gitlab::Database::Migrations::PgBackendPid::MigratorPgBackendPid do
+ let(:block) { -> {} }
+
+ let(:klass) do
+ Class.new do
+ def initialize(block)
+ @block = block
+ end
+
+ def with_advisory_lock
+ @block.call
+ end
+
+ def connection
+ ApplicationRecord.connection
+ end
+ end
+ end
+
+ let(:patched_instance) { klass.prepend(described_class).new(block) }
+
+ it 'wraps the method execution with calls to .say' do
+ expect(Gitlab::Database::Migrations::PgBackendPid).to receive(:say).twice
+ expect(block).to receive(:call)
+
+ patched_instance.with_advisory_lock
+ end
+
+ context 'when an error is raised' do
+ let(:block) { -> { raise ActiveRecord::ConcurrentMigrationError, 'test' } }
+
+ it 'wraps the method execution with calls to .say' do
+ expect(Gitlab::Database::Migrations::PgBackendPid).to receive(:say).twice
+
+ expect do
+ patched_instance.with_advisory_lock
+ end.to raise_error ActiveRecord::ConcurrentMigrationError
+ end
+ end
+ end
+
+ describe Gitlab::Database::Migrations::PgBackendPid::OldMigratorPgBackendPid do
let(:klass) do
Class.new do
def with_advisory_lock_connection
@@ -33,16 +74,20 @@ def with_advisory_lock_connection
describe '.patch!' do
it 'patches ActiveRecord::Migrator' do
- expect(ActiveRecord::Migrator).to receive(:prepend).with(described_class::MigratorPgBackendPid)
+ if ::Gitlab.next_rails?
+ expect(ActiveRecord::Migrator).to receive(:prepend).with(described_class::MigratorPgBackendPid)
+ else
+ expect(ActiveRecord::Migrator).to receive(:prepend).with(described_class::OldMigratorPgBackendPid)
+ end
described_class.patch!
end
end
describe '.say' do
- it 'outputs the connection information' do
- conn = ActiveRecord::Base.connection
+ let(:conn) { ActiveRecord::Base.connection }
+ it 'outputs the connection information' do
expect(conn).to receive(:object_id).and_return(9876)
expect(conn).to receive(:select_value).with('SELECT pg_backend_pid()').and_return(12345)
expect(Gitlab::Database).to receive(:db_config_name).with(conn).and_return('main')
@@ -53,8 +98,6 @@ def with_advisory_lock_connection
end
it 'outputs nothing if ActiveRecord::Migration.verbose is false' do
- conn = ActiveRecord::Base.connection
-
allow(ActiveRecord::Migration).to receive(:verbose).and_return(false)
expect { described_class.say(conn) }.not_to output.to_stdout
diff --git a/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb b/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb
index 75c3a3650d71d..7e355c84e385c 100644
--- a/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb
+++ b/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb
@@ -50,6 +50,8 @@
# Based on https://github.com/rails/rails/blob/v6.1.3.2/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L36-L41
def initialize_connection(config = db_config)
+ return adapter_class.new(config).connect! if ::Gitlab.next_rails?
+
conn_params = config.symbolize_keys.compact
conn_params[:user] = conn_params.delete(:username) if conn_params[:username]
--
GitLab