This project is archived and is in readonly mode.

:dependent options shouldn't cause database hit when new_record

#1401

When I .destroy a model that has a has_many with a :dependent => :nullify, even if its a record that hasn't been saved yet, the app still hits the database.

I would expect:


class Fish < ActiveRecord::Base
  has_many :children, :foreign_key => 'parent_id', :dependent => :nullify
end

Fish.new.destroy

to not hit the database.

Actual behavior is the database is hit with


Fish.update_all "parent_id = NULL", "parent_id = NULL"
# UPDATE fish SET parent_id = NULL WHERE parent_id = NULL

We are seeing this on :dependent => :nullify, but it looks like the same behavior applies to the other :dependent options.

I'm not sure if its better for before_destroy hooks not to be called on new_record?'s or if the code should be changed in assocations.rb to:


when :nullify
  module_eval "before_destroy { |record| #{reflection.class_name}.update_all(%(#{reflection.primary_key_name} = NULL),  %(#{dependent_conditions})) unless new_record? }"

Reported by gilbertd · November 18th, 2008 @ 03:56 PM

Activity

Pratik
- Assigned user set to Pratik
- State changed from new to wontfix
You shouldnt really be calling .destroy on a new record. But in any case, a patch would be great and we can reevaluate this :) - http://guides.rails.info/contrib...

Thanks !

March 10th, 2009 @ 12:40 PM
Richie Vos
- Tag set to active_record, associations, destroy, new_record, patch
Attaching a patch that puts unless new_record? checks in the before_destroy hooks for :nullify and :delete_all.

I do not believe these are necessary for :destroy since in that case its either not going to find any associated records, or if it does it probably should be calling .destroy on them.

As I noted in the commit message, the behavior of performing these writes can cause db deadlocks (at least it was the cause of some in our app).

When running a dependent destroy, first check if we're a new record before firing off db commands to prevent WHERE foreign_key = NULL statements from running.

Not only does this prevent an extra db connection, it also can prevent some bad behavior from occurring when you have rows where the foreign_key actually is set to null. In a nullify case, if you have 2 threads both trying to do something like UPDATE cars SET manu facturer_id = NULL WHERE manufacturer_id = NULL, you can run into deadlocks since both may require a large db read and write.
- 0001-When-running-a-dependent-destroy-first-check-if-we.patch 5.38 KB
March 11th, 2009 @ 02:32 AM
Richie Vos
Pratik, due to the patch could you re-open this?

March 11th, 2009 @ 02:34 AM

Richie Vos

Also wanted to mention, while I understand some gain comes from adding comments around string interpolations, things like this seem like they're pushing it:


before_destroy do |record|                    # before_destroy do |record|
  unless record.new_record?                   #   unless record.new_record?
    nullify_has_many_dependencies(record,     #     nullify_has_many_dependencies(record,
      "#{reflection.name}",                   #       "posts",
      #{reflection.class_name},               #       Post,
      "#{reflection.primary_key_name}",       #       "user_id",
      %@#{dependent_conditions}@)             #       %@...@) # this is a string literal like %(...)
  end                                         #   end
end                                           # end

Because of those comments, to make my change I had to update 6 lines of code instead of just putting a 1-line "unless record.new_record?" in, which seems non-optimal.

March 11th, 2009 @ 02:44 AM