Explicit transactions not rolling back when validation fails on nested attributes

[lcreid]

In certain situations, when a validation fails on a model referenced in nested attributes, when the update attributes is executed inside an explicit transaction, the transaction is not rolled back, and the database is left in a state that's different than it was before the transaction.

Example:

In the controller:

    User.transaction do
      user.update(email: nil, user_setting_attributes: {favourite_color: 'red'})
    end

Note that user.update! does not cause the issue.

The models:

class User < ActiveRecord::Base
  has_one :user_setting
  accepts_nested_attributes_for :user_setting
  validates_presence_of :email
end

class UserSetting < ActiveRecord::Base
  belongs_to :user
end

A complete file that shows a case that causes, and some cases that are similar but don't cause the issue:

unless File.exist?('Gemfile')
  File.write('Gemfile', <<-GEMFILE)
source 'https://rubygems.org'
gem 'activerecord', '4.1.0'
gem 'sqlite3'
GEMFILE

  system 'bundle'
end

require 'bundler'
Bundler.setup(:default)

# Activate the gem you are reporting the issue against.
require 'active_record'
require 'minitest/autorun'
require 'logger'

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :users do |t|
    t.string :email
  end

  create_table :user_settings do |t|
    t.integer :user_id
    t.string :favourite_color
  end
end

class User < ActiveRecord::Base
  has_one :user_setting
  accepts_nested_attributes_for :user_setting
  validates_presence_of :email
end

class UserSetting < ActiveRecord::Base
  belongs_to :user
end

class BugTest < Minitest::Test
  def test_update_without_transaction
    user = User.create!(email: 'godfrey@example.com', user_setting_attributes: {favourite_color: 'orange'})
    assert_equal 'orange', user.user_setting.favourite_color

    assert ! user.update(email: nil, user_setting_attributes: {favourite_color: 'red'})

    user.reload

    assert user.user_setting.present?
    assert_equal 'orange', user.user_setting.favourite_color
  end

  def test_update_with_transaction
    user = User.create!(email: 'godfrey@example.com', user_setting_attributes: {favourite_color: 'orange'})
    assert_equal 'orange', user.user_setting.favourite_color

    User.transaction do
      assert ! user.update(email: nil, user_setting_attributes: {favourite_color: 'red'})
    end

    user.reload

    assert user.user_setting.present?
    assert_equal 'orange', user.user_setting.favourite_color
  end

  def test_update_without_transaction!
    user = User.create!(email: 'godfrey@example.com', user_setting_attributes: {favourite_color: 'orange'})
    assert_equal 'orange', user.user_setting.favourite_color

    assert_raises(ActiveRecord::RecordInvalid) do
      user.update!(email: nil, user_setting_attributes: {favourite_color: 'red'})
    end

    user.reload

    assert user.user_setting.present?
    assert_equal 'orange', user.user_setting.favourite_color
  end

  def test_update_with_transaction!
    user = User.create!(email: 'godfrey@example.com', user_setting_attributes: {favourite_color: 'orange'})
    assert_equal 'orange', user.user_setting.favourite_color

    assert_raises(ActiveRecord::RecordInvalid) do
      User.transaction do
        assert ! user.update!(email: nil, user_setting_attributes: {favourite_color: 'red'})
      end
    end

    user.reload

    assert user.user_setting.present?
    assert_equal 'orange', user.user_setting.favourite_color
  end
end

If the above is in a file called repro.rb, then:

ruby repro.rb

should produce output ending in:

4 runs, 15 assertions, 1 failures, 0 errors, 0 skips

The issues has been observed in 4.1 master and 4.0 master. I haven't yet checked how far back it might go.

The issue was shown to my by @chancancode . I'm documenting the issue here without much further research yet because I didn't want it to get dropped.

[chancancode]

Ah. Thanks for looking into this @lcreid. I dug a bit deeper and found the documentation I was looking for the other day. It turns out that this is indeed expected and documented behaviour (see the example in the docs I linked). And it also turns out that I have been bitten by this more than once (see #12944, also #4566), so clearly this is super confusing 😅

I'll look into the reason behind this (performance maybe?) and see if anything can be changed here in Rails 5.0, either make requires_new the default or perhaps make inner transaction blocks propagate the exceptions.

Closing for now, but thanks again for looking into this with me!

[chancancode]

I'm actually a bit suspicious of the correctness of the documentation I linked, I think this might have been valid – I'll try to get a more definitive answer

[matthewd]

@chancancode FWIW, I agree... that sounds more like it's documenting existing behaviour, rather than intended.

[chancancode]

So it all started with this docrails commit – 53bbbcc, which the documented (but perhaps buggy) behaviour at that time to what we have now.

@evtuhovich, I know this is a long time ago, but do you remember the back story for this change? Have you confirmed that the behaviour you documented is intended? Thanks in advance 😀

[evtuhovich]

I remember this bug, i spent one day to find a solution.

This was default behaviour for nested transaction, because not all databases support it. I only found this and document. So, if you want consistent nested transaction, always use Model.transaction(:requires_new => true)

[chancancode]

I believe the intention was that if you don't use requires_new it should rollback the outermost block, whereas if you do it will rollback that specific block - that was the originally documented behavior. Have you verified with others that the behavior you documented is intened (as opposed to just a buggy implementation)?

[evtuhovich]

I also was sure, that rollback should rollback outremost block. But this is wrong. When you rollback inner transaction nothing happens, outer transaction don't get rollback and continue its execution.

And this is why a spent a day to understand that problem in my project.

[chancancode]

Right, if I understand you correctly, you are saying the documentation (at the time) didn't agree with the actual behaviour of the code (at the time), correct? But in that case, it could be either the code or the documentation that is wrong. In this case I suspect it's the former – which is why I asked if you have confirmed this behaviour is desirable (for example, filed a bug on this and was told by a core team member that the docs are wrong).

I'm trying to find those back stories to support this doc change, because it appears otherwise that the code should have been fixed instead.

[evtuhovich]

I don't think, this is desireble behaviour, i've just documented the state of code at that time, and i hope, this helps other people.

And i sure the code should be fixed, of course.

[chancancode]

Thanks for the update! 😄

For the future, please file a bug report for these kind of things – the documentation should reflect how the code should behave, so we shouldn't change the documentation when the code misbehave. This will cause other people to make incorrect inference about the code and result in things like issues incorrectly being closed, or worse – more code being changed to align itself with the buggy behaviour.

[chancancode]

cc @fxn

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

[robzolkos]

@rafaelfranca @fxn @chancancode I think this issue can be closed as it was addressed in aa59230

(not sure why it didn't autoclose)

[guigs]

@robzolkos The commit you mentioned was not merged. See #15017.

[robbyrussell]

Given this has been around for nearly a decade, perhaps it's safe to close

[rafaelfranca]

The issue is still here. We just didn't have time to fix yet. If someone wants to fix this please go ahead.

[lcreid]

A blast from the past. I'll take a look at this, perhaps this weekend.

[lcreid]

Just to be clear what the desired behaviour is:

When an object has nested attributes, a failure to save the object or any of the objects affected by the nested attributes should leave the database in the state it was before the attempt to save the object. Said another way, the logical transaction should start on entering the save, and should be rolled back to that point on any failure to save.

[lcreid]

The above should actually be true for any autosaved associations, not just those that are implied by accepts_nested_attributes_for.

[lcreid]

@irvingreid and I discussed this issue for a couple of hours. We see three approaches, not exclusive:

1. Make the documentation clearer about what a developer should do to achieve what is probably their desired outcome. To wit, when using the non-! versions of the persistence methods within an explicit transaction, then it's the responsibility of the developer to ensure that a rollback is raised, or data may be persisted that they're not expecting.
2. Write a warning message in the logs when a transaction is committed that had a failure in a persistence method that was called within the transaction.
3. Actually change the code to make the situation less likely to be the foot-gun that it is today. It looks challenging to come up with such a solution, and would probably require some experimentation of different approaches. The existing approach solves some important problems. For example, it minimises or eliminates unnecessary savepoint creation and rollback.

We're going to continue to investigate this issue and try to come up with some more concrete solutions.