From 8640dc278365788658edfce49a4863e749042784 Mon Sep 17 00:00:00 2001 From: kgriffs Date: Fri, 6 Sep 2013 12:39:30 -0500 Subject: [PATCH] fix(Request.client_accepts): Accept parsing doesn't handle application/* This patch adds support to the parser for robust accept header handling, courtesy of mimeparse. Care was taken to make the common cases fast. As part of this change, Request gains a new client_prefers() method that can be used for content negotiation when the service supports multiple media types. Fixes #155 --- README.md | 2 +- README.rst | 6 +- falcon/request.py | 73 ++++++++++++++++++++--- falcon/testing/helpers.py | 10 +--- falcon/tests/test_httperror.py | 2 +- falcon/tests/test_req_vars.py | 106 +++++++++++++++++++++++++++++---- setup.py | 4 +- tox.ini | 2 +- 8 files changed, 169 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 718d6a9..bac361f 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ Falcon is a [high-performance Python framework][home] for building cloud APIs. I **Fast.** Cloud APIs need to turn around requests quickly, and make efficient use of hardware. This is particularly important when serving many concurrent requests. Falcon processes requests [several times faster][bench] than other popular web frameworks. -**Light.** Only the essentials are included, with *six* being the only dependency outside the standard library. We work to keep the code lean, making Falcon easier to test, optimize, and deploy. +**Light.** Only the essentials are included, with *six* and *mimeparse* being the only dependencies outside the standard library. We work to keep the code lean, making Falcon easier to test, optimize, and deploy. **Flexible.** Falcon can be deployed in a variety of ways, depending on your needs. The framework speaks WSGI, and works great with [Python 2.6 and 2.7, PyPy, and Python 3.3][ci]. There's no tight coupling with any async framework, leaving you free to mix-and-match what you need. diff --git a/README.rst b/README.rst index dc70ecb..da0a63f 100644 --- a/README.rst +++ b/README.rst @@ -28,9 +28,9 @@ many concurrent requests. Falcon processes requests `several times faster `__ than other popular web frameworks. -**Light.** Only the essentials are included, with *six* being the only -dependency outside the standard library. We work to keep the code lean, -making Falcon easier to test, optimize, and deploy. +**Light.** Only the essentials are included, with *six* and *mimeparse* +being the only dependencies outside the standard library. We work to keep +the code lean, making Falcon easier to test, optimize, and deploy. **Flexible.** Falcon can be deployed in a variety of ways, depending on your needs. The framework speaks WSGI, and works great with `Python 2.6 diff --git a/falcon/request.py b/falcon/request.py index 73c767d..5f4abbe 100644 --- a/falcon/request.py +++ b/falcon/request.py @@ -18,6 +18,7 @@ limitations under the License. from datetime import datetime +import mimeparse import six from falcon.exceptions import HTTPBadRequest @@ -30,6 +31,8 @@ DEFAULT_ERROR_LOG_FORMAT = (u'{0:%Y-%m-%d %H:%M:%S} [FALCON] [ERROR]' TRUE_STRINGS = ('true', 'True', 'yes') FALSE_STRINGS = ('false', 'False', 'no') +MEDIA_TYPES_XML = ('application/xml', 'text/xml') + class InvalidHeaderValueError(HTTPBadRequest): def __init__(self, msg, href=None, href_text=None): @@ -149,19 +152,73 @@ class Request(object): @property def client_accepts_xml(self): """Return True if the Accept header indicates XML support.""" - return self.client_accepts('application/xml') + return self.client_accepts(MEDIA_TYPES_XML) - def client_accepts(self, media_type): - """Return True if the Accept header indicates a media type support.""" + def client_accepts(self, media_types): + """Returns the client's preferred media type. - accept = self._get_header_by_wsgi_name('ACCEPT') - return ((accept is not None) and - ((media_type in accept) or ('*/*' in accept))) + Args: + media_types: One or more media types. May be a single string ( + of type str), or an iterable collection of strings. + + Returns: + True IFF the client has indicated in the Accept header that + they accept at least one of the specified media types. + """ + + accept = self.accept + + # PERF(kgriffs): Usually the following will be true, so + # try it first. + if isinstance(media_types, str): + if (accept == media_types) or (accept == '*/*'): + return accept + + # NOTE(kgriffs): Convert to a collection to be compatible + # with mimeparse.best_matchapplication/xhtml+xml + media_types = (media_types,) + + # NOTE(kgriffs): Heuristic to quickly check another common case. If + # accept is a single type, and it is found in media_types verbatim, + # return the media type immediately. + elif accept in media_types: + return accept + + # Fall back to full-blown parsing + preferred_type = self.client_prefers(media_types) + return preferred_type is not None + + def client_prefers(self, media_types): + """Returns the client's preferred media type given several choices. + + Args: + media_types: One or more media types from which to choose the + client's preferred type. This value MUST be an iterable + collection of strings. + + Returns: + The client's preferred media type, based on the Accept header, + or None if the client does not accept any of the specified + types. + """ + + try: + # NOTE(kgriffs): best_match will return '' if no match is found + preferred_type = mimeparse.best_match(media_types, self.accept) + except ValueError: + # Value for the accept header was not formatted correctly + preferred_type = '' + + return (preferred_type if preferred_type else None) @property def accept(self): - """Value of the Accept header, or None if not found.""" - return self._get_header_by_wsgi_name('ACCEPT') + """Value of the Accept header, or */* if not found per RFC.""" + accept = self._get_header_by_wsgi_name('ACCEPT') + + # NOTE(kgriffs): Per RFC, missing accept header is + # equivalent to '*/*' + return '*/*' if accept is None else accept @property def app(self): diff --git a/falcon/testing/helpers.py b/falcon/testing/helpers.py index 5ff493c..7523938 100644 --- a/falcon/testing/helpers.py +++ b/falcon/testing/helpers.py @@ -91,9 +91,7 @@ def create_environ(path='/', query_string='', protocol='HTTP/1.1', port='80', 'REQUEST_METHOD': method, 'PATH_INFO': path, 'QUERY_STRING': query_string, - 'HTTP_ACCEPT': '*/*', - 'HTTP_USER_AGENT': ('curl/7.24.0 (x86_64-apple-darwin12.0) ' - 'libcurl/7.24.0 OpenSSL/0.9.8r zlib/1.2.5'), + 'HTTP_USER_AGENT': 'curl/7.24.0 (x86_64-apple-darwin12.0)', 'REMOTE_PORT': '65133', 'RAW_URI': '/', 'REMOTE_ADDR': '127.0.0.1', @@ -131,12 +129,6 @@ def _add_headers_to_environ(env, headers): for name, value in headers.items(): name = name.upper().replace('-', '_') - if value is None: - if name == 'ACCEPT' or name == 'USER_AGENT': - del env['HTTP_' + name] - - continue - if name == 'CONTENT_TYPE': env[name] = value.strip() elif name == 'CONTENT_LENGTH': diff --git a/falcon/tests/test_httperror.py b/falcon/tests/test_httperror.py index 6538e8e..1f72c31 100644 --- a/falcon/tests/test_httperror.py +++ b/falcon/tests/test_httperror.py @@ -175,7 +175,7 @@ class TestHTTPError(testing.TestBase): def test_client_does_not_accept_anything(self): headers = { - 'Accept': None, + 'Accept': '45087gigo;;;;', 'X-Error-Title': 'Storage service down', 'X-Error-Description': ('The configured storage service is not ' 'responding to requests. Please contact ' diff --git a/falcon/tests/test_req_vars.py b/falcon/tests/test_req_vars.py index 7892bf0..c36d735 100644 --- a/falcon/tests/test_req_vars.py +++ b/falcon/tests/test_req_vars.py @@ -87,26 +87,107 @@ class TestReqVars(testing.TestBase): headers = {'Accept': 'application/xml'} req = Request(testing.create_environ(headers=headers)) self.assertTrue(req.client_accepts('application/xml')) + self.assertTrue(req.client_accepts(['application/xml'])) headers = {'Accept': '*/*'} req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts(['application/xml'])) + + headers = {} # NOTE(kgriffs): Equivalent to '*/*' per RFC + req = Request(testing.create_environ(headers=headers)) self.assertTrue(req.client_accepts('application/xml')) + self.assertTrue(req.client_accepts(['application/xml'])) headers = {'Accept': 'application/json'} req = Request(testing.create_environ(headers=headers)) - self.assertFalse(req.client_accepts('application/xml')) + self.assertFalse(req.client_accepts(['application/xml'])) headers = {'Accept': 'application/xm'} req = Request(testing.create_environ(headers=headers)) - self.assertFalse(req.client_accepts('application/xml')) + self.assertFalse(req.client_accepts(['application/xml'])) + + headers = {'Accept': 'application/*'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts(['application/json'])) + self.assertTrue(req.client_accepts(['application/xml'])) + self.assertTrue(req.client_accepts(['application/json', + 'application/xml'])) + + headers = {'Accept': 'text/*'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts('text/plain')) + self.assertTrue(req.client_accepts('text/csv')) + self.assertFalse(req.client_accepts('application/xhtml+xml')) + + headers = {'Accept': 'text/*, application/xhtml+xml; q=0.0'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts('text/plain')) + self.assertTrue(req.client_accepts('text/csv')) + self.assertTrue(req.client_accepts('application/xhtml+xml')) + self.assertTrue(req.client_accepts(('application/xhtml+xml', + 'text/plain', + 'text/csv'))) + + headers = {'Accept': 'text/*; q=0.1, application/xhtml+xml; q=0.5'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts('text/plain')) + + headers = {'Accept': 'text/*, application/*'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts('text/plain')) + self.assertTrue(req.client_accepts('application/json')) + + headers = {'Accept': 'text/*,application/*'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts('text/plain')) + self.assertTrue(req.client_accepts('application/json')) def test_client_accepts_props(self): headers = {'Accept': 'application/xml'} req = Request(testing.create_environ(headers=headers)) - self.assertTrue(req.client_accepts_xml) self.assertFalse(req.client_accepts_json) + headers = {'Accept': 'text/xml'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts_xml) + + headers = {'Accept': 'text/*'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts_xml) + + headers = {'Accept': 'text/xml, application/xml'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts_xml) + + headers = {'Accept': 'application/json'} + req = Request(testing.create_environ(headers=headers)) + self.assertFalse(req.client_accepts_xml) + self.assertTrue(req.client_accepts_json) + + headers = {'Accept': 'application/json, application/xml'} + req = Request(testing.create_environ(headers=headers)) + self.assertTrue(req.client_accepts_xml) + self.assertTrue(req.client_accepts_json) + + def test_client_prefers(self): + headers = {'Accept': 'application/xml'} + req = Request(testing.create_environ(headers=headers)) + preferred_type = req.client_prefers(['application/xml']) + self.assertEquals(preferred_type, 'application/xml') + + headers = {'Accept': '*/*'} + preferred_type = req.client_prefers(('application/xml', + 'application/json')) + + # NOTE(kgriffs): If client doesn't care, "preferr" the first one + self.assertEquals(preferred_type, 'application/xml') + + headers = {'Accept': 'text/*; q=0.1, application/xhtml+xml; q=0.5'} + req = Request(testing.create_environ(headers=headers)) + preferred_type = req.client_prefers(['application/xhtml+xml']) + self.assertEquals(preferred_type, 'application/xhtml+xml') + def test_range(self): headers = {'Range': '10-'} req = Request(testing.create_environ(headers=headers)) @@ -124,8 +205,7 @@ class TestReqVars(testing.TestBase): req = Request(testing.create_environ(headers=headers)) self.assertIs(req.range, None) - headers = {'Range': None} - req = Request(testing.create_environ(headers=headers)) + req = Request(testing.create_environ()) self.assertIs(req.range, None) def test_range_invalid(self): @@ -230,9 +310,11 @@ class TestReqVars(testing.TestBase): date = testing.httpnow() hash = 'fa0d1a60ef6616bb28038515c8ea4cb2' auth = 'HMAC_SHA1 c590afa9bb59191ffab30f223791e82d3fd3e3af' - agent = 'curl/7.24.0 (x86_64-apple-darwin12.0)' + agent = 'testing/1.0.1' + default_agent = 'curl/7.24.0 (x86_64-apple-darwin12.0)' - self._test_attribute_header('Accept', 'x-falcon', 'accept') + self._test_attribute_header('Accept', 'x-falcon', 'accept', + default='*/*') self._test_attribute_header('Authorization', auth, 'auth') @@ -248,7 +330,8 @@ class TestReqVars(testing.TestBase): self._test_attribute_header('If-Unmodified-Since', date, 'if_unmodified_since') - self._test_attribute_header('User-Agent', agent, 'user_agent') + self._test_attribute_header('User-Agent', agent, 'user_agent', + default=default_agent) def test_method(self): self.assertEquals(self.req.method, 'GET') @@ -270,11 +353,10 @@ class TestReqVars(testing.TestBase): # Helpers # ------------------------------------------------------------------------- - def _test_attribute_header(self, name, value, attr): + def _test_attribute_header(self, name, value, attr, default=None): headers = {name: value} req = Request(testing.create_environ(headers=headers)) self.assertEquals(getattr(req, attr), value) - headers = {name: None} - req = Request(testing.create_environ(headers=headers)) - self.assertEqual(getattr(req, attr), None) + req = Request(testing.create_environ()) + self.assertEqual(getattr(req, attr), default) diff --git a/setup.py b/setup.py index 1b4c75c..2cb1555 100644 --- a/setup.py +++ b/setup.py @@ -6,7 +6,9 @@ from setuptools import setup, find_packages, Extension VERSION = imp.load_source('version', path.join('.', 'falcon', 'version.py')) VERSION = VERSION.__version__ -REQUIRES = ['six'] +# NOTE(kgriffs): python-mimeparse is newer than mimeparse, supports Py3 +# TODO(kgriffs): Fork and optimize/modernize python-mimeparse +REQUIRES = ['six', 'python-mimeparse'] if sys.version_info < (2, 7): REQUIRES.append('ordereddict') diff --git a/tox.ini b/tox.ini index 8075b6e..dc24b30 100644 --- a/tox.ini +++ b/tox.ini @@ -24,5 +24,5 @@ commands = py3kwarn falcon deps = flake8 commands = flake8 \ --max-complexity=12 \ - --exclude=.venv,.tox,dist,doc,./falcon/bench/nuts \ + --exclude=./build,.venv,.tox,dist,doc,./falcon/bench/nuts \ --ignore=F403