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

Move calls on Base connection to methods for rake tasks #46270

Merged

Conversation

eileencodes
Copy link
Member

@eileencodes eileencodes commented Oct 18, 2022

This PR aims to contain calls to Base.connection and Base.establish_connection in active record rake tasks and the migration code. In a follow up PR I will swap out the Base class for a temporary class which will allow us to stop clobbering Base in the active record rails tasks.

This work is an important step to achieve moving away from Active Record's dependence on Base.connection and
Base.establish_connection. The reliance on Base.connection is problematic for sharding support and calling Base.establish_connection in the Rake tasks (without any warning message) indicates it's ok for applications to do the same (outside these tasks, it's not).

I've vetted the approach of swapping out Base for a temporary class in another branch but decided that it would be easier to demonstrate and contain the changes if I first contained these calls. The major changes in this PR are:

  • Contain Base.connection in Tasks.migration_connection and replace calls to each
  • Contain Base.establish_connection in Tasks.establish_connection and replace calls to each
  • Add a with_temporary_connection_for_each method for cases where we need to loop over each config and set the connection back afterwards
  • Add a with_temporary_connection(db_config) method for cases where we have one config but need to establish a new connection and set the old one back.
  • Update every place we were looping through configs to establish a connection with the new temporary connection methods.

There are a lot of changes here but I've pulled out everything that didn't need to be in this commit into other PRs.

Once this is merged, I'll create the next PR that replaces Base the new methods in Tasks with a temporary connection and then we will officially be no longer clobbering Base in these tasks. This also reduces complexity because we won't need to ensure we set Base.connection back at the end. Once that is working and all internal methods are using the new temp class I'll deprecate calls on Base.connection in these methods. Most applications just override the task but not the actual methods in Tasks so my hope is this will be smooth-ish. However, nothing will stop applications from still using Base.connection for a very long time if they still want to clobber.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • There are no typos in commit messages and comments.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Feature branch is up-to-date with main (if not - rebase it).
  • Pull request only contains one commit for bug fixes and small features. If it's a larger feature, multiple commits are permitted but must be descriptive.
  • Tests are added if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
  • PR is not in a draft state.
  • CI is passing.

@eileencodes eileencodes force-pushed the extract-calls-on-base-for-rake-tasks branch from 1d8a07e to 09e0b72 Compare October 18, 2022 20:23
@eileencodes
Copy link
Member Author

Waiting for last week's Rails upgrade to be done to merge this.

@eileencodes eileencodes self-assigned this Oct 24, 2022
@eileencodes eileencodes added this to the 7.1.0 milestone Oct 24, 2022
This PR aims to contain calls to `Base.connection` and
`Base.establish_connection` in active record rake tasks and the
migration code. In a follow up PR I will swap out the `Base` class for a
temporary class which will allow us to stop clobbering `Base` in the
active record rails tasks.

This work is an important step to achieve moving away from Active
Record's dependence on `Base.connection` and
`Base.establish_connection`. The reliance on `Base.connection` is
problematic for sharding support and calling `Base.establish_connection`
in the Rake tasks (without any warning message) indicates it's ok for
applications to do the same (outside these tasks, it's not).

I've vetted the approach of swapping out `Base` for a temporary class in
another branch but decided that it would be easier to demonstrate and
contain the changes if I first contained these calls. The major changes
in this PR are:

* Contain `Base.connection` in `Tasks.migration_connection` and replace
calls to each
* Contain `Base.establish_connection` in `Tasks.establish_connection`
and replace calls to each
* Add a `with_temporary_connection_for_each` method for cases where we
need to loop over each config and set the connection back afterwards
* Add a `with_temporary_connection(db_config)` method for cases where we
have one config but need to establish a new connection and set the old
one back.
* Update every place we were looping through configs to establish a
connection with the new temporary connection methods.

There are a lot of changes here but I've pulled out everything that
didn't need to be in this commit into other PRs.

Once this is merged, I'll create the next PR that replaces `Base` the
new methods in `Tasks` with a temporary connection and then we will
officially be no longer clobbering `Base` in these tasks. This also
reduces complexity because we won't need to ensure we set
`Base.connection` back at the end. Once that is working and all internal
methods are using the new temp class I'll deprecate calls on
`Base.connection` in these methods. Most applications just override the
task but not the actual methods in `Tasks` so my hope is this will be
smooth-ish. However, nothing will stop applications from still using
`Base.connection` for a very long time if they still want to clobber.
@eileencodes eileencodes force-pushed the extract-calls-on-base-for-rake-tasks branch from 09e0b72 to 901828f Compare December 8, 2022 20:39
@eileencodes eileencodes merged commit 3e1195c into rails:main Dec 8, 2022
@eileencodes eileencodes deleted the extract-calls-on-base-for-rake-tasks branch December 8, 2022 21:59
@trevorturk
Copy link
Contributor

Hello! I'm happy to file an issue if that would be preferable, and/or create a reproduction case in a public repo, but I thought I'd chime in here to start:

I ran bundle update to get onto the latest Rails edge, but my GitHub Actions CI started failing like so:

DRb::DRbRemoteError: We could not find your database: example_test-1. Available database configurations can be found in config/database.yml file.
To resolve this error:
- Did you create the database for this app, or delete it? You may need to create your database.
- Has the database name changed? Check your database.yml config has the correct database name.
To create your database, run:
        bin/rails db:create
 (ActiveRecord::NoDatabaseError)

If I change my Gemfile to pick from the commit prior to this one, CI still works, so I believe the issue is orginiated here.

I've tried a few workarounds (db:create doesn't help, for example) but I'm wondering if anyone else has hit this already, and/or if you have an inkling as to what might have changed. My GitHub workflow is pretty straightforward:

name: CI
on:
  push:
env:
  RAILS_MASTER_KEY: ${{ secrets.RAILS_MASTER_KEY }}
jobs:
  test:
    runs-on: ubuntu-latest
    services:
      postgres:
        image: postgres:14
        ports:
          - "5432:5432"
        env:
          POSTGRES_DB: example_test
          POSTGRES_USER: user
          POSTGRES_PASSWORD: password
    env:
      RAILS_ENV: test
      DATABASE_URL: "postgres://user:password@localhost:5432/example_test"
    steps:
      - name: Checkout
        uses: actions/checkout@v3
      - name: Set up Ruby
        uses: ruby/setup-ruby@v1
        with:
          ruby-version: '3.1.3'
          bundler-cache: true
      - name: Set up database
        run: bin/rails db:schema:load
      - name: Run tests
        run: bin/rails test

Thank you!

@eileencodes
Copy link
Member Author

Hey @trevorturk - can you make a reproduction? I can't really debug this without a way to reproduce and my local apps and Shopify all passed fine with this change. Also I'd prefer an issue so that if someone else has the problem it's more discoverable. Thanks!

@eileencodes eileencodes mentioned this pull request Dec 13, 2022
2 tasks
eileencodes added a commit to eileencodes/rails that referenced this pull request Dec 13, 2022
In rails#46270 it was reported that there were database errors when running
the tests in an app. I set up a demo app to use parallel testing and was
able to reproduce. The issue was that the `reconstruct_from_schema`
method needs to use the `pool` and not the `connection` because the
database might not exist yet and will raise an error. I don't know how
to test this inside Rails but I verified this behavior in my demo app.
@trevorturk
Copy link
Contributor

Thank you for this, and for all of your work improving Ruby/Rails, it's very much appreciated! 🥳

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