Fixed bug in `cache_page` decorator

[ojomio]

It is necessary to introduce HTTP_X_FLAVOUR header before standard Django CacheMiddleware in request and patch Vary: headers before standard Django CacheMiddleware in response as well

Thus, nesting flavour decorator inside cache decorator is not correct and we need two separate flavour decorators for caching with flavour

[gregmuellegger]

Hi, thanks for the proposal. Splitting the middleware into two only makes sense if they are applied in different ends of the MIDDLEWARE_CLASSES setting. Can you describe/document, what would need to be changed in existing installations?

[ojomio]

Oh, i just followed the django.cache logic. They also split the CacheMiddleware in two, though the later inherit CacheMiddleware from both. Here we actually split the @cache_page logic in two parts - because if we nest it like it was, HTTP_X_FLAVOUR header will be set after the standard CacheMiddleware makes the cache key. I didn't think about applying it project-wide though. But now it makes perfect sense-we do need to use splitted middleware because middleware form a stack, but we need a queue in this case - mobile flavour , then cache fetch/store

[gregmuellegger]

Ok, that makes sense to me :) do you think we would need to adjust the README in this context. It currently states:

django-mobile is shipping with it's own implementation of cache_page to resolve this issue. Please use django_mobile.cache.cache_page instead of django's own cache_page decorator.

You can also use django's caching middlewares django.middleware.cache.UpdateCacheMiddleware and FetchFromCacheMiddleware like you already do. But to make them aware of flavours, you need to add django_mobile.cache.middleware.CacheFlavourMiddleware as second last item in the MIDDLEWARE_CLASSES settings, right before FetchFromCacheMiddleware.

[ojomio]

Yep, i think we should but i'm currently afk

[gregmuellegger]

Alright, I'll keep the PR open until the docs are added.

[amrael]

I'm delighted to see this bug is finally fixed and will be merged, although I fixed and sent almost the same PR two years ago and left unanswered since then ;) https://github.com/gregmuellegger/django-mobile/pull/42

[ojomio]

Oh, thanks! That makes me feel like i'm not an idiot and there is really a bug)

[gregmuellegger]

@amrael Sorry for that! I'm not actively using django-mobile myself, so this might have slipped past my unorganized self from two years ago :grin:

Is one of you interested in taking over the maintenance of the project?

[ojomio]

Uhm, thanks, but I don't think so. I'm good with proposing PRs)

[gregmuellegger]

:smile: alright, keep it up.

[ojomio]

Would you merge the code?

[amrael]

@gregmuellegger no worries, I figured you're too busy to maintain it at that time. I have no intention of taking over the project and it doesn't seem to require serious maintenance at the moment since I haven't come across any other problems over the past two years aside from this issue.

[ojomio]

@gregmuellegger , does anything remain which blocks this pull-request from being merged?

[gregmuellegger]

@ojomio Just released 0.7.0 including your changes: https://pypi.python.org/pypi/django-mobile/0.7.0

[ojomio]

Thank you!