Code review comment for lp:~salgado/launchpad/cached-menus

The key used in the original implementation is not unique, as it doesn't include the context. Also, the key uses a class attribute (_baseclassname) which is not necessarily defined in subclasses, so should not be used there.

The diff below addresses these two issues and also fixes one test that was using a FakeRequest which didn't provide the 'annotations' attribute, so the new code was crashing when attempting to cache stuff.

=== modified file 'lib/canonical/launchpad/doc/menus.txt'
--- lib/canonical/launchpad/doc/menus.txt 2009-06-11 16:02:25 +0000
+++ lib/canonical/launchpad/doc/menus.txt 2009-08-05 15:39:02 +0000
@@ -339,7 +339,6 @@
     # We need to use a real launchpad test request so the view adapter
     # lookups will work. That request also needs to implement
     # IParticipation so that the login machinery will work.
-
     >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
     >>> from zope.security.interfaces import IParticipation
     >>> class InteractiveTestRequest(LaunchpadTestRequest):
@@ -354,7 +353,6 @@
     <a class="menu-link-None sprite icon" title="summary">text--&gt;</a>

     # Clean up our special login.
-
     >>> login(ANONYMOUS)

@@ -436,6 +434,7 @@
     ... self.query = query
     ... self.url1 = url1 # returned from getURL(1)
     ... self.method = 'GET'
+ ... self.annotations = {}
     ...
     ... def getURL(self, level=0):
     ... assert 0 <= level <=1, 'level must be 0 or 1'
@@ -470,9 +469,9 @@
 The following request is for a url that is different from all of the
 facet menu links for 'sesamestreet'. So, all links will be linked.

- >>> request = URI('http://launchpad.dev/sesamestreet/+xxx')
+ >>> request_uri = URI('http://launchpad.dev/sesamestreet/+xxx')
     >>> facets = Facets(street)
- >>> for link in facets.iterlinks(request):
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet/+foo True
     http://launchpad.dev/sesamestreet/+bar True
@@ -480,19 +479,19 @@
 This next request is for +foo, but with a query string. So, both links
 will still be linked.

- >>> request = FakeRequest(
- ... 'http://launchpad.dev/sesamestreet/+foo', query="123")
+ >>> request_uri = URI(
+ ... 'http://launchpad.dev/sesamestreet/+foo?a=123')
     >>> facets = Facets(street)
- >>> for link in facets.iterlinks():
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet/+foo True
     http://launchpad.dev/sesamestreet/+bar True

 Next, we'll use the link +bar. The +bar link will not be linked.

- >>> request = URI('http://launchpad.dev/sesamestreet/+bar')
+ >>> request_uri = URI('http://launchpad.dev/sesamestreet/+bar')
     >>> facets = Facets(street)
- >>> for link in facets.iterlinks(request):
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet/+foo True
     http://launchpad.dev/sesamestreet/+bar False
@@ -500,9 +499,9 @@
 Now, we use +bar with a trailing slash. The +bar link will still be not
 linked.

- >>> request = URI('http://launchpad.dev/sesamestreet/+bar/')
+ >>> request_uri = URI('http://launchpad.dev/sesamestreet/+bar/')
     >>> facets = Facets(street)
- >>> for link in facets.iterlinks(request):
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet/+foo True
     http://launchpad.dev/sesamestreet/+bar False
@@ -527,9 +526,9 @@
     ... 'More explanation about Baz of %s' % self.context.name)
     ... return Link(target, text, summary)

- >>> request = URI('http://launchpad.dev/sesamestreet/+bar')
+ >>> request_uri = URI('http://launchpad.dev/sesamestreet/+bar')
     >>> facets = BlankTargetTestFacets(street)
- >>> for link in facets.iterlinks(request):
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet True
     http://launchpad.dev/sesamestreet/+bar False
@@ -588,10 +587,10 @@
     >>> canonical_url(street)
     u'http://launchpad.dev/sesamestreet'

- >>> request = URI('http://launchpad.dev/sesamestreet')
+ >>> request_uri = URI('http://launchpad.dev/sesamestreet')

     >>> facets = AbsoluteUrlTargetTestFacets(street)
- >>> for link in facets.iterlinks(request):
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet False
     ftp://barlink.example.com/barbarbar True

=== modified file 'lib/canonical/launchpad/webapp/menu.py'
--- lib/canonical/launchpad/webapp/menu.py 2009-08-04 20:07:33 +0000
+++ lib/canonical/launchpad/webapp/menu.py 2009-08-05 16:42:49 +0000
@@ -237,7 +237,7 @@
             # would be no reference to the Link object, which would allow it
             # to be garbage collected during the course of the request.
             cache = request.annotations.setdefault(MENU_ANNOTATION_KEY, {})
- key = (self._baseclassname, name)
+ key = (self.__class__, self.context, name)
             link = cache.get(key)
             if link is None:
                 link = self._buildLink(name)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_menu.py'
--- lib/canonical/launchpad/webapp/tests/test_menu.py 2009-08-03 20:38:34 +0000
+++ lib/canonical/launchpad/webapp/tests/test_menu.py 2009-08-05 16:52:42 +0000
@@ -16,7 +16,6 @@

 class TestMenu(MenuBase):
     links = ['test_link']
- _baseclassname = 'TestMenu'
     times_called = 0

     def test_link(self):
@@ -65,6 +64,19 @@
         cache = request.annotations.get(MENU_ANNOTATION_KEY)
         self.assertEquals([link], cache.values())

+ def test_cache_key_is_unique(self):
+ # The cache key must include the link name, the context of the link
+ # and the class where the link is defined.
+ login(ANONYMOUS)
+ context = object()
+ menu = TestMenu(context)
+ link = menu._get_link('test_link')
+ cache = get_current_browser_request().annotations.get(
+ MENU_ANNOTATION_KEY)
+ self.assertEquals(len(cache.keys()), 1)
+ self.assertContentEqual(
+ cache.keys()[0], (menu.__class__, context, 'test_link'))
+

 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)