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

rails-botbot added the activerecord label

Oct 11, 2018

Copy link

MemberAuthor

gmcgibbon commentedOct 11, 2018

r?@eileencodes
cc@rafaelfranca

rails-bot assignedeileencodes

Oct 11, 2018

gmcgibbon force-pushed theconnection_switch_string_name branch from8a8d8a4 tocd80e72Compare

October 11, 2018 22:06

Copy link

Member

eileencodes commentedOct 11, 2018•
edited
Loading

Right nowconnects_to wraps aroundestablish_connection so it should behave the same exact way.

>> ActiveRecord::Base.establish_connection "primary"ActiveRecord::AdapterNotSpecified: database configuration does not specify adapterfrom (irb):14

>> ActiveRecord::Base.connects_to database: { :writing => "primary" }ActiveRecord::AdapterNotSpecified: database configuration does not specify adapterfrom (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.

Copy link

MemberAuthor

gmcgibbon commentedOct 11, 2018•
edited
Loading

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

Copy link

Member

eileencodes commentedOct 12, 2018

Ohhhh I see - I was thinking aboutconnects_to. Yes in that case we should remove theto_sym call inconnected_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!

Copy link

Member

eileencodes commentedOct 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 testhttps://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 force-pushed theconnection_switch_string_name branch fromcd80e72 to634f546Compare

October 12, 2018 15:06

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

Copy link

MemberAuthor

gmcgibbon commentedOct 12, 2018

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

gmcgibbon commented

Oct 12, 2018

View reviewed changes

activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb Outdated

Copy link

MemberAuthor

gmcgibbonOct 12, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 indatabase.yml?

Copy link

Member

eileencodesOct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 bepostgres://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

MemberAuthor

gmcgibbonOct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

eileencodesOct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 dbend

Otherwise a symbol is expected.

Copy link

MemberAuthor

gmcgibbonOct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I changed the logic around to usedatabase if its a symbol orrole if it isn't. That should solve the issue AFAICT.

Copy link

MemberAuthor

gmcgibbonOct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 force-pushed theconnection_switch_string_name branch 4 times, most recently from195dc38 to8ad5850Compare

October 26, 2018 18:10

eileencodes reviewed

Oct 26, 2018

View reviewed changes

activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb Outdated

Copy link

Member

eileencodesOct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 dbend

Otherwise a symbol is expected.

activerecord/lib/active_record/connection_handling.rb Outdated

Copy link

Member

eileencodesOct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You have a typo in handler 😄

Copy link

MemberAuthor

gmcgibbonOct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

🤦‍♂️

gmcgibbon force-pushed theconnection_switch_string_name branch 3 times, most recently fromc2d0a58 tod468d48Compare

October 26, 2018 20:26

Add support for hash and url configs in connected_to

abf5184

Add support for hash and url configs in database hashof `ActiveRecord::Base.connected_to`.

gmcgibbon force-pushed theconnection_switch_string_name branch fromd468d48 toabf5184Compare

October 26, 2018 20:28

eileencodes merged commit6889e72 intorails:master

Oct 30, 2018

Copy link

Member

eileencodes commentedOct 30, 2018

This looks great! Thank you!

kamipo added a commit that referenced this pull request

Oct 31, 2018

Fix "warning: shadowing outer local variable - role"

b86c2a6

Caused at#34196.

bogdanvlviv reviewed

Nov 1, 2018

View reviewed changes

activerecord/CHANGELOG.md

		@@ -1,3 +1,18 @@
		* Add support for hash and url configs in database hash of`ActiveRecord::Base.connected_to`.

Copy link

Contributor

bogdanvlvivNov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

gmcgibbon deleted the connection_switch_string_name branch

November 1, 2018 21:33

Labels

activerecord

Movatterモバイル変換

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

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

Uh oh!

Conversation

gmcgibbon commentedOct 11, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Summary

Uh oh!

gmcgibbon commentedOct 11, 2018

Uh oh!

eileencodes commentedOct 11, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

gmcgibbon commentedOct 11, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

eileencodes commentedOct 12, 2018

Uh oh!

eileencodes commentedOct 12, 2018

Uh oh!

gmcgibbon commentedOct 12, 2018

Uh oh!

gmcgibbonOct 12, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileencodes commentedOct 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gmcgibbon commentedOct 11, 2018•
edited
Loading

eileencodes commentedOct 11, 2018•
edited
Loading

gmcgibbon commentedOct 11, 2018•
edited
Loading

gmcgibbonOct 12, 2018•
edited
Loading