This project is archived and is in readonly mode.
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 December 30th, 2009 @ 07:42 PM
- Assigned user set to “Eloy Duran”
- Milestone set to “2.3.6”
-
Eloy Duran January 7th, 2010 @ 04:18 PM
- State changed from “new” to “incomplete”
This is not intended, please do investigate further.
-
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 May 16th, 2010 @ 02:41 AM
- Tag changed from “2.x, active_record, callbacks” to “2.x, active_record, bugmash, callbacks”
-
-
-
-
Eloy Duran July 8th, 2010 @ 08:39 AM
- Assigned user changed from “Eloy Duran” to “José Valim”
- Importance changed from “” to “”
-
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 July 9th, 2010 @ 04:16 PM
@Ben
You have following callback.
def cause_after_save_problems raise ActiveRecord::Rollback if fail == :after_save endI 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 endlooking 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) endThis 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 July 18th, 2010 @ 03:47 PM
- Assigned user cleared.
-
-
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 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>
nick (at codevader)
Akira Matsuda
Dmitry Polushkin
Eloy Duran
Jeremy Kemper
José Valim
Neeraj Singh