Code

Opened 3 years ago

Closed 2 years ago

#15255 closed Bug (fixed)

DB Cache table creation (createcachetable) ignores the DB Router

Reported by: zvikico Owned by: aaugustin
Component: Core (Cache system) Version: 1.3-beta
Severity: Normal Keywords:
Cc: aaugustin 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 aaugustin 3 years ago.
15255.2.diff (5.7 KB) - added by aaugustin 3 years ago.
15255.3.diff (6.0 KB) - added by aaugustin 3 years ago.
15255.4.diff (5.9 KB) - added by aaugustin 3 years ago.
15255.5.diff (5.9 KB) - added by aaugustin 3 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by aaugustin

  • Cc aaugustin 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 3 years ago by aaugustin

comment:3 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:4 Changed 3 years ago by jacob

  • Easy pickings unset
  • milestone set to 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 3 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 3 years ago by aaugustin

comment:6 Changed 3 years ago by aaugustin

  • Needs tests unset

New patch includes a test case.

Changed 3 years ago by aaugustin

comment:7 Changed 3 years ago by aaugustin

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

comment:8 Changed 3 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

jacobkm said on IRC I could mark it as RFC.

comment:9 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [16510]:

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

comment:10 Changed 3 years ago by jezdez

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to Accepted
  • UI/UX unset

I rolled back the changes in r16515.

comment:11 Changed 3 years ago by aaugustin

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 3 years ago by aaugustin

comment:12 Changed 3 years ago by aaugustin

  • 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 3 years ago by aaugustin

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

Changed 3 years ago by aaugustin

comment:14 Changed 3 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from reopened to new

comment:15 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:2 Changed 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

In [17114]:

Fixed #15255 -- Ensured createcachetable honors database routers.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.