Skip to content

Commit

Permalink
Don't allow mutations on configuration_hash
Browse files Browse the repository at this point in the history
We want to introduce an object-based DSL for building and modifying
configuration objects. As part of that we want to make sure that users
don't think they can modify configuration_hash values and have them
change the configuration. For that reason we're going to freeze the
Hash here, and have modified places in tests where we were modifying
these hashes.

The commit here also adds a test for the Test Databases and in that work
we found that we were calling `Rails.env` and Active Record doesn't load
Rails.

Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
  • Loading branch information
eileencodes and seejohnrun committed Dec 19, 2019
1 parent fe3db2a commit 154abca
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def config
end

def configuration_hash
@config
@config.freeze
end

# Determines whether a database configuration is for a replica / readonly
Expand Down
15 changes: 11 additions & 4 deletions activerecord/lib/active_record/test_databases.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,25 @@
module ActiveRecord
module TestDatabases # :nodoc:
ActiveSupport::Testing::Parallelization.after_fork_hook do |i|
create_and_load_schema(i, env_name: Rails.env)
create_and_load_schema(i, env_name: ActiveRecord::ConnectionHandling::DEFAULT_ENV.call.to_sym)
end

def self.create_and_load_schema(i, env_name:)
old, ENV["VERBOSE"] = ENV["VERBOSE"], "false"

ActiveRecord::Base.configurations.configs_for(env_name: env_name).each do |db_config|
db_config.configuration_hash[:database] += "-#{i}"
ActiveRecord::Tasks::DatabaseTasks.reconstruct_from_schema(db_config, ActiveRecord::Base.schema_format, nil)
database = "#{db_config.database}-#{i}"

db_config_copy = ActiveRecord::DatabaseConfigurations::HashConfig.new(
env_name,
db_config.spec_name,
db_config.configuration_hash.merge(database: database)
)

ActiveRecord::Tasks::DatabaseTasks.reconstruct_from_schema(db_config_copy, ActiveRecord::Base.schema_format, nil)
end
ensure
ActiveRecord::Base.establish_connection(Rails.env.to_sym)
ActiveRecord::Base.establish_connection(ActiveRecord::ConnectionHandling::DEFAULT_ENV.call.to_sym)
ENV["VERBOSE"] = old
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,8 @@ def test_pool_from_any_process_for_uses_most_recent_spec
wr.binmode

pid = fork do
ActiveRecord::Base.configurations["arunit"]["database"] = file.path
ActiveRecord::Base.establish_connection(:arunit)
config_hash = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", spec_name: "primary").configuration_hash.merge(database: file.path)
ActiveRecord::Base.establish_connection(config_hash)

pid2 = fork do
wr.write ActiveRecord::Base.connection_db_config.database
Expand Down
21 changes: 12 additions & 9 deletions activerecord/test/cases/connection_pool_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,11 @@ def test_reap_inactive

def test_idle_timeout_configuration
@pool.disconnect!
@db_config.configuration_hash.merge!(idle_timeout: "0.02")
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", @db_config)

config = @db_config.configuration_hash.merge(idle_timeout: "0.02")
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new(@db_config.env_name, @db_config.spec_name, config)

pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", db_config)
@pool = ConnectionPool.new(pool_config)
idle_conn = @pool.checkout
@pool.checkin(idle_conn)
Expand All @@ -225,8 +228,10 @@ def test_idle_timeout_configuration

def test_disable_flush
@pool.disconnect!
@db_config.configuration_hash.merge!(idle_timeout: -5)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", @db_config)

config = @db_config.configuration_hash.merge(idle_timeout: -5)
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new(@db_config.env_name, @db_config.spec_name, config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", db_config)
@pool = ConnectionPool.new(pool_config)
idle_conn = @pool.checkout
@pool.checkin(idle_conn)
Expand Down Expand Up @@ -720,12 +725,10 @@ def test_public_connections_access_threadsafe

private
def with_single_connection_pool
old_config = @db_config.configuration_hash
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("arunit", "primary", old_config.dup)

db_config.configuration_hash[:pool] = 1 # this is safe to do, because .dupped PoolConfig also auto-dups its config

config = @db_config.configuration_hash.merge(pool: 1)
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("arunit", "primary", config)
pool_config = ActiveRecord::ConnectionAdapters::PoolConfig.new("primary", db_config)

yield(pool = ConnectionPool.new(pool_config))
ensure
pool.disconnect! if pool
Expand Down
19 changes: 6 additions & 13 deletions activerecord/test/cases/reaper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,7 @@ def test_pool_has_reaper
end

def test_reaping_frequency_configuration
pool_config = duplicated_pool_config
pool_config.db_config.configuration_hash[:reaping_frequency] = "10.01"

pool_config = duplicated_pool_config(reaping_frequency: "10.01")
pool = ConnectionPool.new(pool_config)

assert_equal 10.01, pool.reaper.frequency
Expand All @@ -79,9 +77,7 @@ def test_reaping_frequency_configuration
end

def test_connection_pool_starts_reaper
pool_config = duplicated_pool_config
pool_config.db_config.configuration_hash[:reaping_frequency] = "0.0001"

pool_config = duplicated_pool_config(reaping_frequency: "0.0001")
pool = ConnectionPool.new(pool_config)

conn, child = new_conn_in_thread(pool)
Expand All @@ -97,8 +93,7 @@ def test_connection_pool_starts_reaper
end

def test_reaper_works_after_pool_discard
pool_config = duplicated_pool_config
pool_config.db_config.configuration_hash[:reaping_frequency] = "0.0001"
pool_config = duplicated_pool_config(reaping_frequency: "0.0001")

2.times do
pool = ConnectionPool.new(pool_config)
Expand Down Expand Up @@ -129,9 +124,7 @@ def test_reap_flush_on_discarded_pool

if Process.respond_to?(:fork)
def test_connection_pool_starts_reaper_in_fork
pool_config = duplicated_pool_config
pool_config.db_config.configuration_hash[:reaping_frequency] = "0.0001"

pool_config = duplicated_pool_config(reaping_frequency: "0.0001")
pool = ConnectionPool.new(pool_config)
pool.checkout

Expand Down Expand Up @@ -175,8 +168,8 @@ def test_reaper_does_not_reap_discarded_connection_pools
end

private
def duplicated_pool_config
old_config = ActiveRecord::Base.connection_pool.db_config.configuration_hash
def duplicated_pool_config(merge_config_options = {})
old_config = ActiveRecord::Base.connection_pool.db_config.configuration_hash.merge(merge_config_options)
db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new("arunit", "primary", old_config.dup)
PoolConfig.new("primary", db_config)
end
Expand Down
22 changes: 22 additions & 0 deletions activerecord/test/cases/test_databases_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# frozen_string_literal: true

require "cases/helper"

class TestDatabasesTest < ActiveRecord::TestCase
unless in_memory_db?
def test_databases_are_created
previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "arunit"

base_db_config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", spec_name: "primary")
expected_database = "#{base_db_config.database}-2"

ActiveRecord::Tasks::DatabaseTasks.stub(:reconstruct_from_schema, ->(db_config, _, _) {
assert_equal expected_database, db_config.database
}) do
ActiveRecord::TestDatabases.create_and_load_schema(2, env_name: "arunit")
end
ensure
ENV["RAILS_ENV"] = previous_env
end
end
end

0 comments on commit 154abca

Please sign in to comment.