DatabaseCleaner / database_cleaner-active_record

Strategies for cleaning databases using ActiveRecord. Can be used to ensure a clean state for testing.
MIT License
63 stars 64 forks source link

Transaction strategy configured with symbol(connection name) does not work #100

Open kaorukobo opened 8 months ago

kaorukobo commented 8 months ago

(I will post my suggestion next to this report.)

Summary

When we add another database to a cleaner by specifying a symbol (connection name) with the :transaction strategy, it does not open a transaction.

Look at the reproduction below. Even though the cleaner is configured to clean :db => :bar with the transaction strategy, the transaction is not opened*.

(*In fact, a transaction is opened on ActiveRecord::Base.connection instead.)

Reproduction

#!/usr/bin/env ruby

require "bundler/inline"

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', '~> 7.0.0'
  gem 'sqlite3'
  gem 'rspec'
  gem 'database_cleaner-active_record', git: "https://github.com/DatabaseCleaner/database_cleaner-active_record.git", ref: "aafa5b2"
end

require "yaml"

ActiveRecord::Base.configurations = YAML.load(<<~YAML)
  foo:
    adapter: sqlite3
    database: ":memory:"
  bar:
    adapter: sqlite3
    database: ":memory:"
YAML

RSpec.describe do
  before do
    ActiveRecord::Base.establish_connection :foo

    class Bar < ActiveRecord::Base
      establish_connection :bar
    end
  end

  it "should open a transaction on Bar" do
    cleaner = DatabaseCleaner::Cleaners.new
    cleaner[:active_record, :db => :bar].strategy = :transaction
    cleaner.cleaning do
      # expected: true
      #      got: false
      expect(Bar.connection.transaction_open?).to eq(true)
    end
  end

  it "should not open a transaction on ActiveRecord::Base where we removed it from cleaner" do
    cleaner = DatabaseCleaner::Cleaners.new
    cleaner[:active_record].strategy = nil
    cleaner[:active_record, :db => :bar].strategy = :transaction
    cleaner.cleaning do
      # expected: false
      #      got: true
      expect(ActiveRecord::Base.connection.transaction_open?).to eq(false)
    end
  end
end
kaorukobo commented 8 months ago

Suggested Solution

In the reproduction above, try replacing all the :db => :bar with :db => Bar. The test passes.

Looking into the database_cleaner/active_record/base.rb, it is managing to obtain the ActiveRecord class from a given connection name.

It makes the code have too much responsibility and very complex. That is, it is reading ActiveRecord::Base.configurations, reading config/database.yml, and searching through connection pools and ActiveRecord classes.

This complexity can easily bring bugs such as #10, #18 and #97.

Though it is a breaking change... if we make the cleaner only accept an ActiveRecord class object as a value for :db option, doesn't database_cleaner/active_record/base.rb become far simpler and reduce the possibility of bugs so much?