Thanks for the detailed explanation Russ. I was mainly concerned about 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: > On Sat, Jan 22, 2011 at 2:27 PM, Jim D. <jim.dal...@gmail.com> wrote: > > 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 %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
