Disabling .delete() in querysets in Django

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Disabling .delete() in querysets in Django

אורי
Django developers,

I recently disabled .delete() in querysets in all managers in my project, after encountering a bug in a bulk delete queryset in my tests [https://code.djangoproject.com/ticket/31600]. Actually it was quite simple to disable .delete() in querysets, but I had to define my own QuerySet class:

class QuerySet(models.query.QuerySet):
def delete(self):
raise NotImplementedError("delete is not implemented.")

I also disabled bulk_create() in managers for similar reasons. And I thought, maybe you can introduce optional settings in which if it's set, will disable delete() and bulk_create() in managers and querysets? I think this may be useful, since the delete() and bulk_create() methods have problems - they don't call the model's methods and in some cases (such as in my case) related objects from other models don't get deleted (with delete()) and models don't get validated (with bulk_create()).

I also call each model's self.full_clean() method before saving, which may also be a setting in Django. I don't want invalid objects to be saved to the database.

If you think this is a good idea and want me to implement it, I can try to submit a PR.

By the way, I checked and I found out that I can still delete objects from our admin interface, so it probably doesn't use the delete() queryset.

Thanks,
Uri.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CABD5YeGBinT0uXyCdmw6%3DARAN5URFWk-Sh%3D1wvb8Fbfuj8c_pQ%40mail.gmail.com.
Reply | Threaded
Open this post in threaded view
|

Re: Disabling .delete() in querysets in Django

Javier Buzzi
> I can still delete objects from our admin interface, so it probably doesn't use the delete() queryset.

I think you should look at https://github.com/django/django/blob/master/django/contrib/admin/options.py#L1095-L1103
Individual objects are deleted individually, using the object.delete()

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/f997487a-3b8b-4ae6-b099-e4a7af08e510%40googlegroups.com.
Reply | Threaded
Open this post in threaded view
|

Re: Disabling .delete() in querysets in Django

Tom Forbes
In reply to this post by אורי
I don’t think a Django-wide setting to disable Queryset.delete() is appropriate. As you said, it’s easy enough to configure for the specific models that may (or may not!) benefit from this. 

A Django-wide setting like this would also break just about everything that calls “delete()” on a queryset.

Tom

On 20 May 2020, at 13:57, אורי <[hidden email]> wrote:


Django developers,

I recently disabled .delete() in querysets in all managers in my project, after encountering a bug in a bulk delete queryset in my tests [https://code.djangoproject.com/ticket/31600]. Actually it was quite simple to disable .delete() in querysets, but I had to define my own QuerySet class:

class QuerySet(models.query.QuerySet):
def delete(self):
raise NotImplementedError("delete is not implemented.")

I also disabled bulk_create() in managers for similar reasons. And I thought, maybe you can introduce optional settings in which if it's set, will disable delete() and bulk_create() in managers and querysets? I think this may be useful, since the delete() and bulk_create() methods have problems - they don't call the model's methods and in some cases (such as in my case) related objects from other models don't get deleted (with delete()) and models don't get validated (with bulk_create()).

I also call each model's self.full_clean() method before saving, which may also be a setting in Django. I don't want invalid objects to be saved to the database.

If you think this is a good idea and want me to implement it, I can try to submit a PR.

By the way, I checked and I found out that I can still delete objects from our admin interface, so it probably doesn't use the delete() queryset.

Thanks,
Uri.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CABD5YeGBinT0uXyCdmw6%3DARAN5URFWk-Sh%3D1wvb8Fbfuj8c_pQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2553E532-E331-41E8-A700-7D5AE5BB572B%40tomforb.es.
Reply | Threaded
Open this post in threaded view
|

Re: Disabling .delete() in querysets in Django

Tim Graham-2
In reply to this post by אורי
Hi Uri,

I think you've mostly come across documented behaviors rather than "problems". You haven't provided enough details in the bug report to say for sure on that issue.

I don't think the things you've implemented are generally applicable to many Django projects such that they warrant the addition of new settings.

On Wednesday, May 20, 2020 at 8:57:08 AM UTC-4, Uri wrote:
Django developers,

I recently disabled .delete() in querysets in all managers in my project, after encountering a bug in a bulk delete queryset in my tests [<a href="https://code.djangoproject.com/ticket/31600" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fcode.djangoproject.com%2Fticket%2F31600\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNE04LsPJSZZd0yUsXgauikg9exR8A&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fcode.djangoproject.com%2Fticket%2F31600\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNE04LsPJSZZd0yUsXgauikg9exR8A&#39;;return true;">https://code.djangoproject.com/ticket/31600]. Actually it was quite simple to disable .delete() in querysets, but I had to define my own QuerySet class:

class QuerySet(models.query.QuerySet):
def delete(self):
raise NotImplementedError("delete is not implemented.")

I also disabled bulk_create() in managers for similar reasons. And I thought, maybe you can introduce optional settings in which if it's set, will disable delete() and bulk_create() in managers and querysets? I think this may be useful, since the delete() and bulk_create() methods have problems - they don't call the model's methods and in some cases (such as in my case) related objects from other models don't get deleted (with delete()) and models don't get validated (with bulk_create()).

I also call each model's self.full_clean() method before saving, which may also be a setting in Django. I don't want invalid objects to be saved to the database.

If you think this is a good idea and want me to implement it, I can try to submit a PR.

By the way, I checked and I found out that I can still delete objects from our admin interface, so it probably doesn't use the delete() queryset.

Thanks,
Uri.

אורי
<a href="javascript:" target="_blank" gdf-obfuscated-mailto="hEs8KMT5AQAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">u...@...

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/943a7ea3-074c-42cb-a259-42714e32fc11%40googlegroups.com.
Reply | Threaded
Open this post in threaded view
|

Re: Disabling .delete() in querysets in Django

Javier Buzzi
In reply to this post by Tom Forbes
More to Tom's point, I'm currently working on an old app that the original intent was to never delete anything, ever. The original programmers did something similar to what you did with the exception that they added a "deleted_ts" field to every model, the  model/queryset delete() would just add now() to the aforementioned field. This all seemed great on the surface, until we realized the app needed tests, they wrote none. Sometimes even in the more severe of cases, you want to do delete(), inside tests or otherwise. As an ongoing effort, we have created a soft_delete() and left delete() alone, created a Admin mixin called SoftDeleteMixin that adds an extra button in the details and a new action to the list view.

What you're suggesting definitely something that should be there, i really suggest you reconsider your approach.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/4f4b6233-dde9-4b20-81ba-10ff377a76a9%40googlegroups.com.