Opened 5 years ago

Closed 19 months ago

Last modified 12 months ago

#16891 closed New feature (fixed)

QuerySet.delete() should return number of rows matched

Reported by: Steven Cummings Owned by: Steven Cummings
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Sergey Kolosov, dev@…, Zach Borboa Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Carl Meyer)

Splitting this off from ticket #16549...

Deep in the bowels of django.db the rows modified value from update and delete queries is ignored and discarded. For reasons discussed on ticket 16549, it would sometimes be useful to have access to that value.

Objective of this bug is to passively return rows-modified up through the call-stack and, ultimately, hopefully from each of:

  • Model.save()
  • Model.delete()
  • QuerySet.update()
  • QuerySet.delete()

with consideration for transaction control/mgmt.

Update from comment thread: queryset update already returns rows-matched (and this can't be changed to rows-changed without breaking other things). So it is only deletes that need this change.

Attachments (1)

delete_counts.patch (1.1 KB) - added by Steven Cummings 5 years ago.
Patch to changes so far from git clone

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by Steven Cummings

Owner: changed from nobody to Steven Cummings

Claiming to work on.

comment:2 Changed 5 years ago by Carl Meyer

Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

comment:3 Changed 5 years ago by Steven Cummings

Finally got around to these simple changes:

If the core devs approve of these changes I can get to work on creating the proper patch for SVN and add doc changes.

comment:4 Changed 5 years ago by Anssi Kääriäinen

An idea for the save() patch: return UPDATED, INSERTED, or None instead of 1/0. You could still do if obj.save(): (matches both UPDATED and INSERTED) but if need be, you could see if the object was indeed updated or inserted. The value would be the "outermost" value for multitable inherited models.

I wonder if it would be a good idea to return the amount of deletions per object type when deleting? I see at least three choices:

  • Just a dict model_class -> delete count, for example in your test case this would be {R:1, S:2, T:2}
  • Tuple total_objects_deleted, abovementioned dict: Example: (5, {R:1, S:2, T:2})
  • Tuple amount_of_outer_objs, abovementioned dict: Example: (1, {R:1, S:2, T:2})

To me it seems these could be useful, especially the #2 format. This information can be gotten nearly for free. The inconvenience here is that you can't do if obj.delete(): as (0, {}) isn't False. Maybe this could be postponed for later inclusion with kwarg with_counts=True or something. Or maybe there is just no use case for this.

comment:5 Changed 5 years ago by Steven Cummings

In case anybody is paying more attention here than on django-dev, I made an update to roll the changes back to just the internal query classes for now:

Changed 5 years ago by Steven Cummings

Attachment: delete_counts.patch added

Patch to changes so far from git clone

comment:6 Changed 5 years ago by Steven Cummings

Has patch: set

I haven't gotten much feedback on this ticket, but what I've gotten so far has convinced me to back off of any changes to the base Model API. UpdateQuery already returned rowcounts, which means that only DeleteQuery needed updating. So, the patch is pretty tiny as you can see.

comment:7 Changed 5 years ago by Steven Cummings

Summary: Delete/update should return number of rows modified[patch] Delete/update should return number of rows modified

Updating summary

comment:8 Changed 5 years ago by Aymeric Augustin

#17435 was a duplicate.

comment:9 in reply to:  6 Changed 5 years ago by Sergey Kolosov

Cc: Sergey Kolosov added

Replying to estebistec:

I haven't gotten much feedback on this ticket, but what I've gotten so far has convinced me to back off of any changes to the base Model API. UpdateQuery already returned rowcounts, which means that only DeleteQuery needed updating. So, the patch is pretty tiny as you can see.

Since QuerySet.update returns not the number of rows affected, but the number of rows matched (as I’ve described it in #17435), I believe the UpdateQuery still needs to be patched.

comment:10 Changed 5 years ago by Steven Cummings

Yeah, if there are no objections to rolling #17435 into this bug, I'll go back in and add QuerySet.update to the patch.

comment:11 Changed 5 years ago by Karen Tracey

No, I don't think update() can be switched to start returning the actually-modified row count rather than matched row count. See #17435 for why, the fix for that one is to clarify the doc.

comment:12 in reply to:  11 Changed 5 years ago by Steven Cummings

Replying to kmtracey:

No, I don't think update() can be switched to start returning the actually-modified row count rather than matched row count. See #17435 for why, the fix for that one is to clarify the doc.

So I need to take a closer look here too and see if the delete() counts can mirror update() in that way.

comment:13 Changed 5 years ago by anonymous

Patch needs improvement: set

comment:14 Changed 5 years ago by Carl Meyer

Description: modified (diff)
Summary: [patch] Delete/update should return number of rows modified[patch] queryset delete should return number of rows matched

comment:15 Changed 5 years ago by Steven Cummings

Patch needs improvement: unset

The ambiguity discussed in #17435 doesn't apply to deletes. The number of rows matched and affected can't differ, as a match always leads to a delete. So I think the existing patch is fine in this respect.

Are there any other improvements needed in this patch? It doesn't look like the objects at this level are generally tested in the Django test-suite. On the one hand, more tests are always good. On the other, the delete queries aren't part of Django's published object model (for me, they're an implementation detail change along the way to ticket #16549).

comment:16 Changed 4 years ago by Tim Graham

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Summary: [patch] queryset delete should return number of rows matchedQuerySet.delete() should return number of rows matched

I marked #12086 as a duplicate. The attached patch doesn't make QuerySet.delete() return the number of deleted objects like I would expect (based on what's suggested by the ticket summary). This also needs tests and docs.

comment:17 Changed 2 years ago by Brillgen Developers

Cc: dev@… added

comment:18 Changed 2 years ago by Zach Borboa

Cc: Zach Borboa added

comment:19 Changed 21 months ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Version: 1.3master

PR which I've reviewed.

comment:20 Changed 19 months ago by Alexander

Patch needs improvement: unset

comment:21 Changed 19 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 04e8d890:

Fixed #16891 -- Made Model/QuerySet.delete() return the number of deleted objects.

comment:22 Changed 12 months ago by Simon Charette <charette.s@…>

In 8035cee9:

Fixed #25882 -- Prevented fast deletes matching no rows from crashing on MySQL.

Thanks to Trac aliases gerricom for the report, raphaelmerx for the
attempts to reproduce and Sergey Fedoseev and Tim for the review.

Refs #16891

comment:23 Changed 12 months ago by Simon Charette <charette.s@…>

In c402db2e:

[1.9.x] Fixed #25882 -- Prevented fast deletes matching no rows from crashing on MySQL.

Thanks to Trac aliases gerricom for the report, raphaelmerx for the
attempts to reproduce and Sergey Fedoseev and Tim for the review.

Refs #16891

Backport of 8035cee92293f3319919c8248c7787ba43c05917 from master

Note: See TracTickets for help on using tickets.
Back to Top