Bug Fixes

---------

- Empty location names in model paths when generating a URL using
  ``repoze.bfg.model_url`` based on a model obtained via traversal are
  no longer ignored in the generated URL.  This means that if a
  non-root model object has a ``__name__`` of ``''``, the URL will
  reflect it (e.g. ``model_url`` will generate ``http://foo/bar//baz``
  if an object with the ``__name__`` of ``''`` is a child of bar and
  the parent of baz).  URLs generated with empty path segments are,
  however, still irresolveable by the model graph traverser on request
  ingress (the traverser strips empty path segment names).

Features
--------

- Microspeedups of ``repoze.bfg.traversal.model_path``,
  ``repoze.bfg.traversal.model_path_tuple``,
  and ``repoze.bfg.url.urlencode``.

Documentation
-------------

- Add a note to the ``repoze.bfg.traversal.quote_path_segment`` API
  docs about caching of computed values.

Implementation Changes
----------------------

- Simplification of
  ``repoze.bfg.traversal.TraversalContextURL.__call__`` (it now uses
  ``repoze.bfg.traversal.model_path`` instead of rolling its own
  path-generation).

3 files changed, 74 insertions, 41 deletions

diff --git a/repoze/bfg/tests/test_traversal.py b/repoze/bfg/tests/test_traversal.py
index ebac680d4..c911ebfed 100644
--- a/repoze/bfg/tests/test_traversal.py
+++ b/repoze/bfg/tests/test_traversal.py
@@ -612,6 +612,22 @@ class TraversalContextURLTests(unittest.TestCase):
         self.assertEqual(context_url.virtual_root(), traversed_to)
         self.assertEqual(context.environ['PATH_INFO'], '/one')
 
+    def test_empty_names_not_ignored(self):
+        bar = DummyContext()
+        empty = DummyContext(bar)
+        root = DummyContext(empty)
+        root.__parent__ = None
+        root.__name__ = None
+        empty.__parent__ = root
+        empty.__name__ = ''
+        bar.__parent__ = empty
+        bar.__name__ = 'bar'
+        request = DummyRequest()
+        context_url = self._makeOne(bar, request)
+        result = context_url()
+        self.assertEqual(result, 'http://example.com:5432//bar/')
+        
+
 class TestVirtualRoot(unittest.TestCase):
     def setUp(self):
         cleanUp()
diff --git a/repoze/bfg/traversal.py b/repoze/bfg/traversal.py
index e330a14eb..02b6589ac 100644
--- a/repoze/bfg/traversal.py
+++ b/repoze/bfg/traversal.py
@@ -108,7 +108,7 @@ def find_model(model, path):
         else:
             path = ''
 
-    if path.startswith('/'):
+    if path and path[0] == '/':
         model = find_root(model)
 
     ob, name, _ = ITraverserFactory(model)({'PATH_INFO':path})
@@ -165,10 +165,11 @@ def model_path(model, *elements):
               effectively replaced with a leading ``/``) when the path
               is generated.
     """
-    path = [ quote_path_segment(name) for name in
-            model_path_tuple(model, *elements) ]
-    path = '/'.join(path) or '/'
-    return path
+    path_list = _model_path_list(model, *elements)
+    if path_list:
+        path_list = [ quote_path_segment(name) for name in path_list ]
+        return '/' + '/'.join(path_list)
+    return '/'
 
 def model_path_tuple(model, *elements):
     """
@@ -204,18 +205,20 @@ def model_path_tuple(model, *elements):
               always be ignored (and effectively replaced with ``''``)
               when the path is generated.
     """
-    path = []
-    for location in lineage(model):
-        path.append(location.__name__)
+    path = _model_path_list(model, *elements)
+    if path:
+        return ('',) + tuple(path)
+    return ('',)
 
-    # replace root __name__ with the empty string
-    path.pop() 
-    path.append('')
+def _model_path_list(model, *elements):
+    """ Implementation detail shared by model_path and model_path_tuple """
+    lpath = reversed(list(lineage(model))[:-1])
+    path = [ location.__name__ for location in lpath ]
 
-    path.reverse()
-    path.extend(elements)
-    return tuple(path)
+    if elements:
+        path = path + list(elements)
 
+    return path
 
 def virtual_root(model, request):
     """
@@ -329,14 +332,26 @@ def quote_path_segment(segment):
     URL-quoted using Python's ``urllib.quote``.  If the segment passed
     in is not a string or unicode object, an error will be raised.
     The return value of ``quote_path_segment`` is always a string,
-    never Unicode."""
+    never Unicode.
+
+    .. note:: The return value for each segment passed to this
+              function is cached in a module-scope dictionary for
+              speed: the cached version is returned when possible
+              rather than recomputing the quoted version.  No cache
+              emptying is ever done for the lifetime of an
+              application, however.  If you pass arbitrary
+              user-supplied strings to this function (as opposed to
+              some bounded set of values from a 'working set' known to
+              your application), it may become a memory leak.
+    """
     # The bit of this code that deals with ``_segment_cache`` is an
     # optimization: we cache all the computation of URL path segments
     # in this module-scope dictionary with the original string (or
     # unicode value) as the key, so we can look it up later without
     # needing to reencode or re-url-quote it
-    result = _segment_cache.get(segment)
-    if result is None:
+    try:
+        return _segment_cache[segment]
+    except KeyError:
         if segment.__class__ is unicode: # isinstance slighly slower (~15%)
             result = _url_quote(segment.encode('utf-8'))
         else:
@@ -344,7 +359,7 @@ def quote_path_segment(segment):
         # we don't need a lock to mutate _segment_cache, as the below
         # will generate exactly one Python bytecode (STORE_SUBSCR)
         _segment_cache[segment] = result
-    return result
+        return result
 
 _marker = object()
 
@@ -417,26 +432,22 @@ class TraversalContextURL(object):
         """ Generate a URL based on the lineage of a model obtained
         via traversal.  If any model in the context lineage has a
         unicode name, it will be converted to a UTF-8 string before
-        being attached to the URL.  When composing the path based on
-        the model lineage, empty names in the model graph are ignored.
-        If a ``HTTP_X_VHM_ROOT`` key is present in the WSGI
-        environment, its value will be treated as a 'virtual root
-        path': the path of the URL generated by this will be
-        left-stripped of this virtual root path value.
+        being attached to the URL.  If a ``HTTP_X_VHM_ROOT`` key is
+        present in the WSGI environment, its value will be treated as
+        a 'virtual root path': the path of the URL generated by this
+        will be left-stripped of this virtual root path value.
         """
-        rpath = []
-        for location in lineage(self.context):
-            name = location.__name__
-            if name:
-                rpath.append(quote_path_segment(name))
-        if rpath:
-            path = '/' + '/'.join(reversed(rpath)) + '/'
-        else:
-            path = '/'
+        path = model_path(self.context)
+        if path != '/':
+            path = path + '/'
         request = self.request
+
         # if the path starts with the virtual root path, trim it out
-        vroot_path = request.environ.get(self.vroot_varname)
-        if vroot_path is not None:
+        try:
+            vroot_path = request.environ[self.vroot_varname]
+        except KeyError:
+            pass
+        else:
             if path.startswith(vroot_path):
                 path = path[len(vroot_path):]
                 
diff --git a/repoze/bfg/url.py b/repoze/bfg/url.py
index 0c6031c26..802dfca89 100644
--- a/repoze/bfg/url.py
+++ b/repoze/bfg/url.py
@@ -89,26 +89,32 @@ def urlencode(query, doseq=False):
     with an ``.items()`` method that returns a sequence of two-tuples
     representing key/value pairs.  ``doseq`` controls what happens
     when a sequence is presented as one of the values.  See the Python
-    stdlib documentation for more information.
+    stdlib documentation for ``urllib.urlencode`` for more
+    information.
     """
     if hasattr(query, 'items'):
-        # dictionary
+        # presumed to be a dictionary
         query = query.items()
-    # presumed to be a sequence of two-tuples
+
     newquery = []
     for k, v in query:
+
         if k.__class__ is unicode:
             k = k.encode('utf-8')
 
-        if isinstance(v, (tuple, list)):
+        try:
+            v.__iter__
+        except AttributeError:
+            if v.__class__ is unicode:
+                v = v.encode('utf-8')
+        else:
             L = []
             for x in v:
                 if x.__class__ is unicode:
                     x = x.encode('utf-8')
                 L.append(x)
             v = L
-        elif v.__class__ is unicode:
-            v = v.encode('utf-8')
+
         newquery.append((k, v))
 
     return urllib.urlencode(newquery, doseq=doseq)