DatabaseCleaner / database_cleaner

Strategies for cleaning databases in Ruby. Can be used to ensure a clean state for testing.
https://www.rubydoc.info/github/DatabaseCleaner/database_cleaner
MIT License
2.93k stars 483 forks source link

can not use the transaction strategy when using multiple DBs (with AR)... #22

Closed bmabey closed 12 years ago

bmabey commented 13 years ago

This fails the tests, at least with ActiveRecord. Still need to manually test this to verify it isn't a testing bug. Either way, this needs to be fixed.

kevincolyar commented 13 years ago

I think I'm having this issue, too. I using 5 different database connections (don't ask) in my app and trying to get running transactional tests working has been a nightmare. I think I might be close, but it looks like using:

 DatabaseCleaner[:active_record,{:connection => db}].strategy = :transaction
 DatabaseCleaner[:active_record,{:connection => db}].clean_with :truncation, {:only => tables}

is setting the :only key for all connections, which causes sql errors when the database tries to truncate tables it doesn't know about.

Would appreciate any help getting multiple connections working.

Thanks, Kevin

bmabey commented 13 years ago

Yeah, I noticed that the transactions didn't work with multiple DBs after I had already merged in the changes. :( I haven't had the time to investigate why yet and you are the first person who has run into it.

That is odd about the multiple connections being called because this #clean_with should be called in this case: https://github.com/bmabey/database_cleaner/blob/master/lib/database_cleaner/base.rb#L36-40

Which would only operate on that one connection. However, if for some reason this method was being called your error would make sense:

https://github.com/bmabey/database_cleaner/blob/master/lib/database_cleaner/configuration.rb#L49-51

If you could reproduce your problem in a stand alone app I would be able to investigate further. You can create a new rails app from scratch or use this one as a starting point: https://github.com/bmabey/databasecleaner-debug

kevincolyar commented 13 years ago

I think I've found a work around for the :only problem. I've just removed the tables I don't want from my schemas.

Shoot, well I may have to fork and take a look. If you do figure it out please let me know asap.

Thanks for the reply, Kevin

thoughtless commented 13 years ago

Maybe this deserves its own ticket, but I think it is related.

Currently DatabaseCleaner::Base#clean_with does not set the db on the strategy. That means that connection_klass will not necessarily return the correct connection. This meant that when I used DatabaseCleaner.clean_with(:truncation) in my config.before(:suite) with rspec, only 1 of the two databases was getting truncated.

By doing something like this:

      if strategy.respond_to? :db=
        strategy.db = db
      elsif db!= :default
        raise ArgumentError, "You must provide a strategy object that supports non default databases when you specify a database"
      end

inside the clean_with method, I was able to work around the problem.

thoughtless commented 13 years ago

Something else that may be related is that Class.new(::ActiveRecord::Base) (from DatabaseCleaner::ActiveRecord::Base) will return nil when you call name on it. name is used as the key by ActiveRecord::ConnectionAdapters::ConnectionHandler for differentiating between different databases. That means that calling establish_connection on any of these classes will reset the connection on all the other objects.

The following is rather hacky, but it seems to work.

      def create_connection_klass
        klass = Class.new(::ActiveRecord::Base)
        metaclass = class << klass; self; end
        metaclass.send(:define_method, :name) { "name#{self.object_id.to_s}" }
        klass
      end
thoughtless commented 13 years ago

After digging some more, it seems that the issue is actually much deeper.

ActiveRecord uses the connections to keep track of whether or not a transaction has already been started to decide whether to wrap things like MyModel.save with BEGIN and COMMIT or SAVEPOINT and RELEASE SAVEPOINT (using MySQL as an example). Even if the connection string is identical, if the name (see my previous comment) is not the same, then ActiveRecord will use a separate connection, and therefore transactional database cleaning will not work.

I think the best solution to this would be to allow the user to specify a model rather than just an entry in database.yml. The value specified as the model would be used as the value for the #connection_klass method. For example:

      DatabaseCleaner[:active_record,{:model => 'OtherDatabaseObject'}]
      DatabaseCleaner[:active_record,{:model => 'ActiveRecord::Base'}]
      DatabaseCleaner.strategy = :transaction

In this case, DatabaseCleaner would effectively call OtherDatabaseObject.connection.begin_db_transaction and ActiveRecord::Base.connection.begin_db_transaction when DatabaseCleaner.start was called.

Of course, the user is responsible for calling OtherDatabaseObject.establish_connection, but that has to be done for the app to work anyway.

If this is something you think you may incorporate into DatabaseCleaner, please let me know. I'd love to help. The only reason I haven't made a merge request is that I haven't been able to get the tests passing.

bmabey commented 12 years ago

Hi @thoughtless, thanks for your comments. I apologize for not getting back to you sooner. After looking at this some more I agree that your approach of sending the model (or just the model name) is the better approach. I'm still investigating other options but I think this approach makes the most sense. Thanks again for sharing your findings. I'll let you know when I have resolved the issue.

bmabey commented 12 years ago

see #74 for details on what I ended up doing.. it isn't merged into master yet and I'll need to do a point release since this will be breaking the API.

fillman commented 12 years ago

Have this got into any tag or branch?

bmabey commented 12 years ago

No, this has not been released if that is your question.. Since it is a comatability breaking change I intend to release it as v1.0.0, but before I do that I am going to issue a point release with deprecation warnings. I started on this with a local branch but kept running into odd instance variable module issues. I'll take a look at it again this week to try to get the point release out. For now please use bundler to point to this ref for the needed functionality.

toomanyjoes commented 2 years ago

I am having a problem in my app and this issue seems like it could be related. I cannot tell if this was ever merged and published. Is this fix in the current version of the gem?