[Django] #21169: Deletion in custom reverse managers
Django
----------------------------------------------+--------------------
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
The newly implemented custom reverse managers (cf. #3871) expose
potentially serious behavior when `remove()` and `clear()` are called:
instead of limiting the effects of the removal to the objects considered
related by the chosen custom manager, the methods fall back to the
unrestricted behavior of the default manager, removing more objects from
the relation than is desired.
I'm attaching a test which illustrates the problem for reverse foreign
keys with custom managers. I suppose the same behavior is present when
using generic foreign keys as well as for the forward and backward sides
of many-to-many relations.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sebastian):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
In my patch proposal patch `r17204-custom-reverse-managers.diff` in #3871,
I considered the problem and then took the easy way out by simply
disallowing the `remove()` and `clear()` methods on custom reverse
managers. Obviously, this solution is far from perfect.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:1>
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
I think I am going to revert #3871, we can then improve the patch and
recommit. Of course, if we can solve this in agreed upon way really soon
there is no need to revert + repatch.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:2>
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sebastian):
Unfortunately, I am not entirely sure how to proceed here. When I wrote my
patch proposal for #3871, I remember it wasn't possible to make `remove()`
and many-to-many's `clear()` work as expected without any additional
overhead (necessary to make sure so that we do not remove more objects
from the relation as intended).
That is the reason why I decided to entirely forbid `remove()` and many-
to-many's `clear()` when their outcome would be unexpected. Is this a
reasonable approach here, as well? Ideally, providing these methods would
be great but are they even possible without issuing another query (i.e.,
to make sure that the object removed is actually part of the collection in
case of `remove()`)?
Due to the way it is implemented, foreign-key's `clear()` already works as
expected. So the problem lies only in foreign-key's and many-to-many's
`remove()` and many-to-many's `clear()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:3>
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* stage: Unreviewed => Accepted
Comment:
Hmmh, lets just go for "forbid operations" approach. These methods can be
added back later on if we can fix them in clean way.
Actually, lets go for "no related manager methods" approach. You can also
do __set__ which could clear existing instances outside the manager's
viewable set, and .add() could add something that isn't viewable by the
manager.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:4>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => loic84
* status: new => assigned
Comment:
The problem is not limited to the `__call__` syntax, the same would happen
to default managers.
I'm working on a fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:5>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
I've reworked M2M `RelatedManager.clear()` to account for the default
filtering through a subquery.
https://github.com/django/django/pull/1685
As previously noted, GFK and FK `RelatedManager.clear()` weren't affected
by the issue.
Also as discussed on IRC, `remove()` seems less critical since you specify
the objects manually, so we might get away with a warning in the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:6>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
Just a note to those who try to fix release blockers for 1.6: the release
blocker is for master, not 1.6.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:7>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I did some work to make sure queries used by m2m clear() and remove() are
as simple as possible. Now the clearing queries do not use subqueries when
the manager doesn't have filters. The subqueries in these cases didn't
actually filter anything out. In addition, when subqueries are used, the
subquery has one less join than before. End result is that the queries are
similar to current master when the base manager doesn't have filters, and
a bit more efficient that current HEAD of pull 1685 when there are
filters. The tradeoff is even more complexity in m2m manager
implementation.
To me it seems important that as simple as possible queries are used. This
ensures as little DB resources are used as possible. In addition I believe
old versions of MySQL will simply panic when they see a query with both a
subquery and joins...
Patch at
https://github.com/akaariai/django/commit/9dd75213050e6f85a0b93ddd59243f8d3809b06c
- this doesn't have any tests, and regressions in query efficiency seem
likely. I wonder if CaptureQueriesContext could be used to inspect the
used queries.
BTW I spotted one possible data-corruption issue in current clear() code
for symmetric m2m. Currently the clear is implemented as two separate
delete queries which delete all items from the relation, first in one
direction, then in another. But, if a concurrent add() commits in between
then the added relation is deleted in one direction, but not in anohter.
The sequence is like this for transactions T1 and T2:
{{{
T1: add relation for instances m1 <-> m2. Two rows are added so that the
relation is symmetrical (rows m1:m2 and m2:m1).
T2: m1.clear() starts. First, delete all rows where m1:*. m1:m2 isn't
deleted as that isn't yet visible.
T1: commits, m1:m2 and m2:m1 become visible to T2.
T2: Second part of clear is executed, that is *:m1 is deleted. Now m2:m1
is visible and it is deleted.
T2: commits. m1:m2 relation exists, but m2:m1 doesn't. The symmetry of
m1<->m2 is broken.
}}}
Luckily this seems rare in practice. pull 1685 does the delete in one go,
so this issue is fixed in PR1685. This seems to be impossible to test in a
way that the regression test actually picks up regressions. So, no tests
needed for this case.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:8>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_docs: 0 => 1
* has_patch: 0 => 1
Comment:
For the record I'm going to list all the changes and issues addressed by
this ticket and #21174 since they are related.
The culprits: the `clear()` and `remove()` methods of the M2M, FK, and GFK
related managers, so six methods in total.
Issues:
- All methods except `FK.clear()` completely bypassed default filtering
(i.e. custom `Manager.get_queryset()` with filters).
- All methods except `FK.clear()` caused multiple consecutive db
operations run without transaction. (see #21174)
- Inconsistency between the implementation of `FK.remove()` and
`FK.clear()`, in particular wrt emitted signals.
- `M2M.clean()` and `M2M.remove()` were prone to race conditions for
symmetrical relations. (see #comment:8)
List of changes:
- All methods now respect default filtering, which means you can't delete
or change something that is filtered out.
- All methods now run in a single query, which addresses all transaction
concerns.
- `FK.remove()` changed from successive `save()` calls, to a single
`update()`.
Gotcha: `pre_save` and `post_save` signals aren't fired anymore.
- `M2M.remove()` and `M2M.clear()` rely on subqueries when filtering is
involved.
- Symmetrical `M2M.remove()` and `M2M.clear()` now run in a single query.
- `GFK.remove()` and `GFK.clear()` now perform bulk delete.
Gotcha: `Model.delete()` is not called anymore.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:9>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by loic84):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:10>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
The plan is to introduce pre/post update signals, and after that merge the
changes from this ticket in.
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:11>
Django
Reporter: sebastian | Owner: loic84
Component: Database layer | Version: master
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"01e8ac47b37ccf1b4d9b9710ae2d9b6b486c3c7a"]:
{{{
#!CommitTicketReference repository=""
revision="01e8ac47b37ccf1b4d9b9710ae2d9b6b486c3c7a"
PEP-8 cleanup
Refs #21169
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:15>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"52015b963d263642957aec880b52ad4063b484cd"]:
{{{
#!CommitTicketReference repository=""
revision="52015b963d263642957aec880b52ad4063b484cd"
Used simpler queries for m2m clearing when possible.
Refs #21169
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:13>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
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/21169#comment:12>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"f450bc9f44bc1270eae911daa157c155c29d1d9d"]:
{{{
#!CommitTicketReference repository=""
revision="f450bc9f44bc1270eae911daa157c155c29d1d9d"
Added a bulk option to RelatedManager remove() and clear() methods
Refs #21169
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:14>
Django
Reporter: sebastian | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"91fce675a453764cb233e53c0460826600c828fa"]:
{{{
#!CommitTicketReference repository=""
revision="91fce675a453764cb233e53c0460826600c828fa"
Use 'update_fields' in RelatedManager.clear() when bulk=False.
Thanks Simon Charette for the suggestion.
Refs #21169.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21169#comment:16>