[Django] #21174: Transaction handling broken in related manager modification methods
Django
----------------------------------------------+------------------------
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.6-beta-1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+------------------------
Some of the related manager methods do multiple data-modifying operations
but do not use any transaction management at all. This is clearly broken.
This isn't a regression, so isn't a release blocker for that reason.
However, this is a potential data corruption issue, so might be nice to
fix for 1.6, too.
I am not sure how to test this. Maybe post_delete/save signals could be
used to force exceptions and thus rollback.
--
Ticket URL: <https://code.djangoproject.com/ticket/21174>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | 1.6-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by akaariai):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Old description:
> Some of the related manager methods do multiple data-modifying operations
> but do not use any transaction management at all. This is clearly broken.
> This isn't a regression, so isn't a release blocker for that reason.
> However, this is a potential data corruption issue, so might be nice to
> fix for 1.6, too.
>
> I am not sure how to test this. Maybe post_delete/save signals could be
> used to force exceptions and thus rollback.
New description:
Some of the related manager methods do multiple data-modifying operations
but do not use any transaction management at all. This is clearly broken.
This isn't a regression, so isn't a release blocker for that reason.
However, this is a potential data corruption issue, so might be nice to
fix for 1.6, too.
I am not sure how to test this. Maybe post_delete/save signals could be
used to force exceptions and thus rollback.
EDIT: Forgot to add that a patch for some problematic methods available in
https://github.com/django/django/pull/1684.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:1>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | 1.6-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by loic84):
* cc: loic@… (added)
Comment:
This also has the benefit of fixing #21169 for GFK
RelatedManager.remove().
The same applies to FK RelatedManager.remove() which also suffers from
this issue. I would also implement it `self.using(db).filter(pk__in=[o.pk
for o in objs]).delete()`, solving both transaction handling and default
filtering at once.
I've got the test cases for these stashed in a branch for #21169.
One thing to note though is the mild backward incompatibility: signals
won't be fired anymore.
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:2>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | 1.6-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by akaariai):
The .delete() code takes care of sending signals. Unfortunately it will
also reload the instances back from DB to do so. It might be possible to
use the lower level deletion APIs to send the already existing objects to
be deleted in one go - no reload that way.
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:3>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | 1.6-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by loic84):
It just came back to mind that `RelatedManager.remove()` for FK doesn't
`delete()`, it only `SET_NULL`, there's a bit of an incompatibility
between FK et GFK here...
So yeah for FK the signal incompatibility would be changing the fly of
`save()` by an `update()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:4>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) | 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Anssi Kääriäinen <akaariai@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1df3c49a1a1c11198d181ddd0ce31bbb42e631d8"]:
{{{
#!CommitTicketReference repository=""
revision="1df3c49a1a1c11198d181ddd0ce31bbb42e631d8"
Fixed #21174 -- transaction control in related manager methods
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:5>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) | 1.6-beta-1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"17c3997f6828e88e4646071a8187c1318b65597d"]:
{{{
#!CommitTicketReference repository=""
revision="17c3997f6828e88e4646071a8187c1318b65597d"
Fixed #21169 -- Reworked RelatedManager methods use default filtering
The `remove()` and `clear()` methods of the related managers created by
`ForeignKey`, `GenericForeignKey`, and `ManyToManyField` suffered from a
number of issues. Some operations ran multiple data modifying queries
without
wrapping them in a transaction, and some operations didn't respect default
filtering when it was present (i.e. when the default manager on the
related
model implemented a custom `get_queryset()`).
Fixing the issues introduced some backward incompatible changes:
- The implementation of `remove()` for `ForeignKey` related managers
changed
from a series of `Model.save()` calls to a single `QuerySet.update()`
call.
The change means that `pre_save` and `post_save` signals aren't called
anymore.
- The `remove()` and `clear()` methods for `GenericForeignKey` related
managers now perform bulk delete so `Model.delete()` isn't called
anymore.
- The `remove()` and `clear()` methods for `ManyToManyField` related
managers perform nested queries when filtering is involved, which may
or may not be an issue depending on the database and the data itself.
Refs. #3871, #21174.
Thanks Anssi Kääriäinen and Tim Graham for the reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:6>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) | 1.6-beta-1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by loic84):
Fixed all the missing transactions I could find in `related.py`:
https://github.com/django/django/pull/2489.
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:7>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) | 1.6-beta-1
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"bc9be72bdc9bb4dfc7f967ac3856115f0a6166b8"]:
{{{
#!CommitTicketReference repository=""
revision="bc9be72bdc9bb4dfc7f967ac3856115f0a6166b8"
Fixed transaction handling for a number of operations on related objects.
Thanks Anssi and Aymeric for the reviews. Refs #21174.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21174#comment:8>