public
Description: Ruby on Rails
Homepage: http://rubyonrails.org
Clone URL: git://github.com/rails/rails.git
Add Model#delete instance method, similar to Model.delete class method. [#1086 
state:resolved]

Signed-off-by: Pratik Naik <pratiknaik@gmail.com>
Hongli Lai (Phusion (author)
Sun Sep 21 14:01:32 -0700 2008
lifo (committer)
Sun Sep 21 14:53:44 -0700 2008
commit  46939a9b5a0098fddeac99a8a4331f66bdd0710e
tree    8e233668c2bac790bd54b50d664f813903f199f6
parent  5f83e1844c83c19cf97c6415b943c6ec3cb4bb06
...
1
2
 
 
3
4
5
...
1
2
3
4
5
6
7
0
@@ -1,5 +1,7 @@
0
 *Edge*
0
 
0
+* Add Model#delete instance method, similar to Model.delete class method. #1086 [Hongli Lai]
0
+
0
 * MySQL: cope with quirky default values for not-null text columns.  #1043 [Frederick Cheung]
0
 
0
 * Multiparameter attributes skip time zone conversion for time-only columns [#1030 state:resolved] [Geoff Buesing]
...
1470
1471
1472
1473
 
1474
1475
1476
...
1500
1501
1502
1503
 
1504
1505
1506
...
1470
1471
1472
 
1473
1474
1475
1476
...
1500
1501
1502
 
1503
1504
1505
1506
0
@@ -1470,7 +1470,7 @@ module ActiveRecord
0
                 method_name = "has_one_dependent_delete_for_#{reflection.name}".to_sym
0
                 define_method(method_name) do
0
                   association = send(reflection.name)
0
-                  association.class.delete(association.id) unless association.nil?
0
+                  association.delete unless association.nil?
0
                 end
0
                 before_destroy method_name
0
               when :nullify
0
@@ -1500,7 +1500,7 @@ module ActiveRecord
0
                 method_name = "belongs_to_dependent_delete_for_#{reflection.name}".to_sym
0
                 define_method(method_name) do
0
                   association = send(reflection.name)
0
-                  association.class.delete(association.id) unless association.nil?
1
+                  association.delete unless association.nil?
0
                 end
0
                 before_destroy method_name
0
               else
...
2306
2307
2308
 
 
 
 
 
 
 
 
 
 
2309
2310
2311
...
2306
2307
2308
2309
2310
2311
2312
2313
2314
2315
2316
2317
2318
2319
2320
2321
0
@@ -2306,6 +2306,16 @@ module ActiveRecord #:nodoc:
0
 
0
       # Deletes the record in the database and freezes this instance to reflect that no changes should
0
       # be made (since they can't be persisted).
0
+      #
0
+      # Unlike #destroy, this method doesn't run any +before_delete+ and +after_delete+
0
+      # callbacks, nor will it enforce any association +:dependent+ rules.
0
+      def delete
0
+        self.class.delete(id) unless new_record?
0
+        freeze
0
+      end
0
+
0
+      # Deletes the record in the database and freezes this instance to reflect that no changes should
0
+      # be made (since they can't be persisted).
0
       def destroy
0
         unless new_record?
0
           connection.delete <<-end_sql, "#{self.class.name} Destroy"
...
472
473
474
 
 
 
 
 
 
 
 
 
 
 
 
475
476
477
...
820
821
822
 
 
 
 
 
 
 
 
 
 
 
 
 
 
823
824
825
...
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
...
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
0
@@ -472,6 +472,18 @@ class BasicsTest < ActiveRecord::TestCase
0
     assert topic.instance_variable_get("@custom_approved")
0
   end
0
 
0
+  def test_delete
0
+    topic = Topic.find(1)
0
+    assert_equal topic, topic.delete, 'topic.delete did not return self'
0
+    assert topic.frozen?, 'topic not frozen after delete'
0
+    assert_raise(ActiveRecord::RecordNotFound) { Topic.find(topic.id) }
0
+  end
0
+
0
+  def test_delete_doesnt_run_callbacks
0
+    Topic.find(1).delete
0
+    assert_not_nil Topic.find(2)
0
+  end
0
+
0
   def test_destroy
0
     topic = Topic.find(1)
0
     assert_equal topic, topic.destroy, 'topic.destroy did not return self'
0
@@ -820,6 +832,20 @@ class BasicsTest < ActiveRecord::TestCase
0
     assert_equal [ Topic.find(1) ], [ Topic.find(2).topic ] & [ Topic.find(1) ]
0
   end
0
 
0
+  def test_delete_new_record
0
+    client = Client.new
0
+    client.delete
0
+    assert client.frozen?
0
+  end
0
+
0
+  def test_delete_record_with_associations
0
+    client = Client.find(3)
0
+    client.delete
0
+    assert client.frozen?
0
+    assert_kind_of Firm, client.firm
0
+    assert_raises(ActiveSupport::FrozenObjectError) { client.name = "something else" }
0
+  end
0
+
0
   def test_destroy_new_record
0
     client = Client.new
0
     client.destroy

Comments

This seems like a significant de-optimization. The code does look a bit prettier, but is that worth the cost of loading the record’s fields and instantiating a model object just to do nothing with it?

adkron Sun Sep 21 20:17:38 -0700 2008

Josh, I’m not sure there is a new record being loaded. If you notice before it was association.class.delete now it is association.delete. It appears to me that the class was already loaded.

joshsusser Sun Sep 21 20:35:51 -0700 2008

There is a difference between loading a class and loading a record. But anyway, we talked this over in #rails-contrib. I was assuming AR was being smart and not loading the record to delete it via the association, but that’s not what’s going on. This patch actually makes it easier to add that optimization to AssociationProxy in the future, so it’s actually a win there too.

adkron Sun Sep 21 20:38:22 -0700 2008

I meant ‘it appeared that the record was already loaded.’