Opened 13 years ago

Closed 12 years ago

#15255 closed Bug (fixed)

DB Cache table creation (createcachetable) ignores the DB Router

Reported by: zvikico Owned by: Aymeric Augustin
Component: Core (Cache system) Version: 1.3-beta
Severity: Normal Keywords:
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using database-based cache (using django.core.cache.backends.db.DatabaseCache cache backend), one uses the createcachetable command to create the table. This command completely ignores any database routers (in multiple databases situation) which are installed in the settings file. Instead, one can specify a target database with the --database option.

One could argue that this is not a bug, but rather a feature (undocumented one, none the less). However, the DatabaseCache class itself does use the installed router. This creates a confusion: if the router routes to a different database, one may install the table on the wrong database, but will not be able to access it.

I believe the best approach is to follow the syncdb convention: the cache table should not be created when the router returns false form the allow_syncdb method. The table will only be created when the matching DB is specified (or for the default if it is relevant).

Attachments (5)

15255.diff (3.3 KB ) - added by Aymeric Augustin 13 years ago.
15255.2.diff (5.7 KB ) - added by Aymeric Augustin 13 years ago.
15255.3.diff (6.0 KB ) - added by Aymeric Augustin 13 years ago.
15255.4.diff (5.9 KB ) - added by Aymeric Augustin 13 years ago.
15255.5.diff (5.9 KB ) - added by Aymeric Augustin 13 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedAccepted

Agreed - allow_syncdb() instructions should be honored. As a side note, there is a related refactoring in the test code (db.backends.creation.create_test_db) -- allow_syncdb is being checked there prior to invoking the database cache table command.

comment:2 by Aymeric Augustin, 13 years ago

Cc: Aymeric Augustin added
Has patch: set

Attached patch resolves the problem described above.

While writing it, I noticed that createcachetable could be much more automatic. Currently the docs at http://docs.djangoproject.com/en/dev/topics/cache/#database-caching suggest first creating the table, then adding the cache backend settings. Couldn't we do the opposite: first add the cache backend settings, then create the table? createcachetable would go through the cache backends, find all instances of BaseDatabaseCache, and create the tables with the appropriate name, like this:

from django.conf import settings
from django.core.cache import get_cache

for cache_alias in settings.CACHES:
    cache = get_cache(cache_alias)
    if isinstance(cache, BaseDatabaseCache):
        tablename = cache._table
       ... create the table ...

What do you think? Should I create a different ticket for this?

by Aymeric Augustin, 13 years ago

Attachment: 15255.diff added

comment:3 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: Bug

comment:4 by Jacob, 13 years ago

Easy pickings: unset
milestone: 1.4
Needs tests: set

Patch looks correct, but it needs tests.

And yeah, I really like the idea of createcachetable inspecting settings.CACHES, but please do take that to another ticket.

comment:5 by Jacob, 13 years ago

By the way, there are tests for createcachetable in regressiontests/cache/tests.py; we just need a new test method that tests it with the multidb flag. The test suite should already have an "other" database definition for you to test against.

by Aymeric Augustin, 13 years ago

Attachment: 15255.2.diff added

comment:6 by Aymeric Augustin, 13 years ago

Needs tests: unset

New patch includes a test case.

by Aymeric Augustin, 13 years ago

Attachment: 15255.3.diff added

comment:7 by Aymeric Augustin, 13 years ago

New patch uses the new @override_settings decorator and remove setUp / tearDown.

comment:8 by Aymeric Augustin, 13 years ago

Triage Stage: AcceptedReady for checkin

jacobkm said on IRC I could mark it as RFC.

comment:9 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: newclosed

In [16510]:

Fixed #15255 -- Stopped database cache from ignoring database routers when creating the cache table. Thanks, aaugustin.

comment:10 by Jannis Leidel, 13 years ago

Patch needs improvement: set
Resolution: fixed
Status: closedreopened
Triage Stage: Ready for checkinAccepted
UI/UX: unset

I rolled back the changes in r16515.

comment:11 by Aymeric Augustin, 13 years ago

OK, I wasn't using a multi-db capable django.test.TestCase in the v3 patch.

As far as I can tell, two things were still broken when the change was rolled back:

  • assertNumQueries missed the using= keyword argument
  • we expect two queries, not one

I just uploaded a v4 patch that builds upon jezdez' work by fixing this two issues. The "cache" tests pass on mysql, sqlite and postgresql.

by Aymeric Augustin, 13 years ago

Attachment: 15255.4.diff added

comment:12 by Aymeric Augustin, 13 years ago

Patch needs improvement: unset

Oops. 15255.4.diff and 15255.4.2.diff are the same file. Trac won't let me remove one.

comment:13 by Aymeric Augustin, 13 years ago

I just updated the patch against trunk. I checked that the tests pass under SQLite, PostgreSQL, MySQL and Oracle.

by Aymeric Augustin, 13 years ago

Attachment: 15255.5.diff added

comment:14 by Aymeric Augustin, 13 years ago

Owner: changed from nobody to Aymeric Augustin
Status: reopenednew

comment:15 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:2 by Aymeric Augustin, 12 years ago

Resolution: fixed
Status: newclosed

In [17114]:

Fixed #15255 -- Ensured createcachetable honors database routers.

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