This project is archived and is in readonly mode.

#1401 ✓wontfix
gilbertd

:dependent options shouldn't cause database hit when new_record

Reported by gilbertd | November 18th, 2008 @ 03:56 PM | in 2.x

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? }"

Comments and changes to this ticket

  • Pratik

    Pratik March 10th, 2009 @ 12:40 PM

    • 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 !

  • Richie Vos

    Richie Vos March 11th, 2009 @ 02:32 AM

    • 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.

  • Richie Vos

    Richie Vos March 11th, 2009 @ 02:34 AM

    Pratik, due to the patch could you re-open this?

  • Richie Vos

    Richie Vos March 11th, 2009 @ 02:44 AM

    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.

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>

People watching this ticket

Attachments

Pages