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

Don't replace connections without a writing role #40580

Closed
wants to merge 1 commit into from

Conversation

eugeneius
Copy link
Member

Followup to #40368, #40487, #40488; ultimately trying to unblock #40384.

If there's no corresponding writing connection, we currently overwrite the pool anyway, removing the connection. We should skip it instead.

If there's no corresponding writing connection, we currently overwrite
the pool anyway, removing the connection. We should skip it instead.
@@ -1482,6 +1482,14 @@ def test_only_existing_connections_are_replaced
end
end

def test_connections_with_no_writing_role_are_skipped
AbstractCompany.connects_to(database: { reading: :readonly })
Copy link
Member

Choose a reason for hiding this comment

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

So Rails kind of requires that there's a writing role, at least how it's configured currently. If you don't include one and boot a console, and try to query the db, you'll get an error because the default is to lookup on the writing role. I think this will be easier to change with granular connection management, but I haven't gotten around to doing that. Description of the problem is here #39580

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the context, I hadn't seen #39580.

My actual goal here is #40384, so that I can write tests for some code in our application that switches connections manually. Since that requires pool sharing to be reversible, I'm trying to understand all of the edge cases so that I'm confident they'll reverse correctly.

It's possible to write production code that works fine with no writing connection, as long as you wrap every query in a connected_to block, so I thought it was worthwhile to make that code behave the same during transactional tests. If you'd prefer not to add code to handle this case though, since it's not really supported, I'm fine with ignoring it.

Base automatically changed from master to main January 14, 2021 17:02
@rails-bot
Copy link

rails-bot bot commented Apr 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 14, 2021
@eileencodes
Copy link
Member

I'm going to close this one because I prefer not to handle this case. If it's still problematic / an issue in your app let me know and we can talk about fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants