#5112 if destroy is called on a new object then exception should be raised - Ruby on Rails

Neeraj Singh

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

July 14th, 2010 @ 09:28 PM

Ivan Torres (mexpolk)

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

July 14th, 2010 @ 09:28 PM

Neeraj Singh

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. :-)

July 14th, 2010 @ 09:33 PM

José Valim

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.

July 14th, 2010 @ 09:41 PM

Ivan Torres (mexpolk)

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

July 14th, 2010 @ 09:43 PM

Neeraj Singh

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 :-)

July 18th, 2010 @ 03:50 PM

José Valim

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!

July 18th, 2010 @ 04:23 PM

Ivan Torres (mexpolk)

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?

July 28th, 2010 @ 11:03 PM

José Valim

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!

August 2nd, 2010 @ 03:29 PM

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

Activity