Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Restore state on create when ActiveRecord::RecordInvalid is raised #6073

Merged
merged 1 commit into from

6 participants

@daveyeu

This fixes issue #3217.

Previously, a call to AR#create that raised AR::RecordInvalid would leave a record in an inconsistent state -- it wouldn't actually be saved in the database, but #persisted? => true and #id would be non-nil. This patch also merges clean into 3-2-stable, for now.

@jeremy
Owner

:+1:

@schneems
Collaborator

No comment or activity for 3 months. Change looks good to me, are there any complications that would prevent it from being merged in?

@rafaelfranca

@daveyeu could you add a CHANGELOG entry. I'll merge this one.

@rafaelfranca rafaelfranca was assigned
@daveyeu

There ya go @rafaelfranca, thanks!

@steveklabnik
Collaborator

uh oh, this needs a rebase! Darn CHANGELOGs.

@daveyeu

Do you mean I should squash the commits? Or rebase against rails HEAD? Or both?

Sorry, not too familiar with the protocol, and I hate to force push anything that's public, but I'm happy to if that's the way it goes.

@rafaelfranca

We need a rebase against master. But rebase + squash would be better.

@daveyeu

Ok, rebased and squashed. Rakes green against master too.

@rafaelfranca rafaelfranca merged commit 15184d8 into from
@rafaelfranca

Done. Thank you

@jaredbeck

Given a Herp that has_many :derps, :through => :herp_derps, the following FactoryGirl.create raises no exceptions in rails 3.2.8:

h = create(:herp, derps: [create(:derp)])

In 3.2.9 it raises ActiveRecord::RecordInvalid, and must be re-written in two steps:

h = create(:herp)
h.derps << create(:derp)

@daveyeu, could this change in behavior be the result of this commit? Sorry if this question doesn't belong here. I'm just trying to understand the change. Thanks!

I doubt it? If the caller is doing some inspection of the object's transaction state in order to determine a re-raise, then maybe, but that seems unlikely. I'll take a closer look later when I have a sec.

Is there any more to that exception? A message? And what version of FactoryGirl are you pegged to?

It's not a big deal. Like I said, there's a workaround, so don't feel like you have to look into it, especially if you think it's unrelated.

The exception message was something like "ActiveRecord::RecordInvalid: Herp derps is invalid". I'm using factory_girl 4.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 12, 2012
  1. @daveyeu
This page is out of date. Refresh to see the latest.
View
5 activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Fix AR#create to return an unsaved record when AR::RecordInvalid is
+ raised. Fixes #3217.
+
+ *Dave Yeu*
+
* Fixed table name prefix that is generated in engines for namespaced models
*Wojciech Wnętrzak*
View
2  activerecord/lib/active_record/transactions.rb
@@ -327,7 +327,7 @@ def clear_transaction_record_state #:nodoc:
def restore_transaction_record_state(force = false) #:nodoc:
if defined?(@_start_transaction_state)
@_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) - 1
- if @_start_transaction_state[:level] < 1
+ if @_start_transaction_state[:level] < 1 || force
restore_state = remove_instance_variable(:@_start_transaction_state)
was_frozen = @attributes.frozen?
@attributes = @attributes.dup if was_frozen
View
17 activerecord/test/cases/transactions_test.rb
@@ -204,6 +204,23 @@ def test_callback_rollback_in_create
end
end
+ def test_callback_rollback_in_create_with_record_invalid_exception
+ begin
+ Topic.class_eval <<-eoruby, __FILE__, __LINE__ + 1
+ remove_method(:after_create_for_transaction)
+ def after_create_for_transaction
+ raise ActiveRecord::RecordInvalid.new(Author.new)
+ end
+ eoruby
+
+ new_topic = Topic.create(:title => "A new topic")
+ assert !new_topic.persisted?, "The topic should not be persisted"
+ assert_nil new_topic.id, "The topic should not have an ID"
+ ensure
+ remove_exception_raising_after_create_callback_to_topic
+ end
+ end
+
def test_nested_explicit_transactions
Topic.transaction do
Topic.transaction do
Something went wrong with that request. Please try again.