Opened 3 years ago

Closed 3 years ago

#32788 closed New feature (needsinfo)

Transaction APIs do not consult the DB router to choose DB connection

Reported by: Aditya N Owned by: nobody
Component: Database layer (models, ORM) Version: 3.2
Severity: Normal Keywords: transaction API atomic DB router
Cc: Aditya N Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

From the Django docs, for any ORM query made, the DB alias to use is determined by the following rules:

  • Checks if the using keyword is used either as a parameter in the function call or chained with QuerySet.
  • Consults the DB routers in order until a match is found.
  • Falls back to the default router which always returns default as the alias.

Using the router scheme works perfectly for ORM queries. However, when it comes to using transaction APIs ​(like the transaction.atomic construct), it becomes mandatory to specify the using kwarg explicitly in all of its publicly exposed APIs if a DB other than the default alias has to be chosen.

To illustrate why this is a problem, consider the following scenario:

  • A DB router exists such that it directs queries to a specific database based on a value set in thread-local storage by a middleware.
  • When ORM queries from within the view are fired, they automatically get forwarded to the right DB without explicitly citing the using keyword owing to the router.
  • But if the transaction.atomic construct is used, the using keyword would have to be explicitly specified each time. While this might seem fine, it creates the following problems:
    1. For large code bases, it becomes highly unwieldy to make sure that the using keyword has been mentioned in every transaction API call. Also, if one tries to implement the above scheme in an already existing code base, it would be impractical to add the using keyword in all lines calling the transaction APIs.
    2. It doesn't gel well with the the routers framework.

A proposed work around could to be to add a separate method named db_for_transaction to the routers framework which would be consulted by the transaction APIs first, before falling back to using the default DB alias.

If we can finalise on this, I could submit a PR for the same.

Change History (4)

comment:1 by Aditya N, 3 years ago

Type: UncategorizedBug

comment:2 by Aditya N, 3 years ago

Cc: Aditya N added

comment:3 by Simon Charette, 3 years ago

Thank you for your detailed report but this feature request would likely get more feedback exposure on the mailing list.

A proposed work around could to be to add a separate method named db_for_transaction to the routers framework which would be consulted by the transaction APIs first, before falling back to using the default DB alias.

Can you think of places where this db_for_transaction hook would differ in any way from what db_for_write returns? That's what Django uses internally in such instances

  1. https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L745-L760
  2. https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L950
  3. https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/deletion.py#L400

I get that your asking for a way to avoid explicitly passing atomic(using) all over the place but I'm having a hard time figuring out in which cases a db_for_transaction hook, which cannot be provided any contextual details like other router methods do, can take an an advised decision without relying on global state/thread locals.

Maybe that a better API would be a transaction.default_database(using: str) context manager that abstracts the context state encapsulation?

comment:4 by Mariusz Felisiak, 3 years ago

Resolution: needsinfo
Status: newclosed
Type: BugNew feature

As Simon said, you'll reach a wider audience if you write to the DevelopersMailingList about your ideas. We should reach a consensus on the mailing list before moving this forward.

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