django.db.models.Options contribute_to_class uses default connection instead of using DATABASE_ROUTERS

[Peter Long]

Hi Django developers,

I have been using the development copy of django from the svn trunk. I
think I may have found an area that needs updating now that django
supports multiple databases. I am not very familiar with the django
backend architecture so I am not sure if its a real bug or not and if
so how best to fix it. I would appreciate any input.

At the moment the file django/db/models/options.py has a method
contribute_to_class that is defined as:

def contribute_to_class(self, cls, name):
from django.db import connection

The imported connection is never used in the method, however if it
were used the code would be assuming that the model that is being
contributed to uses the default database. Perhaps that import should
be removed to avoid the temptation to use it and therefore force
developers to think about how to get a connection if one is needed.
Alternatively it could be changed to import the "connections"
dictionary and the "router" object. Something like:

def contribute_to_class(self, cls, name):
from django.db import connections, router
… code that initializes cls but does not need a connection …
db_name = router.db_for_read(cls)
connection = connections[db_name]
… code the needs a connection …

The reason I noticed this in the first place is because I applied the
patch that is part of ticket #6148 (http://code.djangoproject.com/
ticket/6148) to the latest trunk code and then defined two databases,
the default to hold the standard django tables in a local sqllite3
database and one to hold my legacy data on a db2 database with
multiple schemas. I noticed that my schema information was being
dropped from models defined as part of the db2 database. The reason
was that the patch code added to contribute_to_class that used the
connection was using the default database connection and not the db2
connection. As a result, since the patched sqllite instance does not
support schema's it was being dropped from my model's _meta data.
Applying the change described here solved the problem for this ticket.

This could conceivably be considered a bug in the patch for ticket
#6148, but I thought I would raise it here to see if anyone thought it
was something that should be fixed in the original source.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

[Russell Keith-Magee]

On Tue, May 11, 2010 at 2:21 AM, Peter Long <peter...@gmail.com> wrote:
> Hi Django developers,
>
> I have been using the development copy of django from the svn trunk. I
> think I may have found an area that needs updating now that django
> supports multiple databases. I am not very familiar with the django
> backend architecture so I am not sure if its a real bug or not and if
> so how best to fix it. I would appreciate any input.
>
> At the moment the file django/db/models/options.py has a method
> contribute_to_class that is defined as:
>
> def contribute_to_class(self, cls, name):
>    from django.db import connection
>
> The imported connection is never used in the method,

Look closer - it is used, right at the end of contribute_to_class. The
connection is used to get the operations module for the backend, which
is then used to determine if the generated database table name needs
to be truncated.

> however if it
> were used the code would be assuming that the model that is being
> contributed to uses the default database. Perhaps that import should
> be removed to avoid the temptation to use it and therefore force
> developers to think about how to get a connection if one is needed.
> Alternatively it could be changed to import the "connections"
> dictionary and the "router" object. Something like:
>
> def contribute_to_class(self, cls, name):
>        from django.db import connections, router
> … code that initializes cls but does not need a connection …
>        db_name = router.db_for_read(cls)
>        connection = connections[db_name]
> … code the needs a connection …

You won't get any disagreement from me on this. The use of connection
should be removed from Options.contribute_to_class. This isn't just an
idle cleanup thing, either -- anything database specific needs to be
deferred until a specific database operation is being performed. You
can't make database-specific assumptions at the options level.

In practice, this won't affect many users - you need to have very long
table names before truncation is an issue (64+ characters), and you
would need to have a default database with a maximum length that
exceeded that of your other databases.

> The reason I noticed this in the first place is because I applied the
> patch that is part of ticket #6148 (http://code.djangoproject.com/
> ticket/6148) to the latest trunk code and then defined two databases,
> the default to hold the standard django tables in a local sqllite3
> database and one to hold my legacy data on a db2 database with
> multiple schemas. I noticed that my schema information was being
> dropped from models defined as part of the db2 database. The reason
> was that the patch code added to contribute_to_class that used the
> connection was using the default database connection and not the db2
> connection. As a result, since the patched sqllite instance does not
> support schema's it was being dropped from my model's _meta data.
> Applying the change described here solved the problem for this ticket.
>
> This could conceivably be considered a bug in the patch for ticket
> #6148, but I thought I would raise it here to see if anyone thought it
> was something that should be fixed in the original source.

Sounds like a bug in the patch to me. As I said earlier, the options
class doesn't have any knowledge about the database that is being used
for a specific operation, so it can't enforce per-database
restrictions. In the case of db_table truncation, I suspect the right
approach here is to store db_table as an untruncated string, and then
truncate it at time of use inside specific backends or in queries. I
suspect analogous changes will be required for #6148.

This should also be logged as a bug; the truncation behavior that is
currently implemented is definitely wrong.

Yours,
Russ Magee %-)

[Peter Long]

> > The imported connection is never used in the method,
>
> Look closer - it is used, right at the end of contribute_to_class. The
> connection is used to get the operations module for the backend, which
> is then used to determine if the generated database table name needs
> to be truncated.

Oops. I thought that was part of the #6148 patch. :)

> Sounds like a bug in the patch to me. As I said earlier, the options
> class doesn't have any knowledge about the database that is being used
> for a specific operation, so it can't enforce per-database
> restrictions. In the case of db_table truncation, I suspect the right
> approach here is to store db_table as an untruncated string, and then
> truncate it at time of use inside specific backends or in queries. I
> suspect analogous changes will be required for #6148.
>
> This should also be logged as a bug; the truncation behavior that is
> currently implemented is definitely wrong.

Thanks for the information. I will log a bug regarding the db_table
truncation and reference this thread in it. Hopefully I will be able
to describe the problem adequately.

I will also raise the issue on ticket #6148. I have a feeling the code
contributed to this method by the patch for #6148 will just need to be
removed. The side effect being a lot of dependent code would need to
change. The patch adds what appears to be a convenience member called
qualified_name that is constructed by using another new member
db_schema and db_table. It calls
connections.ops.prep_db_table(self.db_schema, self.db_table) which
combines them in a db specific manner and qualified_name is set from
the resulting string. I am not sure where else this member could be
initialized where it would have access to the correct connection.
Sounds more likely that it must just be removed and constructed every
were a table name is needed. It occurred to me to turn qualified_name
into a "lazy loaded" member using a getter and a property(…) on
Options, but as you point out it still needs a connection and in
addition it appears the correct way to get a connection is to "ask" a
router what connection to use for a given Model instance. Since
Options is part of a Model instance's _meta instance I am not sure how
I would call router.db_for_read() from a Options getter method.

Peter Long

[Russell Keith-Magee]

On Wed, May 12, 2010 at 1:14 AM, Peter Long <peter...@gmail.com> wrote:
>> Sounds like a bug in the patch to me. As I said earlier, the options
>> class doesn't have any knowledge about the database that is being used
>> for a specific operation, so it can't enforce per-database
>> restrictions. In the case of db_table truncation, I suspect the right
>> approach here is to store db_table as an untruncated string, and then
>> truncate it at time of use inside specific backends or in queries. I
>> suspect analogous changes will be required for #6148.
>>
>> This should also be logged as a bug; the truncation behavior that is
>> currently implemented is definitely wrong.
>
> Thanks for the information. I will log a bug regarding the db_table
> truncation and reference this thread in it. Hopefully I will be able
> to describe the problem adequately.

FYI - I got itchy fingers, so I opened a bug report:

http://code.djangoproject.com/ticket/13528

Yours
Russ Magee %-)

[Peter Long]

On May 12, 8:54 am, Russell Keith-Magee <russ...@keith-magee.com>
wrote:

>
> FYI - I got itchy fingers, so I opened a bug report:
>
> http://code.djangoproject.com/ticket/13528
>
> Yours
> Russ Magee %-)

No problem. Thanks

Peter Long

[Anssi Kaariainen]

As a related problem the manage.py validate seems to do the same thing
-- that is import the default connection and do any validation using
it.