From cb67e01c07be14994229f584d8b6b6edcf84da78 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 18:52:25 -0600 Subject: [PATCH 01/13] Using WebOb's acceptparse find best mimetype to use We default to text/plain. --- pyramid/httpexceptions.py | 51 +++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/pyramid/httpexceptions.py b/pyramid/httpexceptions.py index 8bf9a0a729..ef1e5c144e 100644 --- a/pyramid/httpexceptions.py +++ b/pyramid/httpexceptions.py @@ -123,12 +123,14 @@ field. Reflecting this, these subclasses have one additional keyword argument: ``location``, which indicates the location to which to redirect. """ +import json from string import Template from zope.interface import implementer from webob import html_escape as _html_escape +from webob.acceptparse import Accept from pyramid.compat import ( class_types, @@ -214,7 +216,7 @@ class HTTPException(Response, Exception): empty_body = False def __init__(self, detail=None, headers=None, comment=None, - body_template=None, **kw): + body_template=None, json_formatter=None, **kw): status = '%s %s' % (self.code, self.title) Response.__init__(self, status=status, **kw) Exception.__init__(self, detail) @@ -225,6 +227,8 @@ def __init__(self, detail=None, headers=None, comment=None, if body_template is not None: self.body_template = body_template self.body_template_obj = Template(body_template) + if json_formatter is not None: + self._json_formatter = json_formatter if self.empty_body: del self.content_type @@ -233,31 +237,64 @@ def __init__(self, detail=None, headers=None, comment=None, def __str__(self): return self.detail or self.explanation + def _json_formatter(self, status, body, title, environ): + return {'message': body, + 'code': status, + 'title': self.title} + def prepare(self, environ): if not self.body and not self.empty_body: html_comment = '' comment = self.comment or '' - accept = environ.get('HTTP_ACCEPT', '') - if accept and 'html' in accept or '*/*' in accept: + accept_value = environ.get('HTTP_ACCEPT', '') + accept = Accept(accept_value) + match = accept.best_match( + ['application/json', + 'text/html', + 'text/plain'], default_match='text/plain') + + if match == 'text/html': self.content_type = 'text/html' + self.charset = 'utf-8' escape = _html_escape page_template = self.html_template_obj br = '
' if comment: html_comment = '' % escape(comment) + elif match == 'application/json': + self.content_type = 'application/json' + self.charset = '' + escape = _no_escape + br = '\n' + if comment: + html_comment = escape(comment) + + class JsonPageTemplate(object): + def __init__(self, excobj): + self.excobj = excobj + + def substitute(self, status, body): + jsonbody = self.excobj._json_formatter( + status=status, + body=body, title=self.excobj.title, + environ=environ) + return json.dumps(jsonbody) + + page_template = JsonPageTemplate(self) else: self.content_type = 'text/plain' + self.charset = 'utf-8' escape = _no_escape page_template = self.plain_template_obj br = '\n' if comment: html_comment = escape(comment) args = { - 'br':br, + 'br': br, 'explanation': escape(self.explanation), 'detail': escape(self.detail or ''), 'comment': escape(comment), - 'html_comment':html_comment, + 'html_comment': html_comment, } body_tmpl = self.body_template_obj if HTTPException.body_template_obj is not body_tmpl: @@ -1001,8 +1038,8 @@ class HTTPInternalServerError(HTTPServerError): code = 500 title = 'Internal Server Error' explanation = ( - 'The server has either erred or is incapable of performing ' - 'the requested operation.') + 'The server has either erred or is incapable of performing ' + 'the requested operation.') class HTTPNotImplemented(HTTPServerError): """ From d9d194b62706dccb8d8b77cff3e424a1947f0e80 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 20:17:29 -0600 Subject: [PATCH 02/13] Use MIMEAccept not Accept Accept doesn't understand the notation of major/minor masks. --- pyramid/httpexceptions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyramid/httpexceptions.py b/pyramid/httpexceptions.py index ef1e5c144e..5500a0a298 100644 --- a/pyramid/httpexceptions.py +++ b/pyramid/httpexceptions.py @@ -130,7 +130,7 @@ from zope.interface import implementer from webob import html_escape as _html_escape -from webob.acceptparse import Accept +from webob.acceptparse import MIMEAccept from pyramid.compat import ( class_types, @@ -247,7 +247,7 @@ def prepare(self, environ): html_comment = '' comment = self.comment or '' accept_value = environ.get('HTTP_ACCEPT', '') - accept = Accept(accept_value) + accept = MIMEAccept(accept_value) match = accept.best_match( ['application/json', 'text/html', From b799e3ced5422fac7ca109e29c4fd32e08157dcf Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 19:37:02 -0600 Subject: [PATCH 03/13] Explicit set Accept header to text/html The default is now text/plain, so explicitly set the accept header for what we want to accept. --- pyramid/tests/test_httpexceptions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyramid/tests/test_httpexceptions.py b/pyramid/tests/test_httpexceptions.py index b94ef30e44..47d138d7be 100644 --- a/pyramid/tests/test_httpexceptions.py +++ b/pyramid/tests/test_httpexceptions.py @@ -230,11 +230,11 @@ def test__default_app_iter_no_comment_html(self): body = list(exc(environ, start_response))[0] self.assertFalse(b'' in body) From 4061d5bc2a8c066e811388070033946d80eaccb7 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 20:13:32 -0600 Subject: [PATCH 04/13] Update tests to verif Content-Type header --- pyramid/tests/test_httpexceptions.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/pyramid/tests/test_httpexceptions.py b/pyramid/tests/test_httpexceptions.py index 47d138d7be..a4b936a388 100644 --- a/pyramid/tests/test_httpexceptions.py +++ b/pyramid/tests/test_httpexceptions.py @@ -212,6 +212,9 @@ def test__default_app_iter_no_comment_plain(self): environ = _makeEnviron() start_response = DummyStartResponse() body = list(exc(environ, start_response))[0] + for header in start_response.headerlist: + if header[0] == 'Content-Type': + self.assertEqual(header[1], 'text/plain; charset=UTF-8') self.assertEqual(body, b'200 OK\n\nexplanation\n\n\n\n\n') def test__default_app_iter_with_comment_plain(self): @@ -220,14 +223,20 @@ def test__default_app_iter_with_comment_plain(self): environ = _makeEnviron() start_response = DummyStartResponse() body = list(exc(environ, start_response))[0] + for header in start_response.headerlist: + if header[0] == 'Content-Type': + self.assertEqual(header[1], 'text/plain; charset=UTF-8') self.assertEqual(body, b'200 OK\n\nexplanation\n\n\n\ncomment\n') - + def test__default_app_iter_no_comment_html(self): cls = self._getTargetSubclass() exc = cls() environ = _makeEnviron() start_response = DummyStartResponse() body = list(exc(environ, start_response))[0] + for header in start_response.headerlist: + if header[0] == 'Content-Type': + self.assertEqual(header[1], 'text/plain; charset=UTF-8') self.assertFalse(b'' in body) - def test__default_app_iter_with_comment_html2(self): + def test__default_app_iter_with_comment_html(self): cls = self._getTargetSubclass() exc = cls(comment='comment & comment') environ = _makeEnviron() From a66ce9e5330abbe5de85b4de49f905293804be0a Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 20:14:01 -0600 Subject: [PATCH 05/13] Add new tests to verify we get what we ask for This simply makes sure we get back the appropriate Content-Type based upon our Accept header. --- pyramid/tests/test_httpexceptions.py | 43 ++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/pyramid/tests/test_httpexceptions.py b/pyramid/tests/test_httpexceptions.py index a4b936a388..223f8ab350 100644 --- a/pyramid/tests/test_httpexceptions.py +++ b/pyramid/tests/test_httpexceptions.py @@ -239,6 +239,49 @@ def test__default_app_iter_no_comment_html(self): self.assertEqual(header[1], 'text/plain; charset=UTF-8') self.assertFalse(b'' in body) + def test__default_app_iter_with_comment_json(self): + cls = self._getTargetSubclass() + exc = cls(comment='comment & comment') + environ = _makeEnviron() + environ['HTTP_ACCEPT'] = 'application/json' + start_response = DummyStartResponse() + body = list(exc(environ, start_response))[0] + import json + retval = json.loads(body.decode('UTF-8')) + self.assertEqual(retval['code'], '200 OK') + self.assertEqual(retval['title'], 'OK') + + def test__default_app_iter_with_custom_json(self): + def json_formatter(status, body, title, environ): + return {'message': body, + 'code': status, + 'title': title, + 'custom': environ['CUSTOM_VARIABLE'] + } + cls = self._getTargetSubclass() + exc = cls(comment='comment', json_formatter=json_formatter) + environ = _makeEnviron() + environ['HTTP_ACCEPT'] = 'application/json' + environ['CUSTOM_VARIABLE'] = 'custom!' + start_response = DummyStartResponse() + body = list(exc(environ, start_response))[0] + import json + retval = json.loads(body.decode('UTF-8')) + self.assertEqual(retval['code'], '200 OK') + self.assertEqual(retval['title'], 'OK') + self.assertEqual(retval['custom'], 'custom!') + def test_custom_body_template(self): cls = self._getTargetSubclass() exc = cls(body_template='${REQUEST_METHOD}') From f16e6ea20899ed6acf7622258b75e42f591d58d7 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 20:16:26 -0600 Subject: [PATCH 07/13] We don't need to explicitly set charset for text/* application/json however doesn't have a charset, so we just specify that as UTF-8 for the purpose of encoding the bytes. --- pyramid/httpexceptions.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pyramid/httpexceptions.py b/pyramid/httpexceptions.py index 5500a0a298..213e4b980b 100644 --- a/pyramid/httpexceptions.py +++ b/pyramid/httpexceptions.py @@ -255,7 +255,6 @@ def prepare(self, environ): if match == 'text/html': self.content_type = 'text/html' - self.charset = 'utf-8' escape = _html_escape page_template = self.html_template_obj br = '
' @@ -263,7 +262,7 @@ def prepare(self, environ): html_comment = '' % escape(comment) elif match == 'application/json': self.content_type = 'application/json' - self.charset = '' + self.charset = None escape = _no_escape br = '\n' if comment: @@ -283,7 +282,6 @@ def substitute(self, status, body): page_template = JsonPageTemplate(self) else: self.content_type = 'text/plain' - self.charset = 'utf-8' escape = _no_escape page_template = self.plain_template_obj br = '\n' @@ -311,7 +309,7 @@ def substitute(self, status, body): body = body_tmpl.substitute(args) page = page_template.substitute(status=self.status, body=body) if isinstance(page, text_type): - page = page.encode(self.charset) + page = page.encode(self.charset if self.charset else 'UTF-8') self.app_iter = [page] self.body = page From c6a950ba64743d18d1ac401bd135b1e83b68ba2d Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 20:35:03 -0600 Subject: [PATCH 08/13] PEP8 --- pyramid/tests/test_httpexceptions.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pyramid/tests/test_httpexceptions.py b/pyramid/tests/test_httpexceptions.py index 361006f292..69c7579cec 100644 --- a/pyramid/tests/test_httpexceptions.py +++ b/pyramid/tests/test_httpexceptions.py @@ -28,9 +28,9 @@ def test_status_201(self): self.assertTrue(isinstance(self._callFUT(201), HTTPCreated)) def test_extra_kw(self): - resp = self._callFUT(404, headers=[('abc', 'def')]) + resp = self._callFUT(404, headers=[('abc', 'def')]) self.assertEqual(resp.headers['abc'], 'def') - + class Test_default_exceptionresponse_view(unittest.TestCase): def _callFUT(self, context, request): from pyramid.httpexceptions import default_exceptionresponse_view @@ -129,7 +129,7 @@ def test_ctor_extends_headers(self): def test_ctor_sets_body_template_obj(self): exc = self._makeOne(body_template='${foo}') self.assertEqual( - exc.body_template_obj.substitute({'foo':'foo'}), 'foo') + exc.body_template_obj.substitute({'foo': 'foo'}), 'foo') def test_ctor_with_empty_body(self): cls = self._getTargetSubclass(empty_body=True) @@ -160,7 +160,7 @@ def test_ctor_with_body_sets_default_app_iter_html(self): self.assertTrue(b'200 OK' in body) self.assertTrue(b'explanation' in body) self.assertTrue(b'detail' in body) - + def test_ctor_with_body_sets_default_app_iter_text(self): cls = self._getTargetSubclass() exc = cls('detail') @@ -173,7 +173,7 @@ def test__str__detail(self): exc = self._makeOne() exc.detail = 'abc' self.assertEqual(str(exc), 'abc') - + def test__str__explanation(self): exc = self._makeOne() exc.explanation = 'def' @@ -348,7 +348,8 @@ def test_custom_body_template_with_custom_variable_doesnt_choke(self): exc = cls(body_template='${REQUEST_METHOD}') environ = _makeEnviron() class Choke(object): - def __str__(self): raise ValueError + def __str__(self): # pragma nocover + raise ValueError environ['gardentheory.user'] = Choke() start_response = DummyStartResponse() body = list(exc(environ, start_response))[0] @@ -380,7 +381,7 @@ def _doit(self, content_type): self.assertTrue(bytes_(exc.status) in result) L.append(result) self.assertEqual(len(L), len(status_map)) - + def test_it_plain(self): self._doit('text/plain') @@ -454,12 +455,11 @@ class DummyStartResponse(object): def __call__(self, status, headerlist): self.status = status self.headerlist = headerlist - + def _makeEnviron(**kw): - environ = {'REQUEST_METHOD':'GET', - 'wsgi.url_scheme':'http', - 'SERVER_NAME':'localhost', - 'SERVER_PORT':'80'} + environ = {'REQUEST_METHOD': 'GET', + 'wsgi.url_scheme': 'http', + 'SERVER_NAME': 'localhost', + 'SERVER_PORT': '80'} environ.update(kw) return environ - From c57b44e38727af4ce93ea703dc63d65d2831ace4 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Tue, 12 Apr 2016 20:42:22 -0600 Subject: [PATCH 09/13] For */* case, MIMEAccept picks first server offer This means that to make "text/plain" the default, we need to specifically make it the first thing we offer. For anything else, since the server offers are all weighted equally, the client order should be accepted. --- pyramid/httpexceptions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyramid/httpexceptions.py b/pyramid/httpexceptions.py index 213e4b980b..422f2f27f1 100644 --- a/pyramid/httpexceptions.py +++ b/pyramid/httpexceptions.py @@ -249,9 +249,9 @@ def prepare(self, environ): accept_value = environ.get('HTTP_ACCEPT', '') accept = MIMEAccept(accept_value) match = accept.best_match( - ['application/json', + ['text/plain', 'text/html', - 'text/plain'], default_match='text/plain') + 'application/json'], default_match='text/plain') if match == 'text/html': self.content_type = 'text/html' From 80f9822d85ba7e7efedff90648a336594171a3d6 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Thu, 14 Apr 2016 01:06:35 -0600 Subject: [PATCH 10/13] Make text/html the preferred server return This matches the original code whereby it would return an HTML page if you sent an Accept header of */*. --- pyramid/httpexceptions.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pyramid/httpexceptions.py b/pyramid/httpexceptions.py index 422f2f27f1..893795c11b 100644 --- a/pyramid/httpexceptions.py +++ b/pyramid/httpexceptions.py @@ -248,10 +248,10 @@ def prepare(self, environ): comment = self.comment or '' accept_value = environ.get('HTTP_ACCEPT', '') accept = MIMEAccept(accept_value) - match = accept.best_match( - ['text/plain', - 'text/html', - 'application/json'], default_match='text/plain') + # Attempt to match text/html or application/json, if those don't + # match, we will always have our default of text/plain + match = accept.best_match(['text/html', 'application/json'], + default_match='text/plain') if match == 'text/html': self.content_type = 'text/html' From d5e3a7cc63135c26563d9fad93e67856ade726d4 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Thu, 14 Apr 2016 01:07:41 -0600 Subject: [PATCH 11/13] Update test to verify the default is text/html --- pyramid/tests/test_httpexceptions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyramid/tests/test_httpexceptions.py b/pyramid/tests/test_httpexceptions.py index 69c7579cec..6c6e16d551 100644 --- a/pyramid/tests/test_httpexceptions.py +++ b/pyramid/tests/test_httpexceptions.py @@ -249,7 +249,7 @@ def test__content_type(self): if header[0] == 'Content-Type': self.assertEqual(header[1], 'text/plain; charset=UTF-8') - def test__content_type_default_is_plain(self): + def test__content_type_default_is_html(self): cls = self._getTargetSubclass() exc = cls() environ = _makeEnviron() @@ -258,7 +258,7 @@ def test__content_type_default_is_plain(self): exc(environ, start_response) for header in start_response.headerlist: if header[0] == 'Content-Type': - self.assertEqual(header[1], 'text/plain; charset=UTF-8') + self.assertEqual(header[1], 'text/html; charset=UTF-8') def test__content_type_text_html(self): cls = self._getTargetSubclass() From bee098d366c304ed8d3ecab382acd57bebbc9f5e Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Thu, 14 Apr 2016 01:08:49 -0600 Subject: [PATCH 12/13] We don't use default_match, so remove it --- pyramid/httpexceptions.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pyramid/httpexceptions.py b/pyramid/httpexceptions.py index 893795c11b..e76f43c8a2 100644 --- a/pyramid/httpexceptions.py +++ b/pyramid/httpexceptions.py @@ -249,9 +249,8 @@ def prepare(self, environ): accept_value = environ.get('HTTP_ACCEPT', '') accept = MIMEAccept(accept_value) # Attempt to match text/html or application/json, if those don't - # match, we will always have our default of text/plain - match = accept.best_match(['text/html', 'application/json'], - default_match='text/plain') + # match, we will fall through to defaulting to text/plain + match = accept.best_match(['text/html', 'application/json']) if match == 'text/html': self.content_type = 'text/html' From 4c6592734ffb5eace320d8e92e33c5866f111749 Mon Sep 17 00:00:00 2001 From: Bert JW Regeer Date: Thu, 14 Apr 2016 10:43:10 -0600 Subject: [PATCH 13/13] Update CHANGES --- CHANGES.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES.txt b/CHANGES.txt index fd8c636a06..d5640ee0b1 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,6 +1,13 @@ unreleased ========== +- Pyramid HTTPExceptions will now take into account the best match for the + clients Accept header, and depending on what is requested will return + text/html, application/json or text/plain. The default for */* is still + text/html, but if application/json is explicitly mentioned it will now + receive a valid JSON response. See: + https://github.com/Pylons/pyramid/pull/2489 + - Python 2.6 is no longer supported by Pyramid. See https://github.com/Pylons/pyramid/issues/2368