Opened 3 years ago

Closed 3 years ago

#33501 closed Bug (fixed)

order_with_respect_to uses incorrect database alias

Reported by: Damian Posener Owned by: Damian Posener
Component: Database layer (models, ORM) Version: 3.2
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Damian Posener)

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. :)

Change History (7)

comment:1 by Damian Posener, 3 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Simon Charette added
Needs tests: set
Owner: changed from nobody to Damian Posener
Status: newassigned
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Thanks for the report.

comment:3 by Simon Charette, 3 years ago

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.

comment:4 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In d35ce682:

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.

in reply to:  3 comment:5 by Damian Posener, 3 years ago

Replying to 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. 👍

comment:6 by Mariusz Felisiak, 3 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 09e499a3:

Fixed #33501 -- Made order_with_respect_to respect database routers.

Note: See TracTickets for help on using tickets.
Back to Top