- Fix a view lookup ordering bug whereby a view with a larger number

  of predicates registered first (literally first, not "earlier") for
  a triad would lose during view lookup to one registered with fewer.

3 files changed, 53 insertions, 16 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 88328042c..cac025ea4 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -16,6 +16,10 @@ Bug Fixes
   ``repoze.bfg.request.Request``.  BFG now *requires* WebOb >= 0.9.7,
   and code was added so that this deprecation warning has disappeared.
 
+- Fix a view lookup ordering bug whereby a view with a larger number
+  of predicates registered first (literally first, not "earlier") for
+  a triad would lose during view lookup to one registered with fewer.
+
 Documentation
 -------------
 
diff --git a/repoze/bfg/configuration.py b/repoze/bfg/configuration.py
index 4f3837058..bc9bbebca 100644
--- a/repoze/bfg/configuration.py
+++ b/repoze/bfg/configuration.py
@@ -206,7 +206,7 @@ class Configurator(object):
 
     def _derive_view(self, view, permission=None, predicates=(),
                      attr=None, renderer_name=None, wrapper_viewname=None,
-                     viewname=None, accept=None):
+                     viewname=None, accept=None, score=MAX_WEIGHT):
         renderer = self._renderer_from_name(renderer_name)
         authn_policy = self.registry.queryUtility(IAuthenticationPolicy)
         authz_policy = self.registry.queryUtility(IAuthorizationPolicy)
@@ -220,7 +220,7 @@ class Configurator(object):
                                      authn_policy, authz_policy, settings,
                                      logger)
         predicated_view = _predicate_wrap(debug_view, predicates)
-        derived_view = _accept_wrap(predicated_view, accept)
+        derived_view = _attr_wrap(predicated_view, accept, score)
         return derived_view
 
     def _override(self, package, path, override_package, override_prefix,
@@ -718,7 +718,7 @@ class Configurator(object):
             containment=containment, custom=custom_predicates)
 
         derived_view = self._derive_view(view, permission, predicates, attr,
-                                         renderer, wrapper, name, accept)
+                                         renderer, wrapper, name, accept, score)
 
         if context is None:
             context = for_
@@ -780,7 +780,8 @@ class Configurator(object):
             else:
                 multiview = MultiView(name)
                 old_accept = getattr(old_view, '__accept__', None)
-                multiview.add(old_view, MAX_WEIGHT, old_accept)
+                old_score = getattr(old_view, '__score__', MAX_WEIGHT)
+                multiview.add(old_view, old_score, old_accept)
             multiview.add(derived_view, score, accept)
             for view_type in (IView, ISecuredView):
                 # unregister any existing views
@@ -1378,11 +1379,6 @@ def _make_predicates(xhr=None, request_method=None, path_info=None,
     predicates = []
     weight = MAX_WEIGHT
 
-    if custom:
-        for predicate in custom:
-            weight = weight - 10
-            predicates.append(predicate)
-
     if xhr:
         def xhr_predicate(context, request):
             return request.is_xhr
@@ -1445,6 +1441,11 @@ def _make_predicates(xhr=None, request_method=None, path_info=None,
         weight = weight - 80
         predicates.append(containment_predicate)
 
+    if custom:
+        for predicate in custom:
+            weight = weight - 100
+            predicates.append(predicate)
+
     # this will be == MAX_WEIGHT if no predicates
     score = weight / (len(predicates) + 1)
     return score, predicates
@@ -1537,6 +1538,10 @@ def decorate_view(wrapped_view, original_view):
         wrapped_view.__accept__ = original_view.__accept__
     except AttributeError:
         pass
+    try:
+        wrapped_view.__score__ = original_view.__score__
+    except AttributeError:
+        pass
     return True
 
 def rendered_response(renderer, response, view, context, request,
@@ -1781,15 +1786,18 @@ def _authdebug_view(view, permission, authn_policy, authz_policy, settings,
 
     return wrapped_view
 
-def _accept_wrap(view, accept):
+def _attr_wrap(view, accept, score):
     # this is a little silly but we don't want to decorate the original
-    # function
-    if accept is None:
-        return view
-    def accept_view(context, request):
+    # function with attributes that indicate accept and score,
+    # so we use a wrapper
+    if (accept is None) and (score == MAX_WEIGHT):
+        return view # defaults
+    def attr_view(context, request):
         return view(context, request)
-    accept_view.__accept__ = accept
-    return accept_view
+    attr_view.__accept__ = accept
+    attr_view.__score__ = score
+    decorate_view(attr_view, view)
+    return attr_view
 
 # note that ``options`` is a b/w compat alias for ``settings`` and
 # ``Configurator`` is a testing dep inj
diff --git a/repoze/bfg/tests/test_configuration.py b/repoze/bfg/tests/test_configuration.py
index db960e2e9..b8b114fb9 100644
--- a/repoze/bfg/tests/test_configuration.py
+++ b/repoze/bfg/tests/test_configuration.py
@@ -1030,6 +1030,31 @@ class ConfiguratorTests(unittest.TestCase):
         request = self._makeRequest(config)
         self._assertNotFound(wrapper, None, request)
 
+    def test_add_view_custom_predicate_bests_standard_predicate(self):
+        view = lambda *arg: 'OK'
+        view2 = lambda *arg: 'NOT OK'
+        config = self._makeOne()
+        def pred1(context, request):
+            return True
+        config.add_view(view=view, custom_predicates=(pred1,))
+        config.add_view(view=view2, request_method='GET')
+        wrapper = self._getViewCallable(config)
+        request = self._makeRequest(config)
+        request.method = 'GET'
+        self.assertEqual(wrapper(None, request), 'OK')
+
+    def test_add_view_custom_more_preds_first_bests_fewer_preds_last(self):
+        view = lambda *arg: 'OK'
+        view2 = lambda *arg: 'NOT OK'
+        config = self._makeOne()
+        config.add_view(view=view, request_method='GET', xhr=True)
+        config.add_view(view=view2, request_method='GET')
+        wrapper = self._getViewCallable(config)
+        request = self._makeRequest(config)
+        request.method = 'GET'
+        request.is_xhr = True
+        self.assertEqual(wrapper(None, request), 'OK')
+
     def _assertRoute(self, config, name, path, num_predicates=0):
         from repoze.bfg.interfaces import IRoutesMapper
         mapper = config.registry.getUtility(IRoutesMapper)