This project is archived and is in readonly mode.

#453 ✓stale
Jacob Burkhart

has_many delete suffers premature disassociation

Reported by Jacob Burkhart | June 19th, 2008 @ 10:16 PM

It seems this has been the behavior of has_many delete since as far back in rails as I could test... but It was surprising to me when I encountered it.

The attached test case (which you can invoke directly assuming you have an active record gem installed) demonstrates the behavior.

The problem stems from having a validation which checks that a particular entity has at least one item on it's has_many association.

In my example, it's that driver has_many cars and a driver must have at least one car.

calling:

some_driver.cars.delete(some_car)

causes the association to be broken, without a chance for this validation to run

This seems unfair to both car and driver validations. Either delete should call validations... Or, the disassociation should not happen until 'save' is called on either the car or driver.

Do you agree?

I'd like to hear confirmation that this is indeed a bug before I begin working on a patch...

Comments and changes to this ticket

  • Christian Weyer

    Christian Weyer July 7th, 2008 @ 11:38 PM

    • Tag set to activerecord, has_many, tested

    I also ran into this and it sucks like hell. It is a clear violation of the validation system in Rails.

    I see two possible solutions:

    • Delete should only remove the element from the collection and wait to do the database stuff until the parent's save method is called. I agree that a direct deletion might be useful sometimes (but put a warning in the documentation about skipping validations then!), so here is another possibility:
    • Do it like collection.<< and collection.build. When using << the attached object is saved instantly whereas build saves the object when the parents save method is called. Such a thing should exists for deletion, too.
  • Rob Dupuis

    Rob Dupuis August 1st, 2008 @ 10:53 PM

    • Tag changed from activerecord, has_many, tested to activerecord, bug, has_many, tested

    I'm hitting this exact problem. Does anyone have a workaround for this? Thanks.

  • Jacob Burkhart

    Jacob Burkhart August 2nd, 2008 @ 03:37 AM

    The workaround is to delete from the collection and then re-assign to the association:

    for example, instead of:

    group.users.delete(bob)

    you have to do something like:

    g_users = group.users.to_a

    g_users.delete(bob)

    group.users = g_users

    group.save!

    bob.destroy

  • Rob Dupuis

    Rob Dupuis August 2nd, 2008 @ 01:31 PM

    I don't think this works (at least for me). As soon as I do the assignment (group.users = g_users) the sql is instantly executed to update the has_many collection. Therefore the update has been committed before I can validate the new collection.

    Thanks

  • Zef RosnBrick

    Zef RosnBrick August 4th, 2008 @ 12:28 AM

    This seems to work for me. It is based on Railscast #75 http://railscasts.com/episodes/75, where Projects have many Tasks. I also give each task a position to allow for reordering (not shown).

    project.rb

    has_many :tasks, :dependent => :destroy

    #validation statements here...

    after_update :save_tasks

    def tasks=(tasks_attributes)

    tasks_attributes[:new_tasks] ||= []

    position = 0

    #For existing tasks

    tasks.each do |task|

    attributes = tasks_attributes[task.id_s]

    unless attributes.nil?

    task.attributes = attributes

    unless task.should_delete

    task.position = position

    position += 1

    end

    end

    end

    #For new tasks

    tasks_attributes[:new_tasks].each do |task_attributes|

    task_attributes[:position] = position

    tasks.build(task_attributes)

    position += 1

    end

    end

    def save_tasks

    tasks.each do |task|

    if (task.should_delete != true)

    task.save(false)

    else

    tasks.delete(task)

    end

    end

    end

    def id_s(element = nil)

    id = self.id || self.object_id

    if element.nil?

    "#{self.class.to_s.underscore}_#{id}"

    else

    "#{self.class.to_s.underscore}_#{id}_#{element}"

    end

    end

    protected

    def validate

    tasks.each_with_index do |task, position|

    errors.add_to_base("Task #{position + 1} Name cannot be blank.") if (task.name.blank?)

    end

    end

    ------------------

    task.rb

    belongs_to :project

    validates_presence_of :name

    attr_accessor :should_delete

    def should_delete

    @should_delete || false

    end

    def should_delete=(bool)

    if (bool == false)

    @should_delete = false

    else

    @should_delete = true

    end

    end

    def id_s(element = nil)

    id = self.id || self.object_id

    if element.nil?

    "#{self.class.to_s.underscore}_#{id}"

    else

    "#{self.class.to_s.underscore}_#{id}_#{element}"

    end

    end

    ------------------

    new.rhtml

    <% form_tag({:controller => Project, :action => "new"}) do %>

    New Project

    <%= error_messages_for :project %>

    <%= label :project, :name, "Name:" %> <%= text_field :project, :name %>

    <%= render :partial => 'task', :collection => @project.tasks %>

    <%= link_to_remote("Add Task", :update => :tasks, :position => :bottom, :url => {:controller => Project, :action => "add_task"}) %>

    <%= submit_tag "Create", :name => 'submit_action' %>

    <%= submit_tag "Cancel", :name => 'submit_action' %>

    <% end %>

    ------------------

    _task.rhtml

    <% unless task.new_record? %>

    <%= label_tag task.id_s("name"), "Task:" %>

    <%= text_field_tag "project[tasks][#{task.id_s}][name]", task.name, :id => task.id_s("name") %>

    <%= check_box_tag "project[tasks][#{task.id_s}][should_delete]", 1, task.should_delete, :id => task.id_s("should_delete") %>

    <%= label_tag task.id_s("should_delete"), "Delete" %>

    <% else %>

    <%= label_tag task.id_s("name"), "Task:" %>

    <%= text_field_tag "project[tasks][new_tasks][][name]", task.name, :id => task.id_s("name") %>

    <%= link_to_function "remove", "removeTask('#{task.id_s}')" %>

    <% end %>

  • Zef RosnBrick

    Zef RosnBrick August 4th, 2008 @ 12:31 AM

    This seems to work for me. It is based on Railscast 75 http://railscasts.com/episodes/75, where Projects have many Tasks. I also give each task a position to allow for reordering (not shown).

    project.rb

    
    has_many :tasks, :dependent => :destroy
    
    #validation statements here...
    
    after_update :save_tasks
    
    def tasks=(tasks_attributes)
    	tasks_attributes[:new_tasks] ||= []
    	position = 0
    	
    	#For existing tasks
    	tasks.each do |task|
    		attributes = tasks_attributes[task.id_s]
    		unless attributes.nil?
    			task.attributes = attributes
    			unless task.should_delete
    				task.position = position
    				position += 1
    			end
    		end
    	end
    	
    	#For new tasks
    	tasks_attributes[:new_tasks].each do |task_attributes|
    		task_attributes[:position] = position
    		tasks.build(task_attributes)
    		position += 1
    	end
    end
    
    def save_tasks
     	tasks.each do |task|
    		if (task.should_delete != true)
        			task.save(false)
    		else
    			tasks.delete(task)
    		end
    	end
    end
    
    def id_s(element = nil)
    	id = self.id || self.object_id
    	if element.nil?
      		"#{self.class.to_s.underscore}_#{id}"
        	else
          		"#{self.class.to_s.underscore}_#{id}_#{element}"
        	end
    end
    
    protected
    	
    def validate
    	tasks.each_with_index do |task, position|
    		errors.add_to_base("Task #{position + 1} Name cannot be blank.") if (task.name.blank?)
    	end
    end
    
    

    ------------------

    task.rb

    
    belongs_to :project
    
    validates_presence_of :name
    
    attr_accessor :should_delete
    	
    def should_delete
    	@should_delete || false
    end
    	
    def should_delete=(bool)
    	if (bool == false)
    		@should_delete = false
    	else
    		@should_delete = true
    	end
    end
    	
    def id_s(element = nil)
    	id = self.id || self.object_id
    	if element.nil?
      		"#{self.class.to_s.underscore}_#{id}"
        	else
          		"#{self.class.to_s.underscore}_#{id}_#{element}"
        	end
    end
    
    

    ------------------

    new.rhtml

    
    <% form_tag({:controller => Project, :action => "new"}) do %>
    	<h1>New Project</h1>
    	
    	<%= error_messages_for :project %>
    
    	<div class="form_div">
    	  <%= label :project, :name, "Name:" %> <%= text_field :project, :name %>
    	</div>
    	
    	<div class="form_div">
    		<div id="tasks">
    		 	<%= render :partial => 'task', :collection => @project.tasks %>
    		</div>
    		<%= link_to_remote("Add Task", :update => :tasks, :position => :bottom, :url => {:controller => Project, :action => "add_task"}) %>
    	</div>
    
      <div class="submit_div">
    		<%= submit_tag "Create", :name => 'submit_action' %>
    		<%= submit_tag "Cancel", :name => 'submit_action' %>
    	</div>
    <% end %>
    
    

    ------------------

    _task.rhtml

    
    <div id="<%= task.id_s %>" class="task">
    	<% unless task.new_record? %>
    		<%= label_tag task.id_s("name"), "Task:" %>
    		<%= text_field_tag "project[tasks][#{task.id_s}][name]", task.name, :id => task.id_s("name") %>
    		<%= check_box_tag "project[tasks][#{task.id_s}][should_delete]", 1, task.should_delete, :id => task.id_s("should_delete") %>
    		<%= label_tag task.id_s("should_delete"), "Delete" %>
    	<% else %>
    		<%= label_tag task.id_s("name"), "Task:" %>
    		<%= text_field_tag "project[tasks][new_tasks][][name]", task.name, :id => task.id_s("name") %>
    		<%= link_to_function "remove", "removeTask('#{task.id_s}')" %>
    	<% end %>
    </div>
    
    
  • Zef RosnBrick

    Zef RosnBrick August 4th, 2008 @ 02:44 AM

    Here is a better solution to allow for sorting of tasks using scriptaculous drag and drop.

    project_controller.rb

    class ProjectController < ApplicationController
    	def index
    		redirect_to :action => 'list'
    	end
    	
    	def new
    		submit_action = params['submit_action']
    		
    		if (submit_action == nil)
    		  @project = Project.new
    		  @project.tasks.build
    		elsif (submit_action == "Create")
    			@project = Project.new(params[:project])
    		  if @project.save
    		    flash[:notice] = "Successfully created project and tasks."
    		    redirect_to :action => 'list'
    		  end
    		elsif (submit_action == "Cancel")
    			redirect_to :action => 'list'
    		end
    	end
    
    	def edit
    		submit_action = params['submit_action']
    		
    		begin
    	  	@project = Project.find(params[:id], :include => :tasks)
    		rescue ActiveRecord::RecordNotFound
    			flash[:notice] = "Invalid Project."
    			redirect_to :action => 'list'
    		else
    			if (submit_action == nil)
    			elsif (submit_action == "Save")
    			  if @project.update_attributes(params[:project])
    			    flash[:notice] = "Successfully updated project and tasks."
    			    redirect_to :action => 'list'
    			  end
    			elsif (submit_action == "Cancel")
    				redirect_to :action => 'list'
    			end
    		end
    	end
    	
    	def delete
    		submit_action = params['submit_action']
    		
    		begin
    	  	@project = Project.find(params[:id])
    		rescue ActiveRecord::RecordNotFound
    			flash[:notice] = "Invalid Project."
    			redirect_to :action => 'list'
    		else
    			if (submit_action == nil)
    			elsif (submit_action == "Delete")
    				@project.destroy
    			  flash[:notice] = "Successfully deleted project and tasks."
    		    redirect_to :action => 'list'
    			elsif (submit_action == "Cancel")
    				redirect_to :action => 'list'
    			end
    		end
    	end
    		
    	def list
    		@projects = Project.find(:all, :include => :tasks)
    	end
    	
    	def add_task
    		render :partial => "task", :object => Task.new()
    	end
    end
    

    application_helper.rb

    # Methods added to this helper will be available to all templates in the application.
    module ApplicationHelper
    	def label_tag(for_tag, name)
    		"<label for='#{for_tag}'>#{name}</label>"
    	end
    end
    

    project.rb

    class Project < ActiveRecord::Base
      has_many :tasks, :dependent => :destroy
    
      validates_presence_of :name
    	validates_uniqueness_of :name
    
      after_update :save_tasks
    
    	def tasks=(tasks_attributes)
    		existing_tasks = {}
    		new_tasks = []
    		
    		tasks_attributes.each_with_index do |task_attributes, position|
    			task_attributes[:position] = position
    			if (task_attributes.has_key?(:id))
    				id = task_attributes.delete(:id)
    				existing_tasks[id] = task_attributes
    			else
    				new_tasks.push(task_attributes)
    			end
    		end
    		
    		#For existing tasks
    		tasks.each do |task|
    			attributes = existing_tasks[task.id.to_s]
    			unless attributes.nil?
    				task.attributes = attributes
    			end
    		end
    		
    		#For new tasks
    		new_tasks.each do |task_attributes|
    			tasks.build(task_attributes)
    		end
    	end
    	  
      def save_tasks
        tasks.each do |task|
    			logger.info("#{task.id}: #{task.should_delete}")
    			if (task.should_delete != true)
          	task.save(false)
    			else
    				tasks.delete(task)
    			end
        end
      end
    	
    	def id_s(element = nil)
    		id = self.id || self.object_id
        if element.nil?
      	  "#{self.class.to_s.underscore}_#{id}"
        else
          "#{self.class.to_s.underscore}_#{id}_#{element}"
        end
      end
    
    	protected
    	
    	def validate
    		tasks.each_with_index do |task, position|
    			errors.add_to_base("Task #{position + 1} Name cannot be blank.") if (task.name.blank?)
    		end
    	end
    end
    

    task.rb

    class Task < ActiveRecord::Base
      belongs_to :project
    
      validates_presence_of :name
    
    	attr_accessor :should_delete
    	
    	def <=> other_task
    		position <=> other_task.position
    	end
    	
    	def should_delete
    		@should_delete || false
    	end
    	
    	def should_delete=(bool)
    		if (bool == false)
    			@should_delete = false
    		else
    			@should_delete = true
    		end
    	end
    	
    	def id_s(element = nil)
    		id = self.id || self.object_id
        if element.nil?
      	  "#{self.class.to_s.underscore}_#{id}"
        else
          "#{self.class.to_s.underscore}_#{id}_#{element}"
        end
      end
    end
    

    new.rhtml

    <% form_tag({:controller => Project, :action => "new"}) do %>
    	<h1>New Project</h1>
    	
    	<%= error_messages_for :project %>
    
    	<div class="form_div">
    	  <%= label :project, :name, "Name:" %> <%= text_field :project, :name %>
    	</div>
    	
    	<div class="form_div">
    		<div id="tasks">
    		 	<%= render :partial => 'task', :collection => @project.tasks.sort %>
    		</div>
    		<%= javascript_tag("createTaskSortable()") %>
    		<%= link_to_remote("Add Task", :update => :tasks, :position => :bottom, :url => {:controller => Project, :action => "add_task"}, :complete => "createTaskSortable()") %>
    	</div>
    
      <div class="submit_div">
    		<%= submit_tag "Create", :name => 'submit_action' %>
    		<%= submit_tag "Cancel", :name => 'submit_action' %>
    	</div>
    <% end %>
    

    edit.rhtml

    <% form_tag({:controller => Project, :action => "edit", :id => @project.id}) do %>
    	<h1>Edit Project</h1>
    	
    	<%= error_messages_for :project %>
    
    	<div class="form_div">
    	  <%= label :project, :name, "Name:" %> <%= text_field :project, :name %>
    	</div>
    	
    	<div class="form_div">
    		<div id="tasks">
    		 	<%= render :partial => 'task', :collection => @project.tasks.sort %>
    		</div>
    		<%= javascript_tag("createTaskSortable()") %>
    		<%= link_to_remote("Add Task", :update => :tasks, :position => :bottom, :url => {:controller => Project, :action => "add_task"}, :complete => "createTaskSortable()") %>
    	</div>
    
      <div class="submit_div">
    		<%= submit_tag "Save", :name => 'submit_action' %>
    		<%= submit_tag "Cancel", :name => 'submit_action' %>
    	</div>
    <% end %>
    

    _task.rhtml

    <div id="<%= task.id_s %>" class="task">
    	<%= image_tag("drag.png", :class => "drag_handle", :id => task.id_s("drag_handle"), :style => "cursor: move;") %>
    	<% unless task.new_record? %>
    		<%= hidden_field_tag "project[tasks][][id]", task.id, :id => task.id_s("id") %>
    	
    		<%= label_tag task.id_s("name"), "Task:" %>	<%= text_field_tag "project[tasks][][name]", task.name, :id => task.id_s("name") %>
    		
    		<%= check_box_tag "project[tasks][][should_delete]", 1, task.should_delete, :id => task.id_s("should_delete") %> <%= label_tag task.id_s("should_delete"), "Delete" %>
    	<% else %>
    		<%= label_tag task.id_s("name"), "Task:" %> <%= text_field_tag "project[tasks][][name]", task.name, :id => task.id_s("name") %>
    		
    		<%= link_to_function "remove", "removeTask('#{task.id_s}')" %>
    	<% end %>
    </div>
    
  • josh

    josh November 22nd, 2008 @ 07:50 PM

    • State changed from “new” to “stale”

    Staling out, please let me know if its still an issue.

  • Jacob Burkhart

    Jacob Burkhart February 9th, 2009 @ 08:01 PM

    Hi Josh,

    Just tested this again with Rails 2.3... Same code I attached with my original ticket. I still think this is a problem.

  • Jacob Burkhart

    Jacob Burkhart February 9th, 2009 @ 08:02 PM

    • Assigned user set to “josh”

    I wouldn't call this ticket stale... perhaps you really mean 'wontfix' ?

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