Fix #43978 migrations on multidb

[codeodor]

Summary

Possible fix for #43978, and other issues I found when trying to load schema.rb within a connected_to block and run migrations with a custom migration context.

Other Information

As part of this, would it make sense to move this code into a module that could then be included in SchemaMigration (and in particular, for client code to be able to re-use when they need their own ApplicationSchemaMigration with the same code but a different superclass?

This is a duplicate of #43979 which will be closed in favor of this, since I originally wrote that one on the wrong branch, and Github won't let me change my branch of the PR, (and I neglected to create my own, using 7-0-stable from the rails/rails repo)

@rafaelfranca tagging you, since you had asked for this adjustment.

[codeodor]

I think it will be nice to add some tests that ensure the migrations run on the specific DB, but I might need some direction on where to add them, as I think this would require a test app, and I haven't previously dug into how that would get tested in the rails repo.

[eileencodes]

Can you make this a method like how def connection is a method to clean this up the logic a bit?

[codeodor]

@eileencodes sorry for my late reply. Dunno why I didn't see the email notification. I think that will be a good improvement. Do you think migration_base_class or migration_connection_class would be short and descriptive enough?

[codeodor]

@eileencodes I went with migrate_on_base_class but happy to adjust if you like another name.

[codeodor]

@eileencodes did you get a chance to look at this since I made the change?

[eileencodes]

Sorry for the delay I've been on vacation and took a break from Rails.

This looks good. Can you squash your commits and I'll merge? Thank you!

[codeodor]

Thanks @eileencodes, I hope you enjoyed your time away, and welcome back!

I added a changelog entry and rebased against the latest code on the main branch.

If there's anything else, please let me know!

While I have you, as I was working on this, I used a custom migration context which had me duplicating some of this Rails code, so I wonder: do you think it would make sense to move this code into a module that could then be included in SchemaMigration (and in particular, for client code to be able to re-use when they need their own ApplicationSchemaMigration with the same code but a different superclass?

[eileencodes]

I was going to merge this and then decided it needed a test. While working from your app to write a test it doesn't appear that this change fixed the bug in your test script. I haven't had time to dig in but I didn't want you to think that I was ignoring your PR.

[eileencodes]

Ok so I looked at this again and I realized that I was confused about how the problem was supposed to present itself. It took me a minute to wrap my mind around loading the schema for the default shard while on shard_2.

Your change here does fix the bug you reported but then I started thinking - is this the right fix for this problem? If you're on shard_2 and something loads default_shard's schema, then shouldn't an error be raised instead? It seems like if you call connected_to and load the wrong schema then Active Record shouldn't continue trying to lookup the shard. Without raising an error it's potentially introducing a security issue. If an app doesn't guarantee uniqueness between shards incorrect data should get loaded accidentally. It might be that I still don't understand this problem, I'm having a hard time understanding when a schema for the wrong shard would get loaded in a real production application.

[codeodor]

@eileencodes in our case, it's not that it's the wrong schema.

We ran into a couple of issues related to this, which is why the "fix" broadened a bit to include migrations. Let me give some history:

Our app has been horizontally sharded since before Rails introduced it, but we have had a lot of custom code to do that. As Rails now supports it, we're trying to use its facilities more and more.

One problem we have in doing that is that we have way too many databases to manage them by hand in the database.yml file (currently about 900). So we have some code that copies configs and tells ApplicationRecord about the shards, and other code that keeps everything in sync. This all works great, but in moving to Rails 7, we came across these issues.

The first was in our test suite, we want to create a new shard at runtime, called shard_2, and load it with the default_shard schema. So in our test script, we create a database, adjust our ApplicationRecord so it knows about the new shard, and then we load the default schema. It's intended to be the same database, but with different data.

So, when it failed to run the schema load on the database we specified, that's when I opened the issue.

I "fixed" that issue with some of the code in this PR, and then continued working on our move to Rails 7. In doing so, we will run a migration against default_shard which then needs to be run against shard_2. That is when I discovered that we had trouble running the migrations on the application record which knew about all of our shards.

So at least in our case, we definitely want to load this schema on the shard. This small PR would allow us to create shards dynamically, rather than having to specify each and every one in the database.yml file.

But I can see where you are coming from -- in the case someone accidentally does this, you don't necessarily want to allow them to harm themselves. (although, I wonder -- would loading the schema on the default blast all of their data away anyway?)

What if we had it raise an error but had the ability to pass in an argument like i_really_want_to_do_this? If that sounds good, do you have any suggestions on how to determine if the schema file loaded matches the one expected for the shard we're on in the connected_to call?

[codeodor]

@eileencodes is there anything I can do to help move this along?

[eileencodes]

At the moment, no. I'm not sure what the right fix is and I don't have the bandwidth right now to dig in. I'm not a fan of having a way for someone to opt-in to incorrect behavior.

I think this boils down to the migrations class is not designed for multiple databases and needs to be rewritten. It's on my list of projects to write up and tackle soon but it's part of a larger effort to rewrite some of the internal connection management so it's easier to follow and less buggy with shards (it just wasn't originally designed to handle this kind of thing).

[codeodor]

@eileencodes thanks for the update!

To me, it seems like a good solution to run it on the connection base class, as that seems to cover all cases. But is the worry that someone might run this where in one place (say a controller) they've wrapped the request in a connected_to call, and then accidentally perform migrations?

If that's the main concern, and I understand not allowing a method argument to opt in to bad behavior, what if there was something like migrate_while_connected_to that would do this?

I'm eager to help in any way I can on this item, as it's the last thing holding us back from moving to Rails 7. (Though with your last comment, we may very well just ship our own patch for this). That said, I figured I'd throw that idea about migrate_while_connected_to idea out there, in case you thought that might be a good solution, I'd be happy to start the work on it as time allows.

Thanks again for taking a look at this and for walking me through your concerns in spite of having a million other things to do. I really appreciate it!