Dirty attributes aren't cleared if save fails. [#174 state:resolved]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>

3 files changed, 30 insertions, 9 deletions

diff --git a/activerecord/lib/active_record/dirty.rb b/activerecord/lib/active_record/dirty.rb
index c6d89e3a05..6034963811 100644
--- a/activerecord/lib/active_record/dirty.rb
+++ b/activerecord/lib/active_record/dirty.rb
@@ -69,19 +69,19 @@ module ActiveRecord
       changed.inject({}) { |h, attr| h[attr] = attribute_change(attr); h }
     end
 
-
-    # Clear changed attributes after they are saved.
+    # Attempts to +save+ the record and clears changed attributes if successful.
     def save_with_dirty(*args) #:nodoc:
-      save_without_dirty(*args)
-    ensure
-      changed_attributes.clear
+      if status = save_without_dirty(*args)
+        changed_attributes.clear
+      end
+      status
     end
 
-    # Clear changed attributes after they are saved.
+    # Attempts to <tt>save!</tt> the record and clears changed attributes if successful.
     def save_with_dirty!(*args) #:nodoc:
-      save_without_dirty!(*args)
-    ensure
+      status = save_without_dirty!(*args)
       changed_attributes.clear
+      status
     end
 
     private
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index 7412e63872..1266eb5036 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -78,7 +78,7 @@ class DirtyTest < ActiveRecord::TestCase
   end
 
   def test_association_assignment_changes_foreign_key
-    pirate = Pirate.create!
+    pirate = Pirate.create!(:catchphrase => 'jarl')
     pirate.parrot = Parrot.create!
     assert pirate.changed?
     assert_equal %w(parrot_id), pirate.changed
@@ -115,6 +115,18 @@ class DirtyTest < ActiveRecord::TestCase
     end
   end
 
+  def test_changed_attributes_should_be_preserved_if_save_failure
+    pirate = Pirate.new
+    pirate.parrot_id = 1
+    assert !pirate.save
+    check_pirate_after_save_failure(pirate)
+
+    pirate = Pirate.new
+    pirate.parrot_id = 1
+    assert_raises(ActiveRecord::RecordInvalid) { pirate.save! }
+    check_pirate_after_save_failure(pirate)
+  end
+
   private
     def with_partial_updates(klass, on = true)
       old = klass.partial_updates?
@@ -123,4 +135,11 @@ class DirtyTest < ActiveRecord::TestCase
     ensure
       klass.partial_updates = old
     end
+
+    def check_pirate_after_save_failure(pirate)
+      assert pirate.changed?
+      assert pirate.parrot_id_changed?
+      assert_equal %w(parrot_id), pirate.changed
+      assert_nil pirate.parrot_id_was
+    end
 end
diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb
index bb4d02c10f..51c8183dee 100644
--- a/activerecord/test/models/pirate.rb
+++ b/activerecord/test/models/pirate.rb
@@ -4,4 +4,6 @@ class Pirate < ActiveRecord::Base
   has_many :treasures, :as => :looter
 
   has_many :treasure_estimates, :through => :treasures, :source => :price_estimates
+
+  validates_presence_of :catchphrase
 end