On Tue, May 18, 2010 at 4:41 AM, Carl Gieringer <carl.gierin...@gmail.com> wrote: > Hi Djangonauts, > > I'd like to call attention to a patch[0] I submitted last week for > #6870[1]. From one perspective, this ticket is a version of the "My > models with foreign keys are deleted when their related model deletes > because of Django's ON CASCADE DELETE behavior." But from another > perspective this ticket is a (the only?) ticket that correctly > identifies a bug with the usage of the pre_delete signal, and other > tickets are distinguished as feature requests: e.g., adding ON DELETE > behaviors, or "Django should set nullable foreign key fields to NULL". > > The Bug > > django.db.models.Model.delete first gathers self._collect_sub_objects > to cache all related objects that will be deleted by this model's > deletion. (This cache includes the model itself.) Model.delete then > calls django.db.models.query.delete_objects, which calls the > pre_delete signal for each of the objects before deleting them. > Performing the caching before sending the signal breaks the signal's > promise with receivers that they will learn about the deletion "pre" > any steps taken towards deletion. > > Describing this as a bug depends upon the characterization of > pre_delete, because hypothetically the signal could offer either a > weak or a strong contract with receivers. A strong promise to > receivers would imply that they will get first crack at a model when > Django learns that the model will be deleted. This is what I assumed > pre_delete was for when I learned about Django's signals. This strong > version of the promise would require that Django take no steps towards > deleting the object before calling the signal (especially not > permanent steps like caching related object for deletion.) This > strong promise would result in the most versatile framework, because > it allows for inversion of control. Inversion of control is > particularly important in a plug-and-play framework like Django where > many different applications come together and must coexist in a single > project. > > On the contrary, pre_delete could also offer a weak promise of just > letting receivers know that deletion is occurring, but restricting > what receivers can do to modify the results of the deletion. This is > what the signal currently does with respect to related models. > (Although I don't think this restriction is intentional...maybe just > an artifact of how it was originally coded up.) I hope that consensus > among the developers will be that the strong version of the promise is > the more robust development choice.
You're correct that this all hinges on the interpretation of the signal deletion contract. > This Bug is Distinct from Feature Requests > > The bug I've described is a design issue. When you combine it with > Django's documented behavior of ON DELETE CASCADE, though, you get the > common complaint that related models are deleted and there's nothing a > pre_delete receiver can do about it. At least two features have been > requested to address this issue. One is that Django should > automatically set nullable foreign key fields to NULL whenever the > keyed object is deleted. (#9308[2]. It says this is fixed but I'm > not experiencing it.) Another is to implement several ON DELETE/ > UPDATE behaviors that are set at field creation (#7539[3]). Even if > both of these features were implemented, it would not change the fact > that right now the pre_delete signal as-used only offers a weak > promise to receivers, and that there may arise in the future another > usage where this weak promise causes frustration to programmers. This is the bit where my opinion hinges. It's only a bug if the strong promise case you describe is what the contract should be; it's working as designed if the weak promise is the right contract. As an abstract principle, I can see that #7539 is theoretically orthogonal to what you are proposing. However, I don't see the practical use case outside of implementing #7539-analog behavior. What exactly is your use case (real or imagined) that requires the strong promise? The 'weak' signal as currently implemented ensures that every object X that is to be deleted is notified before it is deleted. Why do you need to be able to guarantee the state of all related objects as well? > My Patch Modifies _collect_sub_objects to Notify Signal Receivers > > In order to notify receivers of the pre_delete signal (or any signal > for that matter), we need to 1) be at a point where we have identified > the model that needs to send the signal, and 2) be at a point in the > code before we have taken steps to complete the deletion (or whatever > action the signal is relevant to.) The best place for this is at the > beginning of Model's _collect_sub_objects method. At the beginning of > the method, self's related models have not been inspected to add them > to seen_objs, but we have identified which model (namely self) will be > the target of the action. Since _collect_sub_objects recurses along > relations, it will have the opportunity to send the signal for each > related model as soon as it recurses upon the related model. > > The only thing I don't like about my solution is that it piggybacks > sending signals along with collecting related models rather than > sending the signals just before the action is applied. Another > alternative might be to traverse all of the related models first to > send the signals, and then traverse them all again (those that remain > related, that is.) But it would be inefficient to perform the same > traversal twice. So I think sending the signal from within > _collect_sub_objects is a good compromise. Your solution seems like a reasonable approach to the problem. I agree with your criticism - it's not great that it piggybacks, but I suspect spending excessive effort trying avoid the piggyback is a case of YAGNI. Repeated traversal isn't really a good option, either. On a related note -- there have been discussions about optimizing the _collect_sub_objects process; see #13067. My gut reaction is that this will have a significant effect on your signaling proposal, especially if you're using it to implement #7539 analog behavior. Based on my current understanding, if I were forced to choose between the two, I have to say I would pick faster deletes over a strong pre_delete signal. > Another consideration is what alternatives there are to this > solution. I found two. One alternative (#8168[4]) suggests adding > another signal, e.g. pre_pre_delete. Personally I think one > pre_delete signal is enough if only it were sent in the right place. Agreed. 2 signals is about 1 signal too many. > Another alternative > (#6108[5]) suggests sending a list of the objects to-be-deleted along > with the pre_delete signal and allowing the signal receiver to modify > the list. This sounds to me like it is trying to be a ghetto implementation of #7539. I agree that this isn't a good idea. > Anyhoo, I love using Django, I'm deeply indebted to all of you who > have worked on it. Thank you. I'm sorry my introduction to the > mailing list is a short novel, but I hope that this patch results in a > useful addition to the framework. A short novel is better than a haiku :-) You've certainly done your homework and explained your case quite well. Thanks for taking the time to lay it out clearly (and for waiting until 1.2 is out the door before asking for feedback). Yours Russ Magee %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
