Launchpad itself

Merge lp:~stub/launchpad/memcache into lp:launchpad

  1. memcache
  2. Merge into devel
Proposed by Stuart Bishop
Status: Merged
Approved by: Stuart Bishop
Approved revision: no longer in the source branch.
Merged at revision: 11830
Proposed branch: lp:~stub/launchpad/memcache
Merge into: lp:launchpad
Diff against target: 207 lines (+82/-14)
3 files modified
lib/lp/services/memcache/doc/tales-cache.txt (+52/-5)
lib/lp/services/memcache/tales.py (+26/-7)
lib/lp/services/memcache/tests/test_doc.py (+4/-2)
To merge this branch: bzr merge lp:~stub/launchpad/memcache
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+37963@code.launchpad.net

Commit message

Stop generating invalid memcache keys and allow more cache sharing.

Description of the change

Add a 'noparam' option to allow cache to be shared when URLs differ only by the query string, resolving Bug #634326.

Also fix a bug where invalid memcache keys could be generated when the loop key contained a long string, per Bug #634646.

Some minor delinting.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

Thanks for the branch Stuart.

I've rewritten this intro to be clearer:

11 +An optional modifier can also be applied to the visibility. Only 'param'
12 +and 'noparam' are supported with 'param' as the default. Pages that
13 +differ by only the query string do not share cache.

Your test claims param is the default but doesn't show it.

Otherwise it looks fine.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/services/memcache/doc/tales-cache.txt'
--- lib/lp/services/memcache/doc/tales-cache.txt 2010-09-12 05:10:19 +0000
+++ lib/lp/services/memcache/doc/tales-cache.txt 2010-10-15 04:33:59 +0000
@@ -199,6 +199,54 @@
199 <!-- End cache hit: memcache expression (authenticated) [0 seconds] -->199 <!-- End cache hit: memcache expression (authenticated) [0 seconds] -->
200200
201201
202Visibility Modifiers
203--------------------
204
205An optional modifier can also be applied to the visibility. Only 'param'
206and 'noparam' are supported, with 'param' as the default. Pages that
207differ by only the query string do not share cache.
208
209 >>> default_template = TestPageTemplate(dedent("""\
210 ... <div tal:omit-tag=""><tal:cache content="cache:public">
211 ... Arg: <tal:x content="arg" />
212 ... </tal:cache></div>"""))
213 >>> login(ANONYMOUS)
214 >>> request1 = LaunchpadTestRequest(QUERY_STRING='a=1')
215 >>> request2 = LaunchpadTestRequest(QUERY_STRING='a=2')
216 >>> print default_template(arg="First", request=request1)
217 Arg: First
218 >>> print default_template(arg="Second", request=request2)
219 Arg: Second
220
221 >>> param_template = TestPageTemplate(dedent("""\
222 ... <div tal:omit-tag=""><tal:cache content="cache:public param">
223 ... Arg: <tal:x content="arg" />
224 ... </tal:cache></div>"""))
225 >>> login(ANONYMOUS)
226 >>> request3 = LaunchpadTestRequest(QUERY_STRING='a=3')
227 >>> request4 = LaunchpadTestRequest(QUERY_STRING='a=4')
228 >>> print param_template(arg="First", request=request3)
229 Arg: First
230 >>> print param_template(arg="Second", request=request4)
231 Arg: Second
232
233If 'noparam' is used, query parameters are ignored and the cache is shared.
234
235 >>> noparam_template = TestPageTemplate(dedent("""\
236 ... <div tal:omit-tag=""><tal:cache content="cache:public noparam">
237 ... Arg: <tal:x content="arg" />
238 ... </tal:cache></div>"""))
239 >>> login(ANONYMOUS)
240 >>> request5 = LaunchpadTestRequest(QUERY_STRING='a=5')
241 >>> request6 = LaunchpadTestRequest(QUERY_STRING='a=6')
242 >>> print noparam_template(arg="Third", request=request5)
243 Arg: Third
244 >>> print noparam_template(arg="Fourth", request=request6)
245 <!-- Cache hit: memcache expression (public noparam) [0 seconds] -->
246 Arg: Third
247 <!-- End cache hit: memcache expression (public noparam) [0 seconds] -->
248
249
202Nesting & Loops250Nesting & Loops
203---------------251---------------
204252
@@ -300,21 +348,21 @@
300 ... <body>348 ... <body>
301 ... <div tal:omit-tag="" tal:repeat="comment comments">349 ... <div tal:omit-tag="" tal:repeat="comment comments">
302 ... <div tal:replace="cache:350 ... <div tal:replace="cache:
303 ... public,1 hour,comment/comment_num">351 ... public,1 hour,comment/comment_id">
304 ... <span tal:replace="comment/comment" />352 ... <span tal:replace="comment/comment" />
305 ... </div>353 ... </div>
306 ... </div>354 ... </div>
307 ... </body>"""))355 ... </body>"""))
308 >>> comments = [356 >>> comments = [
309 ... {'comment_num': 42, 'comment': "Today's comment"},357 ... {'comment_id': 42, 'comment': "Today's comment"},
310 ... {'comment_num': 12, 'comment': "Yesterday's comment"}]358 ... {'comment_id': 'long!'*40, 'comment': "Yesterday's comment"}]
311 >>> print template(comments=comments)359 >>> print template(comments=comments)
312 <body>360 <body>
313 Today's comment361 Today's comment
314 Yesterday's comment362 Yesterday's comment
315 </body>363 </body>
316364
317 >>> comments.insert(0, {'comment_num': 'foo', 'comment': 'New comment'})365 >>> comments.insert(0, {'comment_id': 'foo', 'comment': 'New comment'})
318 >>> print template(comments=comments)366 >>> print template(comments=comments)
319 <body>367 <body>
320 New comment368 New comment
@@ -349,7 +397,6 @@
349Memcache in templates can be disabled entirely by setting the memcache flag to397Memcache in templates can be disabled entirely by setting the memcache flag to
350'disabled'.398'disabled'.
351399
352 >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
353 >>> from lp.services.features.model import FeatureFlag, getFeatureStore400 >>> from lp.services.features.model import FeatureFlag, getFeatureStore
354 >>> from lp.services.features.webapp import ScopesFromRequest401 >>> from lp.services.features.webapp import ScopesFromRequest
355 >>> from lp.services.features.flags import FeatureController402 >>> from lp.services.features.flags import FeatureController
356403
=== modified file 'lib/lp/services/memcache/tales.py'
--- lib/lp/services/memcache/tales.py 2010-09-12 05:52:41 +0000
+++ lib/lp/services/memcache/tales.py 2010-10-15 04:33:59 +0000
@@ -103,6 +103,18 @@
103 else:103 else:
104 raise SyntaxError("Too many arguments in cache: expression")104 raise SyntaxError("Too many arguments in cache: expression")
105105
106 try:
107 self.visibility, modifier = self.visibility.split()
108 if modifier == 'param':
109 self.include_params = True
110 elif modifier == 'noparam':
111 self.include_params = False
112 else:
113 raise SyntaxError(
114 'visibility modifier must be param or noparam')
115 except ValueError:
116 self.include_params = True
117
106 if self.visibility not in (118 if self.visibility not in (
107 'anonymous', 'public', 'private', 'authenticated'):119 'anonymous', 'public', 'private', 'authenticated'):
108 raise SyntaxError(120 raise SyntaxError(
@@ -170,7 +182,9 @@
170 # We use a sanitized version in the human readable chunk of182 # We use a sanitized version in the human readable chunk of
171 # the key.183 # the key.
172 request = econtext.getValue('request')184 request = econtext.getValue('request')
173 url = str(request.URL) + '?' + str(request.get('QUERY_STRING', ''))185 url = str(request.URL)
186 if self.include_params:
187 url += '?' + str(request.get('QUERY_STRING', ''))
174 url = url.encode('utf8') # Ensure it is a byte string.188 url = url.encode('utf8') # Ensure it is a byte string.
175 sanitized_url = url.translate(self._key_translate_map)189 sanitized_url = url.translate(self._key_translate_map)
176190
@@ -198,9 +212,9 @@
198212
199 # The extra_key is used to differentiate items inside loops.213 # The extra_key is used to differentiate items inside loops.
200 if self.extra_key is not None:214 if self.extra_key is not None:
201 # Encode it to base64 to make it memcached key safe.215 # Encode it to to a memcached key safe string. base64
202 extra_key = unicode(216 # isn't suitable for this because it can contain whitespace.
203 self.extra_key(econtext)).encode('base64').rstrip()217 extra_key = unicode(self.extra_key(econtext)).encode('hex')
204 else:218 else:
205 # If no extra_key was specified, we include a counter in the219 # If no extra_key was specified, we include a counter in the
206 # key that is reset at the start of the request. This220 # key that is reset at the start of the request. This
@@ -268,6 +282,7 @@
268 tag contents and invokes this callback, which will store the282 tag contents and invokes this callback, which will store the
269 result in memcache against the key calculated by the MemcacheExpr.283 result in memcache against the key calculated by the MemcacheExpr.
270 """284 """
285
271 def __init__(self, key, max_age, memcache_expr):286 def __init__(self, key, max_age, memcache_expr):
272 self._key = key287 self._key = key
273 self._max_age = max_age288 self._max_age = max_age
@@ -292,6 +307,7 @@
292 We use a special object so the TALInterpreter knows that this307 We use a special object so the TALInterpreter knows that this
293 information should not be quoted.308 information should not be quoted.
294 """309 """
310
295 def __init__(self, value):311 def __init__(self, value):
296 self.value = value312 self.value = value
297313
@@ -335,9 +351,13 @@
335TALInterpreter.bytecode_handlers_tal["insertText"] = do_insertText_tal351TALInterpreter.bytecode_handlers_tal["insertText"] = do_insertText_tal
336352
337353
338# Just like the original, except MemcacheHit and MemcacheMiss
339# instances are also passed through unharmed.
340def evaluateText(self, expr):354def evaluateText(self, expr):
355 """Replacement for zope.pagetemplate.engine.ZopeContextBase.evaluateText.
356
357 Just like the original, except MemcacheHit and MemcacheMiss
358 instances are also passed through unharmed.
359 """
360
341 text = self.evaluate(expr)361 text = self.evaluate(expr)
342 if (text is None362 if (text is None
343 or isinstance(text, (basestring, MemcacheHit, MemcacheMiss))363 or isinstance(text, (basestring, MemcacheHit, MemcacheMiss))
@@ -346,4 +366,3 @@
346 return unicode(text)366 return unicode(text)
347import zope.pagetemplate.engine367import zope.pagetemplate.engine
348zope.pagetemplate.engine.ZopeContextBase.evaluateText = evaluateText368zope.pagetemplate.engine.ZopeContextBase.evaluateText = evaluateText
349
350369
=== modified file 'lib/lp/services/memcache/tests/test_doc.py'
--- lib/lp/services/memcache/tests/test_doc.py 2010-08-20 20:31:18 +0000
+++ lib/lp/services/memcache/tests/test_doc.py 2010-10-15 04:33:59 +0000
@@ -17,6 +17,7 @@
17 setUp,17 setUp,
18 tearDown,18 tearDown,
19 )19 )
20from canonical.launchpad.webapp.servers import LaunchpadTestRequest
20from canonical.testing.layers import (21from canonical.testing.layers import (
21 LaunchpadFunctionalLayer,22 LaunchpadFunctionalLayer,
22 MemcachedLayer,23 MemcachedLayer,
@@ -49,8 +50,8 @@
49 def pt_getContext(self, args=(), options={}):50 def pt_getContext(self, args=(), options={}):
50 # Build a minimal context. The cache: expression requires51 # Build a minimal context. The cache: expression requires
51 # a request.52 # a request.
52 context = {'request': TestRequest()}53 context = dict(options)
53 context.update(options)54 context.setdefault('request', TestRequest())
54 return context55 return context
5556
5657
@@ -59,6 +60,7 @@
59 test.globs['TestPageTemplate'] = TestPageTemplate60 test.globs['TestPageTemplate'] = TestPageTemplate
60 test.globs['dedent'] = dedent61 test.globs['dedent'] = dedent
61 test.globs['MemcachedLayer'] = MemcachedLayer62 test.globs['MemcachedLayer'] = MemcachedLayer
63 test.globs['LaunchpadTestRequest'] = LaunchpadTestRequest
6264
6365
64def suite_for_doctest(filename):66def suite_for_doctest(filename):