Web exceptions refactoring

aiohttp/web_exceptions.py

@@ -1,6 +1,11 @@
 import warnings
 from typing import Any, Dict, Iterable, List, Optional, Set  # noqa
 
+from multidict import CIMultiDict
+from yarl import URL
+
+from . import hdrs
+from .http import RESPONSES
 from .typedefs import LooseHeaders, StrOrURL
 from .web_response import Response
 
@@ -70,7 +75,7 @@
 # HTTP Exceptions
 ############################################################
 
-class HTTPException(Response, Exception):
+class HTTPException(Exception):
 
     # You should set in subclasses:
     # status = 200
@@ -83,23 +88,71 @@ class HTTPException(Response, Exception):
     def __init__(self, *,
                  headers: Optional[LooseHeaders]=None,
                  reason: Optional[str]=None,
-                 body: Any=None,
                  text: Optional[str]=None,
                  content_type: Optional[str]=None) -> None:
-        if body is not None:
-            warnings.warn(
-                "body argument is deprecated for http web exceptions",
-                DeprecationWarning)
-        Response.__init__(self, status=self.status_code,
-                          headers=headers, reason=reason,
-                          body=body, text=text, content_type=content_type)
-        Exception.__init__(self, self.reason)
-        if self.body is None and not self.empty_body:
-            self.text = "{}: {}".format(self.status, self.reason)
+        if reason is None:
+            try:
+                reason = RESPONSES[self.status_code][0]
+            except Exception:
+                reason = ''
+
+        if text is None:
+            if not self.empty_body:
+                text = "{}: {}".format(self.status_code, reason)
+        else:
+            if self.empty_body:
+                warnings.warn(
+                    ("text argument is deprecated for HTTP status {} ,"
+                     "the response should be provided without a body").format(
+                         self.status_code),
+                    DeprecationWarning,
+                    stacklevel=2)
+
+        if headers is not None:
+            real_headers = CIMultiDict(headers)
+        else:
+            real_headers = CIMultiDict()
+        if content_type is not None:
+            if text:
+                warnings.warn("content_type without text is deprecated",
+                              DeprecationWarning,
+                              stacklevel=2)
+            real_headers[hdrs.CONTENT_TYPE] = content_type
+        elif hdrs.CONTENT_TYPE not in real_headers and text:
+            real_headers[hdrs.CONTENT_TYPE] = 'text/plain'
+
+        super().__init__(reason)
+
+        self._reason = reason
+
+        self._text = text
+        self._headers = real_headers
 
     def __bool__(self) -> bool:
         return True
 
+    @property
+    def status(self) -> int:
+        return self.status_code
+
+    @property
+    def reason(self) -> str:
+        return self._reason
+
+    @property
+    def text(self) -> Optional[str]:
+        return self._text
+
+    @property
+    def headers(self) -> 'CIMultiDict[str]':
+        return self._headers
+
+    def make_response(self) -> Response:
+        return Response(status=self.status_code,
+                        reason=self._reason,
+                        text=self._text,
+                        headers=self._headers)
+
 
 class HTTPError(HTTPException):
     """Base class for exceptions with status codes in the 400s and 500s."""
@@ -155,15 +208,18 @@ def __init__(self,
                  *,
                  headers: Optional[LooseHeaders]=None,
                  reason: Optional[str]=None,
-                 body: Any=None,
                  text: Optional[str]=None,
                  content_type: Optional[str]=None) -> None:
         if not location:
             raise ValueError("HTTP redirects need a location to redirect to.")
         super().__init__(headers=headers, reason=reason,
-                         body=body, text=text, content_type=content_type)
+                         text=text, content_type=content_type)
         self.headers['Location'] = str(location)
-        self.location = location
+        self._location = URL(location)
+
+    @property
+    def location(self) -> URL:
+        return self._location
 
 
 class HTTPMultipleChoices(_HTTPMove):
@@ -241,15 +297,22 @@ def __init__(self,
                  *,
                  headers: Optional[LooseHeaders]=None,
                  reason: Optional[str]=None,
-                 body: Any=None,
                  text: Optional[str]=None,
                  content_type: Optional[str]=None) -> None:
         allow = ','.join(sorted(allowed_methods))
         super().__init__(headers=headers, reason=reason,
-                         body=body, text=text, content_type=content_type)
+                         text=text, content_type=content_type)
         self.headers['Allow'] = allow
-        self.allowed_methods = set(allowed_methods)  # type: Set[str]
-        self.method = method.upper()
+        self._allowed = set(allowed_methods)  # type: Set[str]
+        self._method = method.upper()
+
+    @property
+    def allowed_methods(self) -> Set[str]:
+        return self._allowed
+
+    @property
+    def method(self) -> str:
+        return self._method
 
 
 class HTTPNotAcceptable(HTTPClientError):
@@ -347,11 +410,10 @@ def __init__(self,
                  *,
                  headers: Optional[LooseHeaders]=None,
                  reason: Optional[str]=None,
-                 body: Any=None,
                  text: Optional[str]=None,
                  content_type: Optional[str]=None) -> None:
         super().__init__(headers=headers, reason=reason,
-                         body=body, text=text, content_type=content_type)
+                         text=text, content_type=content_type)
         self.headers['Link'] = '<%s>; rel="blocked-by"' % link
         self.link = link
 

aiohttp/web_protocol.py

@@ -405,7 +405,7 @@ async def start(self) -> None:
                         self._request_handler(request))
                     resp = await task
                 except HTTPException as exc:
-                    resp = exc
+                    resp = exc.make_response()
                 except asyncio.CancelledError:
                     self.log_debug('Ignored premature client disconnection')
                     break

tests/test_web_exceptions.py

@@ -1,47 +1,11 @@
 import collections
 import re
 from traceback import format_exception
-from unittest import mock
 
 import pytest
+from yarl import URL
 
-from aiohttp import helpers, signals, web
-from aiohttp.test_utils import make_mocked_request
-
-
-@pytest.fixture
-def buf():
-    return bytearray()
-
-
-@pytest.fixture
-def http_request(buf):
-    method = 'GET'
-    path = '/'
-    writer = mock.Mock()
-    writer.drain.return_value = ()
-
-    def append(data=b''):
-        buf.extend(data)
-        return helpers.noop()
-
-    async def write_headers(status_line, headers):
-        headers = status_line + '\r\n' + ''.join(
-            [k + ': ' + v + '\r\n' for k, v in headers.items()])
-        headers = headers.encode('utf-8') + b'\r\n'
-        buf.extend(headers)
-
-    writer.buffer_data.side_effect = append
-    writer.write.side_effect = append
-    writer.write_eof.side_effect = append
-    writer.write_headers.side_effect = write_headers
-
-    app = mock.Mock()
-    app._debug = False
-    app.on_response_prepare = signals.Signal(app)
-    app.on_response_prepare.freeze()
-    req = make_mocked_request(method, path, app=app, writer=writer)
-    return req
+from aiohttp import web
 
 
 def test_all_http_exceptions_exported() -> None:
@@ -54,19 +18,6 @@ def test_all_http_exceptions_exported() -> None:
             assert name in web.__all__
 
 
-async def test_HTTPOk(buf, http_request) -> None:
-    resp = web.HTTPOk()
-    await resp.prepare(http_request)
-    await resp.write_eof()
-    txt = buf.decode('utf8')
-    assert re.match(('HTTP/1.1 200 OK\r\n'
-                     'Content-Type: text/plain; charset=utf-8\r\n'
-                     'Content-Length: 7\r\n'
-                     'Date: .+\r\n'
-                     'Server: .+\r\n\r\n'
-                     '200: OK'), txt)
-
-
 def test_terminal_classes_has_status_code() -> None:
     terminals = set()
     for name in dir(web):
@@ -87,20 +38,47 @@ def test_terminal_classes_has_status_code() -> None:
     assert 1 == codes.most_common(1)[0][1]
 
 
-async def test_HTTPFound(buf, http_request) -> None:
-    resp = web.HTTPFound(location='/redirect')
-    assert '/redirect' == resp.location
+async def test_HTTPOk(aiohttp_client) -> None:
+
+    async def handler(request):
+        raise web.HTTPOk()
+
+    app = web.Application()
+    app.router.add_get('/', handler)
+    cli = await aiohttp_client(app)
+
+    resp = await cli.get('/')
+    assert 200 == resp.status
+    txt = await resp.text()
+    assert "200: OK" == txt
+
+
+async def test_HTTPFound(aiohttp_client) -> None:
+
+    async def handler(request):
+        raise web.HTTPFound(location='/redirect')
+
+    app = web.Application()
+    app.router.add_get('/', handler)
+    cli = await aiohttp_client(app)
+
+    resp = await cli.get('/', allow_redirects=False)
+    assert 302 == resp.status
+    txt = await resp.text()
+    assert "302: Found" == txt
     assert '/redirect' == resp.headers['location']
-    await resp.prepare(http_request)
-    await resp.write_eof()
-    txt = buf.decode('utf8')
-    assert re.match('HTTP/1.1 302 Found\r\n'
-                    'Content-Type: text/plain; charset=utf-8\r\n'
-                    'Location: /redirect\r\n'
-                    'Content-Length: 10\r\n'
-                    'Date: .+\r\n'
-                    'Server: .+\r\n\r\n'
-                    '302: Found', txt)
+
+
+def test_HTTPFound_location_str() -> None:
+    exc = web.HTTPFound(location='/redirect')
+    assert exc.location == URL('/redirect')
+    assert exc.headers['Location'] == '/redirect'
+
+
+def test_HTTPFound_location_url() -> None:
+    exc = web.HTTPFound(location=URL('/redirect'))
+    assert exc.location == URL('/redirect')
+    assert exc.headers['Location'] == '/redirect'
 
 
 def test_HTTPFound_empty_location() -> None:
@@ -111,65 +89,42 @@ def test_HTTPFound_empty_location() -> None:
         web.HTTPFound(location=None)
 
 
-async def test_HTTPMethodNotAllowed(buf, http_request) -> None:
-    resp = web.HTTPMethodNotAllowed('get', ['POST', 'PUT'])
-    assert 'GET' == resp.method
-    assert {'POST', 'PUT'} == resp.allowed_methods
-    assert 'POST,PUT' == resp.headers['allow']
-    await resp.prepare(http_request)
-    await resp.write_eof()
-    txt = buf.decode('utf8')
-    assert re.match('HTTP/1.1 405 Method Not Allowed\r\n'
-                    'Content-Type: text/plain; charset=utf-8\r\n'
-                    'Allow: POST,PUT\r\n'
-                    'Content-Length: 23\r\n'
-                    'Date: .+\r\n'
-                    'Server: .+\r\n\r\n'
-                    '405: Method Not Allowed', txt)
-
-
-def test_override_body_with_text() -> None:
-    resp = web.HTTPNotFound(text="Page not found")
-    assert 404 == resp.status
-    assert "Page not found".encode('utf-8') == resp.body
-    assert "Page not found" == resp.text
-    assert "text/plain" == resp.content_type
-    assert "utf-8" == resp.charset
+async def test_HTTPMethodNotAllowed() -> None:
+    exc = web.HTTPMethodNotAllowed('get', ['POST', 'PUT'])
+    assert 'GET' == exc.method
+    assert {'POST', 'PUT'} == exc.allowed_methods
+    assert 'POST,PUT' == exc.headers['allow']
+    assert '405: Method Not Allowed' == exc.text
 
 
-def test_override_body_with_binary() -> None:
-    txt = "<html><body>Page not found</body></html>"
-    with pytest.warns(DeprecationWarning):
-        resp = web.HTTPNotFound(body=txt.encode('utf-8'),
-                                content_type="text/html")
+def test_with_text() -> None:
+    resp = web.HTTPNotFound(text="Page not found")
     assert 404 == resp.status
-    assert txt.encode('utf-8') == resp.body
-    assert txt == resp.text
-    assert "text/html" == resp.content_type
-    assert resp.charset is None
+    assert "Page not found" == resp.text
+    assert "text/plain" == resp.headers['Content-Type']
 
 
-def test_default_body() -> None:
+def test_default_text() -> None:
     resp = web.HTTPOk()
-    assert b'200: OK' == resp.body
+    assert '200: OK' == resp.text
 
 
-def test_empty_body_204() -> None:
+def test_empty_text_204() -> None:
     resp = web.HTTPNoContent()
-    assert resp.body is None
+    assert resp.text is None
 
 
-def test_empty_body_205() -> None:
+def test_empty_text_205() -> None:
     resp = web.HTTPNoContent()
-    assert resp.body is None
+    assert resp.text is None
 
 
-def test_empty_body_304() -> None:
+def test_empty_text_304() -> None:
     resp = web.HTTPNoContent()
-    resp.body is None
+    resp.text is None
 
 
-def test_link_header_451(buf) -> None:
+def test_link_header_451() -> None:
     resp = web.HTTPUnavailableForLegalReasons(link='http://warning.or.kr/')
 
     assert 'http://warning.or.kr/' == resp.link