-
Notifications
You must be signed in to change notification settings - Fork 21.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Part 2: Multi-db improvements, Refactor Active Record configurations #33637
Part 2: Multi-db improvements, Refactor Active Record configurations #33637
Conversation
336f6bf
to
aaec0d6
Compare
@@ -291,7 +292,6 @@ class Base | |||
extend Aggregations::ClassMethods | |||
|
|||
include Core | |||
include DatabaseConfigurations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer a module.
@@configurations.to_h | ||
else | ||
@@configurations | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I waffled back and forth on this something like 800 times. I opted for default being to use the new version because 99% of applications don't muck with the configurations hash. In addition I added some features (blank?, empty?, and []) to the returned object so the disruption should be small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm happy to change this without a deprecation: it's a low-traffic API, and we're fundamentally rearranging enough that anyone relying on the legacy version is going to have a deeply limited view of the world, at best.
I think we should ditch the legacy: true
option entirely, though: ActiveRecord::Base.configurations.to_h
will work just fine, and is a better spelling for "be like the old one" because it works on the old one too -- the only reasonable reason to want the old hash style is because you're trying to support multiple versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some method missing and removed the legacy
stuff.
@@ -127,36 +127,36 @@ class AnimalsBase < ActiveRecord::Base | |||
|
|||
test "db:create and db:drop works on all databases for env" do | |||
require "#{app_path}/config/environment" | |||
ActiveRecord::Base.configurations[Rails.env].each do |namespace, config| | |||
db_create_and_drop namespace, config["database"] | |||
ActiveRecord::Base.configurations.configs_for(Rails.env).each do |db_config| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed these because this is what is in the database tasks code so we ensure all this keeps working.
config ||= DEFAULT_ENV.call.to_sym | ||
spec_name = self == Base ? "primary" : name | ||
self.connection_specification_name = spec_name | ||
config_or_env ||= DEFAULT_ENV.call.to_sym |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall we found that these could be either a configuration hash or an environment. In order to reduce the confusing around the code in connection handling we change a bunch of the variable name so we could keep track.
aaec0d6
to
fead0d8
Compare
activerecord/lib/active_record/connection_adapters/connection_specification.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/database_configurations/database_config.rb
Outdated
Show resolved
Hide resolved
2fdc18e
to
cb5a7c1
Compare
While the three-tier config makes it easier to define databases for multiple database applications, it quickly became clear to offer full support for multiple databases we need to change the way the connections hash was handled. A three-tier config means that when Rails needed to choose a default configuration (in the case a user doesn't ask for a specific configuration) it wasn't clear to Rails which the default was. I [bandaid fixed this so the rake tasks could work](rails#32271) but that fix wasn't correct because it actually doubled up the configuration hashes. Instead of attemping to manipulate the hashes @tenderlove and I decided that it made more sense if we converted the hashes to objects so we can easily ask those object questions. In a three tier config like this: ``` development: primary: database: "my_primary_db" animals: database; "my_animals_db" ``` We end up with an object like this: ``` @Configurations=[ #<ActiveRecord::DatabaseConfigurations::HashConfig:0x00007fd1acbded10 @env_name="development",@spec_name="primary", @config={"adapter"=>"sqlite3", "database"=>"db/development.sqlite3"}>, #<ActiveRecord::DatabaseConfigurations::HashConfig:0x00007fd1acbdea90 @env_name="development",@spec_name="animals", @config={"adapter"=>"sqlite3", "database"=>"db/development.sqlite3"}> ]> ``` The configurations setter takes the database configuration set by your application and turns them into an `ActiveRecord::DatabaseConfigurations` object that has one getter - `@configurations` which is an array of all the database objects. The configurations getter returns this object by default since it acts like a hash in most of the cases we need. For example if you need to access the default `development` database we can simply request it as we did before: ``` ActiveRecord::Base.configurations["development"] ``` This will return primary development database configuration hash: ``` { "database" => "my_primary_db" } ``` Internally all of Active Record has been converted to use the new objects. I've built this to be backwards compatible but allow for accessing the hash if needed for a deprecation period. To get the original hash instead of the object you can either add `to_h` on the configurations call or pass `legacy: true` to `configurations. ``` ActiveRecord::Base.configurations.to_h => { "development => { "database" => "my_primary_db" } } ActiveRecord::Base.configurations(legacy: true) => { "development => { "database" => "my_primary_db" } } ``` The new configurations object allows us to iterate over the Active Record configurations without losing the known environment or specification name for that configuration. You can also select all the configs for an env or env and spec. With this we can always ask any object what environment it belongs to: ``` db_configs = ActiveRecord::Base.configurations.configurations_for("development") => #<ActiveRecord::DatabaseConfigurations:0x00007fd1acbdf800 @Configurations=[ #<ActiveRecord::DatabaseConfigurations::HashConfig:0x00007fd1acbded10 @env_name="development",@spec_name="primary", @config={"adapter"=>"sqlite3", "database"=>"db/development.sqlite3"}>, #<ActiveRecord::DatabaseConfigurations::HashConfig:0x00007fd1acbdea90 @env_name="development",@spec_name="animals", @config={"adapter"=>"sqlite3", "database"=>"db/development.sqlite3"}> ]> db_config.env_name => "development" db_config.spec_name => "primary" db_config.config => { "adapter"=>"sqlite3", "database"=>"db/development.sqlite3" } ``` The configurations object is more flexible than the configurations hash and will allow us to build on top of the connection management in order to add support for primary/replica connections, sharding, and constructing queries for associations that live in multiple databases.
cb5a7c1
to
fdf3f0b
Compare
🎉🎉🎉 |
After rails#33637 some tests in `activerecord/test/cases/tasks/database_tasks_test.rb` don't assert anything. We used to stub `ActiveRecord::Base::configurations` method in those tests like `ActiveRecord::Base.stub(:configurations, @Configurations) {}`. Since rails#33637 `ActiveRecord::Base::configurations` is a `ActiveRecord::DatabaseConfigurations` object(not a Hash object) we can't do so anymore. `ActiveRecord::DatabaseConfigurations` object builds during `ActiveRecord::Base::configurations=`. We can replace `ActiveRecord::Base.stub(:configurations, @Configurations) {}` to ``` begin old_configurations = ActiveRecord::Base.configurations ActiveRecord::Base.configurations = @Configurations # ... ensure ActiveRecord::Base.configurations = old_configurations end ``` Also I fixed tests in `activerecord/test/cases/tasks/legacy_database_tasks_test.rb` But currently It looks like duplication of `activerecord/test/cases/tasks/database_tasks_test.rb`. We should improve those tests or remove them. I've tried (in `activerecord/test/cases/tasks/legacy_database_tasks_test.rb` file): ``` def with_stubbed_configurations old_configurations = ActiveRecord::Base.configurations.to_h ActiveRecord::Base.configurations = @Configurations ActiveRecord::Base.stub(:configurations, ActiveRecord::Base.configurations.to_h) do yield end ensure ActiveRecord::Base.configurations = old_configurations end ``` but it causes erros in tests cases. After discussion we decided to remove `activerecord/test/cases/tasks/legacy_database_tasks_test.rb` Related to rails#33637
I know I'm late to the party but https://github.com/ioquatix/activerecord-configurations is my solution to this problem and this is probably a breaking change for my use css which is a bit of a pain. I'll need to add Rails 6 to the test matrix. |
Just for xref: #32135 |
So, as expected, it fails:
If the expectation was that |
The way that db options are access changed: rails/rails#33637
ActiveRecord::Base.configurations does not return a hash since rails 6, but using config directly still works. See rails/rails#33637
ActiveRecord::Base.configurations does not return a hash since rails 6, but using config directly still works. See rails/rails#33637
While the three-tier config makes it easier to define databases for
multiple database applications, it quickly became clear to offer full
support for multiple databases we need to change the way the connections
hash was handled.
A three-tier config means that when Rails needed to choose a default
configuration (in the case a user doesn't ask for a specific
configuration) it wasn't clear to Rails which the default was. I
bandaid fixed this so the rake tasks could work but that fix
wasn't correct because it actually doubled up the configuration hashes.
Instead of attemping to manipulate the hashes @tenderlove and I decided
that it made more sense if we converted the hashes to objects so we can
easily ask those object questions. In a three tier config like this:
We end up with an object like this:
The configurations setter takes the database configuration set by your
application and turns them into an
ActiveRecord::DatabaseConfigurations
object that has one getter -@configurations
which is an array of all the database objects.The configurations getter returns this object by default since it acts
like a hash in most of the cases we need. For example if you need to
access the default
development
database we can simply request it as wedid before:
This will return primary development database configuration hash:
Internally all of Active Record has been converted to use the new
objects. I've built this to be backwards compatible but allow for
accessing the hash if needed for a deprecation period. To get the
original hash instead of the object you can either add
to_h
on theconfigurations call or pass
legacy: true
to `configurations.The new configurations object allows us to iterate over the Active
Record configurations without losing the known environment or
specification name for that configuration. You can also select all the
configs for an env or env and spec. With this we can always ask
any object what environment it belongs to:
The configurations object is more flexible than the configurations hash
and will allow us to build on top of the connection management in order
to add support for primary/replica connections, sharding, and
constructing queries for associations that live in multiple databases.
cc/ @tenderlove @matthewd @rafaelfranca
PS This is easier to view in the split view.
Left to do:
I think there's more code that can be deleted, especially in ConnectionSpecification but I didn't want to go too far as I've been working on this since February.