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

@@ -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
 --------------

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.datastructures import ImmutableDict, Headers
-from werkzeug.routing import Map, Rule, RequestRedirect, BuildError
+from werkzeug.exceptions import BadRequest, HTTPException, \
-from werkzeug.exceptions import HTTPException, InternalServerError, \
+    InternalServerError, MethodNotAllowed, default_exceptions
-     MethodNotAllowed, BadRequest, default_exceptions
+from werkzeug.routing import BuildError, Map, RequestRedirect, Rule
-
+
-from .helpers import _PackageBoundObject, url_for, get_flashed_messages, \
+from . import cli, json
-     locked_cached_property, _endpoint_from_view_func, find_package, \
+from ._compat import integer_types, reraise, string_types, text_type
-     get_debug_flag
+from .config import Config, ConfigAttribute
-from . import json, cli
+from .ctx import AppContext, RequestContext, _AppCtxGlobals
-from .wrappers import Request, Response
+from .globals import _request_ctx_stack, g, request, session
-from .config import ConfigAttribute, Config
+from .helpers import _PackageBoundObject, \
-from .ctx import RequestContext, AppContext, _AppCtxGlobals
+    _endpoint_from_view_func, find_package, get_debug_flag, \
-from .globals import _request_ctx_stack, request, session, g
+    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
+    _default_template_ctx_processor
-from .signals import request_started, request_finished, got_request_exception, \
+from .wrappers import Request, Response
-     request_tearing_down, appcontext_tearing_down
-from ._compat import reraise, string_types, text_type, integer_types
 
 # a lock used for logger initialization
 _logger_lock = Lock()
@@ -1715,62 +1715,106 @@ class Flask(_PackageBoundObject):
         return False
 
     def make_response(self, rv):
-        """Converts the return value from a view function to a real
+        """Convert the return value from a view function to an instance of
-        response object that is an instance of :attr:`response_class`.
+        :attr:`response_class`.
-
+
-        The following types are allowed for `rv`:
+        :param rv: the return value from the view function. The view function
-
+            must return a response. Returning ``None``, or the view ending
-        .. tabularcolumns:: |p{3.5cm}|p{9.5cm}|
+            without returning, is not allowed. The following types are allowed
-
+            for ``view_rv``:
-        ======================= ===========================================
+
-        :attr:`response_class`  the object is returned unchanged
+            ``str`` (``unicode`` in Python 2)
-        :class:`str`            a response object is created with the
+                A response object is created with the string encoded to UTF-8
-                                string as body
+                as the body.
-        :class:`unicode`        a response object is created with the
+                
-                                string encoded to utf-8 as body
+            ``bytes`` (``str`` in Python 2)
-        a WSGI function         the function is called as WSGI application
+                A response object is created with the bytes as the body.
-                                and buffered as response object
+                
-        :class:`tuple`          A tuple in the form ``(response, status,
+            ``tuple``
-                                headers)`` or ``(response, headers)``
+                Either ``(body, status, headers)``, ``(body, status)``, or
-                                where `response` is any of the
+                ``(body, headers)``, where ``body`` is any of the other types
-                                types defined here, `status` is a string
+                allowed here, ``status`` is a string or an integer, and
-                                or an integer and `headers` is a list or
+                ``headers`` is a dictionary or a list of ``(key, value)``
-                                a dictionary with header values.
+                tuples. If ``body`` is a :attr:`response_class` instance,
-        ======================= ===========================================
+                ``status`` overwrites the exiting value and ``headers`` are
-
+                extended.
-        :param rv: the return value from the view function
+    
+            :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:
+        status = headers = None
-            raise ValueError('View function did not return a response')
+
+        # unpack tuple returns
+        if isinstance(rv, (tuple, list)):
+            len_rv = len(rv)
 
-        if isinstance(status_or_headers, (dict, list)):
+            # a 3-tuple is unpacked directly
-            headers, status_or_headers = status_or_headers, None
+            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,
+                # let the response class set the status and headers instead of
-                                         status=status_or_headers)
+                # waiting to do it manually, so that the class can handle any
-                headers = status_or_headers = None
+                # special logic
+                rv = self.response_class(rv, status=status, headers=headers)
+                status = headers = None
             else:
-                rv = self.response_class.force_type(rv, request.environ)
+                # evaluate a WSGI callable, or coerce a different response
-
+                # class to the correct type
-        if status_or_headers is not None:
+                try:
-            if isinstance(status_or_headers, string_types):
+                    rv = self.response_class.force_type(rv, request.environ)
-                rv.status = status_or_headers
+                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)
 

tests/test_basic.py

@@ -975,64 +975,129 @@ def test_enctype_debug_helper():
         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')
+    @app.route('/text')
-    def from_unicode():
+    def from_text():
         return u'Hällo Wörld'
 
-    @app.route('/string')
+    @app.route('/bytes')
-    def from_string():
+    def from_bytes():
         return u'Hällo Wörld'.encode('utf-8')
 
-    @app.route('/args')
+    @app.route('/full_tuple')
-    def from_tuple():
+    def from_full_tuple():
         return 'Meh', 400, {
             'X-Foo': 'Testing',
             'Content-Type': 'text/plain; charset=utf-8'
         }
 
-    @app.route('/two_args')
+    @app.route('/text_headers')
-    def from_two_args_tuple():
+    def from_text_headers():
         return 'Hello', {
             'X-Foo': 'Test',
             'Content-Type': 'text/plain; charset=utf-8'
         }
 
-    @app.route('/args_status')
+    @app.route('/text_status')
-    def from_status_tuple():
+    def from_text_status():
         return 'Hi, status!', 400
 
-    @app.route('/args_header')
+    @app.route('/response_headers')
-    def from_response_instance_status_tuple():
+    def from_response_headers():
-        return flask.Response('Hello world', 404), {
+        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')
+    assert c.get('/text').data == u'Hällo Wörld'.encode('utf-8')
-    rv = c.get('/args')
+    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'
+    rv = c.get('/text_headers')
-    assert rv2.headers['X-Foo'] == 'Test'
+    assert rv.data == b'Hello'
-    assert rv2.status_code == 200
+    assert rv.headers['X-Foo'] == 'Test'
-    assert rv2.mimetype == 'text/plain'
+    assert rv.status_code == 200
-    rv3 = c.get('/args_status')
+    assert rv.mimetype == 'text/plain'
-    assert rv3.data == b'Hi, status!'
+
-    assert rv3.status_code == 400
+    rv = c.get('/text_status')
-    assert rv3.mimetype == 'text/html'
+    assert rv.data == b'Hi, status!'
-    rv4 = c.get('/args_header')
+    assert rv.status_code == 400
-    assert rv4.data == b'Hello world'
+    assert rv.mimetype == 'text/html'
-    assert rv4.headers['X-Foo'] == 'Bar'
+
-    assert rv4.headers['X-Bar'] == 'Foo'
+    rv = c.get('/response_headers')
-    assert rv4.status_code == 404
+    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) == '<LocalProxy unbound>'
     assert not flask.g