Re: [Django] #15144: Max age set in cache control no longer obeys timeout set with @cache_page decorator

[Django]

#15144: Max age set in cache control no longer obeys timeout set with 
@cache_page
decorator
+---
  Reporter:  jsdalton   | Owner:  nobody 
Status:  new| Milestone:  1.3
 Component:  Cache system   |   Version:  SVN
Resolution: |  Keywords:  blocker, regression
 Stage:  Ready for checkin  | Has_patch:  1  
Needs_docs:  0  |   Needs_tests:  0  
Needs_better_patch:  0  |  
+---
Changes (by russellm):

  * stage:  Unreviewed => Ready for checkin

Comment:

 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #15144: Max age set in cache control no longer obeys timeout set with @cache_page decorator

[Django]

#15144: Max age set in cache control no longer obeys timeout set with 
@cache_page
decorator
---+
  Reporter:  jsdalton  | Owner:  nobody 
Status:  new   | Milestone:  1.3
 Component:  Cache system  |   Version:  SVN
Resolution:|  Keywords:  blocker, regression
 Stage:  Unreviewed| Has_patch:  1  
Needs_docs:  0 |   Needs_tests:  0  
Needs_better_patch:  0 |  
---+
Comment (by jsdalton):

 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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #15144: Max age set in cache control no longer obeys timeout set with @cache_page decorator

[Django]

#15144: Max age set in cache control no longer obeys timeout set with 
@cache_page
decorator
---+
  Reporter:  jsdalton  | Owner:  nobody 
Status:  new   | Milestone:  1.3
 Component:  Cache system  |   Version:  SVN
Resolution:|  Keywords:  blocker, regression
 Stage:  Unreviewed| Has_patch:  1  
Needs_docs:  0 |   Needs_tests:  0  
Needs_better_patch:  0 |  
---+
Changes (by Joshua "jag" Ginsberg ):

  * has_patch:  0 => 1

Comment:

 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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #15144: Max age set in cache control no longer obeys timeout set with @cache_page decorator

[Django]

#15144: Max age set in cache control no longer obeys timeout set with 
@cache_page
decorator
---+
  Reporter:  jsdalton  | Owner:  nobody 
Status:  new   | Milestone:  1.3
 Component:  Cache system  |   Version:  SVN
Resolution:|  Keywords:  blocker, regression
 Stage:  Unreviewed| Has_patch:  0  
Needs_docs:  0 |   Needs_tests:  0  
Needs_better_patch:  0 |  
---+
Comment (by jsdalton):

 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__.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #15144: Max age set in cache control no longer obeys timeout set with @cache_page decorator

[Django]

#15144: Max age set in cache control no longer obeys timeout set with 
@cache_page
decorator
---+
  Reporter:  jsdalton  | Owner:  nobody 
Status:  new   | Milestone:  1.3
 Component:  Cache system  |   Version:  SVN
Resolution:|  Keywords:  blocker, regression
 Stage:  Unreviewed| Has_patch:  0  
Needs_docs:  0 |   Needs_tests:  0  
Needs_better_patch:  0 |  
---+
Changes (by russellm):

  * keywords:  => blocker, regression
  * needs_better_patch:  => 0
  * needs_tests:  => 0
  * needs_docs:  => 0

Comment:

 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.