destroy w/ params and :dependent => destroy

rubyonrails-talk
#1

Hello,

I'm having trouble debugging an override of destroy. I have an object
that in some cases I need to send a parameter when I destroy it. Usually
not though. So I've defined destroy in the model class like this:

class Look < ActiveRecord::Base
  belongs_to :palette

  has_many :colorings, :dependent => :destroy
  has_many :colors, :class_name => 'SiteColor', :dependent => :destroy
  has_one :active_regatta, :class_name => 'Regatta'
  belongs_to :regatta

  def destroy(options = {})
    # special destroy so we don't accidentally default look
    protected_ids = [1]

    super unless (protected_ids.include?(self.id) || options[:override]
)
  end
end

The problem is that when I try to destroy one of these objects, I get an
error on the call to super, "wrong number of arguments (1 for 0)" :

l=Look.find(62)

=> #<Look:0x23d2a28 @attributes={"name"=>nil, "id"=>"62",
"palette_id"=>nil, "regatta_id"=>nil}>

l.destroy

ArgumentError: wrong number of arguments (1 for 0)
  from ./script/../config/../config/../app/models/look.rb:19:in
`destroy'
  from ./script/../config/../config/../app/models/look.rb:19:in
`destroy'
  from (irb):2

Note that there is a :has_many :dependent => :destroy relationship.

It seems that my call to super is getting arguments I don't know about,
but I don't understand how to dig any deeper into this. I tried stepping
into the call to super itself, and I immediately end up in the rescue
clause for ActiveRecord's transaction method:

(rdb:1) where
--> #0
/usr/local/lib/ruby/gems/1.8/gems/activerecord-1.15.3/lib/active_record/connection_adapters/abstract/database_statements.rb:62
in 'transaction'
(rdb:1) l =
[57, 66] in
/usr/local/lib/ruby/gems/1.8/gems/activerecord-1.15.3/lib/active_record/connection_adapters/abstract/database_statements.rb
   57 transaction_open = true
   58 end
   59 yield
   60 end
   61 rescue Exception => database_transaction_rollback
=> 62 if transaction_open
   63 transaction_open = false
   64 rollback_db_transaction
   65 end
   66 raise

FWIW, I considered the possibility that it was one of the dependent
relationships' objects who's destroy method was getting sent a parameter
it wasn't expecting - so I defined both of those to accept a parameter
as well, and then *their* call to super caused this problem.

#2

Maybe you are over-complicating this.

Rails has a "before_destroy" filter.

Just use that, and if the ID of the object being destroyed is 1, abort
the process (by just returning false).

So maybe something as simple as this (just off the top of my head, so
you'll wanna test it of course)

class Look < ActiveRecord::Base
  belongs_to :palette

  has_many :colorings, :dependent => :destroy
  has_many :colors, :class_name => 'SiteColor', :dependent => :destroy
  has_one :active_regatta, :class_name => 'Regatta'
  belongs_to :regatta

  before_destroy { |record| return false if record.id == 1 }

end

Note: it's such a simple check that you don't even need to call a
method... it can be handled right in the Proc.

Also note: the filter chain halts if you return false, which is
probably what you want in this case, but again: test it to be sure.

HTH!

-Danimal

#3

Daniel,

I appreciate the suggestion. I don't think it will cover my case, but I
must admit I had forgoten all about before_destroy. I'll have to put
some more thought into this.

The problem is that I want to be able to force the destroy if necessary.
I'm not sure how I would articulate an override into before_destroy. But
I'll look into it.

Part of the reason for my question is that this seems to be a case where
what I thought I knew doesn't seem to be correct, so I'm looking to
improving my understanding of Rails.

-Avram

#4

Avram,

I'm confused. What do you mean by "want to be able to force the
destroy"? If you have @look as the instance of the Look object you
want to destroy, you just call @look.destroy, right? The
before_destroy hook is just to make sure that you don't destroy the
Look object with ID=1 (or whatever ID you want to protect).

Some other things to consider:

You could use before_destroy but have it call a method instead of an
inline proc. I.e.:

before_destroy :skip_delete_if_not_one

def skip_delete_if_not_one
  return false if id == 1
end

Then, you could add other logic there if need be.

Another thought that just occurred to me: If the default destroy
method takes an options hash, maybe you need to pass that to super as
well? Maybe that's the error you are getting.

Also, I vaguely remember something in the Agile book about this...
i.e. not deleting the user with ID=1. You might see how that was done.

I don't think the problem is that your thinking is off... you are
thinking about this in the right way. But like so many things in the
Rails world, there are lots of ways to do something. So, you can call
a before filter (as I suggested). Or you can override the destroy
method (as you suggested). Both are "the right way"... it's more a
matter of taste, style and what you specifically want to accomplish.

-Danimal

#5

Danimal,

Here's what I want:

Object.find(2).destroy # object 2 is destroyed
Object.find(1).destroy # has no effect
Object.find(1).destroy :override => true # object 1 is destroyed.

I want to prevent someone from being able to delete certain Look objects
by mistake, but I don't want to completely block my ability to destroy
the special ones.

The super destroy method does not take any parameters. The error is "1
for 0" not "0 for 1". It only comes up when I add parameters to my
destroy method.

If I simply change my destroy method to
  def destroy
    super if self.id != 1
  end

I don't have any problems.

But now I can't destroy object 1 intetionally without changing the code.

#6

I wonder if by declaring it as:

def destroy(options = {}) ...

if you are not actually overriding the default destroy method? (I've
got a reasonable amount of Ruby and Rails experience, but I'm by no
means an expert, so I may be misunderstanding something here).

What if you make a second method called: "destroy_override". So
instead of calling: Look.find(1).destroy :override => true you'd call:
Look.find(1).destroy_override

Then, you do your original code but without the options parameter.
That overrides destroy() as a method and does the ID check. Then, in
destroy_override it just calls destroy, probably through an alias for
the original destroy so it doesn't call your overridden method.

I hope that rambling made sense. Let me know if it didn't.

-Danimal

#7

It made sense, it just doesn't prevent accidental destruction of the
object. It requires someone to be aware that they can't indiscriminately
call destroy on objects of this class. It isn't really any different
than requiring people to always say "object.destroy if
!object.protected?.