enable sorting of offers

8 files changed, 264 insertions, 101 deletions

diff --git a/docs/narr/extconfig.rst b/docs/narr/extconfig.rst
index a9ea93e60..4c6c8b70b 100644
--- a/docs/narr/extconfig.rst
+++ b/docs/narr/extconfig.rst
@@ -255,12 +255,12 @@ Pre-defined Phases
 
 :const:`pyramid.config.PHASE1_CONFIG`
 
-- :meth:`pyramid.config.Configurator.add_accept_view_option`
+- :meth:`pyramid.config.Configurator.add_accept_view_order`
 - :meth:`pyramid.config.Configurator.add_renderer`
 - :meth:`pyramid.config.Configurator.add_route_predicate`
 - :meth:`pyramid.config.Configurator.add_subscriber_predicate`
-- :meth:`pyramid.config.Configurator.add_view_predicate`
 - :meth:`pyramid.config.Configurator.add_view_deriver`
+- :meth:`pyramid.config.Configurator.add_view_predicate`
 - :meth:`pyramid.config.Configurator.override_asset`
 - :meth:`pyramid.config.Configurator.set_authorization_policy`
 - :meth:`pyramid.config.Configurator.set_default_csrf_options`
diff --git a/docs/narr/viewconfig.rst b/docs/narr/viewconfig.rst
index 2060393bb..cd49d6e94 100644
--- a/docs/narr/viewconfig.rst
+++ b/docs/narr/viewconfig.rst
@@ -1027,7 +1027,8 @@ See :ref:`environment_chapter` for more information about how, and where to set
 these values.
 
 .. index::
-   single: HTTP caching
+   single: Accept
+   single: Accept content negotation
 
 .. _accept_content_negotation:
 
@@ -1059,10 +1060,11 @@ Similarly, if the client specifies a media type that no view is registered to ha
 There are a few cases in which the client may specify ambiguous constraints:
 
 - ``Accept: */*``.
+- More than one acceptable media type with the same quality.
 - A missing ``Accept`` header.
 - An invalid ``Accept`` header.
 
-In these cases the preferred view is not clearly defined (see :rfc:`7231#section-5.3.2`) and :app:`Pyramid` will select one randomly.
+In these cases the preferred view is not clearly defined (see :rfc:`7231#section-5.3.2`) and :app:`Pyramid` will select one semi-randomly.
 This can be controlled by telling :app:`Pyramid` what the preferred relative ordering is between various media types by using :meth:`pyramid.config.Configurator.add_accept_view_order`.
 For example:
 
@@ -1082,9 +1084,30 @@ For example:
 
 In this case, the ``application/json`` view should always be selected in cases where it is otherwise ambiguous.
 
+.. _default_accept_ordering:
+
 Default Accept Ordering
 +++++++++++++++++++++++
 
+:app:`Pyramid` will always sort multiple views with the same ``(name, context, route_name)`` first by the specificity of the ``accept`` offer.
+This means that ``text/plain`` will always be offered before ``text/*``.
+Similarly ``text/plain;charset=utf8`` will always be offered before ``text/plain``.
+The following order is always preserved between the following offers (more preferred to less preferred):
+
+- ``type/subtype;params``
+- ``type/subtype``
+- ``type/*``
+- ``*/*``
+
+Within each of these levels of specificity, the ordering is ambiguous and may be controlled using :meth:`pyramid.config.Configurator.add_accept_view_order`. For example, to sort ``text/plain`` higher than ``text/html`` and to prefer a ``charset=utf8`` versus a ``charset=latin-1`` within the ``text/plain`` media type:
+
+.. code-block:: python
+
+    config.add_accept_view_order('text/plain', weighs_more_than='text/html')
+    config.add_accept_view_order('text/plain;charset=utf8', weighs_more_than='text/plain;charset=latin-1')
+
+It is an error to try and sort accept headers across levels of specificity. You can only sort a ``type/subtype`` against another ``type/subtype``, not against a ``type/*``. That ordering is a hard requirement.
+
 By default, :app:`Pyramid` defines a very simple priority ordering for views that prefers human-readable responses over JSON:
 
 - ``text/html``
@@ -1099,6 +1122,9 @@ Therefore, the motivation for this ordering is to optimize for readability.
 Media types that are not listed above are ordered randomly during :term:`view lookup` between otherwise-similar views.
 The defaults can be overridden using :meth:`pyramid.config.Configurator.add_accept_view_order` as described above.
 
+.. index::
+   single: HTTP caching
+
 .. _influencing_http_caching:
 
 Influencing HTTP Caching
diff --git a/pyramid/config/routes.py b/pyramid/config/routes.py
index b701a4bef..e2f2e7782 100644
--- a/pyramid/config/routes.py
+++ b/pyramid/config/routes.py
@@ -13,7 +13,10 @@ from pyramid.exceptions import ConfigurationError
 from pyramid.request import route_request_iface
 from pyramid.urldispatch import RoutesMapper
 
-from pyramid.util import as_sorted_tuple
+from pyramid.util import (
+    as_sorted_tuple,
+    is_nonstr_iter,
+)
 
 import pyramid.predicates
 
@@ -223,25 +226,18 @@ class RoutesConfiguratorMixin(object):
 
         accept
 
-          A media type that will be matched against the ``Accept`` HTTP
-          request header.  If this value is specified, it must be a specific
-          media type, such as ``text/html``.  If the media type is acceptable
-          by the ``Accept`` header of the request, or if the ``Accept`` header
-          isn't set at all in the request, this predicate will match. If this
-          does not match the ``Accept`` header of the request, route matching
-          continues.
+          A :term:`media type` that will be matched against the ``Accept``
+          HTTP request header.  This value may be a specific media type such
+          as ``text/html``, or a range like ``text/*``, or a list of the same.
+          If the media type is acceptable by the ``Accept`` header of the
+          request, or if the ``Accept`` header isn't set at all in the
+          request, this predicate will match. If this does not match the
+          ``Accept`` header of the request, route matching continues.
 
           If ``accept`` is not specified, the ``HTTP_ACCEPT`` HTTP header is
           not taken into consideration when deciding whether or not to select
           the route.
 
-
-          .. versionchanged:: 1.10
-              Media ranges such as ``text/*`` will now raise
-              :class:`pyramid.exceptions.ConfigurationError`. Previously,
-              these values had undefined behavior based on the version of
-              WebOb being used and was never fully supported.
-
         effective_principals
 
           If specified, this value should be a :term:`principal` identifier or
@@ -299,17 +295,11 @@ class RoutesConfiguratorMixin(object):
                 stacklevel=3
                 )
 
-        if accept is not None and '*' in accept:
-            warnings.warn(
-                ('The usage of a media range in the "accept" route predicate '
-                 'is deprecated as of Pyramid 1.10. Use a list of explicit '
-                 'media types instead.'),
-                DeprecationWarning,
-                stacklevel=4,
-            )
-
         if accept is not None:
-            accept = accept.lower()
+            if is_nonstr_iter(accept):
+                accept = [accept]
+
+            accept = [accept_option.lower() for accept_option in accept]
 
         # these are route predicates; if they do not match, the next route
         # in the routelist will be tried
diff --git a/pyramid/config/views.py b/pyramid/config/views.py
index 87584b858..45a89e5fb 100644
--- a/pyramid/config/views.py
+++ b/pyramid/config/views.py
@@ -5,6 +5,7 @@ import operator
 import os
 import warnings
 
+from webob.acceptparse import Accept
 from zope.interface import (
     Interface,
     implementedBy,
@@ -67,6 +68,7 @@ from pyramid.view import AppendSlashNotFoundViewFactory
 
 from pyramid.util import (
     as_sorted_tuple,
+    sort_accept_offers,
     TopologicalSorter,
     )
 
@@ -116,14 +118,14 @@ class MultiView(object):
         view = self.match(context, request)
         return view.__discriminator__(context, request)
 
-    def add(self, view, order, accept=None, phash=None, accept_order=None):
+    def add(self, view, order, phash=None, accept=None, accept_order=None):
         if phash is not None:
             for i, (s, v, h) in enumerate(list(self.views)):
                 if phash == h:
                     self.views[i] = (order, view, phash)
                     return
 
-        if accept is None or '*' in accept:
+        if accept is None:
             self.views.append((order, view, phash))
             self.views.sort(key=operator.itemgetter(0))
         else:
@@ -138,15 +140,9 @@ class MultiView(object):
             # dedupe accepts and sort appropriately
             accepts = set(self.accepts)
             accepts.add(accept)
-            if accept_order is not None:
-                sorted_accepts = []
-                for accept in accept_order.sorted():
-                    if accept in accepts:
-                        sorted_accepts.append(accept)
-                        accepts.remove(accept)
-                sorted_accepts.extend(accepts)
-                accepts = sorted_accepts
-            self.accepts = list(accepts)
+            if accept_order:
+                accept_order = accept_order.sorted()
+            self.accepts = sort_accept_offers(accepts, accept_order)
 
     def get_views(self, request):
         if self.accepts and hasattr(request, 'accept'):
@@ -246,6 +242,14 @@ def viewdefaults(wrapped):
         return wrapped(self, *arg, **defaults)
     return functools.wraps(wrapped)(wrapper)
 
+def combine_decorators(*decorators):
+    def decorated(view_callable):
+        # reversed() allows a more natural ordering in the api
+        for decorator in reversed(decorators):
+            view_callable = decorator(view_callable)
+        return view_callable
+    return decorated
+
 class ViewsConfiguratorMixin(object):
     @viewdefaults
     @action_method
@@ -672,8 +676,8 @@ class ViewsConfiguratorMixin(object):
         accept
 
           A :term:`media type` that will be matched against the ``Accept``
-          HTTP request header.  If this value is specified, it must be a
-          specific media type, such as ``text/html``.  If the media type is
+          HTTP request header.  This value may be a specific media type such
+          as ``text/html``, or a range like ``text/*``.  If the media type is
           acceptable by the ``Accept`` header of the request, or if the
           ``Accept`` header isn't set at all in the request, this predicate
           will match. If this does not match the ``Accept`` header of the
@@ -685,12 +689,6 @@ class ViewsConfiguratorMixin(object):
 
           See :ref:`accept_content_negotation` for more information.
 
-          .. versionchanged:: 1.10
-              Media ranges such as ``text/*`` will now raise
-              :class:`pyramid.exceptions.ConfigurationError`. Previously,
-              these values had undefined behavior based on the version of
-              WebOb being used and was never fully supported.
-
         path_info
 
           This value represents a regular expression pattern that will
@@ -818,23 +816,11 @@ class ViewsConfiguratorMixin(object):
                 stacklevel=4,
                 )
 
-        if accept is not None and '*' in accept:
-            warnings.warn(
-                ('The usage of a media range in the "accept" view predicate '
-                 'is deprecated as of Pyramid 1.10. Register multiple views '
-                 'with explicit media ranges and read '
-                 '"Accept Header Content Negotiation" in the '
-                 '"View Configuration" documentation for more information.'),
-                DeprecationWarning,
-                stacklevel=4,
-            )
-
-        if accept is not None and is_nonstr_iter(accept):
-            raise ConfigurationError(
-                'A list is not supported in the "accept" view predicate.',
-            )
-
         if accept is not None:
+            if is_nonstr_iter(accept):
+                raise ConfigurationError(
+                    'A list is not supported in the "accept" view predicate.',
+                )
             accept = accept.lower()
 
         view = self.maybe_dotted(view)
@@ -843,16 +829,8 @@ class ViewsConfiguratorMixin(object):
         containment = self.maybe_dotted(containment)
         mapper = self.maybe_dotted(mapper)
 
-        def combine(*decorators):
-            def decorated(view_callable):
-                # reversed() allows a more natural ordering in the api
-                for decorator in reversed(decorators):
-                    view_callable = decorator(view_callable)
-                return view_callable
-            return decorated
-
         if is_nonstr_iter(decorator):
-            decorator = combine(*map(self.maybe_dotted, decorator))
+            decorator = combine_decorators(*map(self.maybe_dotted, decorator))
         else:
             decorator = self.maybe_dotted(decorator)
 
@@ -1092,7 +1070,17 @@ class ViewsConfiguratorMixin(object):
                 if old_view is not None:
                     break
 
-            def regclosure():
+            old_phash = getattr(old_view, '__phash__', DEFAULT_PHASH)
+            is_multiview = IMultiView.providedBy(old_view)
+            want_multiview = (
+                is_multiview
+                # no component was yet registered for exactly this triad
+                # or only one was registered but with the same phash, meaning
+                # that this view is an override
+                or (old_view is not None and old_phash != phash)
+            )
+
+            if not want_multiview:
                 if hasattr(derived_view, '__call_permissive__'):
                     view_iface = ISecuredView
                 else:
@@ -1104,21 +1092,6 @@ class ViewsConfiguratorMixin(object):
                     name
                     )
 
-            is_multiview = IMultiView.providedBy(old_view)
-            old_phash = getattr(old_view, '__phash__', DEFAULT_PHASH)
-
-            if old_view is None:
-                # - No component was yet registered for any of our I*View
-                #   interfaces exactly; this is the first view for this
-                #   triad.
-                regclosure()
-
-            elif (not is_multiview) and (old_phash == phash):
-                # - A single view component was previously registered with
-                #   the same predicate hash as this view; this registration
-                #   is therefore an override.
-                regclosure()
-
             else:
                 # - A view or multiview was already registered for this
                 #   triad, and the new view is not an override.
@@ -1136,9 +1109,9 @@ class ViewsConfiguratorMixin(object):
                     old_order = getattr(old_view, '__order__', MAX_ORDER)
                     # don't bother passing accept_order here as we know we're
                     # adding another one right after which will re-sort
-                    multiview.add(old_view, old_order, old_accept, old_phash)
+                    multiview.add(old_view, old_order, old_phash, old_accept)
                 accept_order = self.registry.queryUtility(IAcceptOrder)
-                multiview.add(derived_view, order, accept, phash, accept_order)
+                multiview.add(derived_view, order, phash, accept, accept_order)
                 for view_type in (IView, ISecuredView):
                     # unregister any existing views
                     self.registry.adapters.unregister(
@@ -1283,7 +1256,7 @@ class ViewsConfiguratorMixin(object):
         to specify a server-side, relative ordering between accept media types.
 
         ``value`` should be a :term:`media type` as specified by
-        :rfc:`7231#section-5.3.2`. For example, ``text/plain``,
+        :rfc:`7231#section-5.3.2`. For example, ``text/plain;charset=utf8``,
         ``application/json`` or ``text/html``.
 
         ``weighs_more_than`` and ``weighs_less_than`` control the ordering
@@ -1294,10 +1267,47 @@ class ViewsConfiguratorMixin(object):
         .. versionadded:: 1.10
 
         """
-        value = value.lower()
-        if '*' in value:
+        if value == '*/*':
             raise ConfigurationError(
-                '"accept" ordering is done between media types, not ranges')
+                'cannot specify an ordering for an offer of */*')
+
+        def normalize_type(type):
+            return type.lower()
+
+        def check_type(than):
+            than_type, than_subtype, than_params = Accept.parse_offer(than)
+            if (
+                # text/* vs text/plain
+                (offer_subtype == '*') ^ (than_subtype == '*')
+                # text/plain vs text/html;charset=utf8
+                or (bool(offer_params) ^ bool(than_params))
+            ):
+                raise ConfigurationError(
+                    'cannot compare across media range specificity levels')
+            # text/plain;charset=utf8 vs text/html;charset=utf8
+            if offer_params and (
+                offer_subtype != than_subtype or offer_type != than_type
+            ):
+                raise ConfigurationError(
+                    'cannot compare params across media types')
+
+        value = normalize_type(value)
+        offer_type, offer_subtype, offer_params = Accept.parse_offer(value)
+
+        if weighs_more_than:
+            if not is_nonstr_iter(weighs_more_than):
+                weighs_more_than = [weighs_more_than]
+            weighs_more_than = [normalize_type(w) for w in weighs_more_than]
+            for than in weighs_more_than:
+                check_type(than)
+
+        if weighs_less_than:
+            if not is_nonstr_iter(weighs_less_than):
+                weighs_less_than = [weighs_less_than]
+            weighs_less_than = [normalize_type(w) for w in weighs_less_than]
+            for than in weighs_less_than:
+                check_type(than)
+
         discriminator = ('accept view order', value)
         intr = self.introspectable(
             'accept view order',
diff --git a/pyramid/predicates.py b/pyramid/predicates.py
index 262b28d89..5bd98fdf2 100644
--- a/pyramid/predicates.py
+++ b/pyramid/predicates.py
@@ -130,22 +130,17 @@ class HeaderPredicate(object):
         return self.val.match(val) is not None
 
 class AcceptPredicate(object):
-    _use_deprecated_range_fallback = False
-
     def __init__(self, val, config):
         if not is_nonstr_iter(val):
-            self._use_deprecated_range_fallback = '*' in val
             val = (val,)
         self.values = val
 
     def text(self):
-        return 'accept = %s' % (self.values,)
+        return 'accept = %s' % (', '.join(self.values),)
 
     phash = text
 
     def __call__(self, context, request):
-        if self._use_deprecated_range_fallback:
-            return self.values[0] in request.accept
         return bool(request.accept.acceptable_offers(self.values))
 
 class ContainmentPredicate(object):
diff --git a/pyramid/tests/test_util.py b/pyramid/tests/test_util.py
index a76cd2017..907ca7351 100644
--- a/pyramid/tests/test_util.py
+++ b/pyramid/tests/test_util.py
@@ -1109,3 +1109,63 @@ class TestSimpleSerializer(unittest.TestCase):
     def test_dumps(self):
         inst = self._makeOne()
         self.assertEqual(inst.dumps('abc'), bytes_('abc'))
+
+
+class Test_sort_accept_offers(unittest.TestCase):
+    def _callFUT(self, offers, order=None):
+        from pyramid.util import sort_accept_offers
+        return sort_accept_offers(offers, order)
+
+    def test_default_specificities(self):
+        result = self._callFUT(['*/*', 'text/*', 'text/html', 'text/html;charset=utf8'])
+        self.assertEqual(result, [
+            'text/html;charset=utf8', 'text/html', 'text/*', '*/*',
+        ])
+
+    def test_wildcard_type_order(self):
+        result = self._callFUT(
+            ['*/*', 'text/*', 'image/*'],
+            ['image/*', 'text/*'],
+        )
+        self.assertEqual(result, ['image/*', 'text/*', '*/*'])
+
+    def test_specific_type_order(self):
+        result = self._callFUT(
+            ['text/html', 'application/json', 'text/html;charset=utf8', 'text/plain'],
+            ['application/json', 'text/html'],
+        )
+        self.assertEqual(result, [
+            'application/json', 'text/html;charset=utf8', 'text/html', 'text/plain',
+        ])
+
+    def test_params_order(self):
+        result = self._callFUT(
+            ['text/html;charset=utf8', 'text/html;charset=latin1', 'text/html;foo=bar'],
+            ['text/html;charset=latin1', 'text/html;charset=utf8'],
+        )
+        self.assertEqual(result, [
+            'text/html;charset=latin1', 'text/html;charset=utf8', 'text/html;foo=bar',
+        ])
+
+    def test_params_inherit_type_prefs(self):
+        result = self._callFUT(
+            ['text/html;charset=utf8', 'text/plain;charset=latin1'],
+            ['text/plain', 'text/html'],
+        )
+        self.assertEqual(result, ['text/plain;charset=latin1', 'text/html;charset=utf8'])
+
+    def test_params_inherit_wildcard_prefs(self):
+        result = self._callFUT(
+            ['image/png;progressive=1', 'text/html;charset=utf8'],
+            ['text/*', 'image/*'],
+        )
+        self.assertEqual(result, ['text/html;charset=utf8', 'image/png;progressive=1'])
+
+    def test_type_overrides_wildcard_prefs(self):
+        result = self._callFUT(
+            ['text/html;charset=utf8', 'image/png', 'foo/bar', 'text/bar'],
+            ['foo/*', 'text/*', 'image/*', 'image/png', 'text/html'],
+        )
+        self.assertEqual(result, [
+            'image/png', 'text/html;charset=utf8', 'foo/bar', 'text/bar',
+        ])
diff --git a/pyramid/util.py b/pyramid/util.py
index 6655455bf..677880794 100644
--- a/pyramid/util.py
+++ b/pyramid/util.py
@@ -1,3 +1,5 @@
+from collections import defaultdict
+from collections import namedtuple
 from contextlib import contextmanager
 import functools
 try:
@@ -7,6 +9,7 @@ except ImportError:  # pragma: no cover
     compare_digest = None
 import inspect
 import weakref
+from webob.acceptparse import Accept
 
 from pyramid.exceptions import (
     ConfigurationError,
@@ -649,3 +652,79 @@ class SimpleSerializer(object):
 
     def dumps(self, appstruct):
         return bytes_(appstruct)
+
+def sort_accept_offers(offers, order=None):
+    """
+    Sort a list of offers by specificity and preference.
+
+    Supported offers are of the following forms, ordered by specificity
+    (higher to lower):
+
+    - ``type/subtype;params`` and ``type/subtype``
+    - ``type/*``
+    - ``*/*``
+
+    :param offers: A list of offers to be sorted.
+    :param order: A weighted list of offer where items closer to the start of
+                  the list will be a preferred over items closer to the end.
+    :param is_order_parsed: If the list of orders is pre-parsed then do not
+                            parse it again.
+    :return: A list of offers sorted first by specificity (higher to lower)
+             then by ``order``.
+
+    """
+    if order is None:
+        order = []
+
+    max_weight = len(offers)
+
+    def find_order_index(value, default=None):
+        return next((i for i, x in enumerate(order) if x == value), default)
+
+    def offer_sort_key(value):
+        """
+        (category, type_weight, params_weight)
+
+        category:
+            1 - foo/bar and foo/bar;params
+            2 - foo/*
+            3 - */*
+
+        type_weight:
+            if category 1 & 2:
+                - index of type/* in order list
+                - ``max_weight`` if no match is found
+
+            - index of type/subtype in order list
+            - index of type/* in order list + ``max_weight``
+            - ``max_weight * 2`` if no match is found
+
+        params_weight:
+            - index of specific ``type/subtype;params`` in order list
+            - ``max_weight`` if not found
+            - ``max_weight + 1`` if no params at all
+
+        """
+        parsed = Accept.parse_offer(value)
+
+        if value == '*/*':
+            return (3, 0, 0)
+
+        elif parsed.subtype == '*':
+            type_w = find_order_index(value, max_weight)
+            return (2, type_w, 0)
+
+        type_w = find_order_index(parsed.type + '/' + parsed.subtype, None)
+        if type_w is None:
+            type_w = max_weight + find_order_index(
+                parsed.type + '/*', max_weight)
+
+        if parsed.params:
+            param_w = find_order_index(value, max_weight)
+
+        else:
+            param_w = max_weight + 1
+
+        return (1, type_w, param_w)
+
+    return sorted(offers, key=offer_sort_key)
diff --git a/tox.ini b/tox.ini
index 5a73bc426..76428846f 100644
--- a/tox.ini
+++ b/tox.ini
@@ -25,6 +25,9 @@ commands =
 extras =
     testing
 
+deps =
+    -egit+https://github.com/mmerickel/webob.git@accept-parse-offer#egg=webob
+
 [testenv:py27-scaffolds]
 basepython = python2.7
 commands =