Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
realnot opened this issue Oct 19, 2022 · 7 comments
Closed

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

realnot opened this issue Oct 19, 2022 · 7 comments

Comments

@realnot
Copy link

realnot commented Oct 19, 2022

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:
@mthuurne
Copy link
Contributor

mthuurne commented Mar 17, 2023

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 added a commit to ProtixIT/django-model-utils that referenced this issue Mar 17, 2023
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
@mthuurne
Copy link
Contributor

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

mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 20, 2023
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 21, 2023
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
@Mogost
Copy link
Member

Mogost commented Nov 27, 2023

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.

mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 22, 2024
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 26, 2024
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Mar 27, 2024
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Apr 5, 2024
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Apr 9, 2024
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Apr 10, 2024
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
mthuurne added a commit to ProtixIT/django-model-utils that referenced this issue Apr 10, 2024
I don't know whether returning "nothing was deleted" is the best
option, but at least the returned value now has the correct type.

jazzband#541
@mthuurne
Copy link
Contributor

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.

@gmcrocetti
Copy link
Contributor

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
Copy link
Contributor

#622

@charettes
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants