Skip to content
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

Add support for hash and url configs to be used in connected_to #34196

Merged
merged 1 commit into from Oct 30, 2018

Conversation

@gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Oct 11, 2018

Summary

I noticed when using the new connection switching API, if I use a string database name, it bubbles down to this connection resolution method which thinks strings are database URLs. Do we want to support URLs, or is this acceptable behaviour? If not, we should make DatabaseConfig#spec_name return a symbol instead of a string.

@gmcgibbon
Copy link
Member Author

@gmcgibbon gmcgibbon commented Oct 11, 2018

@gmcgibbon gmcgibbon force-pushed the connection_switch_string_name branch from 8a8d8a4 to cd80e72 Oct 11, 2018
@eileencodes
Copy link
Member

@eileencodes eileencodes commented Oct 11, 2018

Right now connects_to wraps around establish_connection so it should behave the same exact way.

>> ActiveRecord::Base.establish_connection "primary"
ActiveRecord::AdapterNotSpecified: database configuration does not specify adapter
	from (irb):14
>> ActiveRecord::Base.connects_to database: { :writing => "primary" }
ActiveRecord::AdapterNotSpecified: database configuration does not specify adapter
	from (irb):15

To me a string that's not a URL causing an error is the correct behavior. I do think however this error message could be improved to note it expected a database URL instead.

@gmcgibbon
Copy link
Member Author

@gmcgibbon gmcgibbon commented Oct 11, 2018

Ok, I assumed it was different because you can't currently use a connection hash because of this to_sym call. What do you think of DatabaseConfig#spec_name returning a symbol then? I can also improve the error in another PR.

@eileencodes
Copy link
Member

@eileencodes eileencodes commented Oct 12, 2018

Ohhhh I see - I was thinking about connects_to. Yes in that case we should remove the to_sym call in connected_to. That was a mistake on my part. Can you update this PR to remove that and add a test for a URL configuration? Thanks!

@eileencodes
Copy link
Member

@eileencodes eileencodes commented Oct 12, 2018

The underlying problem is that there are a billion ways to make a configuration and connect to the database. Take a look at this test https://github.com/rails/rails/blob/master/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb and you'll get an idea of some of the possible scenarios. We may eventually want to say some of these are not going to be possible with a multi-db application but I don't think we're there yet. For example, I'm not sure how a DATABASE_URL would work with multiple databases - since you can only pass one at a time.

@gmcgibbon gmcgibbon force-pushed the connection_switch_string_name branch from cd80e72 to 634f546 Oct 12, 2018
@gmcgibbon gmcgibbon changed the title Allow connection switching with string database names Add support for hash and url configs to be used in connected_to Oct 12, 2018
@gmcgibbon
Copy link
Member Author

@gmcgibbon gmcgibbon commented Oct 12, 2018

Done! I added a test for both hash configs and URLs since they both fail on master.


ActiveRecord::Base.connected_to(database: config) do
handler = ActiveRecord::Base.connection_handler
assert_equal handler, ActiveRecord::Base.connection_handlers[config]
Copy link
Member Author

@gmcgibbon gmcgibbon Oct 12, 2018

I'm not sure how I feel about this. Should we try to lookup the config to store the handler with a symbol key and fallback to this behaviour if its not mentioned in database.yml?

Copy link
Member

@eileencodes eileencodes Oct 25, 2018

Oh 💩 it just occurred to me this isn't going to work. It's fine if it's a symbol but what would the handler be if it was a url? We definitely don't want the handler to be postgres://blah so I think we need to change connects to to take a role and a database at the same time and only if it's a symbol use the database name as a handler. Thoughts?

Copy link
Member Author

@gmcgibbon gmcgibbon Oct 25, 2018

Yeah, I think if we always lookup handlers using symbols, that makes the most sense. Does it make sense to always use roles for handler lookup or do we need to still try using the database spec/env/config/url/??? for legacy support?

Copy link
Member

@eileencodes eileencodes Oct 26, 2018

We need the handler/role for when you're connected already and we need the database for connecting to a database that your model doesn't always want to be connected to (for example, readonly_slow). Role should always be symbol. I'm not sure what legacy support we would need? This is a new method so we can change it until 6 is released.

I've been thinking about this and I think we should either accept a symbol or a hash for the database option. If you need to pass a string then you need to also pass a role / name / handler with it.

ActiveRecord::Base.connected_to(database: { my_url_db: "postgres://blah" }) do
  # do something with this db
end

Otherwise a symbol is expected.

Copy link
Member Author

@gmcgibbon gmcgibbon Oct 26, 2018

I changed the logic around to use database if its a symbol or role if it isn't. That should solve the issue AFAICT.

Copy link
Member Author

@gmcgibbon gmcgibbon Oct 26, 2018

I've been thinking about this and I think we should either accept a symbol or a hash for the database option. If you need to pass a string then you need to also pass a role / name / handler with it.

I've refactored the method to behave this way for both URLs and config hashes. WDYT?

@gmcgibbon gmcgibbon force-pushed the connection_switch_string_name branch 4 times, most recently from 195dc38 to 8ad5850 Oct 26, 2018

ActiveRecord::Base.connected_to(database: config) do
handler = ActiveRecord::Base.connection_handler
assert_equal handler, ActiveRecord::Base.connection_handlers[config]
Copy link
Member

@eileencodes eileencodes Oct 26, 2018

We need the handler/role for when you're connected already and we need the database for connecting to a database that your model doesn't always want to be connected to (for example, readonly_slow). Role should always be symbol. I'm not sure what legacy support we would need? This is a new method so we can change it until 6 is released.

I've been thinking about this and I think we should either accept a symbol or a hash for the database option. If you need to pass a string then you need to also pass a role / name / handler with it.

ActiveRecord::Base.connected_to(database: { my_url_db: "postgres://blah" }) do
  # do something with this db
end

Otherwise a symbol is expected.

config_hash = resolve_config_for_connection(database)
handler = lookup_connection_handler(database.to_sym)
handler = lookup_connection_handler(hanlder_key)
Copy link
Member

@eileencodes eileencodes Oct 26, 2018

You have a typo in handler 😄

Copy link
Member Author

@gmcgibbon gmcgibbon Oct 26, 2018

🤦‍♂️

@gmcgibbon gmcgibbon force-pushed the connection_switch_string_name branch 3 times, most recently from c2d0a58 to d468d48 Oct 26, 2018
Add support for hash and url configs in database hash
of `ActiveRecord::Base.connected_to`.
@gmcgibbon gmcgibbon force-pushed the connection_switch_string_name branch from d468d48 to abf5184 Oct 26, 2018
@eileencodes eileencodes merged commit 6889e72 into rails:master Oct 30, 2018
2 checks passed
@eileencodes
Copy link
Member

@eileencodes eileencodes commented Oct 30, 2018

This looks great! Thank you!

kamipo added a commit that referenced this issue Oct 31, 2018
@@ -1,3 +1,18 @@
* Add support for hash and url configs in database hash of `ActiveRecord::Base.connected_to`.
Copy link
Contributor

@bogdanvlviv bogdanvlviv Nov 1, 2018

Great addition. Thank you! Just want to note that we don't add changelog entries about complementing features that haven't been released.

@gmcgibbon gmcgibbon deleted the connection_switch_string_name branch Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants