Max age set in cache control no longer obeys timeout set with @cache_page decorator

[Jim Dalton]

Per the documentation, I would expect the max-age value set in the Cache Control to obey the timeout value passed to @cache_page when that decorator is used in a view. However, at present the max_age ignores that setting and instead uses the default timeout from the cache. (As a matter of fact, it even ignores the CACHE_MIDDLEWARE_SECONDS settings.)

I believe this is a regression, which from what I can tell, appears to have arisen in [15021]. This changeset introduced the following changes:

The following line, which originally set the value of self.cache_timeout at the start of CacheMiddleware.__init__ to cache_timeout, was removed:

self.cache_timeout = cache_timeout 

The following line was added to CacheMiddleware.__init__:

self.cache_timeout = self.cache.default_timeout 

The result is that max_age is set to the value of whatever the cache timeout setting of the cache used, rather than the value of cache_timeout passed into __init__()as it did formerly.

I wasn't able to find any background on this changeset so I couldn't determine whether this was a purposeful change in behavior for some other reason. For now, though, I can only conclude that this is a bug, since I would expect max-age to match the value of the timeout passed to cache_page.

[Russell Keith-Magee]

I'm a little uncertain as to the exact use case where you are seeing this problem. Yes, the code has been changed to set self.cache_timeout = self.cache.default_timeout, but that default timeout should be the value that has been passed into the cache backend.

Can you reduce this to a relatively simple test case that demonstrates when the wrong value is being used?

Marking as a blocker, regression because if true, this is something that needs to be fixed before release; however, we need more detail before we can confirm.

[Jim Dalton]

Thanks Russ. I uploaded a patch which, for me anyway, produces a failure in the test suite demonstrating the regression.

Basically, in these tests I add an additional view that sets an explicit cache_timeout of 4 seconds. I hit the view once along with the other tests to get it in the cache. After the 2 second sleep, my view with a 4-second timeout should still be hitting the cache. However, it does not, since it's using the timeout value of 1 that was put in when the 'other' cache is set up for the tests.

Also, if you look at the value of the Cache-Control header after the first hit on my view, e.g. if you do:

        # Request from the alternate cache with a new prefix and a custom timeout
        response = other_with_timeout_view(request, '20')
        self.assertEquals(response.content, 'Hello World 20')
        print response['Cache-Control']

It'll print "max-age=1" instead of 4, which is the value passed to the view in the decorator.

I agree with you, something funky is going on, because self.cache_timeout = self.cache.default_timeout *should* work, but at least for me it doesn't. I'll observe that a trivial fix is self.cache_timeout = cache_timeout or self.cache.default_timeout. This does fix the problem, but I'm not sure it's the right answer because I think there is some other stuff that needs to be sorted out properly in init.

[Joshua "jag" Ginsberg <jag@…>]

The problem is in get_cache - it does not do anything with the provided kwargs unless the specified cache is in URL format (i.e. has ':' in it). Patch attached - I'd probably recommend also including jsdalton's unit tests.

-jag

[Joshua "jag" Ginsberg <jag@…>]

Patch to fix the issue

[Jim Dalton]

Okay, I'm uploading a new patch that:

• Includes the regression tests I wrote under test_view_decorator that demonstrating the original issue
• Fixes the issue originally raised in this ticket by incorporating Joshua's patch (15144.patch).
• Includes a new test under CacheMiddlewareTest, test_constructor, which highlight a few bugs in the constructor that were discussed briefly here: ​http://groups.google.com/group/django-developers/browse_thread/thread/1a019e9d30de5c9d
• Fixes the issues uncovered by the new test via a fairly minor rewrite of the __init__ method of CacheMiddleware

With regards to the first issue and fix, that's pretty much been discussed above so no need to say more about it.

With regards to the second issue and fix, the main problem was that if the CacheMiddleware class was being used as middleware, the values for some of the instance attributes were not being set properly. (Basically, the except KeyError code path was never being executed.)

The new test should be pretty self explanatory and should raise several failures when run against the original CacheMiddleware.__init__ code currently in trunk

The rewrite is also hopefully self explanatory as well. In addition to fixing the bugs I tried my best to simplify some of the code in an effort to make it more readable and easy to maintain going forward. A few notes of clarification:

• I changed cache_alias to an instance attribute (i.e. self.cache_alias) in order to make it more testable (since it's not possible to determine which alias is being used from self.cache itself.)
• I smoothed out a lot of places where variables were being explicitly compared against None etc. I think the new test adequately ensures that the appropriate initialization behavior is taking place, and some of those constructions were (IMO) making the code a bit unnecessarily verbose and hard to read.
• I wish there was better testing to ensure the right values are being set in cache_kwargs and passed to get_cache() but I think the other upstream tests should fail if any of that were going wrong so it's probably okay.

Anyhow, please let me know if there are any issues with the patch or anything else that needs to be addressed. (Apologies if I should have opened a new ticket to address those other bugs, I just figured it was easier to get it all taken care of here.)

Cheers.

Jim

[Russell Keith-Magee]

Ok - your test case now makes it clear that there are a couple of bugs. I'm marking RFC, will check in shortly with a couple of minor modifications.

In particular:

• Using try:catch logic instead of "if contains" logic is marginally faster, because it only requires one lookup for all cases, rather than two lookups on the successful lookup case.
• The or-emulate "ternary if" is prone to error, especially in the "if x is None" versus "x is False" case, so we generally prefer the explicit if statement.

[Russell Keith-Magee]

(In [15285]) Fixed #15144 -- Corrected some problems with the Cache middleware when used with multiple cache settings. Thanks to Jim Dalton for the report, and to Jim and Joshua Ginsberg for the work on the patch.

[Jacob]

Milestone 1.3 deleted