mirror of https://github.com/rails/rails
Fix broken url configs
This PR is to fix #36559 but I also found other issues that haven't been reported. The check for `(config.size == 1 && config.values.all? { |v| v.is_a? String })` was naive. The only reason this passed was because we had tests that had single hash size configs, but that doesn't mean we don't want to create a hash config in other cases. So this now checks for `config["database"] || config["adapter"] || ENV["DATABASE_URL"]`. In the end for url configs we still get a UrlConfig but we need to pass through the HashConfig to create the right kind of UrlConfig. The UrlConfig's are really complex and I don't necessarily understand everything that's needed in order to act the same as Rails 5.2. I edited the connection handler test to demonstrate how the previous implementation was broken when checking config size. Now old and new tests pass so I think this is closer to 5.2. Fixes #36559
This commit is contained in:
parent
40246125b9
commit
f2ad69fe7a
|
@ -141,7 +141,7 @@ module ActiveRecord
|
|||
config_without_url.delete "url"
|
||||
|
||||
ActiveRecord::DatabaseConfigurations::UrlConfig.new(env_name, spec_name, url, config_without_url)
|
||||
elsif config["database"] || (config.size == 1 && config.values.all? { |v| v.is_a? String })
|
||||
elsif config["database"] || config["adapter"] || ENV["DATABASE_URL"]
|
||||
ActiveRecord::DatabaseConfigurations::HashConfig.new(env_name, spec_name, config)
|
||||
else
|
||||
config.each_pair.map do |sub_spec_name, sub_config|
|
||||
|
|
|
@ -29,7 +29,7 @@ module ActiveRecord
|
|||
|
||||
def test_establish_connection_uses_spec_name
|
||||
old_config = ActiveRecord::Base.configurations
|
||||
config = { "readonly" => { "adapter" => "sqlite3" } }
|
||||
config = { "readonly" => { "adapter" => "sqlite3", "pool" => "5" } }
|
||||
ActiveRecord::Base.configurations = config
|
||||
resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new(ActiveRecord::Base.configurations)
|
||||
spec = resolver.spec(:readonly)
|
||||
|
|
|
@ -273,6 +273,37 @@ module ActiveRecord
|
|||
}
|
||||
assert_equal expected, actual
|
||||
end
|
||||
|
||||
def test_merge_no_conflicts_with_database_url_and_adapter
|
||||
ENV["DATABASE_URL"] = "postgres://localhost/foo"
|
||||
|
||||
config = { "default_env" => { "adapter" => "postgresql", "pool" => "5" } }
|
||||
actual = resolve_config(config)
|
||||
expected = { "default_env" =>
|
||||
{ "adapter" => "postgresql",
|
||||
"database" => "foo",
|
||||
"host" => "localhost",
|
||||
"pool" => "5"
|
||||
}
|
||||
}
|
||||
assert_equal expected, actual
|
||||
end
|
||||
|
||||
def test_merge_no_conflicts_with_database_url_and_numeric_pool
|
||||
ENV["DATABASE_URL"] = "postgres://localhost/foo"
|
||||
|
||||
config = { "default_env" => { "pool" => 5 } }
|
||||
actual = resolve_config(config)
|
||||
expected = { "default_env" =>
|
||||
{ "adapter" => "postgresql",
|
||||
"database" => "foo",
|
||||
"host" => "localhost",
|
||||
"pool" => 5
|
||||
}
|
||||
}
|
||||
|
||||
assert_equal expected, actual
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in New Issue