serious bug in lfs_get_object_or_404 and lfs_get_object (cache related)

[baffolobill]

Lets assume I have next model:

class MyModel(...):
    slug = SlugField()
    active = BooleanField()

and I will do next:

def my_view(request, slug):
    obj = lfs_get_object_or_404(MyModel, slug=slug, active=True)

If a cache is disabled, nothing bad, or if only the slug is used to filter, otherwise, regardless of the value slug, lfs_get_object_or_404 will return the same value - the object to which the first time has been accessed, because the cache_key in this case always will be prefix-mymodel-True.

Sorry, but I'm too lazy to do a pull request, but here is my solution:

# lfs/caching/utils.py
def lfs_get_object_or_404(klass, *args, **kwargs):
    ...
    ck_kwargs = '-'.join(['%s|%s'%kw for kw in kwargs.items()]).lower()
    cache_key = "%s-%s-%s" % (settings.CACHE_MIDDLEWARE_KEY_PREFIX, klass.__name__.lower(), ck_kwargs)
    ...

[yks-uno]

In my opinion, the LFS cache framework needs refactoring. I have several propositions including:

• class-based (object oriented) cache (some 1 class per model) e.g. ProductCache, CategoryCache etc. - this will unify the handling of cache items: check ids, slugs and other specific attributes of bound models, simplify cache clearing all in one place, and no more eye-burning "settings.CACHE_MIDDLEWARE_KEY_PREFIX" in every second line;
• the signals for cache clearing should (!) be handled by the models themselves i.e. Product.save() should trigger the cache clear (e.g. ProductCache.clear() ), rather than an unrelated module which can (and would) forget to do this at appropriate times;
• this will allow for a more extensible framework, which is now much limited due to requirement to update the LFS source code for every additional cache key bound to a cached model;
• store cache entries NOT as direct keys e.g. "%s-something-%s-%s-%s..." but as dicts e.g. "%s-something-ID" = { "%s": { "%s" ... } }, this will require a couple of additional lines to set the key (still only one try/catch to get it) but will REALLY quite simplify clearing related cache: deleting just ONE item instead of tracking all used-somewhere-by-someone direct keys. I am working on an implementation of this for my project and will be eager to contribute back when finished.

[baffolobill]

yks-uno, could you share your cache related code?

[yks-uno]

Sorry for the delay,

I actually implemented caching as a Manager subclass which can be added to any model that could need caching. The basic cache lookup by instance's PK can be done by the manager's get() or by a new get_cached() method (both have some advantages and disadvantages) This also stores the fetched instance to cache (if not already there). Caching of complex querysets requires apparently separate methods for each queryset, the cache keys are dynamically generated from the parameters and stored in the Manager class to be deleted on a modification signal. On model instance modification, Django emits signals which are listened to and forwarded to the model's manager which then deletes from cache the instance keys and the stored complex queryset keys.

My rationale was, all caching code must be located in one place, cache should be easily disabled and managed, and the place is obviously the Manager class which is responsible for fetching/deleting objects. But this implementation requires changes to a good half of LFS so I'm unsure if this can be accepted. There's no problem for me to share the code if someone likes, though it's nothing novel.

Note on these pesky CACHE_MIDDLEWARE_KEY_PREFIX: As far as I can tell, the Cache key prefixing appeared in Django 1.3, so in old installations it is sort of required to use them in every cache call. Since 1.3, they are added automatically. But in any case, moving all cache-related code into one location would be a solution for both getting rid of CACHE_MIDDLEWARE_KEY_PREFIX abuse and making the code overall more pleasant and modular.

On 20.12.2012 17:23, baffolobill wrote:

yks-uno, could you share your cache related code?

— Reply to this email directly or view it on GitHub https://github.com/diefenbach/django-lfs/issues/46#issuecomment-11573179.

Best Regards, Yuri aka Y.K.S.