Skip to content

Commit

Permalink
remove media range support for offers
Browse files Browse the repository at this point in the history 
see the multitude of reasons in
Pylons/pyramid#3326

the short answer is that they are fundamentally broken in that media
ranges cannot properly match against any accept header with q=0 content
  • Loading branch information
@mmerickel
mmerickel committed Oct 10, 2018
1 parent ab13441 commit d0d538f
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 101 deletions.
5 changes: 0 additions & 5 deletions CHANGES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,6 @@ Feature

- Add Request.remote_host, exposing REMOTE_HOST environment variable.

- Added support for media ranges as offers when matching against the
``Accept`` header via ``acceptparse.AcceptValidHeader.acceptable_offers``.
The algorithm for matching offer ranges against header ranges is described
in the documentation. See https://github.com/Pylons/webob/pull/370

- Added ``acceptparse.Accept.parse_offer`` to codify what types of offers
are compatible with ``acceptparse.AcceptValidHeader.acceptable_offers``,
``acceptparse.AcceptMissingHeader.acceptable_offers``, and
Expand Down
88 changes: 9 additions & 79 deletions src/webob/acceptparse.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,28 +75,7 @@ def _list_1_or_more__compiled_re(element_re):
)


class AcceptOffer(namedtuple('AcceptOffer', ['type', 'subtype', 'params'])):
__slots__ = ()

SPECIFICITY_NONE = 1 # */*
SPECIFICITY_TYPE = 2 # text/*
SPECIFICITY_SUBTYPE = 3 # text/html
SPECIFICITY_PARAMS = 4 # text/html;charset=utf8

@property
def is_range(self):
return self.type == '*' or self.subtype == '*'

@property
def specificity(self):
if self.params:
return self.SPECIFICITY_PARAMS
elif self.subtype != '*':
return self.SPECIFICITY_SUBTYPE
elif self.type != '*':
return self.SPECIFICITY_TYPE
else:
return self.SPECIFICITY_NONE
AcceptOffer = namedtuple('AcceptOffer', ['type', 'subtype', 'params'])


class Accept(object):
Expand Down Expand Up @@ -444,15 +423,10 @@ def parse_offer(cls, offer):
| *params* is a list containing ``(*parameter name*, *value*)``
values.
| The result also supports ``is_range`` and ``specificity``
properties. Specificity is a value from 1 to 4 where ``*/*``
is 1, ``text/*`` is 2, ``text/html`` is 3 and
``text/html;charset=utf8`` is 4.
:raises ValueError: If the offer does not match the required format.
"""
match = cls.media_type_compiled_re.match(offer)
match = cls.media_type_compiled_re.match(offer.lower())
if not match:
raise ValueError('Invalid value for an Accept offer.')

Expand All @@ -461,21 +435,7 @@ def parse_offer(cls, offer):
offer_params = cls._parse_media_type_params(
media_type_params_segment=groups[1],
)
# offer_type, offer_subtype, offer_params, invalid, example
# == * == * true Y/N
# N N N N a/b
# N N Y N a/b;x=y
# N Y N N a/*
# N Y Y Y a/*;x=y
# Y N N Y */b
# Y N Y Y */b;x=y
# Y Y N N */*
# Y Y Y Y */*;x=y
# simplifies to (A and not B or B and C)
if (
(offer_type == '*' and offer_subtype != '*')
or (offer_subtype == '*' and offer_params)
):
if offer_type == '*' or offer_subtype == '*':
raise ValueError('Invalid value for an Accept offer.')
return AcceptOffer(offer_type, offer_subtype, offer_params)

Expand All @@ -491,7 +451,7 @@ def _parse_and_normalize_offers(cls, offers):
parsed_offers = []
for index, offer in enumerate(offers):
try:
parsed_offer = cls.parse_offer(offer.lower())
parsed_offer = cls.parse_offer(offer)
except ValueError:
continue
parsed_offers.append([index, parsed_offer])
Expand Down Expand Up @@ -860,12 +820,8 @@ def acceptable_offers(self, offers):
This uses the matching rules described in :rfc:`RFC 7231, section 5.3.2
<7231#section-5.3.2>`.
Any offers that do not match the media type grammar will be ignored.
This function also supports media ranges (without media type
parameters) but without any specificity. An offered media range is
assigned the highest q-value of any media range from the header that
would match any media type that could be derived from the offer.
Any offers that cannot be parsed via
:meth:`.Accept.parse_offer` will be ignored.
:param offers: ``iterable`` of ``str`` media types (media types can
include media type parameters)
Expand Down Expand Up @@ -894,47 +850,20 @@ def acceptable_offers(self, offers):
acceptable_offers_n_quality_factors = {}
for offer_index, parsed_offer in lowercased_offers_parsed:
offer = offers[offer_index]
offer_is_range = parsed_offer.is_range
offer_type, offer_subtype, offer_media_type_params = parsed_offer
for (
range_type_subtype, range_qvalue, range_media_type_params, __,
) in lowercased_ranges:
range_type, range_subtype = range_type_subtype.split('/', 1)

# if a media range is supplied as an offer then specificity is
# unimportant, we'll just compare for match and use the
# highest matching qvalue
if offer_is_range:
if (
# Accept: anything, offer=*/*
(offer_type == '*' and offer_subtype == '*')

# Accept: text/anything, offer=text/*
or (offer_type == range_type and offer_subtype == '*')

# Accept: */*, offer=anything
or (
range_type == '*' and range_subtype == '*'
and range_media_type_params == []
)
):
prev_match = acceptable_offers_n_quality_factors.get(offer)
if not prev_match or prev_match[0] < range_qvalue:
acceptable_offers_n_quality_factors[offer] = (
range_qvalue, # qvalue of matched range
offer_index,
4, # unused for offers that are media ranges
)
continue

# The specificity values below are based on the list in the
# example in RFC 7231 section 5.3.2 explaining how "media
# ranges can be overridden by more specific media ranges or
# specific media types". We assign specificity to the list
# items in reverse order, so specificity 4, 3, 2, 1 correspond
# to 1, 2, 3, 4 in the list, respectively (so that higher
# specificity has higher precedence).
elif (
if (
offer_type == range_type
and offer_subtype == range_subtype
):
Expand Down Expand Up @@ -1356,7 +1285,8 @@ def acceptable_offers(self, offers):
"""
Return the offers that are acceptable according to the header.
Any offers that do not match the media type grammar will be ignored.
Any offers that cannot be parsed via
:meth:`.Accept.parse_offer` will be ignored.
:param offers: ``iterable`` of ``str`` media types (media types can
include media type parameters)
Expand Down
25 changes: 8 additions & 17 deletions tests/test_acceptparse.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,26 +382,20 @@ def test_parse__valid_header(self, value, expected_list):
list_of_returned = list(returned)
assert list_of_returned == expected_list

@pytest.mark.parametrize('offer, expected_return, is_range, specificity', [
['text/html', ('text', 'html', []), False, 3],
@pytest.mark.parametrize('offer, expected_return', [
['text/html', ('text', 'html', [])],
[
'text/html;charset=utf8',
('text', 'html', [('charset', 'utf8')]),
False, 4,
],
[
'text/html;charset=utf8;x-version=1',
('text', 'html', [('charset', 'utf8'), ('x-version', '1')]),
False, 4,
],
['text/*', ('text', '*', []), True, 2],
['*/*', ('*', '*', []), True, 1],
])
def test_parse_offer__valid(self, offer, expected_return, is_range, specificity):
def test_parse_offer__valid(self, offer, expected_return):
result = Accept.parse_offer(offer)
assert result == expected_return
assert result.is_range == is_range
assert result.specificity == specificity

@pytest.mark.parametrize('offer', [
'',
Expand All @@ -412,6 +406,8 @@ def test_parse_offer__valid(self, offer, expected_return, is_range, specificity)
'*/plain;charset=utf8;x-version=1',
'*/*;charset=utf8',
'text/*;charset=utf8',
'text/*',
'*/*',
])
def test_parse_offer__invalid(self, offer):
with pytest.raises(ValueError):
Expand Down Expand Up @@ -1082,25 +1078,20 @@ def test_acceptable_offers__invalid_offers(
('text/plain', 0.3),
],
),
(
'text/*;q=0.3, text/html;q=0.5, text/html;level=1;q=0.7',
['*/*', 'text/*', 'text/html', 'image/*'],
[('*/*', 0.7), ('text/*', 0.7), ('text/html', 0.5)],
),
(
'text/*;q=0.3, text/html;q=0.5, text/html;level=1;q=0.7',
['text/*', '*/*', 'text/html', 'image/*'],
[('text/*', 0.7), ('*/*', 0.7), ('text/html', 0.5)],
[('text/html', 0.5)],
),
(
'text/html;level=1;q=0.7',
['text/*', '*/*', 'text/html', 'text/html;level=1', 'image/*'],
[('text/*', 0.7), ('*/*', 0.7), ('text/html;level=1', 0.7)],
[('text/html;level=1', 0.7)],
),
(
'*/*',
['text/*'],
[('text/*', 1.0)],
[],
),
(
'',
Expand Down

0 comments on commit d0d538f

Please sign in to comment.