- When two views were registered with the same ``accept`` argument,

  but were otherwise registered with the same arguments, if a request
  entered the application which had an ``Accept`` header that accepted
  *either* of the media types defined by the set of views registered
  with predicates that otherwise matched, a more or less "random" one
  view would "win".  Now, we try harder to use the view callable
  associated with the view configuration that has the most specific
  ``accept`` argument.  Thanks to Alberto Valverde for an initial
  patch.

3 files changed, 201 insertions, 54 deletions

diff --git a/repoze/bfg/configuration.py b/repoze/bfg/configuration.py
index 76f1b5b60..4eccf628c 100644
--- a/repoze/bfg/configuration.py
+++ b/repoze/bfg/configuration.py
@@ -225,7 +225,7 @@ class Configurator(object):
 
     def _derive_view(self, view, permission=None, predicates=(),
                      attr=None, renderer_name=None, wrapper_viewname=None,
-                     viewname=None):
+                     viewname=None, accept=None):
         renderer = self._renderer_from_name(renderer_name)
         authn_policy = self.registry.queryUtility(IAuthenticationPolicy)
         authz_policy = self.registry.queryUtility(IAuthorizationPolicy)
@@ -238,10 +238,10 @@ class Configurator(object):
         debug_view = _authdebug_view(secured_view, permission, 
                                      authn_policy, authz_policy, settings,
                                      logger)
-        derived_view = _predicate_wrap(debug_view, predicates)
+        predicated_view = _predicate_wrap(debug_view, predicates)
+        derived_view = _accept_wrap(predicated_view, accept)
         return derived_view
 
-
     def _override(self, package, path, override_package, override_prefix,
                   _info=u'', PackageOverrides=PackageOverrides):
             pkg_name = package.__name__
@@ -385,7 +385,7 @@ class Configurator(object):
             containment=containment)
 
         derived_view = self._derive_view(view, permission, predicates, attr,
-                                         renderer, wrapper, name)
+                                         renderer, wrapper, name, accept)
         r_for_ = for_
         r_request_type = request_type
         if r_for_ is None:
@@ -413,8 +413,9 @@ class Configurator(object):
                 multiview = old_view
             else:
                 multiview = MultiView(name)
-                multiview.add(old_view, sys.maxint)
-            multiview.add(derived_view, score)
+                old_accept = getattr(old_view, '__accept__', None)
+                multiview.add(old_view, sys.maxint, old_accept)
+            multiview.add(derived_view, score, accept)
             for i in (IView, ISecuredView):
                 # unregister any existing views
                 self.registry.adapters.unregister((r_for_, r_request_type), i,
@@ -677,14 +678,39 @@ class MultiView(object):
 
     def __init__(self, name):
         self.name = name
+        self.media_views = {}
         self.views = []
+        self.accepts = []
 
-    def add(self, view, score):
-        self.views.append((score, view))
-        self.views.sort()
+    def add(self, view, score, accept=None):
+        if accept is None or '*' in accept:
+            self.views.append((score, view))
+            self.views.sort()
+        else:
+            subset = self.media_views.setdefault(accept, [])
+            subset.append((score, view))
+            subset.sort()
+            accepts = set(self.accepts)
+            accepts.add(accept)
+            self.accepts = list(accepts) # dedupe
+
+    def get_views(self, request):
+        if self.accepts and hasattr(request, 'accept'):
+            accepts = self.accepts[:]
+            views = []
+            while accepts:
+                match = request.accept.best_match(accepts)
+                if match is None:
+                    break
+                subset = self.media_views[match]
+                views.extend(subset)
+                accepts.remove(match)
+            views.extend(self.views)
+            return views
+        return self.views
 
     def match(self, context, request):
-        for score, view in self.views:
+        for score, view in self.get_views(request):
             if not hasattr(view, '__predicated__'):
                 return view
             if view.__predicated__(context, request):
@@ -703,7 +729,7 @@ class MultiView(object):
         return view(context, request)
 
     def __call__(self, context, request):
-        for score, view in self.views:
+        for score, view in self.get_views(request):
             try:
                 return view(context, request)
             except NotFound:
@@ -731,6 +757,10 @@ def decorate_view(wrapped_view, original_view):
         wrapped_view.__predicated__ = original_view.__predicated__
     except AttributeError:
         pass
+    try:
+        wrapped_view.__accept__ = original_view.__accept__
+    except AttributeError:
+        pass
     return True
 
 def rendered_response(renderer, response, view, context, request,
@@ -913,16 +943,16 @@ def _owrap_view(view, viewname, wrapper_viewname):
 def _predicate_wrap(view, predicates):
     if not predicates:
         return view
-    def _wrapped(context, request):
+    def predicate_wrapper(context, request):
         if all((predicate(context, request) for predicate in predicates)):
             return view(context, request)
         raise NotFound('predicate mismatch for view %s' % view)
     def checker(context, request):
         return all((predicate(context, request) for predicate in
                     predicates))
-    _wrapped.__predicated__ = checker
-    decorate_view(_wrapped, view)
-    return _wrapped
+    predicate_wrapper.__predicated__ = checker
+    decorate_view(predicate_wrapper, view)
+    return predicate_wrapper
 
 def _secure_view(view, permission, authn_policy, authz_policy):
     wrapped_view = view
@@ -975,6 +1005,16 @@ def _authdebug_view(view, permission, authn_policy, authz_policy, settings,
 
     return wrapped_view
 
+def _accept_wrap(view, accept):
+    # 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):
+        return view(context, request)
+    accept_view.__accept__ = accept
+    return accept_view
+
 # note that ``options`` is a b/w compat alias for ``settings`` and
 # ``Configurator`` is a testing dep inj
 def make_app(root_factory, package=None, filename='configure.zcml',
diff --git a/repoze/bfg/tests/test_configuration.py b/repoze/bfg/tests/test_configuration.py
index a27452cc8..750bd5054 100644
--- a/repoze/bfg/tests/test_configuration.py
+++ b/repoze/bfg/tests/test_configuration.py
@@ -440,22 +440,55 @@ class ConfiguratorTests(unittest.TestCase):
         self.failUnless(IMultiView.providedBy(wrapper))
         self.assertEqual(wrapper(None, None), 'OK')
 
+    def test_add_view_with_accept_multiview_replaces_existing_view(self):
+        from zope.interface import Interface
+        from repoze.bfg.interfaces import IRequest
+        from repoze.bfg.interfaces import IView
+        from repoze.bfg.interfaces import IMultiView
+        def view(context, request):
+            return 'OK'
+        def view2(context, request):
+            return 'OK2'
+        config = self._makeOne()
+        config.registry.registerAdapter(
+            view, (Interface, IRequest), IView, name='')
+        config.add_view(view=view2, accept='text/html')
+        wrapper = self._getViewCallable(config)
+        self.failUnless(IMultiView.providedBy(wrapper))
+        self.assertEqual(len(wrapper.views), 1)
+        self.assertEqual(len(wrapper.media_views), 1)
+        self.assertEqual(wrapper(None, None), 'OK')
+        request = DummyRequest()
+        request.accept = DummyAccept('text/html', 'text/html')
+        self.assertEqual(wrapper(None, request), 'OK2')
+
+    def test_add_view_multiview_replaces_existing_view_with___accept__(self):
+        from zope.interface import Interface
+        from repoze.bfg.interfaces import IRequest
+        from repoze.bfg.interfaces import IView
+        from repoze.bfg.interfaces import IMultiView
+        def view(context, request):
+            return 'OK'
+        def view2(context, request):
+            return 'OK2'
+        view.__accept__ = 'text/html'
+        config = self._makeOne()
+        config.registry.registerAdapter(
+            view, (Interface, IRequest), IView, name='')
+        config.add_view(view=view2)
+        wrapper = self._getViewCallable(config)
+        self.failUnless(IMultiView.providedBy(wrapper))
+        self.assertEqual(len(wrapper.views), 1)
+        self.assertEqual(len(wrapper.media_views), 1)
+        self.assertEqual(wrapper(None, None), 'OK2')
+        request = DummyRequest()
+        request.accept = DummyAccept('text/html')
+        self.assertEqual(wrapper(None, request), 'OK')
+
     def test_add_view_multiview_replaces_multiview(self):
         from zope.interface import Interface
-        from zope.interface import implements
         from repoze.bfg.interfaces import IRequest
         from repoze.bfg.interfaces import IMultiView
-        class DummyMultiView:
-            implements(IMultiView)
-            def __init__(self):
-                self.views = []
-                self.name = 'name'
-            def add(self, view, score):
-                self.views.append(view)
-            def __call__(self, context, request):
-                return 'OK1'
-            def __permitted__(self, context, request):
-                """ """
         view = DummyMultiView()
         config = self._makeOne()
         config.registry.registerAdapter(view, (Interface, IRequest),
@@ -464,7 +497,7 @@ class ConfiguratorTests(unittest.TestCase):
         config.add_view(view=view2)
         wrapper = self._getViewCallable(config)
         self.failUnless(IMultiView.providedBy(wrapper))
-        self.assertEqual(wrapper.views, [view2])
+        self.assertEqual(wrapper.views, [(view2, None)])
         self.assertEqual(wrapper(None, None), 'OK1')
 
     def test_add_view_multiview_call_ordering(self):
@@ -2104,6 +2137,49 @@ class TestMultiView(unittest.TestCase):
         self.assertEqual(mv.views, [(100, 'view')])
         mv.add('view2', 99)
         self.assertEqual(mv.views, [(99, 'view2'), (100, 'view')])
+        mv.add('view3', 100, 'text/html')
+        self.assertEqual(mv.media_views['text/html'], [(100, 'view3')])
+        mv.add('view4', 99, 'text/html')
+        self.assertEqual(mv.media_views['text/html'],
+                         [(99, 'view4'), (100, 'view3')])
+        mv.add('view5', 100, 'text/xml')
+        self.assertEqual(mv.media_views['text/xml'], [(100, 'view5')])
+        self.assertEqual(set(mv.accepts), set(['text/xml', 'text/html']))
+        self.assertEqual(mv.views, [(99, 'view2'), (100, 'view')])
+        mv.add('view6', 98, 'text/*')
+        self.assertEqual(mv.views, [(98, 'view6'),(99, 'view2'), (100, 'view')])
+
+    def test_get_views_request_has_no_accept(self):
+        request = DummyRequest()
+        mv = self._makeOne()
+        mv.views = [(99, lambda *arg: None)]
+        self.assertEqual(mv.get_views(request), mv.views)
+
+    def test_get_views_no_self_accepts(self):
+        request = DummyRequest()
+        request.accept = True
+        mv = self._makeOne()
+        mv.accepts = []
+        mv.views = [(99, lambda *arg: None)]
+        self.assertEqual(mv.get_views(request), mv.views)
+
+    def test_get_views(self):
+        request = DummyRequest()
+        request.accept = DummyAccept('text/html')
+        mv = self._makeOne()
+        mv.accepts = ['text/html']
+        mv.views = [(99, lambda *arg: None)]
+        html_views = [(98, lambda *arg: None)]
+        mv.media_views['text/html'] = html_views
+        self.assertEqual(mv.get_views(request), html_views + mv.views)
+
+    def test_get_views_best_match_returns_None(self):
+        request = DummyRequest()
+        request.accept = DummyAccept(None)
+        mv = self._makeOne()
+        mv.accepts = ['text/html']
+        mv.views = [(99, lambda *arg: None)]
+        self.assertEqual(mv.get_views(request), mv.views)
 
     def test_match_not_found(self):
         from repoze.bfg.exceptions import NotFound
@@ -2231,6 +2307,35 @@ class TestMultiView(unittest.TestCase):
         response = mv.__call_permissive__(context, request)
         self.assertEqual(response, expected_response)
 
+    def test__call__with_accept_match(self):
+        mv = self._makeOne()
+        context = DummyContext()
+        request = DummyRequest()
+        request.accept = DummyAccept('text/html', 'text/xml')
+        expected_response = DummyResponse()
+        def view(context, request):
+            return expected_response
+        mv.views = [(100, None)]
+        mv.media_views['text/xml'] = [(100, view)]
+        mv.accepts = ['text/xml']
+        response = mv(context, request)
+        self.assertEqual(response, expected_response)
+
+    def test__call__with_accept_miss(self):
+        mv = self._makeOne()
+        context = DummyContext()
+        request = DummyRequest()
+        request.accept = DummyAccept('text/plain', 'text/html')
+        expected_response = DummyResponse()
+        def view(context, request):
+            return expected_response
+        mv.views = [(100, view)]
+        mv.media_views['text/xml'] = [(100, None)]
+        mv.accepts = ['text/xml']
+        response = mv(context, request)
+        self.assertEqual(response, expected_response)
+        
+
 class TestRequestOnly(unittest.TestCase):
     def _callFUT(self, arg):
         from repoze.bfg.configuration import requestonly
@@ -2442,10 +2547,6 @@ class DummyRequest:
     def get_response(self, app):
         return app
 
-class DummyRootFactory:
-    def __init__(self, root):
-        self.root = root
-
 class DummyContext:
     pass
 
@@ -2520,6 +2621,29 @@ class DummyConfigurator(object):
     def make_wsgi_app(self):
         return self
     
-class DummyGetSiteManager(object):
-    def sethook(self, reg):
-        self.hook = reg
+class DummyAccept(object):
+    def __init__(self, *matches):
+        self.matches = list(matches)
+
+    def best_match(self, offered):
+        if self.matches:
+            for match in self.matches:
+                if match in offered:
+                    self.matches.remove(match)
+                    return match
+    def __contains__(self, val):
+        return val in self.matches
+
+from zope.interface import implements
+from repoze.bfg.interfaces import IMultiView
+class DummyMultiView:
+    implements(IMultiView)
+    def __init__(self):
+        self.views = []
+        self.name = 'name'
+    def add(self, view, score, accept=None):
+        self.views.append((view, accept))
+    def __call__(self, context, request):
+        return 'OK1'
+    def __permitted__(self, context, request):
+        """ """
diff --git a/repoze/bfg/tests/test_zcml.py b/repoze/bfg/tests/test_zcml.py
index 2834a8cb1..2ad26bc97 100644
--- a/repoze/bfg/tests/test_zcml.py
+++ b/repoze/bfg/tests/test_zcml.py
@@ -854,7 +854,7 @@ class TestHandler(unittest.TestCase):
 
     def test_it(self):
         def foo():
-            pass
+            """ """
         from zope.interface import Interface
         class IWhatever(Interface):
             pass
@@ -881,30 +881,13 @@ class IFactory(Interface):
 class DummyFactory(object):
     implements(IFactory)
     def __call__(self):
-        return 1
+        """ """
         
 class DummyModule:
     __path__ = "foo"
     __name__ = "dummy"
     __file__ = ''
 
-class DummyModuleGrokker:
-    def __init__(self, grokker=None):
-        self.multi_grokker = grokker
-        
-class DummyMartianModule:
-    def grok_dotted_name(self, name, grokker, _info, _configurator,
-                         exclude_filter=None):
-        self.name = name
-        self.info = _info
-        self.configurator = _configurator
-        self.exclude_filter = exclude_filter
-        return True
-
-    def ModuleGrokker(self, grokker=None):
-        self.module_grokker = DummyModuleGrokker(grokker)
-        return self.module_grokker
-
 class DummyContext:
     def __init__(self, resolved=DummyModule):
         self.actions = []