From 697f7b9365304c45216e6c8dd307d931ea1506d0 Mon Sep 17 00:00:00 2001 From: David Lord Date: Mon, 24 Apr 2017 14:11:49 -0700 Subject: [PATCH] refactor make_response to be easier to follow * be explicit about how tuples are unpacked * allow bytes for status value * allow Headers for headers value * use TypeError instead of ValueError * errors are more descriptive * document that view must not return None * update documentation about return values * test more response types * test error messages closes #1676 --- CHANGES | 4 ++ flask/app.py | 170 ++++++++++++++++++++++++++++---------------- tests/test_basic.py | 143 +++++++++++++++++++++++++------------ 3 files changed, 207 insertions(+), 110 deletions(-) diff --git a/CHANGES b/CHANGES index ae592ba86f..11ac64307e 100644 --- a/CHANGES +++ b/CHANGES @@ -29,6 +29,9 @@ Major release, unreleased handled by the app's error handlers. (`#2254`_) - Blueprints gained ``json_encoder`` and ``json_decoder`` attributes to override the app's encoder and decoder. (`#1898`_) +- ``Flask.make_response`` raises ``TypeError`` instead of ``ValueError`` for + bad response types. The error messages have been improved to describe why the + type is invalid. (`#2256`_) .. _#1489: https://github.com/pallets/flask/pull/1489 .. _#1898: https://github.com/pallets/flask/pull/1898 @@ -36,6 +39,7 @@ Major release, unreleased .. _#2017: https://github.com/pallets/flask/pull/2017 .. _#2223: https://github.com/pallets/flask/pull/2223 .. _#2254: https://github.com/pallets/flask/pull/2254 +.. _#2256: https://github.com/pallets/flask/pull/2256 Version 0.12.1 -------------- diff --git a/flask/app.py b/flask/app.py index 1943cfcf83..a938344bb8 100644 --- a/flask/app.py +++ b/flask/app.py @@ -10,30 +10,30 @@ """ import os import sys -from threading import Lock from datetime import timedelta -from itertools import chain from functools import update_wrapper +from itertools import chain +from threading import Lock -from werkzeug.datastructures import ImmutableDict -from werkzeug.routing import Map, Rule, RequestRedirect, BuildError -from werkzeug.exceptions import HTTPException, InternalServerError, \ - MethodNotAllowed, BadRequest, default_exceptions - -from .helpers import _PackageBoundObject, url_for, get_flashed_messages, \ - locked_cached_property, _endpoint_from_view_func, find_package, \ - get_debug_flag -from . import json, cli -from .wrappers import Request, Response -from .config import ConfigAttribute, Config -from .ctx import RequestContext, AppContext, _AppCtxGlobals -from .globals import _request_ctx_stack, request, session, g +from werkzeug.datastructures import ImmutableDict, Headers +from werkzeug.exceptions import BadRequest, HTTPException, \ + InternalServerError, MethodNotAllowed, default_exceptions +from werkzeug.routing import BuildError, Map, RequestRedirect, Rule + +from . import cli, json +from ._compat import integer_types, reraise, string_types, text_type +from .config import Config, ConfigAttribute +from .ctx import AppContext, RequestContext, _AppCtxGlobals +from .globals import _request_ctx_stack, g, request, session +from .helpers import _PackageBoundObject, \ + _endpoint_from_view_func, find_package, get_debug_flag, \ + get_flashed_messages, locked_cached_property, url_for from .sessions import SecureCookieSessionInterface +from .signals import appcontext_tearing_down, got_request_exception, \ + request_finished, request_started, request_tearing_down from .templating import DispatchingJinjaLoader, Environment, \ - _default_template_ctx_processor -from .signals import request_started, request_finished, got_request_exception, \ - request_tearing_down, appcontext_tearing_down -from ._compat import reraise, string_types, text_type, integer_types + _default_template_ctx_processor +from .wrappers import Request, Response # a lock used for logger initialization _logger_lock = Lock() @@ -1715,62 +1715,106 @@ def should_ignore_error(self, error): return False def make_response(self, rv): - """Converts the return value from a view function to a real - response object that is an instance of :attr:`response_class`. - - The following types are allowed for `rv`: - - .. tabularcolumns:: |p{3.5cm}|p{9.5cm}| - - ======================= =========================================== - :attr:`response_class` the object is returned unchanged - :class:`str` a response object is created with the - string as body - :class:`unicode` a response object is created with the - string encoded to utf-8 as body - a WSGI function the function is called as WSGI application - and buffered as response object - :class:`tuple` A tuple in the form ``(response, status, - headers)`` or ``(response, headers)`` - where `response` is any of the - types defined here, `status` is a string - or an integer and `headers` is a list or - a dictionary with header values. - ======================= =========================================== - - :param rv: the return value from the view function + """Convert the return value from a view function to an instance of + :attr:`response_class`. + + :param rv: the return value from the view function. The view function + must return a response. Returning ``None``, or the view ending + without returning, is not allowed. The following types are allowed + for ``view_rv``: + + ``str`` (``unicode`` in Python 2) + A response object is created with the string encoded to UTF-8 + as the body. + + ``bytes`` (``str`` in Python 2) + A response object is created with the bytes as the body. + + ``tuple`` + Either ``(body, status, headers)``, ``(body, status)``, or + ``(body, headers)``, where ``body`` is any of the other types + allowed here, ``status`` is a string or an integer, and + ``headers`` is a dictionary or a list of ``(key, value)`` + tuples. If ``body`` is a :attr:`response_class` instance, + ``status`` overwrites the exiting value and ``headers`` are + extended. + + :attr:`response_class` + The object is returned unchanged. + + other :class:`~werkzeug.wrappers.Response` class + The object is coerced to :attr:`response_class`. + + :func:`callable` + The function is called as a WSGI application. The result is + used to create a response object. .. versionchanged:: 0.9 Previously a tuple was interpreted as the arguments for the response object. """ - status_or_headers = headers = None - if isinstance(rv, tuple): - rv, status_or_headers, headers = rv + (None,) * (3 - len(rv)) - if rv is None: - raise ValueError('View function did not return a response') + status = headers = None + + # unpack tuple returns + if isinstance(rv, (tuple, list)): + len_rv = len(rv) - if isinstance(status_or_headers, (dict, list)): - headers, status_or_headers = status_or_headers, None + # a 3-tuple is unpacked directly + if len_rv == 3: + rv, status, headers = rv + # decide if a 2-tuple has status or headers + elif len_rv == 2: + if isinstance(rv[1], (Headers, dict, tuple, list)): + rv, headers = rv + else: + rv, status = rv + # other sized tuples are not allowed + else: + raise TypeError( + 'The view function did not return a valid response tuple.' + ' The tuple must have the form (body, status, headers),' + ' (body, status), or (body, headers).' + ) + # the body must not be None + if rv is None: + raise TypeError( + 'The view function did not return a valid response. The' + ' function either returned None or ended without a return' + ' statement.' + ) + + # make sure the body is an instance of the response class if not isinstance(rv, self.response_class): - # When we create a response object directly, we let the constructor - # set the headers and status. We do this because there can be - # some extra logic involved when creating these objects with - # specific values (like default content type selection). if isinstance(rv, (text_type, bytes, bytearray)): - rv = self.response_class(rv, headers=headers, - status=status_or_headers) - headers = status_or_headers = None + # let the response class set the status and headers instead of + # waiting to do it manually, so that the class can handle any + # special logic + rv = self.response_class(rv, status=status, headers=headers) + status = headers = None else: - rv = self.response_class.force_type(rv, request.environ) - - if status_or_headers is not None: - if isinstance(status_or_headers, string_types): - rv.status = status_or_headers + # evaluate a WSGI callable, or coerce a different response + # class to the correct type + try: + rv = self.response_class.force_type(rv, request.environ) + except TypeError as e: + new_error = TypeError( + '{e}\nThe view function did not return a valid' + ' response. The return type must be a string, tuple,' + ' Response instance, or WSGI callable, but it was a' + ' {rv.__class__.__name__}.'.format(e=e, rv=rv) + ) + reraise(TypeError, new_error, sys.exc_info()[2]) + + # prefer the status if it was provided + if status is not None: + if isinstance(status, (text_type, bytes, bytearray)): + rv.status = status else: - rv.status_code = status_or_headers + rv.status_code = status + + # extend existing headers with provided headers if headers: rv.headers.extend(headers) diff --git a/tests/test_basic.py b/tests/test_basic.py index 677b4be86a..163b83cfa3 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -975,64 +975,129 @@ def index(): assert 'This was submitted: "index.txt"' in str(e.value) -def test_response_creation(): +def test_response_types(): app = flask.Flask(__name__) + app.testing = True - @app.route('/unicode') - def from_unicode(): + @app.route('/text') + def from_text(): return u'Hällo Wörld' - @app.route('/string') - def from_string(): + @app.route('/bytes') + def from_bytes(): return u'Hällo Wörld'.encode('utf-8') - @app.route('/args') - def from_tuple(): + @app.route('/full_tuple') + def from_full_tuple(): return 'Meh', 400, { 'X-Foo': 'Testing', 'Content-Type': 'text/plain; charset=utf-8' } - @app.route('/two_args') - def from_two_args_tuple(): + @app.route('/text_headers') + def from_text_headers(): return 'Hello', { 'X-Foo': 'Test', 'Content-Type': 'text/plain; charset=utf-8' } - @app.route('/args_status') - def from_status_tuple(): + @app.route('/text_status') + def from_text_status(): return 'Hi, status!', 400 - @app.route('/args_header') - def from_response_instance_status_tuple(): - return flask.Response('Hello world', 404), { + @app.route('/response_headers') + def from_response_headers(): + return flask.Response('Hello world', 404, {'X-Foo': 'Baz'}), { "X-Foo": "Bar", "X-Bar": "Foo" } + @app.route('/response_status') + def from_response_status(): + return app.response_class('Hello world', 400), 500 + + @app.route('/wsgi') + def from_wsgi(): + return NotFound() + c = app.test_client() - assert c.get('/unicode').data == u'Hällo Wörld'.encode('utf-8') - assert c.get('/string').data == u'Hällo Wörld'.encode('utf-8') - rv = c.get('/args') + + assert c.get('/text').data == u'Hällo Wörld'.encode('utf-8') + assert c.get('/bytes').data == u'Hällo Wörld'.encode('utf-8') + + rv = c.get('/full_tuple') assert rv.data == b'Meh' assert rv.headers['X-Foo'] == 'Testing' assert rv.status_code == 400 assert rv.mimetype == 'text/plain' - rv2 = c.get('/two_args') - assert rv2.data == b'Hello' - assert rv2.headers['X-Foo'] == 'Test' - assert rv2.status_code == 200 - assert rv2.mimetype == 'text/plain' - rv3 = c.get('/args_status') - assert rv3.data == b'Hi, status!' - assert rv3.status_code == 400 - assert rv3.mimetype == 'text/html' - rv4 = c.get('/args_header') - assert rv4.data == b'Hello world' - assert rv4.headers['X-Foo'] == 'Bar' - assert rv4.headers['X-Bar'] == 'Foo' - assert rv4.status_code == 404 + + rv = c.get('/text_headers') + assert rv.data == b'Hello' + assert rv.headers['X-Foo'] == 'Test' + assert rv.status_code == 200 + assert rv.mimetype == 'text/plain' + + rv = c.get('/text_status') + assert rv.data == b'Hi, status!' + assert rv.status_code == 400 + assert rv.mimetype == 'text/html' + + rv = c.get('/response_headers') + assert rv.data == b'Hello world' + assert rv.headers.getlist('X-Foo') == ['Baz', 'Bar'] + assert rv.headers['X-Bar'] == 'Foo' + assert rv.status_code == 404 + + rv = c.get('/response_status') + assert rv.data == b'Hello world' + assert rv.status_code == 500 + + rv = c.get('/wsgi') + assert b'Not Found' in rv.data + assert rv.status_code == 404 + + +def test_response_type_errors(): + app = flask.Flask(__name__) + app.testing = True + + @app.route('/none') + def from_none(): + pass + + @app.route('/small_tuple') + def from_small_tuple(): + return 'Hello', + + @app.route('/large_tuple') + def from_large_tuple(): + return 'Hello', 234, {'X-Foo': 'Bar'}, '???' + + @app.route('/bad_type') + def from_bad_type(): + return True + + @app.route('/bad_wsgi') + def from_bad_wsgi(): + return lambda: None + + c = app.test_client() + + with pytest.raises(TypeError) as e: + c.get('/none') + assert 'returned None' in str(e) + + with pytest.raises(TypeError) as e: + c.get('/small_tuple') + assert 'tuple must have the form' in str(e) + + pytest.raises(TypeError, c.get, '/large_tuple') + + with pytest.raises(TypeError) as e: + c.get('/bad_type') + assert 'it was a bool' in str(e) + + pytest.raises(TypeError, c.get, '/bad_wsgi') def test_make_response(): @@ -1272,22 +1337,6 @@ def test_static_route_with_host_matching(): flask.Flask(__name__, host_matching=True, static_folder=None) -def test_none_response(): - app = flask.Flask(__name__) - app.testing = True - - @app.route('/') - def test(): - return None - try: - app.test_client().get('/') - except ValueError as e: - assert str(e) == 'View function did not return a response' - pass - else: - assert "Expected ValueError" - - def test_request_locals(): assert repr(flask.g) == '' assert not flask.g