Add a ActiveRecord.protocol_adapters configuration to map DATABASE_URL protocols to adapters at an application level

[kmcphillips]

This takes a completely different approach to what I attempted in #50112, cc @byroot @matthewd.

Problem

When using a DATABASE_URL to define a database connection, the deployment env config needs to define the adapter used as part of the URL. The adapter is actually an application level concern, and what the env should care about is the DBMS/protocol (ie. "this rails app connects to MySQL" and not "this rails app connects to MySQL and does it using a specific adapter class")

The most obvious usecase here is toggling or transitioning between mysql2 and trilogy as they are both first party DB adapters including in Rails to connect to MySQL, but the same concern holds true for custom adapters and likely any future adapters.

What the env should be able to do is say DATABASE_URL=mysql://host/db?options=whatever and then have the application decide that mysql -> mysql2 or mysql -> trilogy without having to modify the environment.

Solution

Add an Active Record config that provides a mapping from protocol to adapter in database URLs. Use that in the ActiveRecord::DatabaseConfigurations::UrlConfig (the ActiveRecord::DatabaseConfigurations::ConnectionUrlResolver class internally) to map from protocol to adapter.

config.active_record.protocol_adapters.mysql = "trilogy"

Build in some reasonable defaults, including removing a specific postgres -> postgresql hack that was already hardcoded in. In this case:

Protocol	Adapter
mysql	mysql2
sqlite	sqlite3
postgres	postgresql

What should reviewers focus on?

• Oh gosh writing good documentation is hard.
• I use ActiveSupport::InheritableOptions but it's not strictly necessary. It provides some indifferent access and reader conveniences, but a hash would work and just be less forgiving.
• I added sqlite to sqlite3 but I honestly have no idea if this is correct or reasonable.

[adrianna-chang-shopify]

Should we specify the other default mappings here too? (sqlite => sqlite3 and postgres => postgresql?)

[kmcphillips]

I thought about it, but I worry about docs getting out of sync from code. My goal here wasn't to specify all mappings but offer an example of what one would look like (and the most common one).

[kmcphillips]

Code comments:

[matthewd]

Avoiding an exhaustive list makes sense, and agree this is currently the most common reason someone might want to override our default (even though MySQL is not the most common database). It doesn't particularly avoid watermeloning us, because we expect this default to change (and, worse [for documentation clarity], become load_defaults-version dependent), but that's probably fine. Perhaps throw in a "For example," to emphasise that point?

(Full disclosure I'm currently being lazy and not reading surrounding options' docs to factor in their precedents around phrasing etc.)

The "but" here feels a little bumpy to me: seems more appropriate if this was describing AR connection behaviour [but you can customize it]; here we're specifically talking about the config option, so what it does when set is the thing.

[matthewd]

I love this approach, thank you!

sqlite mapping seems good to me -- things like the C API (and CLI executable) incorporate the major version, but AFAICS plain "SQLite" is the correct name of the project, file format, etc.

Using the Options class makes the headline mysql case much prettier than setting a hash key, which I like. The only potential drawback that comes to mind is that it feels like it's a stricter, more typo-proof, API [but isn't]. I almost wonder whether we'd have general value in a similar class that only allowed accessors for extant keys -- so an application overriding an existing default could c.mysql = "trilogy", but a gem registering its own protocol would have to c[:oracle] ||= "oracle_enhanced" 🤔

As a more extreme option, we could use a custom mapping object in order to proactively validate that the supplied adapter name is registered during assignment. Implementation complexity aside, I have mixed feelings on whether that would be a good thing.

[byroot]

Like Matthew, I really like this, and it's nice to see it allows getting rid of some hacks, which validates the approach. 👍

[byroot]

I'm wondering if we should check if adapter maps to a registered adapter before looking it up as a protocol?

It's very unlikely, but assuming someone registered an adapter called mysql, it should have precedence for backward compact concerns.

Not too sure if it's worth it though.

[kmcphillips]

Oh yay! Great. Yeah thanks for the thoughtful feedback. I agree this is a much better solution.

I'll go through some of this small feedback here.

Should I look at load_defaults or just leave that for a future version change?

[kmcphillips]

I've got a yak shave with ActiveSupport::InheritableOptions behaviour over in #50151 that could use a review.

[eileencodes]

Hey @matthewd and @byroot, I personally don't love this change (which I talked to Kevin about). I feel like this is adding complexity for something that happens rarely - switching database adapters. I get that we're encouraging folks to switch from mysql2 to trilogy, but if they do that in 7.1 then this change won't help them do that.

I think it will cause more confusion than problems solved to have two different ways of making the database URLs. I prefer to be able to look at the URL and know exactly what adapter is going to be used. I think that the way URLs work is already confusing enough and this adds another layer of having to guess what's going to happen.

[byroot]

for something that happens rarely - switching database adapters.

This is not to simplify the switch, even though it is a side benefit. It's to allow to decouple the environment from Rails.

Imagine you have a generic CI system that provide a MySQL database and expose it via a standard DATABASE_URL environment variable.

If Rails only understand mysq2:// or trilogy:// then you need to make that CI Rails aware, try to detect the stack being used and give a different environment to Rails and Node apps.

This is a problem we have today at Shopify that I want to fix.

Also I used CI as an example, but the same issue happens with Heroku addons etc.

Even if Trilogy wasn't a thing, I'd want this change.

[kmcphillips]

Ok well, this is green and I'm happy with the implementation.

But, I don't have a strong enough opinion to weigh in on if this is what Rails wants or not. 🤷

[byroot]

Looks good to me, and the change is rather simple and small. @eileencodes how strongly are you against this? Anything you'd like to discuss?

[matthewd]

Even if Trilogy wasn't a thing, I'd want this change.

Yeah, I see this as more about the fact that "there is a MySQL database server here" is an ops / DB team / environment concern, whereas which gem you want to use to talk to it is an application concern.

To take Heroku as an example, I believe they expose a DATABASE_URL by default, and that's intended to be used by a Rails app using pg, a Python app, a Rails app running on JRuby, or a Rails app using some theoretical pillagy adapter. If we did add a new Postgres adapter at some point, it wouldn't make any sense for Heroku the platform to have to navigate/coordinate a global change to the URL scheme.

Which I think is a long winded way of saying that the trilogy transition is the trigger for us to feel this semantic mismatch, but it's always been there.

[eileencodes]

Ok that's fair, both of you have convinced me it's ok to do this. Thanks for taking the time to explain your feelings more strongly.