Opened 6 years ago

Closed 5 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 6 years ago.
15255.2.diff (5.7 KB) - added by Aymeric Augustin 5 years ago.
15255.3.diff (6.0 KB) - added by Aymeric Augustin 5 years ago.
15255.4.diff (5.9 KB) - added by Aymeric Augustin 5 years ago.
15255.5.diff (5.9 KB) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
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 Changed 6 years ago by Aymeric Augustin

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?

Changed 6 years ago by Aymeric Augustin

Attachment: 15255.diff added

comment:3 Changed 5 years ago by Łukasz Rekucki

Severity: Normal
Type: Bug

comment:4 Changed 5 years ago by Jacob

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 Changed 5 years ago by Jacob

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.

Changed 5 years ago by Aymeric Augustin

Attachment: 15255.2.diff added

comment:6 Changed 5 years ago by Aymeric Augustin

Needs tests: unset

New patch includes a test case.

Changed 5 years ago by Aymeric Augustin

Attachment: 15255.3.diff added

comment:7 Changed 5 years ago by Aymeric Augustin

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

comment:8 Changed 5 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

jacobkm said on IRC I could mark it as RFC.

comment:9 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16510]:

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

comment:10 Changed 5 years ago by Jannis Leidel

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 Changed 5 years ago by Aymeric Augustin

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.

Changed 5 years ago by Aymeric Augustin

Attachment: 15255.4.diff added

comment:12 Changed 5 years ago by Aymeric Augustin

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 Changed 5 years ago by Aymeric Augustin

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

Changed 5 years ago by Aymeric Augustin

Attachment: 15255.5.diff added

comment:14 Changed 5 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: reopenednew

comment:15 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:2 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

In [17114]:

Fixed #15255 -- Ensured createcachetable honors database routers.

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