makandra / active_type

Make any Ruby object quack like ActiveRecord
MIT License
1.09k stars 74 forks source link

Rails 5.0.7.2 Rollbacks on Dummy DB Connection instead of ActiveRecord::Base.connection #114

Closed jy-pike13 closed 3 years ago

jy-pike13 commented 4 years ago

Thank you so much for continuing to update this project!

I've been running into issues where failed saves to an ActiveType::Object happens on the Dummy DB Connection in Rails 5. Previously in Rails 4, a failed save on an ActiveType::Object would result in a ActiveRecord::Rollback exception which would get rescued and execute a ROLLBACK to the mysql database. For example if I have this Creator class to create a bookshelf, then fill it with books, but it happens that creating books has an error, the book_shelf would be rolled back.

class Creator < ActiveType::Object
  before_save :perform

  def perform
    @book_shelf = BookShelfBuilder.new
    if @book_shelf.save
      create_books
    else
      @book_shelf.errors.full_messages.each { |message| errors[:invoice] << message }
    end

    errors.blank?
  end
end

When trying to upgrade to Rails 5, because the connection used by ActiveType::Object is the DummyConnection object, the Rollback doesn't get sent to our mysql database, and we now unexpectedly still have a BookShelf row.

We are using ActiveType 0.7.5, but I've had the same issue upgrading to 1.3.0.

Is there anything that you recommend for this specific issue from upgrading Rails 4 to 5? And is there any more information I can provide?

Thanks so much!

triskweline commented 4 years ago

ActiveType is only tested with Rails 5.2, so I recommend retrying with that version first.

kwmcminn commented 4 years ago

ActiveType is only tested with Rails 5.2, so I recommend retrying with that version first.

@triskweline We built a simple Rails 5 App to see if 5.2 would solve this issue. We first tried on 5.0, then 5.1, and 5.2 and all produced the same result(not rolling back the transaction on a failed save). I went ahead and created an identical Rails 4 App, where the same code worked as intended.

You can see Rails 4 & 5 versions below with an identical tests that passes in 4.2.11.1 and fails in 5.2.4.1.

Rails 5: https://github.com/kwmcminn/ActiveType-Issue-Repro-Rails5 Rails 4: https://github.com/kwmcminn/ActiveType-Issue-Repro-Rails4

You can find the test files below: Rails 5: https://github.com/kwmcminn/ActiveType-Issue-Repro-Rails5/blob/master/test/models/dog_creator_test.rb Rails 4: https://github.com/kwmcminn/ActiveType-Issue-Repro-Rails4/blob/master/test/models/cat_creator_test.rb

kratob commented 3 years ago

We've taken a deeper look into this, but feel it's not quite clear how to handle this. The observation that the DummyConnection will catch the rollback is true, but the only alternative is that ActiveType::Object opens an actual database transaction (which happens in Rails 4 more or less by accident), where my expectation would be that ActiveType::Object never talks to the database at all.

It's also not a 100% clear on which database connection to actually perform the rollback, since these days there can be more than one. One could opt into defaulting to ActiveRecord::Base.connection, but this seems like a hack as well.

We decided to keep it as it is, even if it can be unexpected. It's relatively simple to manually wrap the relevant code into a database transaction, as a solution. I'll document this behaviour in the README.