Questions about possible regression in recent changeset addressing cache
Jim D.
I reported a bug earlier today (http://code.djangoproject.com/ticket/
15144) related to a possible regression in changeset 15021 (http://
code.djangoproject.com/changeset/15021). The apparent regression is
that as of this changeset, the cache middleware no longer respects the
timeout value passed to the @cache_page decorator.
I found some time this evening to further research this. I was able to
create a regression test that demonstrated the bug. However, in trying
to create a patch which addresses the bug I'm having a hard time
making sense of what changeset 15021 set out to accomplish.
For whatever reason, I am unable to find any discussion related to it
on this discussion list or in trac. If someone would be kind enough to
direct me to discussion surrounding this changeset or the ticket(s)
associated with it, I would be most appreciative.
Anyhow, that said, it appears to me there are some minor issues with
the code in the __init__ method of CacheMiddleware and there does not
appear to be adequate test coverage around some of the code. Namely:
* The cache_timeout value is being passed along to the cache, but
self.cache_timeout is not being set. This is the problem I identified
in the ticket I filed. The value was previously being set prior to
changeset 15021. Why was this removed and why is it now being set to
the default_timeout of the cache?
* There appears to be an attempt to determine which alias and prefix
to use in the try / except blocks. The problem is that the key is
being accessed via a dict get() method, which will never raise a
KeyError (http://docs.python.org/library/stdtypes.html#dict.get), it
will only return None. So basically the except blocks are never going
to be raised. This could probably be resolved by passing a default
value of e.g. False as the second arg to get() in each instance, and
then changing the try/except block to a condition. It also does seem
like some tests are needed here to properly run these various code
paths and make sure they are being set correctly.
* It seems kind of weird to me that if a 'cache_alias' is set
explicitly, then we don't fall back to CACHE_MIDDLEWARE_SECONDS if a
cache_timeout is not found, but if it's not set then we do fall back
to that setting?
If someone can shed some light on these questions, I can hopefully set
aside some time to write a few tests and rework the code a bit if it
does turn out that is needed.
Many thanks...
Russell Keith-Magee
> Howdy,
>
> I reported a bug earlier today (http://code.djangoproject.com/ticket/
> 15144) related to a possible regression in changeset 15021 (http://
> code.djangoproject.com/changeset/15021). The apparent regression is
> that as of this changeset, the cache middleware no longer respects the
> timeout value passed to the @cache_page decorator.
>
> I found some time this evening to further research this. I was able to
> create a regression test that demonstrated the bug. However, in trying
> to create a patch which addresses the bug I'm having a hard time
> making sense of what changeset 15021 set out to accomplish.
>
> For whatever reason, I am unable to find any discussion related to it
> on this discussion list or in trac. If someone would be kind enough to
> direct me to discussion surrounding this changeset or the ticket(s)
> associated with it, I would be most appreciative.
To fill in some of the blanks here; r15021 was an attempt to improve
the testing of some of the features introduced in r15005, when
multiple cache backends were introduced.
In particular, I was concerned about the argument handling in the
cache_page decorator. The cache_page decorator is just a wrapper
around the combined cache middleware; my concern fell into two areas:
* options passed to the decorator wouldn't be used correctly by the
underlying implementation
* Unspecified options on the decorator would fall back to the wrong
defaults (middleware-specific defaults, rather than decorator
defaults)
The test cases that are contained in r15021 cover the areas that I was
concerned about.
> Anyhow, that said, it appears to me there are some minor issues with
> the code in the __init__ method of CacheMiddleware and there does not
> appear to be adequate test coverage around some of the code.
Entirely possible. Like I said, I added a bunch of tests that covered
cases that I could see would be a problem (or were problems, and were
fixed by the rest of the patch). I make no claim that the rest of the
test suite is adequate.
> * The cache_timeout value is being passed along to the cache, but
> self.cache_timeout is not being set. This is the problem I identified
> in the ticket I filed. The value was previously being set prior to
> changeset 15021. Why was this removed and why is it now being set to
> the default_timeout of the cache?
As I noted in the ticket, it wasn't removed; the logic just gets
invoked in a different way.
It appears that you've now provided a test case, and Jag has
volunteered a patch; I'll be sure to take a look at both in the near
future.
> * There appears to be an attempt to determine which alias and prefix
> to use in the try / except blocks. The problem is that the key is
> being accessed via a dict get() method, which will never raise a
> KeyError (http://docs.python.org/library/stdtypes.html#dict.get), it
> will only return None. So basically the except blocks are never going
> to be raised. This could probably be resolved by passing a default
> value of e.g. False as the second arg to get() in each instance, and
> then changing the try/except block to a condition. It also does seem
> like some tests are needed here to properly run these various code
> paths and make sure they are being set correctly.
This is probably a case of code evolving against a deadline. At the
time r15021 was committed, I was working against the alpha deadline,
and I wanted to make sure that the feature added a couple of days
earlier didn't have any major bugs. As a result, the implementation
evolved quickly, and it's entirely possible a couple of facepalm
errors slipped in.
> * It seems kind of weird to me that if a 'cache_alias' is set
> explicitly, then we don't fall back to CACHE_MIDDLEWARE_SECONDS if a
> cache_timeout is not found, but if it's not set then we do fall back
> to that setting?
Admittedly, this is a logic jump that may not be obvious. The idea
here is that the existence of the cache_alias argument is the most
effectively way of determining whether the middleware is being used as
middleware, or as a decorator that is wrapping the middleware.
If you're using the middleware as middleware, you should be using the
CACHE_MIDDLEWARE_ALIAS alias. If you're using the decorator, the
decorator *always* passes in the cache alias; if you don't specify the
alias, it uses 'default'.
Prior to r15005, the distinction didn't matter because there was only
one cache backend. Now that we have multiple aliases, it is important
to distinguish between the alias you use for middleware and the alias
you use for decorators, because they aren't necessarily the same.
> If someone can shed some light on these questions, I can hopefully set
> aside some time to write a few tests and rework the code a bit if it
> does turn out that is needed.
I hope I've been able to shed a little light. Like I said; I make no
claim that the code is perfect, just that it passes all the tests that
I generated. If you find areas that aren't tested, any contributions
of tests and/or patches would be gratefully accepted.
Yours,
Russ Magee %-)
Jim D.
jumping into to attempt to rework a few things without a better
understanding of what the code is supposed to be doing.
If I find a few minutes today, I was thinking about writing a few
tests specifically against the __init__ method of CacheMiddleware to
ensure the right attributes are being set in the right way going
forward, and then perhaps proposing a few minor tweaks to to the code
to straighten things out. I'll put up a patch if I manage to get
anywhere.
Jim
On Jan 23, 3:33 am, Russell Keith-Magee <russ...@keith-magee.com>
wrote: