Skip to content

feat: add support for async operations #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 8, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 26 additions & 32 deletions dynamic_db_router/router.py
Original file line number Diff line number Diff line change
@@ -1,29 +1,35 @@
import threading
from functools import wraps
from uuid import uuid4
from contextvars import ContextVar

from django.db import connections

THREAD_LOCAL = threading.local()

DB_FOR_READ_OVERRIDE = ContextVar('DB_FOR_READ_OVERRIDE', default='default')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarification: Does ContextVar still support synchronous threads? Or does this only work for async?

Copy link

@davidbm04 davidbm04 May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, contextvar supports synchronous threads. I'll link the documentation but it seems that contextvar behaves similar to threading.local(), which is what was previously used, when values are assigned to different threads. I also tested this out by using the modifed context manager in a synchronous function and everything worked as expected. Here's the link to doc: contextvar

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic. Great work, David!

DB_FOR_WRITE_OVERRIDE = ContextVar('DB_FOR_WRITE_OVERRIDE', default='default')


class DynamicDbRouter(object):
"""A router that decides what db to read from based on a variable
local to the current thread.
"""

def db_for_read(self, model, **hints):
return getattr(THREAD_LOCAL, 'DB_FOR_READ_OVERRIDE', ['default'])[-1]

return DB_FOR_READ_OVERRIDE.get()
# return getattr(THREAD_LOCAL, 'DB_FOR_READ_OVERRIDE', ['default'])[-1]

def db_for_write(self, model, **hints):
return getattr(THREAD_LOCAL, 'DB_FOR_WRITE_OVERRIDE', ['default'])[-1]

return DB_FOR_WRITE_OVERRIDE.get()
# return getattr(THREAD_LOCAL, 'DB_FOR_WRITE_OVERRIDE', ['default'])[-1]

def allow_relation(self, *args, **kwargs):
return True

def allow_syncdb(self, *args, **kwargs):
return None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete extra spaces.

def allow_migrate(self, *args, **kwargs):
return None

Expand Down Expand Up @@ -83,45 +89,33 @@ def lowest_id_account():
def __init__(self, database, read=True, write=False):
self.read = read
self.write = write
self.database = database

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this code do in the event the database is a string? Is there any reason to remove the check below? Or was this code only checked with database being a dictionary?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If database is a string, the code assumes that it's a valid database alias already configured in Django’s DATABASES setting. No special handling is needed, we just use this string directly to reference the database. If database is a dictionary, we would need a more dynamic setup where you might not have a pre-configured database alias, and instead you would need to provide all necessary connection details. There isn't any reason to remove the check, since i was already defining the databases in the settings file, i was just focused on it working for strings. I'll add the check back in my next push

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's add it back. The explicit exception is helpful. Given we now have python 3, we could also do database: str | dict to make use of Python's type hints.

self.created_db_config = False
if isinstance(database, str):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidbm04 let's restore these other if clauses for opening a PR with the original repo.

self.database = database
elif isinstance(database, dict):
# Note: this invalidates the docs above. Update them
# eventually.
if isinstance(database, dict):
self.created_db_config = True
self.unique_db_id = str(uuid4())
connections.databases[self.unique_db_id] = database
self.database = self.unique_db_id
else:
msg = ("database must be an identifier for an existing db, "
"or a complete configuration.")
raise ValueError(msg)


def __enter__(self):
if not hasattr(THREAD_LOCAL, 'DB_FOR_READ_OVERRIDE'):
THREAD_LOCAL.DB_FOR_READ_OVERRIDE = ['default']
if not hasattr(THREAD_LOCAL, 'DB_FOR_WRITE_OVERRIDE'):
THREAD_LOCAL.DB_FOR_WRITE_OVERRIDE = ['default']
read_db = (self.database if self.read
else THREAD_LOCAL.DB_FOR_READ_OVERRIDE[-1])
write_db = (self.database if self.write
else THREAD_LOCAL.DB_FOR_WRITE_OVERRIDE[-1])
THREAD_LOCAL.DB_FOR_READ_OVERRIDE.append(read_db)
THREAD_LOCAL.DB_FOR_WRITE_OVERRIDE.append(write_db)
self.original_read_db = DB_FOR_READ_OVERRIDE.get()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When are the enter and exit methods called in this context?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if i understand this question... Isn't the enter and exit method called automatically when using with? enter should be called at the beginning and exit at the end: doc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have noted this is for my own education ;) I'd figured but I'd never written a context manager for Python so I wanted to confirm. Thanks for the learning, David!

self.original_write_db = DB_FOR_WRITE_OVERRIDE.get()
if self.read:
DB_FOR_READ_OVERRIDE.set(self.database)
if self.write:
DB_FOR_WRITE_OVERRIDE.set(self.database)
return self

def __exit__(self, exc_type, exc_value, traceback):
THREAD_LOCAL.DB_FOR_READ_OVERRIDE.pop()
THREAD_LOCAL.DB_FOR_WRITE_OVERRIDE.pop()
DB_FOR_READ_OVERRIDE.set(self.original_read_db)
DB_FOR_WRITE_OVERRIDE.set(self.original_write_db)
if self.created_db_config:
connections[self.unique_db_id].close()
del connections.databases[self.unique_db_id]

def __call__(self, querying_func):
@wraps(querying_func)
def inner(*args, **kwargs):
# Call the function in our context manager
with self:
return querying_func(*args, **kwargs)
return inner