Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• 

  Neeraj Singh July 14th, 2010 @ 09:28 PM

  • State changed from “new” to “open”
  • Assigned user changed from “Neeraj Singh” to “José Valim”

  Attached is patch with test.

  • 5112_fix_for_edge.diff 2 KB

• You flagged this item as spam.
  

  Ivan Torres (mexpolk) July 14th, 2010 @ 09:28 PM

  • Tag changed from “rails 3, activerecord” to “rails 3, activerecord, delete, delete_all, persistence”
  • Assigned user changed from “José Valim” to “Neeraj Singh”

  Hi Neeraj,

  I'm not that sure about that. Right now what AR does is to check if the record is persisted before trying to reach the database. That's why it does not throw and exception.

  But weather the record is persisted or not, the record still has to be freezed to avoid further changes nor trying to persist it later.

  In addition to that suppose that you have a collection of nested attributes, and your are not sure if some of those are already persisted on the db. What if you send a bulk delete (delete_all(ids)) to those records? is the action supposed to throw an exception?

  This is the actual code:

  
      def delete
        self.class.delete(id) if persisted?
        @destroyed = true
        freeze
      end
  
      def destroy
        if persisted?
         self.class.unscoped.where(self.class.arel_table[self.class.primary_key].eq(id)).delete_all
        end
  
        @destroyed = true
        freeze
      end
  

• 

  Neeraj Singh July 14th, 2010 @ 09:33 PM

  I am on the fence on this one. No strong opinion either way.

  This came up during a discussion with José Valim while discussing some other issue. I'll let him decide. :-)

• 

  José Valim July 14th, 2010 @ 09:41 PM

  If Rails provides a interface to delete all related objects like in associations, it is Rails responsibility to call delete/destroy only in the persisted records so it won't raise an error (and probably freezing all the others). In any case, I feel the case Ivan described is quite unlikely to happen.

• You flagged this item as spam.
  

  Ivan Torres (mexpolk) July 14th, 2010 @ 09:43 PM

  Yes, I know that my case is unlikely to happen. But I rather prefer not have to deal with additional validations @post.delete if @post.persisted?

  but is up to you.

  Thanks

• 

  Neeraj Singh July 18th, 2010 @ 03:50 PM

  • Assigned user changed from “Neeraj Singh” to “José Valim”

  I am fine with marking this ticket as 'wontfix'.

  Assigning it to Mr. Valim . It's his call now :-)

• 

  José Valim July 18th, 2010 @ 04:23 PM

  I think most of the time, if you are calling destroy in a not persisted record, you would like to know about it.

  Neeraj, someone raised an issue about nested attributes may try to destroy invalid records. Basically, if you send both new attributes and the _destroy field, nested attributes should not call destroy. Can you add a test or ensure we already have one? It can be done in another patch!

  Thanks!

• You flagged this item as spam.
  

  Ivan Torres (mexpolk) July 28th, 2010 @ 11:03 PM

  I'm sorry to be such a pain about this, but...

  I think that to keep consistency across the framework, I rather suggest using destroy! (with a bang) if you need an exception thrown. This behavior is the same that you can find on save vs. save! update_attributes vs. update_attributes! etc.

  ¿Don't you think?

• 

  José Valim August 2nd, 2010 @ 03:29 PM

  • State changed from “open” to “hold”

  Ok, I'm postponing this discussion since we cannot change the API after RC. Sorry and thanks for your work Neeraj!