[Django] #33501: order_with_respect_to uses incorrect database alias
Django
-------------------------------------+-------------------------------------
Reporter: Damian | Owner: nobody
Posener |
Type: | Status: new
Uncategorized |
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When calling set_RELATED_order on a model that uses order_with_respect_to,
if a DB router exists that routes that model to a non-default database,
the routing is ignored, causing a datbase error to occur.
I've tracked this down to the method_set_order function here:
https://github.com/django/django/blob/3.2.12/django/db/models/base.py#L2117
This function updates `using=None` to `using=DEFAULT_DB_ALIAS`, which then
overrides any database routing set up on the model manager. It should just
pass through `using=None` so that the manager can deal with determining
the correct connection to use.
I believe this got inadvertently broken in #30106 - previously the `using`
parameter was actually being ignored, but when that got fixed it caused
this new error. :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33501>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> When calling set_RELATED_order on a model that uses
> order_with_respect_to, if a DB router exists that routes that model to a
> non-default database, the routing is ignored, causing a datbase error to
> occur.
>
> I've tracked this down to the method_set_order function here:
> https://github.com/django/django/blob/3.2.12/django/db/models/base.py#L2117
>
> This function updates `using=None` to `using=DEFAULT_DB_ALIAS`, which
> then overrides any database routing set up on the model manager. It
> should just pass through `using=None` so that the manager can deal with
> determining the correct connection to use.
>
> I believe this got inadvertently broken in #30106 - previously the
> `using` parameter was actually being ignored, but when that got fixed it
> caused this new error. :)
New description:
PR: https://github.com/django/django/pull/15409
When calling set_RELATED_order on a model that uses order_with_respect_to,
if a DB router exists that routes that model to a non-default database,
the routing is ignored, causing a datbase error to occur.
I've tracked this down to the method_set_order function here:
https://github.com/django/django/blob/3.2.12/django/db/models/base.py#L2117
This function updates `using=None` to `using=DEFAULT_DB_ALIAS`, which then
overrides any database routing set up on the model manager. It should just
pass through `using=None` so that the manager can deal with determining
the correct connection to use.
I believe this got inadvertently broken in #30106 - previously the `using`
parameter was actually being ignored, but when that got fixed it caused
this new error. :)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33501#comment:1>
Django
-------------------------------------+-------------------------------------
| Posener
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* cc: Simon Charette (added)
* needs_tests: 0 => 1
* owner: nobody => Damian Posener
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
Thanks for the report.
--
Ticket URL: <https://code.djangoproject.com/ticket/33501#comment:2>
Django
-------------------------------------+-------------------------------------
| Posener
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Interestingly this means that the code prior to
8d2dcc47cd7f6069a1fa2c5b67a329cec5846b7e was broken in another way. It use
to open a transaction against the default database but the following
chunked updates were performed against whatever `db_for_write` returned
afterwards making them non-atomic when performed against the non-default
database.
This makes me realize that `bulk_update` doesn't set `self._for_write =
True` before accessing `self.db` (like all other create and update
queryset methods do) which means the aforementioned issue is affects this
function as well so I'll file another issue for it and assign it to
myself.
You patch looks great Damian! It'd be even better if you write a test that
ensures the updates are performed against the right database to prevent
future regressions. I suggest you `grep` the test directory for
`override_settings(DATABASE_ROUTERS` to get an idea of how you can route
queries to the `other` test database and use
`assertNumQueries(using='other')` to ensure it was properly routed.
--
Ticket URL: <https://code.djangoproject.com/ticket/33501#comment:3>
Django
-------------------------------------+-------------------------------------
| Posener
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d35ce682e31ea4a86c2079c60721fae171f03d7c" d35ce682]:
{{{
#!CommitTicketReference repository=""
revision="d35ce682e31ea4a86c2079c60721fae171f03d7c"
Fixed #33506 -- Made QuerySet.bulk_update() perform atomic writes against
write database.
The lack of _for_write = True assignment in bulk_update prior to
accessing self.db resulted in the db_for_read database being used to
wrap batched UPDATEs in a transaction.
Also tweaked the batch queryset creation to also ensure they are
executed against the same database as the opened transaction under all
circumstances.
Refs #23646, #33501.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33501#comment:4>
Django
-------------------------------------+-------------------------------------
| Posener
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Damian Posener):
Replying to [comment:3 Simon Charette]:
> You patch looks great Damian! It'd be even better if you write a test
that ensures the updates are performed against the right database to
prevent future regressions. I suggest you `grep` the test directory for
`override_settings(DATABASE_ROUTERS` to get an idea of how you can route
queries to the `other` test database and use
`assertNumQueries(using='other')` to ensure it was properly routed.
Thanks for the pointer, I've managed to sort out a regression test and
have added it to my PR above. 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/33501#comment:5>
Django
-------------------------------------+-------------------------------------
| Posener
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33501#comment:6>
Django
-------------------------------------+-------------------------------------
| Posener
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"09e499a39eec9342cb594bfd27939cfd1046f797" 09e499a3]:
{{{
#!CommitTicketReference repository=""
revision="09e499a39eec9342cb594bfd27939cfd1046f797"
Fixed #33501 -- Made order_with_respect_to respect database routers.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33501#comment:7>