Changing DatabaseWrapper._cursor() to take a dict instead of settings
Adrian Holovaty
am building a custom manager that lets me specify which DATABASE_NAME
to use for a particular model. It looks something like this:
class OtherDatabaseManager(models.Manager):
"""
This Manager lets you set the DATABASE_NAME on a per-model basis.
"""
def __init__(self, database_name, *args, **kwargs):
models.Manager.__init__(self, *args, **kwargs)
self.database_name = database_name
def get_query_set(self):
qs = models.Manager.get_query_set(self)
qs.query.connection = self.get_db_wrapper()
return qs
def get_db_wrapper(self):
from django.db.backends.postgresql_psycopg2.base import DatabaseWrapper
# Monkeypatch the settings file. This is not thread-safe!
old_db_name = settings.DATABASE_NAME
settings.DATABASE_NAME = self.database_name
wrapper = DatabaseWrapper()
wrapper._cursor(settings)
settings.DATABASE_NAME = old_db_name
return wrapper
class MyModel(models.Model):
# ...
objects = OtherDatabaseManager('my_custom_database)
This is not unlike the code here:
http://www.eflorenzano.com/blog/post/easy-multi-database-support-django/
--
One ugliness about this is that it has to monkeypatch the settings
file in order to change the DATABASE_NAME, before passing it to
DatabaseWrapper._cursor(). So my proposal is to change
DatabaseWrapper._cursor() to accept a settings *dictionary* instead of
a settings *object*.
The only place in the Django codebase that calls
DatabaseWrapper._cursor() is BaseDatabaseWrapper.cursor() in
db/backends/__init__.py. That, plus the fact that this method name is
preceded by an underscore, leads me to believe there are no
backwards-incompatibility issues with making this change. But I'm
bringing it up here in case anybody wants to point out problems before
I commit it.
Adrian
Alex Gaynor
Alex
--
"I disapprove of what you say, but I will defend to the death your right to say it." --Voltaire
"The people's good is the highest law."--Cicero
Adrian Holovaty
> I've been looking at adding multiple database support to Django for a little
> while now and as a part of my plan of action the first step is to remove all
> references to the global settings in django.db.backends.* and replace them
> with a settings dict as you suggest(minus stuff like settings.DEBUG
> obviously). My proposal goes a little further than yours however since I
> propose passing the settings dict in into the constructor of the connection
> itself, not in the _cursor method, the advantage here is that when we create
> the connection we have all the information we need then.
Hey, this sounds like a good improvement (passing the settings to the
constructor instead of _cursor). Do you have any patches or at least a
link your proposal, if you've written one? I can't seem to find
anything.
Adrian
Alex Gaynor
As for the settings thing I don't have any code for it yet, but I can work on it.
Alex Gaynor
Malcolm Tredinnick
[...]
> One ugliness about this is that it has to monkeypatch the settings
> file in order to change the DATABASE_NAME, before passing it to
> DatabaseWrapper._cursor(). So my proposal is to change
> DatabaseWrapper._cursor() to accept a settings *dictionary* instead of
> a settings *object*.
I think the direction is a good one. Something that far down the chain
probably shouldn't have a hard dependency on settings. We insulate the
users from needing to pass around settings everywhere manually, but it's
not too onerous to do so ourselves.
Whether you pass the dictionary to _cursor or to the constructor as Alex
proposes is for you to decide, I think. We can evolve that over time if
we need to.
Regards,
Malcolm
Alex Gaynor
Adrian Holovaty
> I've posted a patch, but I'd like to here from one of the external DB
> backend maintainers, I know we technically don't have any backwards
> compatibility requirement here, but I don't think we want to jerk them
> around too much. Another thing is I want to encourage all external backends
> to use DATABASE_OPTIONS for extra options, rather than additional settings,
> that way they can be DB specific once we have multidb.
Thanks for the patch, Alex. I had already started doing it on my own
and was pleased to see we came up with almost exactly the same
solution. The main differences are that I called it
BaseDatabaseWrapper.settings_dict instead of
BaseDatabaseWrapper.settings (to make it clear it's a dictionary and
make it grepable) and I assigned to a local settings_dict variable
whenever the code needed to refer to self.settings_dict many times.
http://code.djangoproject.com/changeset/10026
This will most probably break external database backends, so I'll send
out a separate django-developers note about that, in hopes of getting
their attention.
Adrian
Narso
> This is not unlike the code here:http://www.eflorenzano.com/blog/post/easy-multi-database-support-django/
> --
>
Hi, forgive my poor English.
It would be very good to have multiple database support, but to be
complete, each database may have different engine.
How do you think to support the relations (and of course select_related
() ) from a model in one database to another?
Narso.
Alex Gaynor
Yuri Baburov
Maybe let's go a bit further towards pool of db connections and wrap a
bunch of lines importing backend into some function that depends on
engine argument?
And later call it with settings.DATABASE_ENGINE to import default backend.
http://code.djangoproject.com/browser/django/trunk/django/db/__init__.py?rev=10026
Lines 12-38.
Best regards, Yuri V. Baburov, ICQ# 99934676, Skype: yuri.baburov,
MSN: bu...@live.com
James Bennett
> Maybe let's go a bit further towards pool of db connections and wrap a
> bunch of lines importing backend into some function that depends on
> engine argument?
FWIW I'm still strongly of the opinion that database connection
pooling does not belong in an ORM like Django's.
--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."
Yuri Baburov
>
> On Wed, Mar 11, 2009 at 11:39 PM, Yuri Baburov <bur...@gmail.com> wrote:
>> Maybe let's go a bit further towards pool of db connections and wrap a
>> bunch of lines importing backend into some function that depends on
>> engine argument?
>
> FWIW I'm still strongly of the opinion that database connection
> pooling does not belong in an ORM like Django's.
But I didn't tell whether connection pooling should belong to Django
ORM or not! And neither I told how do I understand "connection
pooling"...
Let's just wrap those lines into a function! :)
Adrian suggested a new way to write function get_db_wrapper in comments at
http://www.eflorenzano.com/blog/post/easy-multi-database-support-django/
But why not have a method get_db_engine(engine_name) in the Django ORM code?
Alex Gaynor
FWIW that's on my list of stuff that would happen for multi-db support, but if someone wants to file a ticket and write up a patch(or I can) now that's certainly fine as it's really just code cleanup.
Adrian Holovaty
> bunch of lines importing backend into some function that depends on
> engine argument?
> And later call it with settings.DATABASE_ENGINE to import default backend.
>
> http://code.djangoproject.com/browser/django/trunk/django/db/__init__.py?rev=10026
> Lines 12-38.
Yes, good idea, Yuri! It would be nice to encapsulate the logic of
loading the appropriate Django database backend class from
django.db.backends based on a given backend name.
I've created a ticket here:
http://code.djangoproject.com/ticket/10487
If anybody (Alex?) wants to code up a patch, I'll get it in ASAP.
Adrian
Alex Gaynor
Adrian Holovaty
>> I've created a ticket here:
>>
>> http://code.djangoproject.com/ticket/10487
>>
>> If anybody (Alex?) wants to code up a patch, I'll get it in ASAP.
>>
>> Adrian
>
> I've gone ahead and uploaded a patch(at this rate they'll be nothing to do
> over the summer :) ).
Thanks, Alex. I've committed it in
http://code.djangoproject.com/changeset/10045 .
Adrian
Alex Gaynor
Any thoughts are welcome,