Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #7309 (closed enhancement: duplicate)

Opened 3 years ago

Last modified 2 years ago

[XPATCH] Improve has_many :dependent API consistency

Reported by: mlandauer Assigned to: nzkoz
Priority: normal Milestone: 2.x
Component: ActiveRecord Version: edge
Severity: normal Keywords:
Cc: tarmo

Description

If :dependent is not set on a has_many association there is currently a confusing inconsistency between doing a destroy on the main object and doing a delete on the association.

Example:

class Directory < ActiveRecord::Base
  has_many :files
end

directory.destroy will not do anything to the files, while directory.files.delete(file) will set the directory_id of the file object to NULL. This inconsistency is confusing.

The attached patch adds a further :dependent option :none to maintain backward compatibility for everyone who doesn't have :dependent set at all. So,

class Directory < ActiveRecord::Base
  has_many :files, :dependent => :none
end

Now the :dependent => :none set directory.destroy will not do anything to the files and in a consistent way directory.files.delete(file) will remove the file from the collection but not do anything to the file.

The patch also makes directories.files.delete(file) and directories.files.clear also behave consistently when :dependent => :nullify, :destroy and :delete_all.

This patch covers the same ground as #5209 and #6904 but hopefully in a more consistent way.

Attachments

improve_has_many_dependent_api_consistency.diff (10.0 kB) - added by mlandauer on 01/23/07 04:45:35.
improve_has_many_dependent_api_consistency_2.diff (9.7 kB) - added by manfred on 06/30/07 07:46:45.
Same patch only with slightly changed implementation and documentation. Updated to patch against r7156.
improve_has_many_dependent_api_consistency_3.diff (10.1 kB) - added by mlandauer on 09/25/07 06:57:50.

Change History

01/23/07 04:45:35 changed by mlandauer

  • attachment improve_has_many_dependent_api_consistency.diff added.

01/23/07 04:52:29 changed by mlandauer

I removed the :dependent => :none option as I realised it doesn't make sense ever to delete an object on an association and not modify it (even if using database on delete cascade)

The patch still improves the consistency of the :dependent => :nullify, :destroy and :delete_all in how they affect the behaviour of deleting a directory by doing directory.destroy and deleting a file by doing directory.files.delete(file) or directory.files.clear

06/30/07 03:08:17 changed by nzkoz

  • owner changed from core to nzkoz.
  • status changed from new to assigned.

You should get some people from #rails-contrib to review this patch. It's definitely a bug and with some positive reviews I'm happy to commit something along these lines.

06/30/07 07:46:45 changed by manfred

  • attachment improve_has_many_dependent_api_consistency_2.diff added.

Same patch only with slightly changed implementation and documentation. Updated to patch against r7156.

06/30/07 07:48:21 changed by manfred

+1 for inclusing, it's a good patch.

07/01/07 06:38:31 changed by tarmo

  • cc set to tarmo.

07/27/07 07:18:54 changed by notahat

Note that the same issue with :dependent => :nullify deleting associated records also applies to has_one relationships.

07/31/07 01:03:25 changed by notahat

I've taken a different approach to this in #9129. It deals with has_one relationships as well, and doesn't change the default behaviour when :dependent isn't set.

I'm happy to merge the patches if anyone has comments on the pros and cons of each approach.

09/25/07 06:57:50 changed by mlandauer

  • attachment improve_has_many_dependent_api_consistency_3.diff added.

09/25/07 07:03:35 changed by mlandauer

I like manfred's updates to the patch, so I've taken his version as is and updated it so that it patches against r7626.

I've uploaded this as "improve_has_many_dependent_api_consistency_3.diff".

09/25/07 07:16:20 changed by alanr

+1 from me.

09/26/07 04:17:42 changed by julians37

+1

09/26/07 04:20:14 changed by mlandauer

  • keywords set to verified.

10/13/07 23:30:14 changed by nzkoz

  • keywords deleted.
  • summary changed from [PATCH] Improve has_many :dependent API consistency to [XPATCH] Improve has_many :dependent API consistency.

Hey guys,

This no longer applies cleanly. but some comments anyway:

At first this looked wrong to me:

  def clear
    delete_all
    self
  end

I'm still a little unclear why that method was changed, but perhaps we should rename delete_all to something that doesn't have other connotations elsewhere in the application?

A few comments in the test cases would be nice too, so people getting failures there know what they're working on.

10/15/07 21:37:03 changed by mlandauer

  • status changed from assigned to closed.
  • resolution set to duplicate.

After further consideration it looks like this patch is not necessary anymore as the main purpose of the patch has already been addressed (in a slightly different way) in the current edge. The :dependent => :nullify option becomes exactly like :dependent => nil.

Furthermore, the test cases don't cover the modification to the clear method. So, bad me, I should never have put that change in there.

So, I'm closing this ticket as a duplicate.