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

fix(ActiveRecord): correctly connect to database in Rails 7.2+ #175

Merged
merged 4 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test-postgresql.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ jobs:
- { ruby: 2.6.2, postgresql: 11, active_record: '~> 6.0.0' }
- { ruby: 3.1.1, postgresql: 11, active_record: '~> 6.1.0' }
- { ruby: 3.1.1, postgresql: 14, active_record: '~> 7.0.0' }
- { ruby: 3.1.1, postgresql: 14, active_record: '~> 7.2.0' }
name: test (ruby=${{ matrix.entry.ruby }}, postgresql=${{ matrix.entry.postgresql }}, active_record=${{ matrix.entry.active_record }})
steps:
- uses: actions/checkout@v2
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### 2.1.2 (Next)

* Your contribution here.
markokajzer marked this conversation as resolved.
Show resolved Hide resolved
* [#175](https://github.com/slack-ruby/slack-ruby-bot-server/pull/175): Fix(activerecord): correctly check for database in rails 7.2+ - [@markokajzer](https://github.com/markokajzer).

### 2.1.1 (2023/07/25)

Expand Down
13 changes: 10 additions & 3 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,21 @@ case ENV.fetch('DATABASE_ADAPTER', nil)
when 'mongoid' then
gem 'kaminari-mongoid'
gem 'mongoid', ENV['MONGOID_VERSION'] || '~> 7.3.0'
gem 'mongoid-scroll'
gem 'mongoid-scroll', '~> 1.0.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one had a breaking change in 2.0 that's easy to fix. It's something like this (from dblock/slack-strava@88af16b#diff-633cee3ca2d39648d625e12b921138aa0a932ebe9375ba53102cbabc339e5afe).

Screenshot 2024-12-01 at 7 03 38 PM

gem 'mongoid-shell'

group :development, :test do
gem 'database_cleaner-mongoid', '~> 2.0.1'
end
when 'activerecord' then
gem 'activerecord', ENV['ACTIVERECORD_VERSION'] || '~> 6.0.0'
gem 'otr-activerecord'
gem 'pagy_cursor'
gem 'pagy_cursor', '~> 0.6.1'
gem 'pg'

group :development, :test do
gem 'database_cleaner-active_record', '~> 2.2.0'
end
when nil
warn "Missing ENV['DATABASE_ADAPTER']."
else
Expand All @@ -23,7 +31,6 @@ group :development, :test do
gem 'bundler'
gem 'byebug'
gem 'capybara', '~> 3.36.0'
gem 'database_cleaner', '~> 1.8.5'
gem 'fabrication'
gem 'faker'
gem 'faraday', '0.17.5'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@
module SlackRubyBotServer
module DatabaseAdapter
def self.check!
ActiveRecord::Base.connection_pool.with_connection(&:active?)
if ActiveRecord.version >= Gem::Version.new('7.2')
ActiveRecord::Base.connection.database_exists?
else
ActiveRecord::Base.connection_pool.with_connection(&:active?)
end

raise 'Unexpected error.' unless ActiveRecord::Base.connected?
rescue StandardError => e
warn "Error connecting to PostgreSQL: #{e.message}"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require 'database_cleaner'
require 'database_cleaner/active_record'

RSpec.configure do |config|
config.before :suite do
Expand Down
14 changes: 14 additions & 0 deletions spec/database_adapters/mongoid/database_cleaner.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
require 'database_cleaner/mongoid'

RSpec.configure do |config|
config.before :suite do
DatabaseCleaner.strategy = :deletion
DatabaseCleaner.clean_with :deletion
end

config.around :each do |example|
DatabaseCleaner.cleaning do
example.run
end
end
end
37 changes: 37 additions & 0 deletions spec/slack-ruby-bot-server/app_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,43 @@
expect(app.instance).to be_an_instance_of(app)
end
end
context '#prepare!' do
context 'when connection cannot be established' do
context 'with ActiveRecord >= 7.2' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a spec for 7.2+ vs pre-7.2.

quick explanation:

DatabaseCleaner leases the connection, so ActiveRecord::Base.connected? is true when the spec starts, but we want to simulate what should happen in prod:

  1. ActiveRecord::Base.connected? is false
  2. ActiveRecord::Base.connection.database_exists? (or ActiveRecord::Base.connection_pool.with_connection(&:active?)) connects to the database
  3. ActiveRecord::Base.connected? is true

in pre-7.2 that does not seem to work, the specs ultimately fail because DatabaseCleaner tries to operate on a closed connection. i've not been able to come up with a good spec for pre-7.2 so not sure if we should keep the spec, lmkwyt!

i added comments as well because it's maybe not obvious what is going on


along the way i had to do the following changes

  1. pin pagy_cursor to ~> 0.6.1
    • the method pagy_cursor only supports 2 parameters on 0.8, see here
  2. pin database_cleaner to a higher version to support ActiveRecord 7.2

i ran the full spec suite locally with all versions from the matrix and all passed, so let's hope ci does us the honor as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

i ran the full spec suite locally with all versions from the matrix and all passed, so let's hope ci does us the honor as well

First time contributor PRs marked as pending is a little annoying (a 1-time problem), but you can always enable CI on your fork for next time you run into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but you can always enable CI on your fork

thanks for the tip, i somehow overlooked this lol

before do
skip 'incorrect database adapter' unless ENV['DATABASE_ADAPTER'] == 'activerecord'
skip 'incorrect ActiveRecord version' if ActiveRecord.version < Gem::Version.new('7.2')

# Make sure ActiveRecord is not connected in any way before the spec starts
ActiveRecord::Base.connection_pool.disconnect!
end

it 'raises' do
expect(ActiveRecord::Base).not_to be_connected

# Simulate that #database_exists? does not connect to the database
allow(ActiveRecord::Base).to receive_message_chain(:connection, :database_exists?)
expect { subject.prepare! }
.to raise_error('Unexpected error.')
end
end

context 'with ActiveRecord < 7.2' do
before do
skip 'incorrect database adapter' unless ENV['DATABASE_ADAPTER'] == 'activerecord'
skip 'incorrect ActiveRecord version' if ActiveRecord.version >= Gem::Version.new('7.2')
end

it 'raises' do
# In ActiveRecord < 7.1, `disconnect!` closes the connection that has already been leased by
# DatabaseCleaner, so we cannot do that trick here to simulate a not established connection.
allow(ActiveRecord::Base).to receive(:connected?).and_return(false)
expect { subject.prepare! }
.to raise_error('Unexpected error.')
end
end
end
end
context '#purge_inactive_teams!' do
it 'purges teams' do
expect(Team).to receive(:purge!)
Expand Down