[Django] #17520: write queries on related managers use the db_for_read database
Django
----------------------------------------------+--------------------
Reporter: tobias | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
If you execute a write query (update() or delete()) on a queryset obtained
from a related manager, it appears to use the db_for_read database instead
of the db_for_write database for that query.
The attached patch reproduces the issue on trunk. It also exists in
1.3.1.
--
Ticket URL: <https://code.djangoproject.com/ticket/17520>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by tobias):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
The issue appears to be that query._db gets set when the queryset is
created from a related manager. As such, when the !QuerySet.db property
checks to see what write db to use, it assumes using() has been called
manually and does not call db_for_write on the router as it should.
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:1>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Changes (by tobias):
* needs_better_patch: 0 => 1
Comment:
The patch only has a test currently (not sure why that got unset, I didn't
uncheck anything).
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:2>
Django
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by lrekucki):
* component: Database layer (models, ORM) => Documentation
* stage: Unreviewed => Accepted
Comment:
I don't think this is a bug. Django by default saves the objects to the
database they we're fetched from. When using a related manager on a
fetched object, Django assumes you want them fetched from the same DB
instead of consulting the router (both for read or write, at least that's
what is currently tested for) as it's the reasonable thing to do in most
cases.
If you want to specify another database you can use {{{db_manager()}}}.
Passing {{{None}}} to it should force the manager to consulting the
router. That said, this most likely needs better explanation in the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:3>
Django
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Comment:
Indeed, the test case looks valid and fails.
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:3>
Django
Reporter: tobias | Owner: slafs
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => slafs
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:4>
Django
Reporter: tobias | Owner: slafs
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by slafs):
The problem seems to be very subtle. The thing is that the `update` method
on queryset is using `self.db` which uses `self._db` to determine the
database. When we have a related manager, the queryset from it comes with
"polluted" `_db`
([https://github.com/django/django/blob/dc8814bf7dc5139f3d11b8f9f44f54c7f77d2714/django/db/models/fields/related.py#L407,L408
because of this] ).
When i tried a simplest approach with cleaning the _db after getting the
`qs` like this:
{{{
if self._db is None:
qs._db = None
}}}
it turns out that there's some feature tests that gets broken. For example
[https://github.com/django/django/blob/dc8814bf7dc5139f3d11b8f9f44f54c7f77d2714/tests/multiple_database/tests.py#L426
this] (which I think isn't documented anywhere). Will try to propose
something different.
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:5>
Django
Reporter: tobias | Owner: slafs
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by slafs):
I proposed a first solution in
https://github.com/slafs/django/commit/e1d3f0cd6af2ee54e7bdd0ab29cfae27d90483dd
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:6>
Django
Reporter: tobias | Owner: slafs
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
I attached a newer version of tobias patch.
I also added a patch with a test case that shows that the ManyToMany
scenario is also broken (you can check it
[https://github.com/slafs/django/commit/0b647b06b22f3b94b7e4a3a28b508517a607352a
here] also)
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:7>
Django
Reporter: tobias | Owner: slafs
Type: Bug | Status: assigned
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by slafs):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:8>
Django
Reporter: tobias | Owner: slafs
Component: Database layer | Version: 1.3
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: assigned => closed
* resolution: => fixed
Comment:
Fixed in Django 1.7 with #13724 /
9595183d03cfd0d94ae2dd506a3d2b86cf5c74a7.
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:9>
Django
Reporter: tobias | Owner: slafs
Type: Bug | Status: closed
Component: Database layer | Version: 1.3
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by tobiasmcnulty):
Many thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/17520#comment:10>