[Django] #16891: Delete/update should return number of rows modified
Django
---------------------------+-------------------------------
Reporter: estebistec | Owner: nobody
Type: Uncategorized | Status: new
Milestone: | Component: Uncategorized
Version: 1.3 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------+-------------------------------
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.
* https://code.djangoproject.com/ticket/16549
--
Ticket URL: <https://code.djangoproject.com/ticket/16891>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: estebistec | Owner: estebistec
Milestone: | Component: Uncategorized
Version: 1.3 | Severity: Normal
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+-------------------------------
Changes (by estebistec):
* owner: nobody => estebistec
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Claiming to work on.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:1>
Django
Reporter: | Owner: estebistec
estebistec | Status: new
Type: New | Component: Database layer
feature | (models, ORM)
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Triage Stage: Accepted | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by carljm):
* component: Uncategorized => Database layer (models, ORM)
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:2>
Django
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Comment (by estebistec):
Finally got around to these simple changes:
*
https://github.com/estebistec/django/commit/b48a87afc324f5546b6654fa7638e406b397c0d6
*
https://github.com/estebistec/django/commit/28ace32980b370fd17ae35019bfe8d055c673684
If the core devs approve of these changes I can get to work on creating
the proper patch for SVN and add doc changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:3>
Django
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by akaariai):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:4>
Django
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by estebistec):
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:
*
https://github.com/estebistec/django/commit/f178b72af132dd1ba588b89fe6915f5e9ba841d0
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:5>
Django
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by estebistec):
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Comment (by estebistec):
Updating summary
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Comment (by aaugustin):
#17435 was a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by sergeykolosov):
* cc: sergeykolosov (added)
Comment:
Replying to [comment:6 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Comment (by estebistec):
Yeah, if there are no objections to rolling #17435 into this bug, I'll go
back in and add QuerySet.update to the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Comment (by estebistec):
Replying to [comment:11 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by anonymous):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Description changed by carljm:
Old description:
> 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.
>
> * https://code.djangoproject.com/ticket/16549
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.
matched (and this can't be changed to rows-changed without breaking other
things). So it is only deletes that need this change.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by estebistec):
* needs_better_patch: 1 => 0
Comment:
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
* needs_tests: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by brillgen):
* cc: dev@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | estebistec
Component: Database layer | Status: new
(models, ORM) | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by zborboa-google):
* cc: zborboa-google (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 1 => 0
* version: 1.3 => master
* needs_tests: 1 => 0
Comment:
[https://github.com/django/django/pull/4258 PR] which I've reviewed.
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
| estebistec
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by BlindHunter):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
| estebistec
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"04e8d890aec8e996d568565555164a27a6a76057" 04e8d890]:
{{{
#!CommitTicketReference repository=""
revision="04e8d890aec8e996d568565555164a27a6a76057"
Fixed #16891 -- Made Model/QuerySet.delete() return the number of deleted
objects.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
| estebistec
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"8035cee92293f3319919c8248c7787ba43c05917" 8035cee9]:
{{{
#!CommitTicketReference repository=""
revision="8035cee92293f3319919c8248c7787ba43c05917"
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: estebistec | Owner:
| estebistec
Type: New feature | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"c402db2e2f3315dc3292526a6bdae36630026dfe" c402db2e]:
{{{
#!CommitTicketReference repository=""
revision="c402db2e2f3315dc3292526a6bdae36630026dfe"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16891#comment:23>