Repository: twisted/twisted

Issue:
Problem in web.error str conversion
|[<img alt="technic's avatar" src="https://avatars.githubusercontent.com/u/1092613?s=50" width="50" height="50">](https://github.com/technic)| @technic reported|
|-|-|
|Trac ID|trac#10271|
|Type|defect|
|Created|2021-10-31 22:06:47Z|

Hi,

This method returns an error

https://github.com/twisted/twisted/blob/3d0be31827585ade5078624e17a1c15240eece02/src/twisted/web/error.py#L92

"can't concat str and None" because self.message is None.


in __init__() message is empty string, and the code is 520, then _codeToMessage() returns None, and finally self.message is None.

I suggest to change the code to something like `self.message = message or "Unknown"`


<details><summary>Searchable metadata</summary>

```
trac-id__10271 10271
type__defect defect
reporter__technic technic
priority__normal normal
milestone__None None
branch__ 
branch_author__ 
status__new new
resolution__None None
component__web web
keywords__None None
time__1635718007391402 1635718007391402
changetime__1635718007391402 1635718007391402
version__None None
owner__None None

```
</details>


Discussion:
twm: Could we stop changing the PR title format? :roll_eyes: 

Patch:
diff --git a/src/twisted/web/error.py b/src/twisted/web/error.py
index a741949735..cc151d4205 100644
--- a/src/twisted/web/error.py
+++ b/src/twisted/web/error.py
@@ -22,22 +22,20 @@ __all__ = [
 
 
 from collections.abc import Sequence
-from typing import cast
+from typing import Optional, Union, cast
 
 from twisted.python.compat import nativeString
 from twisted.web._responses import RESPONSES
 
 
-def _codeToMessage(code):
+def _codeToMessage(code: Union[int, bytes]) -> Optional[bytes]:
     """
     Returns the response message corresponding to an HTTP code, or None
     if the code is unknown or unrecognized.
 
-    @type code: L{bytes}
-    @param code: Refers to an HTTP status code, for example C{http.NOT_FOUND}.
+    @param code: HTTP status code, for example C{http.NOT_FOUND}.
 
     @return: A string message or none
-    @rtype: L{bytes}
     """
     try:
         return RESPONSES.get(int(code))
@@ -49,17 +47,23 @@ class Error(Exception):
     """
     A basic HTTP error.
 
-    @type status: L{bytes}
     @ivar status: Refers to an HTTP status code, for example C{http.NOT_FOUND}.
 
-    @type message: L{bytes}
     @param message: A short error message, for example "NOT FOUND".
 
-    @type response: L{bytes}
     @ivar response: A complete HTML document for an error page.
     """
 
-    def __init__(self, code, message=None, response=None):
+    status: bytes
+    message: Optional[bytes]
+    response: Optional[bytes]
+
+    def __init__(
+        self,
+        code: Union[int, bytes],
+        message: Optional[bytes] = None,
+        response: Optional[bytes] = None,
+    ) -> None:
         """
         Initializes a basic exception.
 
@@ -70,11 +74,12 @@ class Error(Exception):
             instead.
 
         @type message: L{bytes}
-        @param message: A short error message, for example "NOT FOUND".
+        @param message: A short error message, for example C{b"NOT FOUND"}.
 
         @type response: L{bytes}
         @param response: A complete HTML document for an error page.
         """
+
         message = message or _codeToMessage(code)
 
         Exception.__init__(self, code, message, response)
@@ -84,24 +89,38 @@ class Error(Exception):
             # downloadPage gives a bytes, Agent gives an int, and it worked by
             # accident previously, so just make it keep working.
             code = b"%d" % (code,)
+        elif len(code) != 3 or not code.isdigit():
+            # Status codes must be 3 digits. See
+            # https://httpwg.org/specs/rfc9110.html#status.code.extensibility
+            raise ValueError(f"Not a valid HTTP status code: {code!r}")
 
         self.status = code
         self.message = message
         self.response = response
 
     def __str__(self) -> str:
-        return nativeString(self.status + b" " + self.message)
+        s = self.status
+        if self.message:
+            s += b" " + self.message
+        return nativeString(s)
 
 
 class PageRedirect(Error):
     """
     A request resulted in an HTTP redirect.
 
-    @type location: L{bytes}
     @ivar location: The location of the redirect which was not followed.
     """
 
-    def __init__(self, code, message=None, response=None, location=None):
+    location: Optional[bytes]
+
+    def __init__(
+        self,
+        code: Union[int, bytes],
+        message: Optional[bytes] = None,
+        response: Optional[bytes] = None,
+        location: Optional[bytes] = None,
+    ) -> None:
         """
         Initializes a page redirect exception.
 
@@ -111,7 +130,7 @@ class PageRedirect(Error):
             descriptive string that is used instead.
 
         @type message: L{bytes}
-        @param message: A short error message, for example "NOT FOUND".
+        @param message: A short error message, for example C{b"NOT FOUND"}.
 
         @type response: L{bytes}
         @param response: A complete HTML document for an error page.
@@ -131,27 +150,30 @@ class InfiniteRedirection(Error):
     """
     HTTP redirection is occurring endlessly.
 
-    @type location: L{bytes}
     @ivar location: The first URL in the series of redirections which was
         not followed.
     """
 
-    def __init__(self, code, message=None, response=None, location=None):
+    location: Optional[bytes]
+
+    def __init__(
+        self,
+        code: Union[int, bytes],
+        message: Optional[bytes] = None,
+        response: Optional[bytes] = None,
+        location: Optional[bytes] = None,
+    ) -> None:
         """
         Initializes an infinite redirection exception.
 
-        @type code: L{bytes}
         @param code: Refers to an HTTP status code, for example
             C{http.NOT_FOUND}. If no C{message} is given, C{code} is mapped to a
             descriptive string that is used instead.
 
-        @type message: L{bytes}
-        @param message: A short error message, for example "NOT FOUND".
+        @param message: A short error message, for example C{b"NOT FOUND"}.
 
-        @type response: L{bytes}
         @param response: A complete HTML document for an error page.
 
-        @type location: L{bytes}
         @param location: The location response-header field value. It is an
             absolute URI used to redirect the receiver to a location other than
             the Request-URI so the request can be completed.
@@ -174,7 +196,10 @@ class RedirectWithNoLocation(Error):
     @since: 11.1
     """
 
-    def __init__(self, code, message, uri):
+    message: bytes
+    uri: bytes
+
+    def __init__(self, code: Union[bytes, int], message: bytes, uri: bytes) -> None:
         """
         Initializes a page redirect exception when no location is given.
 
diff --git a/src/twisted/web/newsfragments/10271.bugfix b/src/twisted/web/newsfragments/10271.bugfix
new file mode 100644
index 0000000000..93d872fe32
--- /dev/null
+++ b/src/twisted/web/newsfragments/10271.bugfix
@@ -0,0 +1,1 @@
+twisted.web.error.Error.__str__ no longer raises an exception when the error's message attribute is None. Additionally, it validates that code is a plausible 3-digit HTTP status code.


Test patch:
diff --git a/src/twisted/web/test/test_error.py b/src/twisted/web/test/test_error.py
index d8e2b65634..f4953adddf 100644
--- a/src/twisted/web/test/test_error.py
+++ b/src/twisted/web/test/test_error.py
@@ -42,19 +42,29 @@ class ErrorTests(unittest.TestCase):
     def test_noMessageValidStatus(self):
         """
         If no C{message} argument is passed to the L{Error} constructor and the
-        C{code} argument is a valid HTTP status code, C{code} is mapped to a
-        descriptive string to which C{message} is assigned.
+        C{code} argument is a valid HTTP status code, C{message} is set to the
+        HTTP reason phrase for C{code}.
         """
         e = error.Error(b"200")
         self.assertEqual(e.message, b"OK")
+        self.assertEqual(str(e), "200 OK")
 
-    def test_noMessageInvalidStatus(self):
+    def test_noMessageForStatus(self):
         """
         If no C{message} argument is passed to the L{Error} constructor and
-        C{code} isn't a valid HTTP status code, C{message} stays L{None}.
+        C{code} isn't a known HTTP status code, C{message} stays L{None}.
         """
-        e = error.Error(b"InvalidCode")
+        e = error.Error(b"999")
         self.assertEqual(e.message, None)
+        self.assertEqual(str(e), "999")
+
+    def test_invalidStatus(self):
+        """
+        If C{code} isn't plausibly an HTTP status code (i.e., composed of
+        digits) it is rejected with L{ValueError}.
+        """
+        with self.assertRaises(ValueError):
+            error.Error(b"InvalidStatus")
 
     def test_messageExists(self):
         """
@@ -63,6 +73,7 @@ class ErrorTests(unittest.TestCase):
         """
         e = error.Error(b"200", b"My own message")
         self.assertEqual(e.message, b"My own message")
+        self.assertEqual(str(e), "200 My own message")
 
     def test_str(self):
         """
@@ -107,7 +118,7 @@ class PageRedirectTests(unittest.TestCase):
         If no C{message} argument is passed to the L{PageRedirect} constructor
         and C{code} isn't a valid HTTP status code, C{message} stays L{None}.
         """
-        e = error.PageRedirect(b"InvalidCode", location=b"/foo")
+        e = error.PageRedirect(b"999", location=b"/foo")
         self.assertEqual(e.message, None)
 
     def test_messageExistsLocationExists(self):
@@ -160,8 +171,9 @@ class InfiniteRedirectionTests(unittest.TestCase):
         constructor and C{code} isn't a valid HTTP status code, C{message} stays
         L{None}.
         """
-        e = error.InfiniteRedirection(b"InvalidCode", location=b"/foo")
+        e = error.InfiniteRedirection(b"999", location=b"/foo")
         self.assertEqual(e.message, None)
+        self.assertEqual(str(e), "999")
 
     def test_messageExistsLocationExists(self):
         """