This project is archived and is in readonly mode.

#5112 ✓hold
Neeraj Singh

if destroy is called on a new object then exception should be raised

Reported by Neeraj Singh | July 14th, 2010 @ 05:43 PM | in 3.x

As discussed with José Valim calling destroy on a new object should raise exception.

Comments and changes to this ticket

  • Neeraj Singh

    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.

  • Ivan Torres (mexpolk)

    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

    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

    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.

  • Ivan Torres (mexpolk)

    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

    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

    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!

  • Ivan Torres (mexpolk)

    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

    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!

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Pages