Code

Opened 7 years ago

Closed 4 years ago

Last modified 3 years ago

#6108 closed Uncategorized (wontfix)

send all_objects_to_be_deleted in the pre_delete signal

Reported by: Gábor Farkas <gabor@…> Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: carljm, plandry@…, cgieringer Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

when django deletes an object, it also deletes all related objects.

it would be great to send the list of "to be deleted objects" in the pre_delete signal, because then a developer could implement in his application things like only-delete-object-when-there-are-no-related-objects (by raising an Exception in the listener-function), etc.

it can be implemented by an one-line change in db/models/query.py (patch attached).

Attachments (1)

pre_delete.patch (1.8 KB) - added by Gábor Farkas <gabor@…> 7 years ago.

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by Gábor Farkas <gabor@…>

comment:1 Changed 7 years ago by Simon G <dev@…>

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:2 Changed 7 years ago by jacob

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Design decision needed

This unfortunately needs to be more complicated to handle all the use cases for this feature. We need the following features:

  • Listeners should be able to modify the list of to-be-deleted objects. I think that'll work with your patch, but a test is needed to make sure that happens.
  • Because each doomed object gets a signal sent listener code might end up running multiple times. So we might need to figure out which objects have already been signaled. We could avoid a duplicate list (and the duplicate memory that would require) with some other lightweight structure -- perhaps the list of objects-to-delete could be a list of (object, has_been_notified)?
  • Another option to avoid duplicated callbacks would be an initial bulk-pre-delete signal (that is, use the same signal but with instance=None and instance_list=list_of_doomed_objects.
  • We also need to verify that we can in fact implement ON DELETE CASCADE-like behavior with this. If not, it's something of a worthless addition.

comment:3 Changed 7 years ago by Gábor Farkas <gabor@…>

could you please elaborate on the"ON DELETE CASCADE-like behavior" part?

what kind of behaviors do you imagine here?

as i see, the current django-delete behavior is basically on-delete-cascade...
so did you mean maybe to implement "ON DELETE SET NULL" and other such behaviors?
or "filtered cascading"?

from what i see here, most of the situations where you want to delete less objects,
than what django wants do delete, are implementable.

in short:

i will write the testcases, if you tell me what kind of situations i need to test :)

also, from the 2 possibilities to avoid duplicate-callbacks, i think the initial bulk-pre-delete signal one is the best.

comment:4 Changed 6 years ago by mh

honestly, _please_ implement this (by merging this patch or calling the default Manager delete method, I won't be picky about this)

with the current code base (unless there is some magic hidden in db/models/query.py ...) i found no possibility to prevent a object from being deleted - apart from creating an ON DELETE - Trigger in the DB that silently prevents the deletion of a record

this is, however, not possible in all environments (if all databases were full-featured, this whole hack would not need to exist ...), and leads to unnecessary separation/fragmentation of logic. so please fix this issue one way or another

comment:5 Changed 6 years ago by gabor

the following (ugly) code should work:

overload the delete() method of the object like this:

    def delete(self):
        from django.utils.datastructures import SortedDict
        s = SortedDict()
        self._collect_sub_objects(s)
        #here s will contain all the objects that are planned to be deleted
        if s.items() == [(type(u), { u.pk : u })]:
            super(YourModel, self).delete()
        else:
            raise Exception("related objects found")

the code is untested, but i'm doing something very similar and it works.
if you want to make it 100% safe, you have to run it in a serializable-transaction unfortunately :(
otherwise it could happen, that the call to _collect_sub_objects returns
that all is fine, but while the code proceeds to delete-the-object,
some other process already inserted new "dependent" objects,
and those will get deleted.

yes, it contains a call to a private undocumented method.
no, afaik no other way exists.

comment:6 Changed 5 years ago by gabor

if anyone is planning to use above-mentioned code-snippet, please note, that for django-1.0 it has to be changed slightly:

def delete(self):
    from django.db.models.query import CollectedObjects
    s = CollectedObjects()
    self._collect_sub_objects(s)
    #here s will contain all the objects that are planned to be deleted
    if s.items() == [(type(u), { u.pk : u })]:
        super(YourModel, self).delete()
    else:
        raise Exception("related objects found")

yes, this is a good example of why not to use private undocumented methods (because they might change between versions...)

comment:7 Changed 5 years ago by carljm

  • Cc carljm added

comment:8 Changed 5 years ago by guettli

  • Cc hv@… added

comment:9 Changed 5 years ago by guettli

Related: #7539 Add ON DELETE and ON UPDATE support to Django

comment:10 Changed 5 years ago by anonymous

  • Cc plandry@… added

comment:11 Changed 4 years ago by cgieringer

  • Cc cgieringer added

Related: #6870. My two cents is that the signal's arguments are fine as-is, it just needs to be called at an earlier point in time. Signals are simple and elegant; they tell you just enough information to find the rest of the information you might want. Adding the list of to-be-deleted objects is unnecessary because a signal listener could create this list without the signal including it.

comment:12 Changed 4 years ago by carljm

  • Resolution set to wontfix
  • Status changed from new to closed

Sending a list of related objects to be deleted with the pre_delete signal is fraught with complications, given that the pre_delete signal is sent once for every individual object to be deleted. Either objects get sent as part of the related-list potentially many times over (in which case how do you reconcile conflicts if the listener in one case removes the object and in another case doesn't?), or you have to make some arbitrary decisions about when a given object is included or not.

In any case, now that #7539 is fixed, all of the justifying use-cases here have workable alternatives.

comment:13 Changed 3 years ago by guettli

  • Cc hv@… removed
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.