feat: add support for async operations #1
Conversation
DavidZheyuYang
changed the title
| self.created_db_config = False | ||
| if isinstance(database, str): |
There was a problem hiding this comment.
@davidbm04 let's restore these other if clauses for opening a PR with the original repo.
dynamic_db_router/router.py
Outdated
| def allow_syncdb(self, *args, **kwargs): | ||
| return None | ||
|
|
||
|
|
||
| from django.db import connections | ||
|
|
||
| THREAD_LOCAL = threading.local() | ||
|
|
||
| DB_FOR_READ_OVERRIDE = ContextVar('DB_FOR_READ_OVERRIDE', default='default') |
There was a problem hiding this comment.
clarification: Does ContextVar still support synchronous threads? Or does this only work for async?
There was a problem hiding this comment.
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
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
When are the enter and exit methods called in this context?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
…s str along with initial else condition that throws an error specifying what the database parameter should be of type. add comments and clean up
dynamic_db_router/router.py
Outdated
| # Run queries | ||
| class in_database: | ||
| """ | ||
| A context manager and decorator for setting a specific database for the duration of a block of code. |
There was a problem hiding this comment.
Shall we return the full comments here given we'll do a PR with the main repo?
No description provided.