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

Merged
merged 1 commit into from Aug 12, 2012

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.

@rafaelfranca
Ruby on Rails member
@jeremy
Ruby on Rails member

👍

@schneems
Ruby on Rails member

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
Ruby on Rails member

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

@rafaelfranca rafaelfranca was assigned Aug 11, 2012
@daveyeu

There ya go @rafaelfranca, thanks!

@steveklabnik
Ruby on Rails member

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
Ruby on Rails member

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 rails:master Aug 12, 2012
@rafaelfranca
Ruby on Rails member

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