django-model-utils icon indicating copy to clipboard operation
django-model-utils copied to clipboard

delete() method don't returns number of affected rows

Open realnot opened this issue 3 years ago • 6 comments

Problem

As documentation says: https://docs.djangoproject.com/en/dev/ref/models/querysets/#django.db.models.query.QuerySet.delete

Performs an SQL delete query on all rows in the QuerySet and returns the number of objects deleted and a dictionary with the number of deletions per object type.

Instead overridden delete() method return None

Tips: since you are performing an update and not a delete, please consider using bulk_update.

Environment

  • Django Model Utils version: 4.2.0
  • Django version: 4.1
  • Python version: 3.9
  • Other libraries used, if any:

realnot avatar Oct 19 '22 15:10 realnot

I'm running into the same issue when adding type annotations: mypy warns that SoftDeletableQuerySetMixin.delete() is incompatible with QuerySet.delete().

What would be the correct returned value: (0, {}) because nothing was actually deleted, or should the pretend-deleted objects be counted?

The latter might be tricky, as it would have to take into account how many objects already had is_removed set to True prior to this query.

mthuurne avatar Mar 17 '23 15:03 mthuurne

Note that SoftDeletableModel has the same issue as SoftDeletableQuerySetMixin in the case soft deletion is used.

mthuurne avatar Mar 20 '23 12:03 mthuurne

I think soft deletion is supposed to mimic normal deletion. So suppose you do a soft delete of something and show the user that N records have been deleted. To him, those are the real N records that have been deleted. The fact that they are soft deleted is more your internal implementation.

Mogost avatar Nov 27 '23 21:11 Mogost

I wrote:

What would be the correct returned value: (0, {}) because nothing was actually deleted, or should the pretend-deleted objects be counted?

The latter might be tricky, as it would have to take into account how many objects already had is_removed set to True prior to this query.

Mogost wrote:

I think soft deletion is supposed to mimic normal deletion. So suppose you do a soft delete of something and show the user that N records have been deleted. To him, those are the real N records that have been deleted. The fact that they are soft deleted is more your internal implementation.

I agree that would be the most intuitive behavior from the user's perspective.

Maybe a pragmatic solution would be to return the number of objects updated by soft-delete, whether they were previously soft-deleted or not. In practice, it's likely that objects that were already soft-deleted would be not be included in a query to soft-delete other records, as SoftDeletableManager filters out soft-deleted objects.

mthuurne avatar Apr 12 '24 13:04 mthuurne

I believe we can easily mimic the original API ?

class SoftDeletableQuerySetMixin:
    """
    QuerySet for SoftDeletableModel. Instead of removing instance sets
    its ``is_removed`` field to True.
    """

    def delete(self):
        """
        Soft delete objects from queryset (set their ``is_removed``
        field to True)
        """
        number_of_deleted_objects = self.update(is_removed=True)
        return number_of_deleted_objects, {self.model._meta.label: number_of_deleted_objects}

This is valid because

  • We don't need to worry about related objects as when we're "hard" deleting the records
  • All we need to count is the model being updated (the dict part mimics this line)

What you folks think ?

gmcrocetti avatar Jun 06 '24 01:06 gmcrocetti

https://github.com/jazzband/django-model-utils/pull/622

gmcrocetti avatar Jun 06 '24 18:06 gmcrocetti

Closing since #622 was merged and part of the recent 5.0 release.

charettes avatar Sep 11 '24 12:09 charettes