This project is archived and is in readonly mode.

#3045 ✓stale
Ben Alavi

nested_attributes doesn't rollback parent when before_save/after_save callbacks fail

Reported by Ben Alavi | August 12th, 2009 @ 09:08 PM | in 2.3.10

When saving an object and nested models validation errors on the nested models cause the entire save to be aborted. However, if a nested model returns false in a before_save callback, or raises an exception in an after_save callback only the nested model remains unsaved while the parent is still saved.

The related documentation for AutosaveAssociation (http://api.rubyonrails.org/classes/ActiveRecord/AutosaveAssociation...) only mentions that the autosaved associations will fail on validation, and doesn't mention whether they should fail on callback failures.

This seems inconsistent, at least, with the documentation for ActiveRecord::Callbacks which state that they will rollback the entire transaction.

I've tested by starting a new project, freezing in edge Rails and then setting this up:

class CreateUsersAndWidgets < ActiveRecord::Migration
  def self.up
    create_table :users do |t|
      t.string :name
    end

    create_table :widgets do |t|
      t.integer :user_id
      t.string :name
    end
  end

  def self.down
  end
end

app/models/user.rb

class User < ActiveRecord::Base
  has_many :widgets

  accepts_nested_attributes_for :widgets
end

app/models/widget.rb

class Widget < ActiveRecord::Base  
  attr_accessor :fail
  belongs_to :user

  before_save :cause_before_save_problems
  after_save :cause_after_save_problems
  validate :cause_validation_problems

  def cause_validation_problems
    errors.add(:fail, 'in validation') if fail == :in_validation
  end

  def cause_before_save_problems
    return false if fail == :before_save
  end

  def cause_after_save_problems
    raise ActiveRecord::Rollback if fail == :after_save
  end
end

test/unit/user_test.rb

require File.join(File.dirname(__FILE__), '..', 'test_helper')

class UserTest < ActiveSupport::TestCase
  self.use_transactional_fixtures = false 

  def test_user_rolls_back_when_widget_validation_fails
    @user = User.create :name => 'Foo', :widgets_attributes => { '0' => { :name => 'Bar', :fail => :in_validation } }

    assert @user.widgets.first.new_record?
    assert @user.new_record?
  end

  def test_user_rolls_back_when_widget_before_save_fails
    @user = User.create :name => 'Foo', :widgets_attributes => { '0' => { :name => 'Bar', :fail => :before_save } }

    assert @user.widgets.first.new_record?
    assert @user.new_record?
  end

  def test_user_rolls_back_when_widget_after_save_fails
    @user = User.create :name => 'Foo', :widgets_attributes => { '0' => { :name => 'Bar', :fail => :after_save } }

    assert @user.widgets.first.new_record?
    assert @user.new_record?
  end
end

Here is the test run output:

Loaded suite test/unit/user_test
Started
FF.
Finished in 0.044073 seconds.

  1) Failure:
test_user_rolls_back_when_widget_after_save_fails(UserTest) [/test/unit/user_test.rb:23]:
<false> is not true.

  2) Failure:
test_user_rolls_back_when_widget_before_save_fails(UserTest) [/test/unit/user_test.rb:17]:
<false> is not true.

3 tests, 5 assertions, 2 failures, 0 errors

And the related log for all 3 tests (running against PostgreSQL):

SQL (0.1ms)   SET client_min_messages TO 'panic'
SQL (0.1ms)   SET client_min_messages TO 'notice'
SQL (0.1ms)   BEGIN
SQL (0.5ms)   INSERT INTO "users" ("name") VALUES(E'Foo') RETURNING "id"
SQL (0.3ms)   INSERT INTO "widgets" ("name", "user_id") VALUES(E'Bar', 5) RETURNING "id"
SQL (1.3ms)   COMMIT
SQL (0.1ms)   BEGIN
SQL (0.2ms)   INSERT INTO "users" ("name") VALUES(E'Foo') RETURNING "id"
SQL (0.8ms)   COMMIT
SQL (0.1ms)   BEGIN
SQL (0.1ms)   ROLLBACK

With some digging around I've found that in database_statement.rb when the ActiveRecord::Rollback exception is raised by the after_save or before_save callbacks in the child, the value of transaction_open is false so no rollback is triggered. With the significant chain of events that happen before this it's hard to tell if that behavior is intended or not.

I'd dig more but the task seems daunting and I thought I would check if anybody knows of a specific reason for this being the intended behavior?

If this is the intended behavior it seems counter to what you'd expect from the documentation for ActiveRecord::Callbacks so it probably deserves a note in the docs at least.

Comments and changes to this ticket

  • Eloy Duran

    Eloy Duran December 30th, 2009 @ 07:42 PM

    • Assigned user set to “Eloy Duran”
    • Milestone set to 2.3.6
  • Eloy Duran

    Eloy Duran January 7th, 2010 @ 04:18 PM

    • State changed from “new” to “incomplete”

    This is not intended, please do investigate further.

  • Eloy Duran

    Eloy Duran January 7th, 2010 @ 04:21 PM

    • Tag changed from 2.x, active_record, callbacks, nested_attributes_for to 2.x, active_record, callbacks

    Also it doesn't really seem to have to do with nested attributes, but more with the association callbacks and possible some autosave association code.

  • Rizwan Reza

    Rizwan Reza May 16th, 2010 @ 02:41 AM

    • Tag changed from 2.x, active_record, callbacks to 2.x, active_record, bugmash, callbacks
  • Jeremy Kemper

    Jeremy Kemper May 23rd, 2010 @ 05:54 PM

    • Milestone changed from 2.3.6 to 2.3.7
  • Jeremy Kemper

    Jeremy Kemper May 24th, 2010 @ 09:40 AM

    • Milestone changed from 2.3.7 to 2.3.8
  • Jeremy Kemper

    Jeremy Kemper May 25th, 2010 @ 11:45 PM

    • Milestone changed from 2.3.8 to 2.3.9
  • Eloy Duran

    Eloy Duran July 8th, 2010 @ 08:39 AM

    • Assigned user changed from “Eloy Duran” to “José Valim”
    • Importance changed from “” to “”
  • Neeraj Singh

    Neeraj Singh July 8th, 2010 @ 11:28 AM

    • Assigned user changed from “José Valim” to “Neeraj Singh”

    I am working on #5053. Will take a look at this ticket too . For the time being assigning it to myself.

  • Neeraj Singh

    Neeraj Singh July 9th, 2010 @ 04:16 PM

    @Ben

    You have following callback.

    def cause_after_save_problems
      raise ActiveRecord::Rollback if fail == :after_save
    end
    

    I am able to reproduce this problem with above callback.

    However problem goes away if I change the method to this

    def cause_after_save_problems
      raise 'boom' if fail == :after_save
    end
    

    looking at code I found following snippet

    rescue Exception => database_transaction_rollback
      if transaction_open && !outside_transaction?
        transaction_open = false
        decrement_open_transactions
        if open_transactions == 0
          rollback_db_transaction
        else
          rollback_to_savepoint
        end
      end
      raise unless database_transaction_rollback.is_a?(ActiveRecord::Rollback)
    end
    

    This is one place I found where my code will behave differently from yours. In mycase database_transaction_rollback will be a RuntimeError and in your case it will be ActiveRecord::Rollback. And that influences this piece of code

    raise unless database_transaction_rollback.is_a?(ActiveRecord::Rollback)
    
  • Neeraj Singh

    Neeraj Singh July 18th, 2010 @ 03:47 PM

    • Assigned user cleared.
  • Jeremy Kemper

    Jeremy Kemper August 30th, 2010 @ 02:28 AM

    • Milestone changed from 2.3.9 to 2.3.10
  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:32 PM

    • State changed from “incomplete” to “open”
    • Tag changed from 2.x, active_record, bugmash, callbacks to 2x, active_record, bugmash, callbacks

    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 core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • Santiago Pastorino

    Santiago Pastorino February 2nd, 2011 @ 04:32 PM

    • State changed from “open” to “stale”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages