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
1=== modified file 'lib/lp/services/memcache/doc/tales-cache.txt'
2--- lib/lp/services/memcache/doc/tales-cache.txt 2010-09-12 05:10:19 +0000
3+++ lib/lp/services/memcache/doc/tales-cache.txt 2010-10-15 04:33:59 +0000
4@@ -199,6 +199,54 @@
5 <!-- End cache hit: memcache expression (authenticated) [0 seconds] -->
6
7
8+Visibility Modifiers
9+--------------------
10+
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.
14+
15+ >>> default_template = TestPageTemplate(dedent("""\
16+ ... <div tal:omit-tag=""><tal:cache content="cache:public">
17+ ... Arg: <tal:x content="arg" />
18+ ... </tal:cache></div>"""))
19+ >>> login(ANONYMOUS)
20+ >>> request1 = LaunchpadTestRequest(QUERY_STRING='a=1')
21+ >>> request2 = LaunchpadTestRequest(QUERY_STRING='a=2')
22+ >>> print default_template(arg="First", request=request1)
23+ Arg: First
24+ >>> print default_template(arg="Second", request=request2)
25+ Arg: Second
26+
27+ >>> param_template = TestPageTemplate(dedent("""\
28+ ... <div tal:omit-tag=""><tal:cache content="cache:public param">
29+ ... Arg: <tal:x content="arg" />
30+ ... </tal:cache></div>"""))
31+ >>> login(ANONYMOUS)
32+ >>> request3 = LaunchpadTestRequest(QUERY_STRING='a=3')
33+ >>> request4 = LaunchpadTestRequest(QUERY_STRING='a=4')
34+ >>> print param_template(arg="First", request=request3)
35+ Arg: First
36+ >>> print param_template(arg="Second", request=request4)
37+ Arg: Second
38+
39+If 'noparam' is used, query parameters are ignored and the cache is shared.
40+
41+ >>> noparam_template = TestPageTemplate(dedent("""\
42+ ... <div tal:omit-tag=""><tal:cache content="cache:public noparam">
43+ ... Arg: <tal:x content="arg" />
44+ ... </tal:cache></div>"""))
45+ >>> login(ANONYMOUS)
46+ >>> request5 = LaunchpadTestRequest(QUERY_STRING='a=5')
47+ >>> request6 = LaunchpadTestRequest(QUERY_STRING='a=6')
48+ >>> print noparam_template(arg="Third", request=request5)
49+ Arg: Third
50+ >>> print noparam_template(arg="Fourth", request=request6)
51+ <!-- Cache hit: memcache expression (public noparam) [0 seconds] -->
52+ Arg: Third
53+ <!-- End cache hit: memcache expression (public noparam) [0 seconds] -->
54+
55+
56 Nesting & Loops
57 ---------------
58
59@@ -300,21 +348,21 @@
60 ... <body>
61 ... <div tal:omit-tag="" tal:repeat="comment comments">
62 ... <div tal:replace="cache:
63- ... public,1 hour,comment/comment_num">
64+ ... public,1 hour,comment/comment_id">
65 ... <span tal:replace="comment/comment" />
66 ... </div>
67 ... </div>
68 ... </body>"""))
69 >>> comments = [
70- ... {'comment_num': 42, 'comment': "Today's comment"},
71- ... {'comment_num': 12, 'comment': "Yesterday's comment"}]
72+ ... {'comment_id': 42, 'comment': "Today's comment"},
73+ ... {'comment_id': 'long!'*40, 'comment': "Yesterday's comment"}]
74 >>> print template(comments=comments)
75 <body>
76 Today's comment
77 Yesterday's comment
78 </body>
79
80- >>> comments.insert(0, {'comment_num': 'foo', 'comment': 'New comment'})
81+ >>> comments.insert(0, {'comment_id': 'foo', 'comment': 'New comment'})
82 >>> print template(comments=comments)
83 <body>
84 New comment
85@@ -349,7 +397,6 @@
86 Memcache in templates can be disabled entirely by setting the memcache flag to
87 'disabled'.
88
89- >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
90 >>> from lp.services.features.model import FeatureFlag, getFeatureStore
91 >>> from lp.services.features.webapp import ScopesFromRequest
92 >>> from lp.services.features.flags import FeatureController
93
94=== modified file 'lib/lp/services/memcache/tales.py'
95--- lib/lp/services/memcache/tales.py 2010-09-12 05:52:41 +0000
96+++ lib/lp/services/memcache/tales.py 2010-10-15 04:33:59 +0000
97@@ -103,6 +103,18 @@
98 else:
99 raise SyntaxError("Too many arguments in cache: expression")
100
101+ try:
102+ self.visibility, modifier = self.visibility.split()
103+ if modifier == 'param':
104+ self.include_params = True
105+ elif modifier == 'noparam':
106+ self.include_params = False
107+ else:
108+ raise SyntaxError(
109+ 'visibility modifier must be param or noparam')
110+ except ValueError:
111+ self.include_params = True
112+
113 if self.visibility not in (
114 'anonymous', 'public', 'private', 'authenticated'):
115 raise SyntaxError(
116@@ -170,7 +182,9 @@
117 # We use a sanitized version in the human readable chunk of
118 # the key.
119 request = econtext.getValue('request')
120- url = str(request.URL) + '?' + str(request.get('QUERY_STRING', ''))
121+ url = str(request.URL)
122+ if self.include_params:
123+ url += '?' + str(request.get('QUERY_STRING', ''))
124 url = url.encode('utf8') # Ensure it is a byte string.
125 sanitized_url = url.translate(self._key_translate_map)
126
127@@ -198,9 +212,9 @@
128
129 # The extra_key is used to differentiate items inside loops.
130 if self.extra_key is not None:
131- # Encode it to base64 to make it memcached key safe.
132- extra_key = unicode(
133- self.extra_key(econtext)).encode('base64').rstrip()
134+ # Encode it to to a memcached key safe string. base64
135+ # isn't suitable for this because it can contain whitespace.
136+ extra_key = unicode(self.extra_key(econtext)).encode('hex')
137 else:
138 # If no extra_key was specified, we include a counter in the
139 # key that is reset at the start of the request. This
140@@ -268,6 +282,7 @@
141 tag contents and invokes this callback, which will store the
142 result in memcache against the key calculated by the MemcacheExpr.
143 """
144+
145 def __init__(self, key, max_age, memcache_expr):
146 self._key = key
147 self._max_age = max_age
148@@ -292,6 +307,7 @@
149 We use a special object so the TALInterpreter knows that this
150 information should not be quoted.
151 """
152+
153 def __init__(self, value):
154 self.value = value
155
156@@ -335,9 +351,13 @@
157 TALInterpreter.bytecode_handlers_tal["insertText"] = do_insertText_tal
158
159
160-# Just like the original, except MemcacheHit and MemcacheMiss
161-# instances are also passed through unharmed.
162 def evaluateText(self, expr):
163+ """Replacement for zope.pagetemplate.engine.ZopeContextBase.evaluateText.
164+
165+ Just like the original, except MemcacheHit and MemcacheMiss
166+ instances are also passed through unharmed.
167+ """
168+
169 text = self.evaluate(expr)
170 if (text is None
171 or isinstance(text, (basestring, MemcacheHit, MemcacheMiss))
172@@ -346,4 +366,3 @@
173 return unicode(text)
174 import zope.pagetemplate.engine
175 zope.pagetemplate.engine.ZopeContextBase.evaluateText = evaluateText
176-
177
178=== modified file 'lib/lp/services/memcache/tests/test_doc.py'
179--- lib/lp/services/memcache/tests/test_doc.py 2010-08-20 20:31:18 +0000
180+++ lib/lp/services/memcache/tests/test_doc.py 2010-10-15 04:33:59 +0000
181@@ -17,6 +17,7 @@
182 setUp,
183 tearDown,
184 )
185+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
186 from canonical.testing.layers import (
187 LaunchpadFunctionalLayer,
188 MemcachedLayer,
189@@ -49,8 +50,8 @@
190 def pt_getContext(self, args=(), options={}):
191 # Build a minimal context. The cache: expression requires
192 # a request.
193- context = {'request': TestRequest()}
194- context.update(options)
195+ context = dict(options)
196+ context.setdefault('request', TestRequest())
197 return context
198
199
200@@ -59,6 +60,7 @@
201 test.globs['TestPageTemplate'] = TestPageTemplate
202 test.globs['dedent'] = dedent
203 test.globs['MemcachedLayer'] = MemcachedLayer
204+ test.globs['LaunchpadTestRequest'] = LaunchpadTestRequest
205
206
207 def suite_for_doctest(filename):